Add seek forward/backward controls in MediaNotification

Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance, while it is 5 for N.

BUG=656563, 656574
Committed: https://crrev.com/cc56b29ef844962a6bb3e0e75a76a9339030847f
Cr-Commit-Position: refs/heads/master@{#435304}

[Zhiqiang Zhang (Slow), 2016-11-11 17:03:37]

Description was changed from

==========
Add seek forward/backward controls in MediaNotification

BUG=656563,656574
==========

to

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance.

BUG=656563,656574
==========

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

Description was changed from

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance.

BUG=656563,656574
==========

to

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance, while it is 5 for N.

BUG=656563,656574
==========

[Zhiqiang Zhang (Slow), 2016-11-11 17:13:09]

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

[Zhiqiang Zhang (Slow), 2016-11-11 17:13:09]

PTAL

I'm making decision by myself here for the BigView in pre-N, which now we
display 3 buttons at most. The reason is that having 5 icons in the BigView
notification is really ugly.

I've made the code easy to change if we decide to do the other way.

[whywhat, 2016-11-11 20:17:58]

lgtm although I'm still thinking how the code for actions could be simplified -
I find it not very easy to follow atm.

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

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:818:
SparseArray<Integer> mapActionToIndex = new SparseArray<Integer>();
did you mean SparseIntArray?

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:841:
// a indicator for the play/pause button for computing compact view buttons.
nit: s/a/an/

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:919:
MediaSessionAction.PLAY_PAUSE, MediaSessionAction.NEXT_TRACK,
nit: one constant per line please

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:921:
MediaSessionAction.SEEK_BACKWARD,
I'd say SEEK_BACKWARD is higher in priority than forward? (rewind 30 seconds to
repeat something the user missed for distraction)

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

> although I'm still thinking how the code for actions could be simplified - I
find it not very easy to follow atm.

Yeah, I agree. I think it should be better if we can unify all these buttons
instead of treating play/pause/stop separately. Will send for another round of
review after I finished :)

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

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:818:
SparseArray<Integer> mapActionToIndex = new SparseArray<Integer>();
On 2016/11/11 20:17:58, whywhat wrote:
> did you mean SparseIntArray?

Done.

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:841:
// a indicator for the play/pause button for computing compact view buttons.
On 2016/11/11 20:17:58, whywhat wrote:
> nit: s/a/an/

Done.

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:919:
MediaSessionAction.PLAY_PAUSE, MediaSessionAction.NEXT_TRACK,
On 2016/11/11 20:17:58, whywhat wrote:
> nit: one constant per line please

Ah, `cl format` is not so smart :(

https://codereview.chromium.org/2498683002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:921:
MediaSessionAction.SEEK_BACKWARD,
On 2016/11/11 20:17:58, whywhat wrote:
> I'd say SEEK_BACKWARD is higher in priority than forward? (rewind 30 seconds
to
> repeat something the user missed for distraction)

Done.

[Zhiqiang Zhang (Slow), 2016-11-14 16:06:12]

PTAL the changes after #4 :)

Did some refactoring on the code for buttons. It should be cleaner now.

[whywhat, 2016-11-15 00:09:14]

It came up in a discussion with Mounir that there are basically three categories
of action sets:
- too small, e.g. 3 or below where we just show all the supported actions in
both big and compact view,
- all, i.e. 5*, where we should just pick what pair (next/prev or fwd/rwd) to
show in compact (and pre-N big view)
- in between - meaning we have only one complete pair and one incomplete - we
should prefer the complete pair
(*) for media notifications we don't have Stop as a button so shouldn't count
it, right? We always have a cross in the top corner of the notification

Perhaps it should allow you to simplify the code.
So far I'm still not happy with the amount of maps and sets created and copied
for a collection of up-to-five elements.

