Add media controls in media notification

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

Index: chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java b/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
index 59c4d87c0fab6a47a9603f0ee247e74c2ade3cf9..5e1a2a224b8c48cc8ecc74c648ce1e339fd1d6f6 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java
@@ -11,6 +11,8 @@ import android.text.TextUtils;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.content_public.common.MediaMetadata;
 
+import java.util.EnumSet;
+
 /**
  * Exposes information about the current media notification to the external clients.
  */
@@ -55,6 +57,7 @@ public class MediaNotificationInfo {
         private int mId = INVALID_ID;
         private Intent mContentIntent = null;
         private MediaNotificationListener mListener = null;
+        private EnumSet<MediaSessionAction> mMediaSessionActions = null;

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

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.

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

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<>
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.
In the API we say the UA can ignore some controls and do its own. For now, we
could handle play/pause ourselves.

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

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.
It feels like the spec is too vague on this.
Ok, but I hope there's a plan to fix this uncertainty :)

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

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.

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

ArraySet is your friend.

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

Just found it's API level 23 :(

[whywhat, 2016/10/28 16:38:01]

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.

[Zhiqiang Zhang (Slow), 2016/11/01 14:55:25]

I'm leaving HashSet here...

 
         /**
          * Initializes the builder with the default values.
@@ -68,18 +71,19 @@ public class MediaNotificationInfo {
             assert mListener != null;
 
             return new MediaNotificationInfo(
-                    mMetadata,
-                    mIsPaused,
-                    mOrigin,
-                    mTabId,
-                    mIsPrivate,
-                    mIcon,
-                    mLargeIcon,
-                    mDefaultLargeIcon,
-                    mActions,
-                    mId,
-                    mContentIntent,
-                    mListener);
+                mMetadata,
+                mIsPaused,
+                mOrigin,
+                mTabId,
+                mIsPrivate,
+                mIcon,
+                mLargeIcon,
+                mDefaultLargeIcon,
+                mActions,
+                mId,
+                mContentIntent,
+                mListener,
+                mMediaSessionActions);
         }
 
         public Builder setMetadata(MediaMetadata metadata) {
@@ -141,6 +145,11 @@ public class MediaNotificationInfo {
             mListener = listener;
             return this;
         }
+
+        public Builder setMediaSessionActions(EnumSet<MediaSessionAction> actions) {
+            mMediaSessionActions = actions;
+            return this;
+        }
     }
 
     /**
@@ -204,6 +213,11 @@ public class MediaNotificationInfo {
     public final MediaNotificationListener listener;
 
     /**
+     * The actions enabled in MediaSession.
+     */
+    public final EnumSet<MediaSessionAction> mediaSessionActions;
+
+    /**
      * @return if play/pause actions are supported by this notification.
      */
     public boolean supportsPlayPause() {
@@ -234,19 +248,10 @@ public class MediaNotificationInfo {
      * @param contentIntent the intent to send when the notification is selected.
      * @param listener The listener for the control events.
      */
-    private MediaNotificationInfo(
-            MediaMetadata metadata,
-            boolean isPaused,
-            String origin,
-            int tabId,
-            boolean isPrivate,
-            int icon,
-            Bitmap largeIcon,
-            int defaultLargeIcon,
-            int actions,
-            int id,
-            Intent contentIntent,
-            MediaNotificationListener listener) {
+    private MediaNotificationInfo(MediaMetadata metadata, boolean isPaused, String origin,
+            int tabId, boolean isPrivate, int icon, Bitmap largeIcon, int defaultLargeIcon,
+            int actions, int id, Intent contentIntent, MediaNotificationListener listener,
+            EnumSet<MediaSessionAction> mediaSessionActions) {
         this.metadata = metadata;
         this.isPaused = isPaused;
         this.origin = origin;
@@ -259,6 +264,7 @@ public class MediaNotificationInfo {
         this.id = id;
         this.contentIntent = contentIntent;
         this.listener = listener;
+        this.mediaSessionActions = mediaSessionActions;

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

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.

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

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.

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

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]

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

     }
 
     @Override
@@ -267,22 +273,22 @@ public class MediaNotificationInfo {
         if (!(obj instanceof MediaNotificationInfo)) return false;
 
         MediaNotificationInfo other = (MediaNotificationInfo) obj;
-        return isPaused == other.isPaused
-                && isPrivate == other.isPrivate
-                && tabId == other.tabId
+        return isPaused == other.isPaused && isPrivate == other.isPrivate && tabId == other.tabId
                 && icon == other.icon
                 && (largeIcon == other.largeIcon
-                        || (largeIcon != null && largeIcon.sameAs(other.largeIcon)))
-                && defaultLargeIcon == other.defaultLargeIcon
-                && mActions == other.mActions
+                           || (largeIcon != null && largeIcon.sameAs(other.largeIcon)))
+                && defaultLargeIcon == other.defaultLargeIcon && mActions == other.mActions
                 && id == other.id
                 && (metadata == other.metadata
-                        || (metadata != null && metadata.equals(other.metadata)))
+                           || (metadata != null && metadata.equals(other.metadata)))
                 && TextUtils.equals(origin, other.origin)
                 && (contentIntent == other.contentIntent
-                        || (contentIntent != null && contentIntent.equals(other.contentIntent)))
+                           || (contentIntent != null && contentIntent.equals(other.contentIntent)))
                 && (listener == other.listener
-                        || (listener != null && listener.equals(other.listener)));
+                           || (listener != null && listener.equals(other.listener)))
+                && (mediaSessionActions == other.mediaSessionActions
+                           || (mediaSessionActions != null
+                                      && mediaSessionActions.equals(other.mediaSessionActions)));
     }
 
     @Override
@@ -299,6 +305,7 @@ public class MediaNotificationInfo {
         result = 31 * result + mActions;
         result = 31 * result + id;
         result = 31 * result + listener.hashCode();
+        result = 31 * result + mediaSessionActions.hashCode();
         return result;
     }
 }