Add seek forward/backward controls in MediaNotification

chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java b/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
index bcd4e568cbe5befdbd7ac01f838185b11738748a..c0544a7ed577d085365406e042733ead949c1fda 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java
@@ -39,6 +39,11 @@ import org.chromium.blink.mojom.MediaSessionAction;
 import org.chromium.chrome.R;
 import org.chromium.content_public.common.MediaMetadata;
 
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
 import javax.annotation.Nullable;
 
 /**
@@ -58,7 +63,10 @@ public class MediaNotificationManager {
     private static final int N_LARGE_ICON_SIZE_DP = 94;
 
     // The maximum number of actions in CompactView media notification.
-    private static final int MAXIMUM_NUM_ACTIONS_IN_COMPACT_VIEW = 3;
+    private static final int COMPACT_VIEW_ACTIONS_COUNT = 3;
+
+    // The maximum number of actions in BigView media notification.
+    private static final int BIG_VIEW_ACTIONS_COUNT = isRunningN() ? 5 : 3;
 
     // We're always used on the UI thread but the LOCK is required by lint when creating the
     // singleton.
@@ -87,6 +95,10 @@ public class MediaNotificationManager {
                 "MediaNotificationManager.ListenerService.PREVIOUS_TRACK";
         private static final String ACTION_NEXT_TRACK =
                 "MediaNotificationManager.ListenerService.NEXT_TRACK";
+        private static final String ACTION_SEEK_FORWARD =
+                "MediaNotificationManager.ListenerService.SEEK_FORWARD";
+        private static final String ACTION_SEEK_BACKWARD =
+                "MediaNotificationmanager.ListenerService.SEEK_BACKWARD";
 
         @Override
         public IBinder onBind(Intent intent) {
@@ -161,6 +173,12 @@ public class MediaNotificationManager {
                     case KeyEvent.KEYCODE_MEDIA_NEXT:
                         manager.onMediaSessionAction(MediaSessionAction.NEXT_TRACK);
                         break;
+                    case KeyEvent.KEYCODE_MEDIA_FAST_FORWARD:
+                        manager.onMediaSessionAction(MediaSessionAction.SEEK_FORWARD);
+                        break;
+                    case KeyEvent.KEYCODE_MEDIA_REWIND:
+                        manager.onMediaSessionAction(MediaSessionAction.SEEK_BACKWARD);
+                        break;
                     default:
                         break;
                 }
@@ -180,6 +198,10 @@ public class MediaNotificationManager {
                 manager.onMediaSessionAction(MediaSessionAction.PREVIOUS_TRACK);
             } else if (ACTION_NEXT_TRACK.equals(action)) {
                 manager.onMediaSessionAction(MediaSessionAction.NEXT_TRACK);
+            } else if (ACTION_SEEK_FORWARD.equals(action)) {
+                manager.onMediaSessionAction(MediaSessionAction.SEEK_FORWARD);
+            } else if (ACTION_SEEK_BACKWARD.equals(action)) {
+                manager.onMediaSessionAction(MediaSessionAction.SEEK_BACKWARD);
             }
         }
     }
@@ -465,11 +487,29 @@ public class MediaNotificationManager {
     // ListenerService running for the notification. Only non-null when showing.
     private ListenerService mService;

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

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.

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

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

 
-    private final String mPlayDescription;
-    private final String mPauseDescription;
-    private final String mStopDescription;
-    private final String mPreviousTrackDescription;
-    private final String mNextTrackDescription;
+    /**
+     * The class containing all the information for adding a button in the notification for an
+     * action.
+     */
+    private static final class MediaButtonInfo {
+        // The resource id of this media button icon.

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

Use javadoc syntax here; Eclipse will show the comment if you hover over the
variable later.

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

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

Done.

+        public int iconResId;
+        // The resource id of this media button description.
+        public int descriptionResId;
+        // The intent string to be fired when this media button is clicked.
+        public String intentString;
+
+        public MediaButtonInfo(int buttonResId, int descriptionResId, String intentString) {
+            this.iconResId = buttonResId;
+            this.descriptionResId = descriptionResId;
+            this.intentString = intentString;
+        }
+    }
+
+    private SparseArray<MediaButtonInfo> mActionToButtonInfo = new SparseArray<MediaButtonInfo>();
+
+    @VisibleForTesting
+    static final int CUSTOM_MEDIA_SESSION_ACTION_STOP = MediaSessionAction.LAST + 1;

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

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.

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

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.

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