WDYT?

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

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:497:
public int buttonResId;
nit: comment the public fields?

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:508:
private SparseArray<MediaButtonInfo> mMapMediaSessionActionToMediaButtonInfo;
nit: maybe use a shorter name like mActionToButtonInfo or something?

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:510:
private static final int CUSTOM_MEDIA_SESSION_ACTION_STOP =
MediaSessionAction.LAST + 1;
nit: have we considered having this action in the API too?

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:567:
private void initMediaButtonInfo() {
nit: maybe merge it back with the ctor as it's not called from anywhere else.
Can the application context change (if the language is changed)? Ideally we
would build this structure once per language change, not once per tab per
playback...

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:568:
mMapMediaSessionActionToMediaButtonInfo = new SparseArray<MediaButtonInfo>();
nit: could be done where the member is defined, no?

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:571:
mContext.getResources().getString(R.string.accessibility_play),
context.getResources().getString() repeats many times, it deserves a separate
method, (or pass context and R. id to MediaButtonInfo to get it in the ctor? I'd
prefer a method in MediaNotificationManager I think)

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:778:
private long computeMediaSessionActions() {
nit: I wonder if you could have getMediaSessionActions on MediaNotificationInfo

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:851:
actions.remove(MediaSessionAction.PLAY);
nit: maybe just remove the extra action in the if's below?

if (mMNI.isPaused) {
  actions.remove(MSA.PAUSE);
} else {
  actions.remove(MSA.PLAY);
}

It doesn't seem more confusing than removing and adding while it's shorter.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:859:
actions = new HashSet<Integer>();
seems like if play/pause is not supported, we don't support any other action
too?

you could also avoid the else branch by initializing |actions| with a new
HashSet and then calling addAll(mMNI.mSA) in the true branch.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:866:
List<Integer> bigViewActions = computeBigViewActions(actions);
I think this is still very hard to comprehend.

First, on pre-N there's no difference between bigView and compactViewActions,
correct?

could you get computeCompactViewActions just return the int[] of indices in
bigViewActions that need to be set? For pre-N you would just create an array of
bigViewActions size with 0-N and return it, right?

For N+ you would need to find BIG_N - COMPACT_N actions to remove.

computeBigActions could return a SparseIntArray (or better int[]?) itself.

The code would look something like:

// perhaps initialize in the ctor?
private static final int BIG_VIEW_ACTIONS_COUNT = IsRunningN() ? 5 : 3;

...

// contains ordered MSA values
int[] bigViewActions = computeTopPriorityActions(actions,
BIG_VIEW_ACTIONS_COUNT);

for (int action : bigViewActions) {
  MediaButtonInfo info = mMediaButtonInfos.get(action);
  builder.addAction(info...);
}

// contains indices for actions in bigViewActions which we want to keep for
compact view
int[] compactViewActionIndices = computeTopPriorityActionIndices(bigViewActions,
COMPACT_VIEW_ACTIONS_COUNT);
style.setShowActionsInCompactView(compactViewActionsIndices);

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:940:
int[] actionPriorities = {
nit: rename to actionsByPriority, consider making it a class constant
same with actionsByOrder below

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:959:
// First, select at most |MAXIMUM_NUM_ACTIONS_IN_COMPACT_VIEW| actions by
priority.
nit: |maxNumActions|

[Zhiqiang Zhang (Slow), 2016-11-15 12:00:15]

On 2016/11/15 00:09:14, whywhat wrote:
> It came up in a discussion with Mounir that there are basically three
categories
> of action sets:
> - too small, e.g. 3 or below where we just show all the supported actions in
> both big and compact view,
> - all, i.e. 5*, where we should just pick what pair (next/prev or fwd/rwd) to
> show in compact (and pre-N big view)
> - in between - meaning we have only one complete pair and one incomplete - we
> should prefer the complete pair
> (*) for media notifications we don't have Stop as a button so shouldn't count
> it, right? We always have a cross in the top corner of the notification
> 
> Perhaps it should allow you to simplify the code.
> So far I'm still not happy with the amount of maps and sets created and copied
> for a collection of up-to-five elements.
> 
> WDYT?

On media button selection, I'd say nexttrack/previoustrack should be more
important even if they are not in pair.
Spotify free account only has nexttrack and previoustrack is disabled.

Another use case is playlists. If the user is listening to the first chapter of
an audiobook playlist,
it doesn't make much sense to have a "previoustrack" button.

Otherwise, I'll try to reduce the number of sets/maps, especially by simplifying
the logic for computing the CompactView buttons.

[Zhiqiang Zhang (Slow), 2016-11-15 14:48:42]

Patchset #7 (id:120001) has been deleted

[Zhiqiang Zhang (Slow), 2016-11-15 14:48:53]

Patchset #7 (id:140001) has been deleted

[Zhiqiang Zhang (Slow), 2016-11-15 14:52:55]

PTAL

Did more refactoring based on your comments :)

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

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:497:
public int buttonResId;
On 2016/11/15 00:09:14, whywhat wrote:
> nit: comment the public fields?

Done.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:508:
private SparseArray<MediaButtonInfo> mMapMediaSessionActionToMediaButtonInfo;
On 2016/11/15 00:09:14, whywhat wrote:
> nit: maybe use a shorter name like mActionToButtonInfo or something?

Done.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:510:
private static final int CUSTOM_MEDIA_SESSION_ACTION_STOP =
MediaSessionAction.LAST + 1;
On 2016/11/15 00:09:14, whywhat wrote:
> nit: have we considered having this action in the API too?

Maybe later if we really need it.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:567:
private void initMediaButtonInfo() {
On 2016/11/15 00:09:14, whywhat wrote:
> nit: maybe merge it back with the ctor as it's not called from anywhere else.
> Can the application context change (if the language is changed)?

Done

> Ideally we
> would build this structure once per language change, not once per tab per
> playback...

Now we only store the id in MediaButtonInfo, and the string will be fetched
every time the notification updates (though it's a bit expensive, but the old
behavior also does this).

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:568:
mMapMediaSessionActionToMediaButtonInfo = new SparseArray<MediaButtonInfo>();
On 2016/11/15 00:09:14, whywhat wrote:
> nit: could be done where the member is defined, no?

Done.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:571:
mContext.getResources().getString(R.string.accessibility_play),
On 2016/11/15 00:09:14, whywhat wrote:
> context.getResources().getString() repeats many times, it deserves a separate
> method, (or pass context and R. id to MediaButtonInfo to get it in the ctor?
I'd
> prefer a method in MediaNotificationManager I think)

Now we store the res id in MediaButtonInfo and the string is fetched every time
it's used.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:778:
private long computeMediaSessionActions() {
On 2016/11/15 00:09:14, whywhat wrote:
> nit: I wonder if you could have getMediaSessionActions on
MediaNotificationInfo

Hmm, it's MediaNotificationInfo is already like a struct and all fields are
public. Do we need a getter?

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:851:
actions.remove(MediaSessionAction.PLAY);
On 2016/11/15 00:09:14, whywhat wrote:
> nit: maybe just remove the extra action in the if's below?
> 
> if (mMNI.isPaused) {
>   actions.remove(MSA.PAUSE);
> } else {
>   actions.remove(MSA.PLAY);
> }
> 
> It doesn't seem more confusing than removing and adding while it's shorter.

Done.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:859:
actions = new HashSet<Integer>();
On 2016/11/15 00:09:14, whywhat wrote:
> seems like if play/pause is not supported, we don't support any other action
> too?

Maybe it's better to keep the current behavior. Added a TODO for this.

> you could also avoid the else branch by initializing |actions| with a new
> HashSet and then calling addAll(mMNI.mSA) in the true branch.

Done.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:866:
List<Integer> bigViewActions = computeBigViewActions(actions);
Mostly done with small modifications to your proposal

On 2016/11/15 00:09:14, whywhat wrote:
> First, on pre-N there's no difference between bigView and compactViewActions,
> correct?
> 
> could you get computeCompactViewActions just return the int[] of indices in
> bigViewActions that need to be set? For pre-N you would just create an array
of
> bigViewActions size with 0-N and return it, right?
> 
> For N+ you would need to find BIG_N - COMPACT_N actions to remove.

Here I prefer to use the same logic for pre-N and post-N. But in general, we are
now computing the BigView actions, and then remove part of them for compact view
until the number of actions gets <=3.

> computeBigActions could return a SparseIntArray (or better int[]?) itself.

List<Integer> maybe? int[] requires another step of conversion.

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:940:
int[] actionPriorities = {
On 2016/11/15 00:09:14, whywhat wrote:
> nit: rename to actionsByPriority, consider making it a class constant
> same with actionsByOrder below

Done and made a member since it's now used in two places (one here, and one for
removing actions for CompactView).

https://codereview.chromium.org/2498683002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:959:
// First, select at most |MAXIMUM_NUM_ACTIONS_IN_COMPACT_VIEW| actions by
priority.
On 2016/11/15 00:09:14, whywhat wrote:
> nit: |maxNumActions|

No longer applicable.

[Zhiqiang Zhang (Slow), 2016-11-21 22:22:07]

PTAL

Had discussion with Mounir last week and we decide to prefer action pairs first.

Here's the implementation (for action pairs), although I still think PS#7
(action by priority) is cleaner.

[whywhat, 2016-11-22 22:57:45]

lgtm

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

https://codereview.chromium.org/2498683002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:921:
assert (actions.contains(MediaSessionAction.PLAY) ? 1 : 0)
nit: this statement blows my mind.
Can it be that none of those exist? If yes, the statement is incorrect, if not,
the comment is incorrect.

I wonder if you could write it like:

((actions.contains(MediaSessionAction.PLAY) !=
actions.contains(MediaSessionAction.PAUSE)) &&
!actions.contains(CUSTOM_MSA_STOP)) || 
(actions.contains(CUSTOM_MSA_STOP) && !actions.contains(MSA.PLAY) &&
!actions.contains(MSA.PAUSE))

But it doesn't seem much better and I failed to simplify it further :/

[whywhat, 2016-11-22 22:58:15]

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

https://codereview.chromium.org/2498683002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:974:
private int[] computeCompactViewActionIndices(List<Integer> actions) {
nit: maybe you could unit tests these at least?

[Zhiqiang Zhang (Slow), 2016-11-23 14:43:58]

zqzhang@chromium.org changed reviewers:
+ dfalcantara@chromium.org, ochang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-23 14:43:59]

PTAL for Anton on the added unit tests.

+dfalcantara: chrome/android/ (except for MediaNotificationManager.java which is
already reviewed)
+ochang: media_session.mojom

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

https://codereview.chromium.org/2498683002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:921:
assert (actions.contains(MediaSessionAction.PLAY) ? 1 : 0)
On 2016/11/22 22:57:44, whywhat wrote:
> nit: this statement blows my mind.
> Can it be that none of those exist? If yes, the statement is incorrect, if
not,
> the comment is incorrect.
> 
> I wonder if you could write it like:
> 
> ((actions.contains(MediaSessionAction.PLAY) !=
> actions.contains(MediaSessionAction.PAUSE)) &&
> !actions.contains(CUSTOM_MSA_STOP)) || 
> (actions.contains(CUSTOM_MSA_STOP) && !actions.contains(MSA.PLAY) &&
> !actions.contains(MSA.PAUSE))
> 
> But it doesn't seem much better and I failed to simplify it further :/

Fixed the comment: s/At most one/Exactly one/

https://codereview.chromium.org/2498683002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:974:
private int[] computeCompactViewActionIndices(List<Integer> actions) {
On 2016/11/22 22:58:15, whywhat wrote:
> nit: maybe you could unit tests these at least?

Done.

[Oliver Chang, 2016-11-23 16:52:49]

mojom lgtm

[gone, 2016-11-28 19:23:21]

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

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:488:
private ListenerService mService;
These member fields should all be grouped together at the top of the class
instead of having all of these random functions in between.  It makes it
extremely difficult to see what needs to be initialized in the constructor and
doubly difficult to figure out what this class actually stores references to. 
It's even weirder because you're defining a new static final class between two
private member fields, then defining another static final constant afterward.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:495:
// The resource id of this media button icon.
Use javadoc syntax here; Eclipse will show the comment if you hover over the
variable later.

/** The resource ID of this media button icon. */

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:512:
static final int CUSTOM_MEDIA_SESSION_ACTION_STOP = MediaSessionAction.LAST + 1;
Any reason you're adding a random one here instead of actually adding it to the
mojom enum?  This'll bork UMA if it ever gets recorded and can potentially cause
conflicts if you ever accidentally create another custom media action.  It's
especially hard to figure out if you've done that because this constant is
defined 512 lines in.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:861:
int numAddedActions = 0;
Where do you use this variable?

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:941:
selectedActions = new HashSet<Integer>();
new HashSet<>();

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:962:
List<Integer> sortedActions = new ArrayList<Integer>();
new ArrayList<>();

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:972:
* The method assumes exactly one of PLAY/PAUSE/STOP can be present.
Should assert this directly in the code.  Without reading the comment it makes
it look like your STOP button is being immediately overwritten by the
PREVIOUS_TRACK button or SEEK_BACKWARD button.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1899: <message
name="IDS_ACCESSIBILITY_SEEK_FORWARD" desc="Content description for the seek
forward button that seeks media to a later position.">
Don't need to say "content description".  This is for human translators to read
and figure out what they're trying to translate, not for developers trying to
figure out where this thing goes.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationButtonComputationTest.java
(right):

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationButtonComputationTest.java:28:
ArrayList<Integer> actions = new ArrayList<Integer>();
new ArrayList<>();

[Zhiqiang Zhang (Slow), 2016-11-29 16:45:40]

Thanks for the review.
PTAL for Dan

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

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:488:
private ListenerService mService;
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> These member fields should all be grouped together at the top of the class
> instead of having all of these random functions in between.  It makes it
> extremely difficult to see what needs to be initialized in the constructor and
> doubly difficult to figure out what this class actually stores references to. 
> It's even weirder because you're defining a new static final class between two
> private member fields, then defining another static final constant afterward.

Done. Just realized the the definition of inner classes can be put in the back
(unlike C++)

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:495:
// The resource id of this media button icon.
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> Use javadoc syntax here; Eclipse will show the comment if you hover over the
> variable later.
> 
> /** The resource ID of this media button icon. */

Done.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:512:
static final int CUSTOM_MEDIA_SESSION_ACTION_STOP = MediaSessionAction.LAST + 1;
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> Any reason you're adding a random one here instead of actually adding it to
the
> mojom enum?  This'll bork UMA if it ever gets recorded and can potentially
cause
> conflicts if you ever accidentally create another custom media action.  It's
> especially hard to figure out if you've done that because this constant is
> defined 512 lines in.

The mojom enum is consistent with the API spec in Blink, which does not have
action STOP yet. Currently STOP action is handled internally in the browser
process and is never passed though mojo, so there's no need to worry about UMA
atm.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:861:
int numAddedActions = 0;
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> Where do you use this variable?

Removed. Thanks!

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:941:
selectedActions = new HashSet<Integer>();
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> new HashSet<>();

Done.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:962:
List<Integer> sortedActions = new ArrayList<Integer>();
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> new ArrayList<>();

Done.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:972:
* The method assumes exactly one of PLAY/PAUSE/STOP can be present.
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> Should assert this directly in the code.  Without reading the comment it makes
> it look like your STOP button is being immediately overwritten by the
> PREVIOUS_TRACK button or SEEK_BACKWARD button.

Done.

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1899: <message
name="IDS_ACCESSIBILITY_SEEK_FORWARD" desc="Content description for the seek
forward button that seeks media to a later position.">
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> Don't need to say "content description".  This is for human translators to
read
> and figure out what they're trying to translate, not for developers trying to
> figure out where this thing goes.

Removed all "content description".

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationButtonComputationTest.java
(right):

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationButtonComputationTest.java:28:
ArrayList<Integer> actions = new ArrayList<Integer>();
On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> new ArrayList<>();

Done.

[gone, 2016-11-29 18:20:55]

lgtm on my end

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

https://codereview.chromium.org/2498683002/diff/220001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:512:
static final int CUSTOM_MEDIA_SESSION_ACTION_STOP = MediaSessionAction.LAST + 1;
On 2016/11/29 16:45:40, Zhiqiang Zhang wrote:
> On 2016/11/28 19:23:21, dfalcantara (check my queue) wrote:
> > Any reason you're adding a random one here instead of actually adding it to
> the
> > mojom enum?  This'll bork UMA if it ever gets recorded and can potentially
> cause
> > conflicts if you ever accidentally create another custom media action.  It's
> > especially hard to figure out if you've done that because this constant is
> > defined 512 lines in.
> 
> The mojom enum is consistent with the API spec in Blink, which does not have
> action STOP yet. Currently STOP action is handled internally in the browser
> process and is never passed though mojo, so there's no need to worry about UMA
> atm.

Acknowledged.

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

https://codereview.chromium.org/2498683002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:550:
public int iconResId;
nit: stick newline in to separate lines 550 and 551, and another between 552 and
553

[Zhiqiang Zhang (Slow), 2016-11-29 19:18:55]

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

https://codereview.chromium.org/2498683002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:550:
public int iconResId;
On 2016/11/29 18:20:55, dfalcantara (check my queue) wrote:
> nit: stick newline in to separate lines 550 and 551, and another between 552
and
> 553

Done.

[Zhiqiang Zhang (Slow), 2016-11-29 19:19:16]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-29 19:19:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, ochang@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2498683002/#ps280001
(title: "nits")

[commit-bot: I haz the power, 2016-11-29 19:19:35]

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-29 19:24:35]

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

[commit-bot: I haz the power, 2016-11-29 19:24:37]

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Zhiqiang Zhang (Slow), 2016-11-29 21:42:05]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-29 21:42:06]

The patchset sent to the CQ was uploaded after l-g-t-m from ochang@chromium.org,
avayvod@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2498683002/#ps300001
(title: "rebased")

[commit-bot: I haz the power, 2016-11-29 21:42:52]

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-29 23:46:21]

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

[commit-bot: I haz the power, 2016-11-29 23:46:22]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Zhiqiang Zhang (Slow), 2016-11-30 13:38:01]

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

