Skip to content

Commit

Permalink
Revert "Move Audio Service off Service Manager"
Browse files Browse the repository at this point in the history
This reverts commit a4d27f5.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 721832 as the
culprit for failures in the build cycles as shown on:
https://analysis.chromium.org/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2E0ZDI3ZjUxM2ViNDBhODk1MDVmMDQyZDY4NjkzZWQ2NzAyNDAwZDkM

Sample Failed Build: https://ci.chromium.org/b/8894942016495385648

Sample Failed Step: browser_tests

Original change's description:
> Move Audio Service off Service Manager
> 
> All the details around if/when a service instance is started and whether
> it's in- or out-of-process have been centralized into the implementation
> of a single GetAudioService() helper in Content.
> 
> A few other helpers are added to ease the transition off the Service
> Manager, though they aren't necessarily ideal APIs. For example,
> client library APIs which once took a Service Manager Connector may now
> take a RepeatingCallback to bind a specific type of interface.
> 
> In addition to GetAudioService(), Content provides helpers for
> AudioSystem construction and StreamFactory binding from any sequence.
> 
> Because all timeout/lifetime management logic is now generically
> supported within Mojo and Content and requires no service implementation
> details to get right (and because unit tests launching service processes
> is no longer supported outside of Content), all Audio Service test
> coverage relevant to lifetime management is deemed superfluous and has
> been removed.
> 
> Bug: 977637
> Change-Id: I4c59948eafce2322547bf2b0479961e095b3edee
> Tbr: oshima@chromium.org
> Tbr: guidou@chromium.org
> Tbr: vollick@chromium.org
> Tbr: xiaohuic@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1941280
> Commit-Queue: Ken Rockot <rockot@google.com>
> Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#721832}


Change-Id: Ic59416fc7c5f5eda2c3b8b211773f9761d2ea03d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 977637
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1952526
Cr-Commit-Position: refs/heads/master@{#721896}
  • Loading branch information
Findit committed Dec 5, 2019
1 parent 74a9f08 commit 06d2237
Show file tree
Hide file tree
Showing 110 changed files with 2,091 additions and 857 deletions.
12 changes: 12 additions & 0 deletions chrome/browser/chrome_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@
#elif defined(OS_MACOSX)
#include "chrome/browser/apps/intent_helper/mac_apps_navigation_throttle.h"
#include "chrome/browser/chrome_browser_main_mac.h"
#include "services/audio/public/mojom/constants.mojom.h"
#elif defined(OS_CHROMEOS)
#include "ash/public/cpp/tablet_mode.h"
#include "ash/public/mojom/constants.mojom.h"
Expand Down Expand Up @@ -2224,6 +2225,17 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
#endif
}

void ChromeContentBrowserClient::AdjustUtilityServiceProcessCommandLine(
const service_manager::Identity& identity,
base::CommandLine* command_line) {
#if defined(OS_MACOSX)
// On Mac, the audio service requires a CFRunLoop, provided by a UI message
// loop, to run AVFoundation and CoreAudio code. See https://crbug.com/834581.
if (identity.name() == audio::mojom::kServiceName)
command_line->AppendSwitch(switches::kMessageLoopTypeUi);
#endif
}

std::string
ChromeContentBrowserClient::GetApplicationClientGUIDForQuarantineCheck() {
return std::string(chrome::kApplicationClientIDStringForAVScanning);
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/chrome_content_browser_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
const base::FilePath& profile_path) override;
void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
int child_process_id) override;
void AdjustUtilityServiceProcessCommandLine(
const service_manager::Identity& identity,
base::CommandLine* command_line) override;
std::string GetApplicationClientGUIDForQuarantineCheck() override;
std::string GetApplicationLocale() override;
std::string GetAcceptLangs(content::BrowserContext* context) override;
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/chromeos/chrome_browser_main_chromeos.cc
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,6 @@
#include "components/user_manager/user.h"
#include "components/user_manager/user_manager.h"
#include "components/user_manager/user_names.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/media_capture_devices.h"
Expand Down Expand Up @@ -651,7 +650,7 @@ void ChromeBrowserMainPartsChromeos::PreProfileInit() {
new default_app_order::ExternalLoader(true /* async */));
}

audio::SoundsManager::Create(content::GetAudioServiceStreamFactoryBinder());
audio::SoundsManager::Create(content::GetSystemConnector()->Clone());

// |arc_service_launcher_| must be initialized before NoteTakingHelper.
NoteTakingHelper::Initialize();
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/chromeos/login/kiosk_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@
#include "components/crx_file/crx_verifier.h"
#include "components/prefs/pref_service.h"
#include "components/signin/public/identity_manager/identity_manager.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/system_connector.h"
#include "content/public/browser/web_ui.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/test_utils.h"
Expand All @@ -98,6 +98,7 @@
#include "services/audio/public/cpp/fake_system_info.h"
#include "services/audio/public/cpp/sounds/audio_stream_handler.h"
#include "services/audio/public/cpp/sounds/sounds_manager.h"
#include "services/service_manager/public/cpp/connector.h"
#include "ui/aura/window.h"
#include "ui/base/accelerators/accelerator.h"

