Add media controls in media notification

Add media controls in media notification

This CL enables media controls in the media notification. Currently,
only switch track is enabled.

BUG=656563

[Zhiqiang Zhang (Slow), 2016-10-24 12:15:08]

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

[Zhiqiang Zhang (Slow), 2016-10-24 17:24:49]

Patchset #3 (id:40001) has been deleted

[whywhat, 2016-10-24 20:54:01]

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
I think we're still discouraged from using enums in Java. See how mActions is
done.
Should there be some default value for the media session actions?
Could this be merged with mActions even?

I wonder if the website should be able to disable play/pause button. It's not
possible with the current API.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:267:
this.mediaSessionActions = mediaSessionActions;
Are EnumSet-s immutable? If not, you might get into trouble with updating the
mediaSessionAction objects passed to the Builder as its changes will reflect on
MediaNotificationInfo (since you're just passing a pointer around).
You probably want to clone() the EnumSet here or in Builder even if you won't
use the int.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationListener.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationListener.java:47:
* Called when the user initiated a MediaSessionAction.
nit: a MediaSessionAction doesn't explain anything (and it's not even a {@link
}).
Something like "Called when user performed one of the supported media actions
(like fast forward or next track)" sounds better IMHO.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:710:
int numActions = 0;
nit: maybe extract actions/style setup as a function?

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:721:
++numActions;
nit: this seems redundant as it will increment numActions in both cases. Maybe
initialize it with a constant like MIN_NUMBER_OF_ACTIONS = 1; with a comment
saying play or pause button is always present?

https://codereview.chromium.org/2441403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2441403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:152:
UserGestureIndicator gestureIndicator(UserGestureToken::create());
Have you checked this actually sets the user gesture for the time when the event
is processed? I'd assume dispatchEvent to work asynchronously and
UserGestureIndicator not surviving that.