[commit-bot: I haz the power, 2016-11-30 13:38:19]

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-30 15:25:40]

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

[commit-bot: I haz the power, 2016-11-30 15:25:43]

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

[Zhiqiang Zhang (Slow), 2016-11-30 15:51:20]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-30 15:51:21]

The patchset sent to the CQ was uploaded after l-g-t-m from ochang@chromium.org,
avayvod@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2498683002/#ps340001
(title: "fixed compact view computation when STOP is present")

[commit-bot: I haz the power, 2016-11-30 15:51:42]

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-30 17:11:16]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1480521080582160,
"parent_rev": "f651cb9a86d4f4448223de4b6188533402883143", "commit_rev":
"6c75b8d76168f887b57eb12d7c7bf8c97d3d96cc"}

[commit-bot: I haz the power, 2016-11-30 17:11:43]

Description was changed from

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance, while it is 5 for N.

BUG=656563,656574
==========

to

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance, while it is 5 for N.

BUG=656563,656574
==========

[commit-bot: I haz the power, 2016-11-30 17:11:45]

Committed patchset #16 (id:340001)

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

Description was changed from

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance, while it is 5 for N.

BUG=656563,656574
==========

to

==========
Add seek forward/backward controls in MediaNotification

This CL adds support for MediaSession seekforward and
seekbackward to MediaNotification. On pre-N, a maximum of 3
buttons is shown in BigView to improve the notification
appearance, while it is 5 for N.

BUG=656563,656574

Committed: https://crrev.com/cc56b29ef844962a6bb3e0e75a76a9339030847f
Cr-Commit-Position: refs/heads/master@{#435304}
==========

[commit-bot: I haz the power, 2016-11-30 17:14:17]

Patchset 16 (id:??) landed as
https://crrev.com/cc56b29ef844962a6bb3e0e75a76a9339030847f
Cr-Commit-Position: refs/heads/master@{#435304}