Pretend the video has no audio track if it is playing as part of autoplay muted.

Pretend the video has no audio track if it is playing as part of autoplay muted.

This is to prevent autoplay muted videos to behave as audible videos because they
might never be audible as far as the user is concerned. These will avoid taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=sandersd@chromium.org,foolip@chromium.org,zqzhang@chromium.org

Committed: https://crrev.com/910111369092ad1c3432833491eae73b87a6c33d
Cr-Commit-Position: refs/heads/master@{#429859}

[mlamouri (slow - plz ping), 2016-08-17 14:23:57]

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

[commit-bot: I haz the power, 2016-08-17 14:24:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-08-17 14:29:43]

Would something like this fly in principle?

[commit-bot: I haz the power, 2016-08-17 14:34:32]

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

[commit-bot: I haz the power, 2016-08-17 14:34:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2016-08-17 16:51:25]

What's the intent here? I don't see any actual disable of the audio track
happening; does this actually prevent audio playout?

[mlamouri (slow - plz ping), 2016-08-18 09:59:45]

Description was changed from

==========
Pretent the video has no audio track if it is playing as part of autoplay muted.

BUG=638463
R=dalecurtis@chromium.org, sandersd@chromium.org
==========

to

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is a WIP to prevent autoplay muted videos to behave as audible videos
because
they might never be audible as far as the user is concerned. These will avoid
taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=dalecurtis@chromium.org, sandersd@chromium.org
==========

[mlamouri (slow - plz ping), 2016-08-18 10:01:03]

On 2016/08/17 at 16:51:25, dalecurtis wrote:
> What's the intent here? I don't see any actual disable of the audio track
happening; does this actually prevent audio playout?

Sorry, I put this together in a hurry, I put the bug link and thought it would
be enough. I've added a CL description. Also note that it's a draft/WIP for
feedback on the idea. It's a bit hacky so I want to make sure you guys are happy
with this before going deeper.

[sandersd (OOO until July 31), 2016-08-18 17:59:48]

On 2016/08/18 10:01:03, Mounir Lamouri wrote:
> On 2016/08/17 at 16:51:25, dalecurtis wrote:
> > What's the intent here? I don't see any actual disable of the audio track
> happening; does this actually prevent audio playout?
> 
> Sorry, I put this together in a hurry, I put the bug link and thought it would
> be enough. I've added a CL description. Also note that it's a draft/WIP for
> feedback on the idea. It's a bit hacky so I want to make sure you guys are
happy
> with this before going deeper.

We have a general problem with WebMediaPlayer that the play()/pause() states
coming from HTMLMediaElement are not very clear. Plus we have the 'defer load'
infrastructure in parallel.

Now may be a good time to look at this and try to improve things. Specifically,
WMPI wants to know:
  - Whether the element is playing or paused, *before* HAVE_METADATA.
  - Based on this CL, also *why* a play was requested. (Autoplay, user gesture,
script)

From that, WMPI should be able to implement all the rest of what you want in
ComputePlayState().

I'm not very excited about trying to change the semantics of play()/pause(), so
I'm only really suggesting adding metadata (or accessor methods like you have
done, *but* we also need state change notifications for them).

[mlamouri (slow - plz ping), 2016-08-19 14:28:18]

On 2016/08/18 at 17:59:48, sandersd wrote:
> On 2016/08/18 10:01:03, Mounir Lamouri wrote:
> > On 2016/08/17 at 16:51:25, dalecurtis wrote:
> > > What's the intent here? I don't see any actual disable of the audio track
> > happening; does this actually prevent audio playout?
> > 
> > Sorry, I put this together in a hurry, I put the bug link and thought it
would
> > be enough. I've added a CL description. Also note that it's a draft/WIP for
> > feedback on the idea. It's a bit hacky so I want to make sure you guys are
happy
> > with this before going deeper.
> 
> We have a general problem with WebMediaPlayer that the play()/pause() states
coming from HTMLMediaElement are not very clear. Plus we have the 'defer load'
infrastructure in parallel.
> 
> Now may be a good time to look at this and try to improve things.
Specifically, WMPI wants to know:
>   - Whether the element is playing or paused, *before* HAVE_METADATA.

Do you mean whether `play()` was called in JS? The element can't be playing
before "HAVE_METADATA", can it?