[mlamouri (slow - plz ping), 2016-10-25 10:38:13]

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:714:
builder.addAction(R.drawable.ic_vidcontrol_play, mPlayDescription,
mPreviousTrackDescription

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:730:
builder.addAction(R.drawable.ic_vidcontrol_play, mPlayDescription,
mNextTrackDescription

https://codereview.chromium.org/2441403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2441403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:152:
UserGestureIndicator gestureIndicator(UserGestureToken::create());
On 2016/10/24 at 20:54:01, whywhat wrote:
> Have you checked this actually sets the user gesture for the time when the
event is processed? I'd assume dispatchEvent to work asynchronously and
UserGestureIndicator not surviving that.

I think dispatchEvent() is sync, see EventTarget::dispatchEvent()

[Zhiqiang Zhang (Slow), 2016-10-25 12:45:35]

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

[Zhiqiang Zhang (Slow), 2016-10-25 12:45:36]

PTAL (Moved mlamouri to CC list).

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/24 20:54:01, whywhat wrote:
> I think we're still discouraged from using enums in Java. See how mActions is
> done.
I think it should be fine for this case since the native MediaSessionAction is
also an enum (see ContenttSetting.java). Also, as these actions could
activated/deactivated, EnumSet should be a better data structure than List<> or
Set<>

> Should there be some default value for the media session actions?
> Could this be merged with mActions even?

The EnumSet reflects the actions enabled in blink::MediaSession, I think we
should keep a copy of this. As we are handling play/pause ourselves, we could
just ignore the play/pause enums in this set.

It's a bit hard to merge MediaSessionAction with the existing actions in
MediaNotificationInfo for now. We can do the merge if we have future plans on
when to handle play/pause in the UA and when to handle them in the page.

> I wonder if the website should be able to disable play/pause button. It's not
> possible with the current API.

In the API we say the UA can ignore some controls and do its own. For now, we
could handle play/pause ourselves.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:267:
this.mediaSessionActions = mediaSessionActions;
On 2016/10/24 20:54:01, whywhat wrote:
> Are EnumSet-s immutable? If not, you might get into trouble with updating the
> mediaSessionAction objects passed to the Builder as its changes will reflect
on
> MediaNotificationInfo (since you're just passing a pointer around).
> You probably want to clone() the EnumSet here or in Builder even if you won't
> use the int.

Hmm, we have the same problem with metadata and largeIcon. I did have seen the
issue you described, but since cloning MediaMetadata and Bitmap costs much CPU
and memory, there's a reason to only keep the reference.

I think we could do clone() for EnumSet since it's relatively cheap.
Done.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationListener.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationListener.java:47:
* Called when the user initiated a MediaSessionAction.
On 2016/10/24 20:54:01, whywhat wrote:
> nit: a MediaSessionAction doesn't explain anything (and it's not even a {@link
> }).
> Something like "Called when user performed one of the supported media actions
> (like fast forward or next track)" sounds better IMHO.

Done.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:710:
int numActions = 0;
On 2016/10/24 20:54:01, whywhat wrote:
> nit: maybe extract actions/style setup as a function?

Done.

BTW, I think we need to remove the word "MediaStyle" from
"setMediaStyleLayoutForNotificationBuilder()" and
"setMediaStyleNotificationText()" in a follow-up since we only have one style
now.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:714:
builder.addAction(R.drawable.ic_vidcontrol_play, mPlayDescription,
On 2016/10/25 10:38:13, mlamouri wrote:
> mPreviousTrackDescription

Done.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:721:
++numActions;
On 2016/10/24 20:54:01, whywhat wrote:
> nit: this seems redundant as it will increment numActions in both cases. Maybe
> initialize it with a constant like MIN_NUMBER_OF_ACTIONS = 1; with a comment
> saying play or pause button is always present?

Hmm, I think we should the pattern consistent (increment numAction once an
action is added). It's a bit odd to have numAction==2 at line#717. I can move
++numActions out since they are in both branches.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:730:
builder.addAction(R.drawable.ic_vidcontrol_play, mPlayDescription,
On 2016/10/25 10:38:13, mlamouri wrote:
> mNextTrackDescription

Done.

[whywhat, 2016-10-25 15:39:37]

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/25 at 12:45:35, Zhiqiang Zhang wrote:
> On 2016/10/24 20:54:01, whywhat wrote:
> > I think we're still discouraged from using enums in Java. See how mActions
is
> > done.
> I think it should be fine for this case since the native MediaSessionAction is
also an enum (see ContenttSetting.java). Also, as these actions could
activated/deactivated, EnumSet should be a better data structure than List<> or
Set<>

No, let's use IntDef.
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...

See
https://groups.google.com/a/google.com/d/topic/clank-team/5aEW4J9dc5c/discussion
for one of the recent discussions on enums in clank.

> 
> > Should there be some default value for the media session actions?
> > Could this be merged with mActions even?
> 
> The EnumSet reflects the actions enabled in blink::MediaSession, I think we
should keep a copy of this. As we are handling play/pause ourselves, we could
just ignore the play/pause enums in this set.

It feels like the spec is too vague on this.

> 
> It's a bit hard to merge MediaSessionAction with the existing actions in
MediaNotificationInfo for now. We can do the merge if we have future plans on
when to handle play/pause in the UA and when to handle them in the page.
> 
> > I wonder if the website should be able to disable play/pause button. It's
not
> > possible with the current API.
> 
> In the API we say the UA can ignore some controls and do its own. For now, we
could handle play/pause ourselves.

Ok, but I hope there's a plan to fix this uncertainty :)

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:267:
this.mediaSessionActions = mediaSessionActions;
On 2016/10/25 at 12:45:35, Zhiqiang Zhang wrote:
> On 2016/10/24 20:54:01, whywhat wrote:
> > Are EnumSet-s immutable? If not, you might get into trouble with updating
the
> > mediaSessionAction objects passed to the Builder as its changes will reflect
on
> > MediaNotificationInfo (since you're just passing a pointer around).
> > You probably want to clone() the EnumSet here or in Builder even if you
won't
> > use the int.
> 
> Hmm, we have the same problem with metadata and largeIcon. I did have seen the
issue you described, but since cloning MediaMetadata and Bitmap costs much CPU
and memory, there's a reason to only keep the reference.
> 
> I think we could do clone() for EnumSet since it's relatively cheap.
> Done.

MediaMetadata doesn't sound expensive to copy too. Bitmap maybe, but I believe
Bitmap must be immutable anyway.

[Zhiqiang Zhang (Slow), 2016-10-25 18:54:23]

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/25 15:39:37, whywhat wrote:
> No, let's use IntDef.
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> 
> See
>
https://groups.google.com/a/google.com/d/topic/clank-team/5aEW4J9dc5c/discussion
> for one of the recent discussions on enums in clank.

Thanks, let's just use int. I tried IntDef but there's no compile-time error
even if I pass some crazy value :(

Also HashSet<Integer> should be OK since its initial capacity is 16 and we are
unlikely to have too many HashSets.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:267:
this.mediaSessionActions = mediaSessionActions;
On 2016/10/25 15:39:37, whywhat wrote:
> On 2016/10/25 at 12:45:35, Zhiqiang Zhang wrote:
> > On 2016/10/24 20:54:01, whywhat wrote:
> > > Are EnumSet-s immutable? If not, you might get into trouble with updating
> the
> > > mediaSessionAction objects passed to the Builder as its changes will
reflect
> on
> > > MediaNotificationInfo (since you're just passing a pointer around).
> > > You probably want to clone() the EnumSet here or in Builder even if you
> won't
> > > use the int.
> > 
> > Hmm, we have the same problem with metadata and largeIcon. I did have seen
the
> issue you described, but since cloning MediaMetadata and Bitmap costs much CPU
> and memory, there's a reason to only keep the reference.
> > 
> > I think we could do clone() for EnumSet since it's relatively cheap.
> > Done.
> 
> MediaMetadata doesn't sound expensive to copy too. Bitmap maybe, but I believe
> Bitmap must be immutable anyway.

Maybe we should fix the metadata clone in a follow-up.
Done for the actions.

[whywhat, 2016-10-25 19:19:37]

@IntDef's should be failing some Android lint build step.

I've also heard Peter (peconn) implemented some Java enum generating magic -
maybe we could reuse it?

Other than that and ArraySet comment, lgtm.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/25 at 18:54:23, Zhiqiang Zhang wrote:
> On 2016/10/25 15:39:37, whywhat wrote:
> > No, let's use IntDef.
> >
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> > 
> > See
> >
https://groups.google.com/a/google.com/d/topic/clank-team/5aEW4J9dc5c/discussion
> > for one of the recent discussions on enums in clank.
> 
> Thanks, let's just use int. I tried IntDef but there's no compile-time error
even if I pass some crazy value :(
> 
> Also HashSet<Integer> should be OK since its initial capacity is 16 and we are
unlikely to have too many HashSets.

ArraySet is your friend.

[Zhiqiang Zhang (Slow), 2016-10-28 14:18:01]

Rebased onto the MediaSession refactoring patch. The new diff should be trivial
and no need to review I think.

Using new assets for previous/next track.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/25 19:19:37, whywhat wrote:
> On 2016/10/25 at 18:54:23, Zhiqiang Zhang wrote:
> > On 2016/10/25 15:39:37, whywhat wrote:
> > > No, let's use IntDef.
> > >
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> > > 
> > > See
> > >
>
https://groups.google.com/a/google.com/d/topic/clank-team/5aEW4J9dc5c/discussion
> > > for one of the recent discussions on enums in clank.
> > 
> > Thanks, let's just use int. I tried IntDef but there's no compile-time error
> even if I pass some crazy value :(
> > 
> > Also HashSet<Integer> should be OK since its initial capacity is 16 and we
are
> unlikely to have too many HashSets.
> 
> ArraySet is your friend.

Just found it's API level 23 :(

[PEConn, 2016-10-28 15:44:56]

The enum magic I added was [1] - basically just adding IntDefs for the generated
Java enums. We can now use them on the JNI boundary [2], but I don't think
there's any checking there.

[1] https://codereview.chromium.org/1932183002/
[2] https://codereview.chromium.org/2262883002

[whywhat, 2016-10-28 16:38:02]

From peconn's comment I figure you could autogenerate the Java enum for actions
from the C++ code which would use IntDefs and int for the values.

See this enum for an example:
https://cs.chromium.org/chromium/src/net/test/url_request/url_request_failed_...

sample_for_tests in jni_generator is out-of-date about this :/

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/28 at 14:18:01, Zhiqiang Zhang wrote:
> On 2016/10/25 19:19:37, whywhat wrote:
> > On 2016/10/25 at 18:54:23, Zhiqiang Zhang wrote:
> > > On 2016/10/25 15:39:37, whywhat wrote:
> > > > No, let's use IntDef.
> > > >
> >
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> > > > 
> > > > See
> > > >
> >
https://groups.google.com/a/google.com/d/topic/clank-team/5aEW4J9dc5c/discussion
> > > > for one of the recent discussions on enums in clank.
> > > 
> > > Thanks, let's just use int. I tried IntDef but there's no compile-time
error
> > even if I pass some crazy value :(
> > > 
> > > Also HashSet<Integer> should be OK since its initial capacity is 16 and we
are
> > unlikely to have too many HashSets.
> > 
> > ArraySet is your friend.
> 
> Just found it's API level 23 :(

Ah, well. Consider using CopyOnWriteArraySet or just an int and a helping method
like we use for actions. But you can keep HashSet if you want too.

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:714:
int numActions = 0;
nit: I wonder if numActions could be just mediaSessionActions.size() + 1 or
something?

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:738:
numActions = Math.min(numActions, 3);
nit: 3 is a magic number - give it a name.

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:96:
if (mMediaSessionObserver != null) {
can it be null?

https://codereview.chromium.org/2441403002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2441403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:154: if
(m_scriptState->domWindow())
I think you should use something like this:

  DCHECK(getExecutionContext()->isDocument());
  Document* document = toDocument(getExecutionContext());

Move it to a separate method (getDocument()) if it's used somewhere else.

[Zhiqiang Zhang (Slow), 2016-11-01 14:55:26]

On 2016/10/28 16:38:02, whywhat wrote:
> From peconn's comment I figure you could autogenerate the Java enum for
actions
> from the C++ code which would use IntDefs and int for the values.
> 
> See this enum for an example:
>
https://cs.chromium.org/chromium/src/net/test/url_request/url_request_failed_...
> 
> sample_for_tests in jni_generator is out-of-date about this :/
> 
Let's do in the simpler way and refactor later. Currently using int should not
be a big issue.

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
(right):

https://codereview.chromium.org/2441403002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:60:
private EnumSet<MediaSessionAction> mMediaSessionActions = null;
On 2016/10/28 16:38:01, whywhat wrote:
> On 2016/10/28 at 14:18:01, Zhiqiang Zhang wrote:
> > On 2016/10/25 19:19:37, whywhat wrote:
> > > On 2016/10/25 at 18:54:23, Zhiqiang Zhang wrote:
> > > > On 2016/10/25 15:39:37, whywhat wrote:
> > > > > No, let's use IntDef.
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> > > > > 
> > > > > See
> > > > >
> > >
>
https://groups.google.com/a/google.com/d/topic/clank-team/5aEW4J9dc5c/discussion
> > > > > for one of the recent discussions on enums in clank.
> > > > 
> > > > Thanks, let's just use int. I tried IntDef but there's no compile-time
> error
> > > even if I pass some crazy value :(
> > > > 
> > > > Also HashSet<Integer> should be OK since its initial capacity is 16 and
we
> are
> > > unlikely to have too many HashSets.
> > > 
> > > ArraySet is your friend.
> > 
> > Just found it's API level 23 :(
> 
> Ah, well. Consider using CopyOnWriteArraySet or just an int and a helping
method
> like we use for actions. But you can keep HashSet if you want too.

I'm leaving HashSet here...

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
(right):

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:714:
int numActions = 0;
On 2016/10/28 16:38:02, whywhat wrote:
> nit: I wonder if numActions could be just mediaSessionActions.size() + 1 or
> something?

No, we are actually ignoring actions except for PREVIOUS/NEXT track

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:738:
numActions = Math.min(numActions, 3);
On 2016/10/28 16:38:02, whywhat wrote:
> nit: 3 is a magic number - give it a name.

Done.

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
(right):

https://codereview.chromium.org/2441403002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:96:
if (mMediaSessionObserver != null) {
On 2016/10/28 16:38:02, whywhat wrote:
> can it be null?

Checking mMediaSessionObserver.getMediaSession() now.

https://codereview.chromium.org/2441403002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2441403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:154: if
(m_scriptState->domWindow())
On 2016/10/28 16:38:02, whywhat wrote:
> I think you should use something like this:
> 
>   DCHECK(getExecutionContext()->isDocument());
>   Document* document = toDocument(getExecutionContext());
> 
> Move it to a separate method (getDocument()) if it's used somewhere else.

Done.

[Zhiqiang Zhang (Slow), 2016-11-01 18:08:46]

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

[commit-bot: I haz the power, 2016-11-01 18:09:03]

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-11-01 18:30:54]

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

[commit-bot: I haz the power, 2016-11-01 18:30:55]

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

[Zhiqiang Zhang (Slow), 2016-11-01 19:35:33]

The CL is merged to the plumbing one for higher-level review. I'm closing this
one.