WebContentsAudioMuter: Mute all audio output from a WebContentsImpl.

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}

[miu, 2014-09-20 22:48:58]

miu@chromium.org changed reviewers:
+ avi@chromium.org, dalecurtis@chromium.org

[miu, 2014-09-20 22:52:38]

avi: PTAL at web_contents.h and web_contents_impl.* changes.

dalecurtis: PTAL at content/browser/media/*, and feel free to comment on the
rest as well.

[Avi (use Gerrit), 2014-09-21 00:20:00]

The web contents stuff looks decent; some commentary.

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_muter.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
No "(c)" on new files.

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_muter.cc:28: // on the audio
thread.  Once that's fixed, this class will no longer be needed.
"Once that's fixed"... is there a bug to do that already? Link if there is, file
one and link if there isn't.

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
File content/browser/media/capture/web_contents_audio_muter.h (right):

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_muter.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
No "(c)" on new files.

https://codereview.chromium.org/586303004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/586303004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.h:70: class
WebContentsAudioMuter;
alphabetical order

https://codereview.chromium.org/586303004/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/586303004/diff/1/content/public/browser/web_c...
content/public/browser/web_contents.h:311: virtual void ToggleAudioMute(bool
mute) = 0;
"Toggle" means to switch between two settings (without taking a new setting
value as a parameter), so this isn't a correct use of the word. Perhaps
"SetAudioMuted(bool mute)" to match "IsAudioMuted"?

[miu, 2014-09-21 02:33:33]

Thanks for the quick turnaround, Avi.  Made changes per all your comments:

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_muter.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/09/21 00:20:00, Avi wrote:
> No "(c)" on new files.

Done.

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_muter.cc:28: // on the audio
thread.  Once that's fixed, this class will no longer be needed.
On 2014/09/21 00:20:00, Avi wrote:
> "Once that's fixed"... is there a bug to do that already? Link if there is,
file
> one and link if there isn't.

Done.

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
File content/browser/media/capture/web_contents_audio_muter.h (right):

https://codereview.chromium.org/586303004/diff/1/content/browser/media/captur...
content/browser/media/capture/web_contents_audio_muter.h:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
On 2014/09/21 00:20:00, Avi wrote:
> No "(c)" on new files.

Done.

https://codereview.chromium.org/586303004/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/586303004/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.h:70: class
WebContentsAudioMuter;
On 2014/09/21 00:20:00, Avi wrote:
> alphabetical order

Done.  (I had renamed the class while working the code, but forgot to change the
ordering here.)

https://codereview.chromium.org/586303004/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/586303004/diff/1/content/public/browser/web_c...
content/public/browser/web_contents.h:311: virtual void ToggleAudioMute(bool
mute) = 0;
On 2014/09/21 00:20:00, Avi wrote:
> "Toggle" means to switch between two settings (without taking a new setting
> value as a parameter), so this isn't a correct use of the word. Perhaps
> "SetAudioMuted(bool mute)" to match "IsAudioMuted"?

Done.

[Avi (use Gerrit), 2014-09-21 02:55:22]

non-media stuff lgtm with fix noted below

https://codereview.chromium.org/586303004/diff/20001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/586303004/diff/20001/content/content_browser....
content/content_browser.gypi:853:
'browser/media/capture/web_contents_audio_muter.h',
alphabetize

[miu, 2014-09-21 18:57:51]

https://codereview.chromium.org/586303004/diff/20001/content/content_browser....
File content/content_browser.gypi (right):

https://codereview.chromium.org/586303004/diff/20001/content/content_browser....
content/content_browser.gypi:853:
'browser/media/capture/web_contents_audio_muter.h',
On 2014/09/21 02:55:22, Avi wrote:
> alphabetize

Oh, duh.  Done.  And grepped the change to make sure there weren't any other bad
orderings.

[miu, 2014-09-21 20:46:50]

BTW--For anyone that's curious, here's the UI change that leverages the
WebContentsAudioMuter: https://codereview.chromium.org/591963002/

[DaleCurtis, 2014-09-22 17:45:45]

This effectively means a capturing session can't be muted via the tab controls? 
Also, upon end of capturing, the mute state is not returned. -- just pointing
out what may not have been obvious.

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
content/browser/media/capture/web_contents_audio_muter.cc:65: : public
base::RefCountedThreadSafe<MuteDestination>,
I think it's easier to reason about if you just use a WeakPtrFactory.

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
content/browser/media/capture/web_contents_audio_muter.cc:117: 
Extra line.

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
content/browser/media/capture/web_contents_audio_muter.cc:119: :
destination_(new MuteDestination(web_contents)), is_muting_(false) {
Stack allocate?

https://codereview.chromium.org/586303004/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/586303004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1003: if (!audio_muter_.get())
No need for .get() these days for a boolean test.

https://codereview.chromium.org/586303004/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1007:
DCHECK(audio_muter_.get());
Ditto.

https://codereview.chromium.org/586303004/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/586303004/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:310: virtual bool IsAudioMuted() const =
0;
I don't see this one used anywhere?

[miu, 2014-09-22 19:44:58]

> This effectively means a capturing session can't be muted via the tab
controls? 
> Also, upon end of capturing, the mute state is not returned. -- just pointing
> out what may not have been obvious.

Good points.  Both are intended behaviors.  Discussion with the UX team answered
these questions the same way: It's better not to give users a chance to become
confused, forgetting that they muted the tab, and thinking audio is broken. 
Regarding your first point, they might get confused about tab mirroring being
broken (no audio) if the tab was muted before starting capture.  Regarding your
second point, since they had audio while tab mirroring, we don't want them to
think audio's broken after tab capture ends.  Thankfully, these decisions have
made it sooooo much simpler to implement tab muting in libcontent.

Responded to all your comments, but didn't change code in all cases:

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
content/browser/media/capture/web_contents_audio_muter.cc:65: : public
base::RefCountedThreadSafe<MuteDestination>,
On 2014/09/22 17:45:45, DaleCurtis wrote:
> I think it's easier to reason about if you just use a WeakPtrFactory. 

There are tasks posted to two different threads that need to dereference the
pointer to WebContentsAudioMuter, so a WeakPtr won't be sufficient.

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
content/browser/media/capture/web_contents_audio_muter.cc:117: 
On 2014/09/22 17:45:45, DaleCurtis wrote:
> Extra line.

Done.

https://codereview.chromium.org/586303004/diff/40001/content/browser/media/ca...
content/browser/media/capture/web_contents_audio_muter.cc:119: :
destination_(new MuteDestination(web_contents)), is_muting_(false) {
On 2014/09/22 17:45:45, DaleCurtis wrote:
> Stack allocate?

Not sure what you mean.  If you're suggesting I make MuteDestination a member
via composition (rather than pointer-to-MuteDestination), I can't do that since
I would need to keep the WebContentsAudioMuter alive until the
Start/StopMirroring() tasks on the IO thread complete.  This would mean blocking
the UI thread on calls to the Start/StopMuting() methods.

https://codereview.chromium.org/586303004/diff/40001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/586303004/diff/40001/content/public/browser/w...
content/public/browser/web_contents.h:310: virtual bool IsAudioMuted() const =
0;
On 2014/09/22 17:45:45, DaleCurtis wrote:
> I don't see this one used anywhere?

It will be used by the UI code.  See https://codereview.chromium.org/591963002/.

[DaleCurtis, 2014-09-22 19:59:50]

Thanks for the replies, I see now.  lgtm!  Can't wait :)

[miu, 2014-09-22 21:44:55]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-09-22 21:46:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/586303004/60001

[commit-bot: I haz the power, 2014-09-22 22:50:08]

Committed patchset #4 (id:60001) as f57c398540374f66464480e39546b955ec14caad

[commit-bot: I haz the power, 2014-09-22 22:50:44]

Patchset 4 (id:??) landed as
https://crrev.com/50f9789a2baac93fd5ac908ae01e4bec597955cf
Cr-Commit-Position: refs/heads/master@{#296078}