>   - Based on this CL, also *why* a play was requested. (Autoplay, user
gesture, script)

This CL doesn't really need that much precision. Something might be autoplaying
from script. Here, we want to know if something is playing muted before the user
interacted which can be interpreted as "autoplaying while muted". I wouldn't be
in favour of adding states that we wouldn't use or even know what to do.

> From that, WMPI should be able to implement all the rest of what you want in
ComputePlayState().
> 
> I'm not very excited about trying to change the semantics of play()/pause(),
so I'm only really suggesting adding metadata (or accessor methods like you have
done, *but* we also need state change notifications for them).

Hmm, I guess I could call ::UpdatePlayState() when ::setVolume() is called which
should make this change more in the spirit of computing states in
ComputePlayState().

[sandersd (OOO until July 31), 2016-08-19 20:57:27]

> Do you mean whether `play()` was called in JS? The element can't be playing
> before "HAVE_METADATA", can it?

I mean whether HTMLMediaElement will call play() when we report HAVE_METADATA.

HTMLMediaElement has a playing state that is rather more complex than
WebMediaPlayer ever finds out about, but we would like to change that.

> This CL doesn't really need that much precision. Something might be
autoplaying
> from script [...] I wouldn't be
> in favour of adding states that we wouldn't use or even know what to do.

I agree. I'm trying to lay out the design space we would like to cover; from
that we should only implement the parts that are currently needed.

> Hmm, I guess I could call ::UpdatePlayState() when ::setVolume() is called
which
> should make this change more in the spirit of computing states in
> ComputePlayState().

That will be a requirement for me to approve the actual code portion of this CL
;-)

[mlamouri (slow - plz ping), 2016-10-04 21:15:16]

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

[commit-bot: I haz the power, 2016-10-04 21:16:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-10-04 21:16:29]

Description was changed from

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is a WIP to prevent autoplay muted videos to behave as audible videos
because
they might never be audible as far as the user is concerned. These will avoid
taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=dalecurtis@chromium.org, sandersd@chromium.org
==========

to

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is to prevent autoplay muted videos to behave as audible videos because
they
might never be audible as far as the user is concerned. These will avoid taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=sandersd@chromium.org
==========

[mlamouri (slow - plz ping), 2016-10-04 21:16:29]

mlamouri@chromium.org changed reviewers:
- dalecurtis@chromium.org

[mlamouri (slow - plz ping), 2016-10-04 21:17:13]

mlamouri@chromium.org changed reviewers:
+ foolip@chromium.org

[mlamouri (slow - plz ping), 2016-10-04 21:17:13]

PTAL

[commit-bot: I haz the power, 2016-10-04 21:56:59]

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

