Plumb audio muting state to guest web-contents.

Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing
when it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
Committed: https://crrev.com/2f797c780f8638733e71c1a3da185000939cfbe7
Cr-Commit-Position: refs/heads/master@{#368073}

[wjmaclean, 2016-01-05 20:53:19]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-05 20:54:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1554233004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1554233004/1

[commit-bot: I haz the power, 2016-01-05 21:55:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-05 21:55:35]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-01-06 16:25:09]

Description was changed from

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio playing or
not playing when it should/shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

to

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing when
it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

[wjmaclean, 2016-01-06 16:25:25]

Description was changed from

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing when
it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

to

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing
when it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

[wjmaclean, 2016-01-06 16:26:06]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org

[wjmaclean, 2016-01-06 16:26:43]

creis@, can you please take a look? It's mostly plumbing plus some short tests.

[wjmaclean, 2016-01-06 16:26:54]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-06 16:27:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1554233004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1554233004/20001

[commit-bot: I haz the power, 2016-01-06 17:37:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-06 17:37:11]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-01-06 18:28:10]

On 2016/01/06 16:26:43, wjmaclean wrote:
> creis@, can you please take a look? It's mostly plumbing plus some short
tests.

creis@ ... please hold off reviewing ... I'm going to look into remodelling this
CL based on WebContentsObserver interacting with GuestViewBase.

[wjmaclean, 2016-01-06 21:43:38]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-06 21:44:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1554233004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1554233004/40001

[wjmaclean, 2016-01-06 21:46:22]

creis@ - I've re-designed this using WebContentsObserver ... ptal?

[Charlie Reis, 2016-01-06 22:13:51]

Thanks.  I was happy to find that OOPIFs appear to work properly here already--
muting a tab also mutes its OOPIFs.  (I guess the logic is tied more to the
WebContents than the process.)

Mostly looks good; a few nits and questions below.

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:787:
EXPECT_FALSE(guest->IsAudioMuted());
Is it possible to mute a guest without muting the embedder?  Just wondering if
we should test what happens in that case (and if so, what happens if the
embedder is then muted and unmuted).

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:807: if
(!guest->IsAudioMuted() && !observer.muting_update_observed())
observer.muting_update_observed() can't be true here, can it?  We just created
the observer and it hasn't gotten a chance to observe anything.

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:808: loop_runner->Run();
nit: Most of the test observer classes I'm familiar with (e.g.,
TestNavigationObserver) keep the MessageLoopRunner internally and have a Wait
method to expose it.  Maybe that would be a bit cleaner here?

https://codereview.chromium.org/1554233004/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/app_creates_webview/manifest.json
(right):

https://codereview.chromium.org/1554233004/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/app_creates_webview/manifest.json:2:
"name": "<webview> test that WebContents setAudioMuting propagates to guest.",
This test app doesn't seem specific to muting.  It's just a case that creates a
webview on command, right?

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

https://codereview.chromium.org/1554233004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1123: // If we have guests,
they should have the same audio muting state.
If we use WebContentsObserver for this, it's general and we should omit the
comment (in case it gets stale).

On the flip side, if we just need to notify the guest WebContents(es), could we
iterate over inner_web_contents_tree_nodes_?  Or is that only populated in
--use-cross-process-frames-for-guests?

[wjmaclean, 2016-01-06 22:36:09]

Please see replies below ... I'm preparing a revised CL based on your comments,
but if any of my replies seem misguided ping me directly ...

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:787:
EXPECT_FALSE(guest->IsAudioMuted());
On 2016/01/06 22:13:51, Charlie Reis wrote:
> Is it possible to mute a guest without muting the embedder?  Just wondering if
> we should test what happens in that case (and if so, what happens if the
> embedder is then muted and unmuted).

At present there is no pathway to mute a guest directly, i.e. the only pathway
involves muting the embedder.

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:807: if
(!guest->IsAudioMuted() && !observer.muting_update_observed())
On 2016/01/06 22:13:51, Charlie Reis wrote:
> observer.muting_update_observed() can't be true here, can it?  We just created
> the observer and it hasn't gotten a chance to observe anything.

I suspect you're right, but I left the check in for safety. I can remove it if
you prefer.

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:808: loop_runner->Run();
On 2016/01/06 22:13:51, Charlie Reis wrote:
> nit: Most of the test observer classes I'm familiar with (e.g.,
> TestNavigationObserver) keep the MessageLoopRunner internally and have a Wait
> method to expose it.  Maybe that would be a bit cleaner here?

Sure, I can do that.

https://codereview.chromium.org/1554233004/diff/40001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/app_creates_webview/manifest.json
(right):

https://codereview.chromium.org/1554233004/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/app_creates_webview/manifest.json:2:
"name": "<webview> test that WebContents setAudioMuting propagates to guest.",
On 2016/01/06 22:13:51, Charlie Reis wrote:
> This test app doesn't seem specific to muting.  It's just a case that creates
a
> webview on command, right?

True. I'll rename this to indicate it's just an app that creates a WebView on
demand.

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

https://codereview.chromium.org/1554233004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1123: // If we have guests,
they should have the same audio muting state.
On 2016/01/06 22:13:51, Charlie Reis wrote:
> If we use WebContentsObserver for this, it's general and we should omit the
> comment (in case it gets stale).
> 
> On the flip side, if we just need to notify the guest WebContents(es), could
we
> iterate over inner_web_contents_tree_nodes_?  Or is that only populated in
> --use-cross-process-frames-for-guests?

I included the comment to explain who the (typical?) users of this notification
might be, but we can omit it. According to lfg@ inner_web_contents_tree_nodes_
is purely a WebView-over-OOPIF (Woo?) concept.

At present guests *are* the only consumers of this event, but that could
presumably change.

[wjmaclean, 2016-01-06 23:15:45]

Revised CL uploaded ... ptal?

[Charlie Reis, 2016-01-07 00:11:03]

Thanks, LGTM.

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/1554233004/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:787:
EXPECT_FALSE(guest->IsAudioMuted());
On 2016/01/06 22:36:09, wjmaclean wrote:
> On 2016/01/06 22:13:51, Charlie Reis wrote:
> > Is it possible to mute a guest without muting the embedder?  Just wondering
if
> > we should test what happens in that case (and if so, what happens if the
> > embedder is then muted and unmuted).
> 
> At present there is no pathway to mute a guest directly, i.e. the only pathway
> involves muting the embedder.

Acknowledged.

[wjmaclean, 2016-01-07 13:42:51]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-01-07 13:42:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1554233004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1554233004/80001

[commit-bot: I haz the power, 2016-01-07 14:52:10]

Description was changed from

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing
when it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

to

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing
when it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

[commit-bot: I haz the power, 2016-01-07 14:52:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-07 14:53:02]

Description was changed from

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing
when it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070
==========

to

==========
Plumb audio muting state to guest web-contents.

Currently, if a WebContents changes its muting state, this change is not
shared with any guests it may have. This can lead to audio not playing
when it should, or playing when it shouldn't.

This CL adds the necessary plumbing so that guest WebContents have the
same muting state as their embedder WebContents.

BUG=567070

Committed: https://crrev.com/2f797c780f8638733e71c1a3da185000939cfbe7
Cr-Commit-Position: refs/heads/master@{#368073}
==========

[commit-bot: I haz the power, 2016-01-07 14:53:03]

Patchset 5 (id:??) landed as
https://crrev.com/2f797c780f8638733e71c1a3da185000939cfbe7
Cr-Commit-Position: refs/heads/master@{#368073}