Acknowledged.

 
     private NotificationCompat.Builder mNotificationBuilder;
 
@@ -506,16 +546,46 @@ public class MediaNotificationManager {
                     MediaNotificationManager.this.onMediaSessionAction(
                             MediaSessionAction.NEXT_TRACK);
                 }
+
+                @Override
+                public void onFastForward() {
+                    MediaNotificationManager.this.onMediaSessionAction(
+                            MediaSessionAction.SEEK_FORWARD);
+                }
+
+                @Override
+                public void onRewind() {
+                    MediaNotificationManager.this.onMediaSessionAction(
+                            MediaSessionAction.SEEK_BACKWARD);
+                }
             };
 
     private MediaNotificationManager(Context context, int notificationId) {
         mContext = context;
-        mPlayDescription = context.getResources().getString(R.string.accessibility_play);
-        mPauseDescription = context.getResources().getString(R.string.accessibility_pause);
-        mStopDescription = context.getResources().getString(R.string.accessibility_stop);
-        mPreviousTrackDescription =
-                context.getResources().getString(R.string.accessibility_previous_track);
-        mNextTrackDescription = context.getResources().getString(R.string.accessibility_next_track);
+
+        mActionToButtonInfo.put(MediaSessionAction.PLAY,
+                new MediaButtonInfo(R.drawable.ic_media_control_play, R.string.accessibility_play,
+                        ListenerService.ACTION_PLAY));
+        mActionToButtonInfo.put(MediaSessionAction.PAUSE,
+                new MediaButtonInfo(R.drawable.ic_media_control_pause, R.string.accessibility_pause,
+                        ListenerService.ACTION_PAUSE));
+        mActionToButtonInfo.put(CUSTOM_MEDIA_SESSION_ACTION_STOP,
+                new MediaButtonInfo(R.drawable.ic_media_control_stop, R.string.accessibility_stop,
+                        ListenerService.ACTION_STOP));
+        mActionToButtonInfo.put(MediaSessionAction.PREVIOUS_TRACK,
+                new MediaButtonInfo(R.drawable.ic_media_control_skip_previous,
+                        R.string.accessibility_previous_track,
+                        ListenerService.ACTION_PREVIOUS_TRACK));
+        mActionToButtonInfo.put(MediaSessionAction.NEXT_TRACK,
+                new MediaButtonInfo(R.drawable.ic_media_control_skip_next,
+                        R.string.accessibility_next_track, ListenerService.ACTION_NEXT_TRACK));
+        mActionToButtonInfo.put(MediaSessionAction.SEEK_FORWARD,
+                new MediaButtonInfo(R.drawable.ic_media_control_fast_forward,
+                        R.string.accessibility_seek_forward, ListenerService.ACTION_SEEK_FORWARD));
+        mActionToButtonInfo.put(MediaSessionAction.SEEK_BACKWARD,
+                new MediaButtonInfo(R.drawable.ic_media_control_fast_rewind,
+                        R.string.accessibility_seek_backward,
+                        ListenerService.ACTION_SEEK_BACKWARD));
     }
 
     /**
@@ -708,6 +778,12 @@ public class MediaNotificationManager {
         if (mMediaNotificationInfo.mediaSessionActions.contains(MediaSessionAction.NEXT_TRACK)) {
             actions |= PlaybackStateCompat.ACTION_SKIP_TO_NEXT;
         }
+        if (mMediaNotificationInfo.mediaSessionActions.contains(MediaSessionAction.SEEK_FORWARD)) {
+            actions |= PlaybackStateCompat.ACTION_FAST_FORWARD;
+        }
+        if (mMediaNotificationInfo.mediaSessionActions.contains(MediaSessionAction.SEEK_BACKWARD)) {
+            actions |= PlaybackStateCompat.ACTION_REWIND;
+        }
         return actions;
     }
 
@@ -761,48 +837,48 @@ public class MediaNotificationManager {
     }
 
     private void addNotificationButtons(NotificationCompat.Builder builder) {
+        Set<Integer> actions = new HashSet<Integer>();
+
+        // TODO(zqzhang): handle other actions when play/pause is not supported? See
+        // https://crbug.com/667500
+        if (mMediaNotificationInfo.supportsPlayPause()) {
+            actions.addAll(mMediaNotificationInfo.mediaSessionActions);
+            if (mMediaNotificationInfo.isPaused) {
+                actions.remove(MediaSessionAction.PAUSE);
+                actions.add(MediaSessionAction.PLAY);
+            } else {
+                actions.remove(MediaSessionAction.PLAY);
+                actions.add(MediaSessionAction.PAUSE);
+            }
+        }
+
+        if (mMediaNotificationInfo.supportsStop()) {
+            actions.add(CUSTOM_MEDIA_SESSION_ACTION_STOP);
+        }
+
+        List<Integer> bigViewActions = computeBigViewActions(actions);
+
+        int numAddedActions = 0;

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

Where do you use this variable?

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

Removed. Thanks!

+
+        for (int action : bigViewActions) {
+            MediaButtonInfo buttonInfo = mActionToButtonInfo.get(action);
+            builder.addAction(buttonInfo.iconResId,
+                    mContext.getResources().getString(buttonInfo.descriptionResId),
+                    createPendingIntent(buttonInfo.intentString));
+        }
+
         // Only apply MediaStyle when NotificationInfo supports play/pause.
         if (mMediaNotificationInfo.supportsPlayPause()) {
             NotificationCompat.MediaStyle style = new NotificationCompat.MediaStyle();
             style.setMediaSession(mMediaSession.getSessionToken());
 
-            int numAddedActions = 0;
+            int[] compactViewActionIndices = computeCompactViewActionIndices(bigViewActions);
 
-            if (mMediaNotificationInfo.mediaSessionActions.contains(
-                        MediaSessionAction.PREVIOUS_TRACK)) {
-                builder.addAction(R.drawable.ic_media_control_skip_previous,
-                        mPreviousTrackDescription,
-                        createPendingIntent(ListenerService.ACTION_PREVIOUS_TRACK));
-                ++numAddedActions;
-            }
-            if (mMediaNotificationInfo.isPaused) {
-                builder.addAction(R.drawable.ic_media_control_play, mPlayDescription,
-                        createPendingIntent(ListenerService.ACTION_PLAY));
-            } else {
-                // If we're here, the notification supports play/pause button and is playing.
-                builder.addAction(R.drawable.ic_media_control_pause, mPauseDescription,
-                        createPendingIntent(ListenerService.ACTION_PAUSE));
-            }
-            ++numAddedActions;
-            if (mMediaNotificationInfo.mediaSessionActions.contains(
-                        MediaSessionAction.NEXT_TRACK)) {
-                builder.addAction(R.drawable.ic_media_control_skip_next, mNextTrackDescription,
-                        createPendingIntent(ListenerService.ACTION_NEXT_TRACK));
-                ++numAddedActions;
-            }
-            numAddedActions = Math.min(numAddedActions, MAXIMUM_NUM_ACTIONS_IN_COMPACT_VIEW);
-            int[] compactViewActions = new int[numAddedActions];
-            for (int i = 0; i < numAddedActions; ++i) compactViewActions[i] = i;
-            style.setShowActionsInCompactView(compactViewActions);
+            style.setShowActionsInCompactView(compactViewActionIndices);
             style.setCancelButtonIntent(createPendingIntent(ListenerService.ACTION_CANCEL));
             style.setShowCancelButton(true);
             builder.setStyle(style);
         }
-
-        if (mMediaNotificationInfo.supportsStop()) {
-            builder.addAction(R.drawable.ic_media_control_stop, mStopDescription,
-                    createPendingIntent(ListenerService.ACTION_STOP));
-        }
     }
 
     private Bitmap drawableToBitmap(Drawable drawable) {
@@ -832,4 +908,108 @@ public class MediaNotificationManager {
         }
         return artist + " - " + album;
     }
+
+    /**
+     * Compute the actions to be shown in BigView media notification.
+     * The method assumes exactly one of PLAY/PAUSE/STOP can be present.
+     * Actions in pairs are preferred if there are more actions than |BIG_VIEW_ACTIONS_COUNT|.
+     *
+     * TODO(zqzhang): get UX feedback to decide which actions to select when the number of actions
+     * is greater than that can be displayed. See https://crbug.com/667500
+     */
+    private List<Integer> computeBigViewActions(Set<Integer> actions) {
+        // Exactly one of PLAY/PAUSE/STOP can exist.
+        assert (actions.contains(MediaSessionAction.PLAY) ? 1 : 0)
+                        + (actions.contains(MediaSessionAction.PAUSE) ? 1 : 0)
+                        + (actions.contains(CUSTOM_MEDIA_SESSION_ACTION_STOP) ? 1 : 0)
+                == 1;
+        int[] actionByOrder = {
+                MediaSessionAction.PREVIOUS_TRACK,
+                MediaSessionAction.SEEK_BACKWARD,
+                MediaSessionAction.PLAY,
+                MediaSessionAction.PAUSE,
+                MediaSessionAction.SEEK_FORWARD,
+                MediaSessionAction.NEXT_TRACK,
+                CUSTOM_MEDIA_SESSION_ACTION_STOP,
+        };
+
+        // First, select at most |BIG_VIEW_ACTIONS_COUNT| actions by priority.
+        Set<Integer> selectedActions;
+        if (actions.size() <= BIG_VIEW_ACTIONS_COUNT) {
+            selectedActions = actions;
+        } else {
+            selectedActions = new HashSet<Integer>();

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

new HashSet<>();

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

Done.

+            if (actions.contains(CUSTOM_MEDIA_SESSION_ACTION_STOP)) {
+                selectedActions.add(CUSTOM_MEDIA_SESSION_ACTION_STOP);
+            } else {
+                if (actions.contains(MediaSessionAction.PLAY)) {
+                    selectedActions.add(MediaSessionAction.PLAY);
+                } else {
+                    selectedActions.add(MediaSessionAction.PAUSE);
+                }
+                if (actions.contains(MediaSessionAction.PREVIOUS_TRACK)
+                        && actions.contains(MediaSessionAction.NEXT_TRACK)) {
+                    selectedActions.add(MediaSessionAction.PREVIOUS_TRACK);
+                    selectedActions.add(MediaSessionAction.NEXT_TRACK);
+                } else {
+                    selectedActions.add(MediaSessionAction.SEEK_BACKWARD);
+                    selectedActions.add(MediaSessionAction.SEEK_FORWARD);
+                }
+            }
+        }
+
+        // Second, sort the selected actions.
+        List<Integer> sortedActions = new ArrayList<Integer>();

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

new ArrayList<>();

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

Done.

+        for (int action : actionByOrder) {
+            if (selectedActions.contains(action)) sortedActions.add(action);
+        }
+
+        return sortedActions;
+    }
+
+    /**
+     * Compute the actions to be shown in CompactView media notification.
+     * The method assumes exactly one of PLAY/PAUSE/STOP can be present.

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

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.

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

Done.

+     * Actions in pairs are preferred if there are more actions than |COMPACT_VIEW_ACTIONS_COUNT|.
+     */
+    @VisibleForTesting
+    static int[] computeCompactViewActionIndices(List<Integer> actions) {
+        if (actions.size() <= COMPACT_VIEW_ACTIONS_COUNT) {
+            // If the number of actions is less than |COMPACT_VIEW_ACTIONS_COUNT|, just return an
+            // array of 0, 1, ..., |actions.size()|-1.
+            int[] actionsArray = new int[actions.size()];
+            for (int i = 0; i < actions.size(); ++i) actionsArray[i] = i;
+            return actionsArray;
+        }
+
+        if (actions.contains(CUSTOM_MEDIA_SESSION_ACTION_STOP)) {
+            int[] actionsArray = new int[1];
+            actionsArray[0] = actions.indexOf(CUSTOM_MEDIA_SESSION_ACTION_STOP);
+        }
+
+        int[] actionsArray = new int[COMPACT_VIEW_ACTIONS_COUNT];
+        if (actions.contains(MediaSessionAction.PREVIOUS_TRACK)
+                && actions.contains(MediaSessionAction.NEXT_TRACK)) {
+            actionsArray[0] = actions.indexOf(MediaSessionAction.PREVIOUS_TRACK);
+            if (actions.contains(MediaSessionAction.PLAY)) {
+                actionsArray[1] = actions.indexOf(MediaSessionAction.PLAY);
+            } else {
+                actionsArray[1] = actions.indexOf(MediaSessionAction.PAUSE);
+            }
+            actionsArray[2] = actions.indexOf(MediaSessionAction.NEXT_TRACK);
+            return actionsArray;
+        }
+
+        assert actions.contains(MediaSessionAction.SEEK_BACKWARD)
+                && actions.contains(MediaSessionAction.SEEK_FORWARD);
+        actionsArray[0] = actions.indexOf(MediaSessionAction.SEEK_BACKWARD);
+        if (actions.contains(MediaSessionAction.PLAY)) {
+            actionsArray[1] = actions.indexOf(MediaSessionAction.PLAY);
+        } else {
+            actionsArray[1] = actions.indexOf(MediaSessionAction.PAUSE);
+        }
+        actionsArray[2] = actions.indexOf(MediaSessionAction.SEEK_FORWARD);
+
+        return actionsArray;
+    }
 }