[commit-bot: I haz the power, 2016-10-04 21:57:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sandersd (OOO until July 31), 2016-10-04 23:53:04]

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl_unittest.cc (right):

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:665: TEST_F(WebMediaPlayerImplTest,
SetVolumeCallsDidPlay) {
We don't have tests that all the other triggers result in play state
computation.

I assume you are more worried about this one since the link is indirect. In that
case, a comment above the UpdatePlayState() call would be more effective. (And
will help more with the inevitable refactors of the code.)

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:683: EXPECT_CALL(delegate_,
DidPlay(_, true, true, false, _));
Move these right before the respective SetDelegateState() calls?

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:688:
SetDelegateState(state.delegate_state);
This seems to be testing two different things. Probably you should be calling
ComputePlayState() and only worrying about the DidPlay() result?

[mlamouri (slow - plz ping), 2016-10-05 16:19:49]

Dan, I think this will not work because multiple calls to DidPlay() break some
sanity checks. We could remove them. Otherwise, we need to make the delegate
aware of the state. Are you sure you don't want to give a shot at adding an
"AUTOPLAYING_MUTED" state? :)

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl_unittest.cc (right):

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:665: TEST_F(WebMediaPlayerImplTest,
SetVolumeCallsDidPlay) {
On 2016/10/04 at 23:53:04, sandersd wrote:
> We don't have tests that all the other triggers result in play state
computation.
> 
> I assume you are more worried about this one since the link is indirect. In
that case, a comment above the UpdatePlayState() call would be more effective.
(And will help more with the inevitable refactors of the code.)

Added the comment and removed the test.

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:683: EXPECT_CALL(delegate_,
DidPlay(_, true, true, false, _));
On 2016/10/04 at 23:53:04, sandersd wrote:
> Move these right before the respective SetDelegateState() calls?

Done.

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:688:
SetDelegateState(state.delegate_state);
On 2016/10/04 at 23:53:04, sandersd wrote:
> This seems to be testing two different things. Probably you should be calling
ComputePlayState() and only worrying about the DidPlay() result?

I'm not sure I'm following this. ComputePlayState() doesn't call DidPlay() but
SetDelegateState() does. I can as well pass "PLAYING" to SetDelegateState to
reduce the overhead.

[mlamouri (slow - plz ping), 2016-10-05 16:19:53]

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

[commit-bot: I haz the power, 2016-10-05 16:20:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[foolip, 2016-10-05 16:56:56]

Does WebMediaPlayer need to know *why* it's muted, shouldn't simply being muted
be a sufficient cue to skip media notifications?

[sandersd (OOO until July 31), 2016-10-05 17:38:18]

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl_unittest.cc (right):

https://codereview.chromium.org/2251073003/diff/70001/media/blink/webmediapla...
media/blink/webmediaplayer_impl_unittest.cc:688:
SetDelegateState(state.delegate_state);
On 2016/10/05 16:19:49, mlamouri wrote:
> On 2016/10/04 at 23:53:04, sandersd wrote:
> > This seems to be testing two different things. Probably you should be
calling
> ComputePlayState() and only worrying about the DidPlay() result?
> 
> I'm not sure I'm following this. ComputePlayState() doesn't call DidPlay() but
> SetDelegateState() does. I can as well pass "PLAYING" to SetDelegateState to
> reduce the overhead.

Sorry, I should have said UpdatePlayState().

[mlamouri (slow - plz ping), 2016-10-05 20:08:54]

On 2016/10/05 at 16:56:56, foolip wrote:
> Does WebMediaPlayer need to know *why* it's muted, shouldn't simply being
muted be a sufficient cue to skip media notifications?

Maybe. This is something we are considering but I don't think we should do this
as a quick fix for the issue.

[mlamouri (slow - plz ping), 2016-10-05 20:09:25]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 22:24:07]

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

[commit-bot: I haz the power, 2016-10-05 22:24:07]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-10-06 10:46:52]

On 2016/10/05 20:08:54, mlamouri wrote:
> On 2016/10/05 at 16:56:56, foolip wrote:
> > Does WebMediaPlayer need to know *why* it's muted, shouldn't simply being
> muted be a sufficient cue to skip media notifications?
> 
> Maybe. This is something we are considering but I don't think we should do
this
> as a quick fix for the issue.

Does it need to be quick, is this something that needs to be merged to a release
branch soon?

This mostly affects WebMediaPlayerImpl so if sandersd@ is happy I can rs, but I
would like more separate between HTMLMediaElement "glue" concepts like
autoplaying and looping, and WebMediaPlayer.

[mlamouri (slow - plz ping), 2016-10-07 13:45:53]

On 2016/10/06 at 10:46:52, foolip wrote:
> On 2016/10/05 20:08:54, mlamouri wrote:
> > On 2016/10/05 at 16:56:56, foolip wrote:
> > > Does WebMediaPlayer need to know *why* it's muted, shouldn't simply being
> > muted be a sufficient cue to skip media notifications?
> > 
> > Maybe. This is something we are considering but I don't think we should do
this
> > as a quick fix for the issue.
> 
> Does it need to be quick, is this something that needs to be merged to a
release branch soon?
> 
> This mostly affects WebMediaPlayerImpl so if sandersd@ is happy I can rs, but
I would like more separate between HTMLMediaElement "glue" concepts like
autoplaying and looping, and WebMediaPlayer.

I think this will be merged to the Beta channel (M55). It is a fairly bad user
experience and came up multiple times on Twitter, Stack Overflow and internally
(at Google).

[foolip, 2016-10-10 11:33:07]

mlamouri@, can you elaborate a bit on the risks of simply using muted as the
signal here? From a distance, it looks like it'd be simpler code-wise, simpler
to explain to web developers wondering when the notification is shown, and I'm
wondering what might break.

