Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(287)

Issue 499483003: AudioMirroringManager becomes a global LazyInstance. (Closed)

Created:
6 years, 3 months ago by miu
Modified:
6 years, 3 months ago
Reviewers:
DaleCurtis, sky
CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

AudioMirroringManager becomes a global LazyInstance. While attempting to resolve a flaky browser_test, it became clear that AudioMirroringManager was not outliving its use. In the original change that introduced this class, it was instantiated and owned by BrowserMainLoop as a matter of convenience. However, with some debugging, it's clear that it must outlive objects that can outlive BrowserMainLoop (e.g., WebContentsAudioInputStream). Side notes: I've checked the feasibility of other solutions, confirming that the shutdown of AudioManager does NOT guarantee complete teardown of an AudioInputStream, so it's not sufficient to simply change the destruction order of the objects in BrowserMainLoop to resolve this problem. As AudioMirroringManager provides a browser-wide service and owns no objects, it seems reasonable for it to exist as a global LazyInstance. BUG=396413 Committed: https://crrev.com/8b4f7fa775288ec5306b913f133830ed5f771064 Cr-Commit-Position: refs/heads/master@{#292258}

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -51 lines) Patch
M content/browser/browser_main_loop.h View 3 chunks +0 lines, -5 lines 0 comments Download
M content/browser/browser_main_loop.cc View 2 chunks +0 lines, -5 lines 0 comments Download
M content/browser/media/capture/audio_mirroring_manager.h View 4 chunks +9 lines, -2 lines 0 comments Download
M content/browser/media/capture/audio_mirroring_manager.cc View 4 chunks +17 lines, -37 lines 1 comment Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
miu
miu@chromium.org changed reviewers: + dalecurtis@chromium.org, sky@chromium.org
6 years, 3 months ago (2014-08-27 19:54:09 UTC) #1
miu
dalecurtis: PTAL. sky: Need OWNERS stamp for deletions in browser_main_loop.* and render_process_host_impl.cc. https://codereview.chromium.org/499483003/diff/1/content/browser/media/capture/audio_mirroring_manager.cc File content/browser/media/capture/audio_mirroring_manager.cc ...
6 years, 3 months ago (2014-08-27 19:54:58 UTC) #2
DaleCurtis
lgtm
6 years, 3 months ago (2014-08-27 20:25:50 UTC) #3
sky
LGTM
6 years, 3 months ago (2014-08-27 21:40:34 UTC) #4
miu
The CQ bit was checked by miu@chromium.org
6 years, 3 months ago (2014-08-27 21:54:11 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/499483003/1
6 years, 3 months ago (2014-08-27 21:55:19 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1) as 033c98e7d33ea0e93b797b9b3e69e9a12ca66a50
6 years, 3 months ago (2014-08-27 23:04:06 UTC) #7
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 02:55:18 UTC) #8
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/8b4f7fa775288ec5306b913f133830ed5f771064
Cr-Commit-Position: refs/heads/master@{#292258}

Powered by Google App Engine
This is Rietveld 408576698