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

Issue 586303004: WebContentsAudioMuter: Mute all audio output from a WebContentsImpl. (Closed)

Created:
6 years, 3 months ago by miu
Modified:
6 years, 3 months ago
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

WebContentsAudioMuter: Mute all audio output from a WebContentsImpl. Adds support in libcontent for muting all audio output from a WebContentsImpl. This is meant to be activated via user actions in the browser tab strip UI (follow-up change pending). To be clear, this change introduces the functionality required to mute audio, but there is nothing in this change to activate it. The plan is to provide this feature experimentally behind about:flags, and gather data to drive later launch decisions. Main approach: Introduce the WebContentsAudioMuter, which leverages the existing WebContents audio capture mechanisms (AudioMirroringManager) to divert all audio output to a "null" destination. In other words, this is essentially "audio capture to nowhere." BUG=360372 Committed: https://crrev.com/50f9789a2baac93fd5ac908ae01e4bec597955cf Cr-Commit-Position: refs/heads/master@{#296078}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Changes per avi's comments. #

Total comments: 2

Patch Set 3 : Moar alphabetizing. ;-) #

Total comments: 10

Patch Set 4 : Addressed Dale's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+253 lines, -0 lines) Patch
M content/browser/media/capture/web_contents_audio_input_stream.cc View 1 3 chunks +20 lines, -0 lines 0 comments Download
A content/browser/media/capture/web_contents_audio_muter.h View 1 1 chunk +43 lines, -0 lines 0 comments Download
A content/browser/media/capture/web_contents_audio_muter.cc View 1 2 3 1 chunk +153 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 3 chunks +6 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 2 chunks +25 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M content/public/browser/web_contents.h View 1 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (2 generated)
miu
avi: PTAL at web_contents.h and web_contents_impl.* changes. dalecurtis: PTAL at content/browser/media/*, and feel free to ...
6 years, 3 months ago (2014-09-20 22:52:38 UTC) #2
Avi (use Gerrit)
The web contents stuff looks decent; some commentary. https://codereview.chromium.org/586303004/diff/1/content/browser/media/capture/web_contents_audio_muter.cc File content/browser/media/capture/web_contents_audio_muter.cc (right): https://codereview.chromium.org/586303004/diff/1/content/browser/media/capture/web_contents_audio_muter.cc#newcode1 content/browser/media/capture/web_contents_audio_muter.cc:1: // ...
6 years, 3 months ago (2014-09-21 00:20:00 UTC) #3
miu
Thanks for the quick turnaround, Avi. Made changes per all your comments: https://codereview.chromium.org/586303004/diff/1/content/browser/media/capture/web_contents_audio_muter.cc File content/browser/media/capture/web_contents_audio_muter.cc ...
6 years, 3 months ago (2014-09-21 02:33:33 UTC) #4
Avi (use Gerrit)
non-media stuff lgtm with fix noted below https://codereview.chromium.org/586303004/diff/20001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/586303004/diff/20001/content/content_browser.gypi#newcode853 content/content_browser.gypi:853: 'browser/media/capture/web_contents_audio_muter.h', alphabetize
6 years, 3 months ago (2014-09-21 02:55:22 UTC) #5
miu
https://codereview.chromium.org/586303004/diff/20001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/586303004/diff/20001/content/content_browser.gypi#newcode853 content/content_browser.gypi:853: 'browser/media/capture/web_contents_audio_muter.h', On 2014/09/21 02:55:22, Avi wrote: > alphabetize Oh, ...
6 years, 3 months ago (2014-09-21 18:57:51 UTC) #6
miu
BTW--For anyone that's curious, here's the UI change that leverages the WebContentsAudioMuter: https://codereview.chromium.org/591963002/
6 years, 3 months ago (2014-09-21 20:46:50 UTC) #7
DaleCurtis
This effectively means a capturing session can't be muted via the tab controls? Also, upon ...
6 years, 3 months ago (2014-09-22 17:45:45 UTC) #8
miu
> This effectively means a capturing session can't be muted via the tab controls? > ...
6 years, 3 months ago (2014-09-22 19:44:58 UTC) #9
DaleCurtis
Thanks for the replies, I see now. lgtm! Can't wait :)
6 years, 3 months ago (2014-09-22 19:59:50 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/586303004/60001
6 years, 3 months ago (2014-09-22 21:46:05 UTC) #12
commit-bot: I haz the power
Committed patchset #4 (id:60001) as f57c398540374f66464480e39546b955ec14caad
6 years, 3 months ago (2014-09-22 22:50:08 UTC) #13
commit-bot: I haz the power
6 years, 3 months ago (2014-09-22 22:50:44 UTC) #14
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/50f9789a2baac93fd5ac908ae01e4bec597955cf
Cr-Commit-Position: refs/heads/master@{#296078}

Powered by Google App Engine
This is Rietveld 408576698