If there is a small risk that'd be acceptable but not for M55, then doing first
this change, merging that, and then doing the simpler fix, might be an option.

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2310:
scheduleEvent(EventTypeNames::volumechange);
This will change the order of events from volumechange, pause to pause,
volumechange. I guess there's no test coverage for this, can you preserve the
behavior and file a follow-up to test
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-muted?

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3026: return muted() &&
isLockedPendingUserGesture();
This will return true even if not actually playing, intentional?

[mlamouri (slow - plz ping), 2016-10-31 15:27:42]

Decision was made that we will not always hide the notification when something
is muted. However, with might do for muted stream when the page is backgrounded
because in this case the notification would have no effect and would be
confusing. Though, we don't plan to do this soon (maybe Q1) and I think we will
still need tho autoplayMuted signal.

foolip@, sanderds@, would you mind taking another look?

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2310:
scheduleEvent(EventTypeNames::volumechange);
On 2016/10/10 at 11:33:07, foolip wrote:
> This will change the order of events from volumechange, pause to pause,
volumechange. I guess there's no test coverage for this, can you preserve the
behavior and file a follow-up to test
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-muted?

Just to clarify, do you want me to revert the change or keep this change and
update the spec? My understanding is that you want to revert the change. In
which case, I will have to replace this with some ugly work arounds. Is this
okay?

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3026: return muted() &&
isLockedPendingUserGesture();
On 2016/10/10 at 11:33:07, foolip wrote:
> This will return true even if not actually playing, intentional?

That was on purpose but I've added a check to make it less confusing.

[mlamouri (slow - plz ping), 2016-10-31 17:29:21]

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

[commit-bot: I haz the power, 2016-10-31 17:29:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 19:13:58]

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

[commit-bot: I haz the power, 2016-10-31 19:13:59]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-10-31 21:02:35]

WMPI change LGTM.

[foolip, 2016-11-01 16:12:21]

lgtm % event order

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2251073003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2310:
scheduleEvent(EventTypeNames::volumechange);
On 2016/10/31 15:27:41, mlamouri wrote:
> On 2016/10/10 at 11:33:07, foolip wrote:
> > This will change the order of events from volumechange, pause to pause,
> volumechange. I guess there's no test coverage for this, can you preserve the
> behavior and file a follow-up to test
> https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-muted?
> 
> Just to clarify, do you want me to revert the change or keep this change and
> update the spec? My understanding is that you want to revert the change. In
> which case, I will have to replace this with some ugly work arounds. Is this
> okay?

Maintaining the event order seems best, yeah.

[mlamouri (slow - plz ping), 2016-11-03 14:51:30]

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

[commit-bot: I haz the power, 2016-11-03 14:51:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-11-03 14:55:20]

mlamouri@chromium.org changed reviewers:
+ zqzhang@chromium.org

[mlamouri (slow - plz ping), 2016-11-03 14:55:21]

The event order is back to normal. Only WMPI is notified after the video is
paused. I've added a comment.

I've also added integration tests to make sure we don't regress this in a way
that WMPI/MediaElement wouldn't be aware of.

zqzhang@, can you PTAL at the Java test? :)

[commit-bot: I haz the power, 2016-11-03 16:16:13]

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

[commit-bot: I haz the power, 2016-11-03 16:16:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhiqiang Zhang (Slow), 2016-11-03 16:41:09]

lgtm % nits

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java
(right):

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:33:
return (AudioManager) getActivity().getApplicationContext().getSystemService(
Some thoughts: we can avoid this if we can further abstract the
AudioFocusManager to a unified interface shared by different platforms.

Ditto the MockAudioFocusChangeListener.

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:97:
assertEquals(AudioManager.AUDIOFOCUS_GAIN,
mAudioFocusChangeListener.getAudioFocusState());
nit: I forgot whether focus loss is sync or not, but maybe we should wait for 1
second and check something does not happen (as we do in other tests)?

If we can mock AudioManager, then it will be easier.

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:114:
assertFalse(DOMUtils.isMediaPaused(tab.getWebContents(), VIDEO_ID));
ditto

[mlamouri (slow - plz ping), 2016-11-03 18:19:46]

Description was changed from

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is to prevent autoplay muted videos to behave as audible videos because
they
might never be audible as far as the user is concerned. These will avoid taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=sandersd@chromium.org
==========

to

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is to prevent autoplay muted videos to behave as audible videos because
they
might never be audible as far as the user is concerned. These will avoid taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=sandersd@chromium.org,foolip@chromium.org,zqzhang@chromium.org
==========

[mlamouri (slow - plz ping), 2016-11-03 18:20:06]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-11-03 18:20:06]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
sandersd@chromium.org, zqzhang@chromium.org
Link to the patchset: https://codereview.chromium.org/2251073003/#ps170001
(title: "review comments")