Expand Down Expand Up @@ -2372,7 +2373,7 @@ class KioskVirtualKeyboardTestSoundsManagerTestImpl
}

auto handler = std::make_unique<audio::AudioStreamHandler>(
content::GetAudioServiceStreamFactoryBinder(), iter->second);
content::GetSystemConnector()->Clone(), iter->second);
if (!handler->IsInitialized()) {
LOG(WARNING) << "Can't initialize AudioStreamHandler for key = " << key;
return false;
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/chromeos/login/lock/screen_locker_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#include "components/account_id/account_id.h"
#include "components/session_manager/core/session_manager.h"
#include "components/user_manager/scoped_user_manager.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/system_connector.h"
#include "content/public/test/browser_task_environment.h"
#include "content/public/test/test_service_manager_context.h"
#include "device/bluetooth/dbus/bluez_dbus_manager.h"
Expand Down Expand Up @@ -78,7 +78,7 @@ class ScreenLockerUnitTest : public testing::Test {
observer_ = std::make_unique<audio::TestObserver>((base::DoNothing()));
audio::AudioStreamHandler::SetObserverForTesting(observer_.get());

audio::SoundsManager::Create(content::GetAudioServiceStreamFactoryBinder());
audio::SoundsManager::Create(content::GetSystemConnector()->Clone());
input_method::InputMethodManager::Initialize(
// Owned by InputMethodManager
new input_method::MockInputMethodManagerImpl());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,14 @@
#include "chrome/common/url_constants.h"
#include "chromeos/constants/chromeos_features.h"
#include "components/user_manager/user_manager.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/system_connector.h"
#include "extensions/browser/event_router.h"
#include "extensions/common/api/virtual_keyboard.h"
#include "extensions/common/api/virtual_keyboard_private.h"
#include "media/audio/audio_system.h"
#include "services/audio/public/cpp/audio_system_factory.h"
#include "services/service_manager/public/cpp/connector.h"
#include "ui/aura/event_injector.h"
#include "ui/aura/window_tree_host.h"
#include "ui/base/ime/constants.h"
Expand Down Expand Up @@ -170,8 +172,10 @@ ChromeVirtualKeyboardDelegate::~ChromeVirtualKeyboardDelegate() {}
void ChromeVirtualKeyboardDelegate::GetKeyboardConfig(
OnKeyboardSettingsCallback on_settings_callback) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (!audio_system_)
audio_system_ = content::CreateAudioSystemForAudioService();
if (!audio_system_) {
audio_system_ =
audio::CreateAudioSystem(content::GetSystemConnector()->Clone());
}
audio_system_->HasInputDevices(
base::BindOnce(&ChromeVirtualKeyboardDelegate::OnHasInputDevices,
weak_this_, std::move(on_settings_callback)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,18 @@
#include "chrome/browser/extensions/api/tabs/tabs_constants.h"
#include "chrome/browser/extensions/extension_tab_util.h"
#include "chrome/browser/profiles/profile.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/media_device_id.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/system_connector.h"
#include "content/public/browser/web_contents.h"
#include "extensions/browser/event_router.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/common/error_utils.h"
#include "extensions/common/permissions/permissions_data.h"
#include "media/audio/audio_system.h"
#include "services/audio/public/cpp/audio_system_factory.h"
#include "services/service_manager/public/cpp/connector.h"
#include "url/gurl.h"
#include "url/origin.h"

Expand Down Expand Up @@ -132,8 +134,10 @@ std::string WebrtcAudioPrivateFunction::device_id_salt() const {

media::AudioSystem* WebrtcAudioPrivateFunction::GetAudioSystem() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (!audio_system_)
audio_system_ = content::CreateAudioSystemForAudioService();
if (!audio_system_) {
audio_system_ =
audio::CreateAudioSystem(content::GetSystemConnector()->Clone());
}
return audio_system_.get();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@
#include "chrome/common/buildflags.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/media_device_id.h"
#include "content/public/browser/system_connector.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test_utils.h"
#include "extensions/common/permissions/permission_set.h"
Expand All @@ -44,6 +44,8 @@
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
#include "services/audio/public/cpp/audio_system_factory.h"
#include "services/service_manager/public/cpp/connector.h"
#include "testing/gtest/include/gtest/gtest.h"

#if defined(OS_WIN)
Expand All @@ -70,7 +72,7 @@ void GetAudioDeviceDescriptions(bool for_input,
AudioDeviceDescriptions* device_descriptions) {
base::RunLoop run_loop;
std::unique_ptr<media::AudioSystem> audio_system =
content::CreateAudioSystemForAudioService();
audio::CreateAudioSystem(content::GetSystemConnector()->Clone());
audio_system->GetDeviceDescriptions(
for_input,
base::BindOnce(
Expand Down
8 changes: 3 additions & 5 deletions chrome/browser/media/webrtc/audio_debug_recordings_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@
#include "base/task/post_task.h"
#include "base/time/time.h"
#include "components/webrtc_logging/browser/text_log_list.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/system_connector.h"
#include "media/audio/audio_debug_recording_session.h"
#include "services/audio/public/cpp/debug_recording_session_factory.h"
#include "services/service_manager/public/cpp/connector.h"

using content::BrowserThread;

Expand Down Expand Up @@ -111,11 +112,8 @@ void AudioDebugRecordingsHandler::DoStartAudioDebugRecordings(
log_directory, ++current_audio_debug_recordings_id_);
host->EnableAudioDebugRecordings(prefix_path);

mojo::PendingRemote<audio::mojom::DebugRecording> debug_recording;
content::GetAudioService().BindDebugRecording(
debug_recording.InitWithNewPipeAndPassReceiver());
audio_debug_recording_session_ = audio::CreateAudioDebugRecordingSession(
prefix_path, std::move(debug_recording));
prefix_path, content::GetSystemConnector()->Clone());

if (delay.is_zero()) {
const bool is_stopped = false, is_manual_stop = false;
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/ui/ash/assistant/assistant_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@
#include "chromeos/constants/chromeos_switches.h"
#include "chromeos/services/assistant/public/features.h"
#include "components/session_manager/core/session_manager.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/network_service_instance.h"
#include "content/public/browser/service_process_host.h"
#include "content/public/browser/system_connector.h"
#include "content/public/common/content_switches.h"
#include "services/audio/public/mojom/constants.mojom.h"
#include "services/device/public/mojom/constants.mojom.h"
#include "services/identity/public/mojom/identity_service.mojom.h"
#include "services/media_session/public/mojom/constants.mojom.h"
Expand Down Expand Up @@ -183,7 +183,8 @@ void AssistantClient::RequestWakeLockProvider(

void AssistantClient::RequestAudioStreamFactory(
mojo::PendingReceiver<audio::mojom::StreamFactory> receiver) {
content::GetAudioService().BindStreamFactory(std::move(receiver));
content::GetSystemConnector()->Connect(audio::mojom::kServiceName,
std::move(receiver));
}

void AssistantClient::RequestAudioDecoderFactory(
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/vr/sounds_manager_audio_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// found in the LICENSE file.

#include "chrome/browser/vr/sounds_manager_audio_delegate.h"
#include "content/public/browser/audio_service.h"
#include "content/public/browser/system_connector.h"
#include "services/audio/public/cpp/sounds/sounds_manager.h"

namespace vr {
Expand All @@ -30,7 +30,7 @@ bool SoundsManagerAudioDelegate::RegisterSound(
DCHECK(sounds_.find(id) == sounds_.end());

if (sounds_.empty())
audio::SoundsManager::Create(content::GetAudioServiceStreamFactoryBinder());
audio::SoundsManager::Create(content::GetSystemConnector()->Clone());

sounds_[id] = std::move(data);
return audio::SoundsManager::Get()->Initialize(id, *sounds_[id]);
Expand Down
1 change: 1 addition & 0 deletions chromeos/services/assistant/assistant_state_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "ash/public/mojom/assistant_state_controller.mojom.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "services/service_manager/public/cpp/connector.h"

namespace chromeos {
namespace assistant {
Expand Down
4 changes: 2 additions & 2 deletions content/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,9 @@ jumbo_source_set("browser") {
"//net/server:http_server",
"//ppapi/buildflags",
"//printing/buildflags",
"//services/audio",
"//services/audio:lib",
"//services/audio/public/cpp",
"//services/audio/public/cpp:manifest",
"//services/audio/public/mojom",
"//services/content:impl",
"//services/content/public/cpp",
Expand Down Expand Up @@ -433,7 +434,6 @@ jumbo_source_set("browser") {
"appcache/appcache_working_set.h",
"appcache/chrome_appcache_service.cc",
"appcache/chrome_appcache_service.h",
"audio/audio_service.cc",
"background_fetch/background_fetch_context.cc",
"background_fetch/background_fetch_context.h",
"background_fetch/background_fetch_cross_origin_filter.cc",
Expand Down
2 changes: 0 additions & 2 deletions content/browser/audio/OWNERS

This file was deleted.

Loading

0 comments on commit 06d2237

Please sign in to comment.