[commit-bot: I haz the power, 2016-11-03 18:20:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-11-03 18:23:39]

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java
(right):

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:33:
return (AudioManager) getActivity().getApplicationContext().getSystemService(
On 2016/11/03 at 16:41:09, Zhiqiang Zhang wrote:
> Some thoughts: we can avoid this if we can further abstract the
AudioFocusManager to a unified interface shared by different platforms.
> 
> Ditto the MockAudioFocusChangeListener.

I agree but I have no idea how. We did the same for MediaSessionTest.java that
relies heavily on the AudioFocusManager. If you have thought how to do this,
could you file a bug with the ideas? :)

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:97:
assertEquals(AudioManager.AUDIOFOCUS_GAIN,
mAudioFocusChangeListener.getAudioFocusState());
On 2016/11/03 at 16:41:09, Zhiqiang Zhang wrote:
> nit: I forgot whether focus loss is sync or not, but maybe we should wait for
1 second and check something does not happen (as we do in other tests)?
> 
> If we can mock AudioManager, then it will be easier.

Added 500ms sleep like we do in MediaSessionTest.java

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:114:
assertFalse(DOMUtils.isMediaPaused(tab.getWebContents(), VIDEO_ID));
On 2016/11/03 at 16:41:09, Zhiqiang Zhang wrote:
> ditto

ditto

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:146:

Here too :)

[commit-bot: I haz the power, 2016-11-03 19:30:02]

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

[commit-bot: I haz the power, 2016-11-03 19:30:04]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mlamouri (slow - plz ping), 2016-11-03 21:38:20]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2016-11-03 21:39:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 02:17:54]

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

[commit-bot: I haz the power, 2016-11-04 02:17:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mlamouri (slow - plz ping), 2016-11-04 09:32:32]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2016-11-04 09:32:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 11:28:53]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2016-11-04 11:30:58]

Description was changed from

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is to prevent autoplay muted videos to behave as audible videos because
they
might never be audible as far as the user is concerned. These will avoid taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=sandersd@chromium.org,foolip@chromium.org,zqzhang@chromium.org
==========

to

==========
Pretend the video has no audio track if it is playing as part of autoplay muted.

This is to prevent autoplay muted videos to behave as audible videos because
they
might never be audible as far as the user is concerned. These will avoid taking
an audio power blocker for them and also prevent showing a media notification.

BUG=638463
R=sandersd@chromium.org,foolip@chromium.org,zqzhang@chromium.org

Committed: https://crrev.com/910111369092ad1c3432833491eae73b87a6c33d
Cr-Commit-Position: refs/heads/master@{#429859}
==========

[commit-bot: I haz the power, 2016-11-04 11:31:02]

Patchset 10 (id:??) landed as
https://crrev.com/910111369092ad1c3432833491eae73b87a6c33d
Cr-Commit-Position: refs/heads/master@{#429859}

[Zhiqiang Zhang (Slow), 2016-11-04 14:17:56]

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java
(right):

https://codereview.chromium.org/2251073003/diff/150001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/media/ui/AutoplayMutedNotificationTest.java:33:
return (AudioManager) getActivity().getApplicationContext().getSystemService(
On 2016/11/03 18:23:39, mlamouri wrote:
> On 2016/11/03 at 16:41:09, Zhiqiang Zhang wrote:
> > Some thoughts: we can avoid this if we can further abstract the
> AudioFocusManager to a unified interface shared by different platforms.
> > 
> > Ditto the MockAudioFocusChangeListener.
> 
> I agree but I have no idea how. We did the same for MediaSessionTest.java that
> relies heavily on the AudioFocusManager. If you have thought how to do this,
> could you file a bug with the ideas? :)

There's already one, https://crbug.com/651069 :)