Delay the signal for "MediaSession goes uncontrollable" message

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

Index: chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java b/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
index 9a7ba3833f525af17c8765be2a34a662f8571126..57c2dde165b714c12260e51f4ee81e1b3502d999 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
@@ -8,6 +8,7 @@ import android.app.Activity;
 import android.content.Intent;
 import android.graphics.Bitmap;
 import android.media.AudioManager;
+import android.os.Handler;
 import android.text.TextUtils;
 
 import org.chromium.base.ContextUtils;
@@ -43,6 +44,7 @@ public class MediaSessionTabHelper implements MediaImageCallback {
 
     private static final String UNICODE_PLAY_CHARACTER = "\u25B6";
     private static final int MINIMAL_FAVICON_SIZE = 114;
+    private static final int HIDE_NOTIFICATION_DELAY_MILLIS = 1000;
 
     private Tab mTab;
     private Bitmap mPageMediaImage = null;
@@ -60,6 +62,11 @@ public class MediaSessionTabHelper implements MediaImageCallback {
     private MediaMetadata mCurrentMetadata = null;
     private MediaImageManager mMediaImageManager = null;
     private Set<Integer> mMediaSessionActions = new HashSet<Integer>();
+    private Handler mHandler;
+    // The delayed task to hide notification. Hiding notification can be immediate or delayed.
+    // Delayed hiding will schedule this delayed task to |mHandler|. The task will be canceled when
+    // showing or immediate hiding.
+    private Runnable mHideNotificationDelayedTask;
 
     @VisibleForTesting
     @Nullable
@@ -70,8 +77,10 @@ public class MediaSessionTabHelper implements MediaImageCallback {
     private MediaNotificationListener mControlsListener = new MediaNotificationListener() {
         @Override
         public void onPlay(int actionSource) {
-            MediaSessionUMA
-                    .recordPlay(MediaSessionTabHelper.convertMediaActionSourceToUMA(actionSource));
+            if (isNotificationHiddingOrHidden()) return;
+
+            MediaSessionUMA.recordPlay(
+                    MediaSessionTabHelper.convertMediaActionSourceToUMA(actionSource));
 
             if (mMediaSessionObserver.getMediaSession() != null) {
                 mMediaSessionObserver.getMediaSession().resume();
@@ -80,6 +89,8 @@ public class MediaSessionTabHelper implements MediaImageCallback {
 
         @Override
         public void onPause(int actionSource) {
+            if (isNotificationHiddingOrHidden()) return;
+
             MediaSessionUMA.recordPause(
                     MediaSessionTabHelper.convertMediaActionSourceToUMA(actionSource));
 
@@ -90,8 +101,10 @@ public class MediaSessionTabHelper implements MediaImageCallback {
 
         @Override
         public void onStop(int actionSource) {
-            MediaSessionUMA
-                    .recordStop(MediaSessionTabHelper.convertMediaActionSourceToUMA(actionSource));
+            if (isNotificationHiddingOrHidden()) return;
+
+            MediaSessionUMA.recordStop(
+                    MediaSessionTabHelper.convertMediaActionSourceToUMA(actionSource));
 
             if (mMediaSessionObserver.getMediaSession() != null) {
                 mMediaSessionObserver.getMediaSession().stop();
@@ -107,30 +120,67 @@ public class MediaSessionTabHelper implements MediaImageCallback {
         }
     };
 
-    void hideNotification() {
-        if (mTab == null) {
-            return;
+    private void hideNotificationDelayed() {
+        if (mTab == null) return;
+        if (mHideNotificationDelayedTask != null) return;

[whywhat, 2016/11/04 14:34:44]

shouldn't you post a new task and cancel the current one in this case?

[Zhiqiang Zhang (Slow), 2016/11/04 14:50:48]

In that case if the helper receives a `!isControllable` state every 0.5 seconds,
the notification will never hide. Though it's not likely to happen, but we
should make MSTH robust against this case.

+
+        mHideNotificationDelayedTask = new Runnable() {
+            @Override
+            public void run() {
+                mHideNotificationDelayedTask = null;
+                hideNotificationInternal();
+            }
+        };
+        mHandler.postDelayed(mHideNotificationDelayedTask, HIDE_NOTIFICATION_DELAY_MILLIS);
+
+        mNotificationInfoBuilder = null;

[whywhat, 2016/11/04 14:34:44]

do you need to do this here?

why hideNotificationInternal can't check mTab for null and also null the
notification builder? (eliminating small duplication between
hideNotificationDelayed and hideNotificationImmediately)

In the delayed case showNotification would never be called since you check for
builder to not be null? Or are these legitimate cases where show should not be
called?

[Zhiqiang Zhang (Slow), 2016/11/04 14:50:48]

So mNotificationInfoBuilder is an indicator whether the notification is or is
going to be hidden. We need to nullify it here so that we ignore further
controls coming from the notification while it is going to be hidden.

Maybe we can move the nullification to hideNotificationInternal(), and also do:
```
boolean isNotificationHiddingOrHidden() {
  return mNotificationInfoBuilder == null || mHideNotificationDelayedTask ==
null;
}
```
We should be very careful if we decide to do this change, since lots of logic in
MSTH relies on it.

[Zhiqiang Zhang (Slow), 2016/11/04 16:10:26]

Sorry, didn't fully replied.
In this case, mNotificationInfoBuilder is non-null as long as the session is
controllable. So when it is null (hiding or going to hide), there's no need to
update the notification (when image downloaded, title updated, metadata updated,
etc.)

+    }
+
+    private void hideNotificationImmediately() {
+        if (mTab == null) return;
+        if (mHideNotificationDelayedTask != null) {
+            mHandler.removeCallbacks(mHideNotificationDelayedTask);
+            mHideNotificationDelayedTask = null;
         }
+
+        hideNotificationInternal();
+        mNotificationInfoBuilder = null;
+    }
+
+    /**
+     * This method performs the common steps for hiding the notification. It should only be called
+     * by {@link #hideNotificationDelayed()} and {@link #hideNotificationImmediately()}.
+     */
+    private void hideNotificationInternal() {
         MediaNotificationManager.hide(mTab.getId(), R.id.media_playback_notification);
         Activity activity = getActivityFromTab(mTab);
         if (activity != null) {
             activity.setVolumeControlStream(mPreviousVolumeControlStream);
         }
-        mNotificationInfoBuilder = null;
+    }
+
+    private void showNotification() {
+        assert mNotificationInfoBuilder != null;
+        if (mHideNotificationDelayedTask != null) {
+            mHandler.removeCallbacks(mHideNotificationDelayedTask);
+            mHideNotificationDelayedTask = null;
+        }
+        MediaNotificationManager.show(
+                ContextUtils.getApplicationContext(), mNotificationInfoBuilder.build());
     }
 
     private MediaSessionObserver createMediaSessionObserver(MediaSession mediaSession) {
         return new MediaSessionObserver(mediaSession) {
             @Override
             public void mediaSessionDestroyed() {
-                hideNotification();
+                hideNotificationImmediately();
                 cleanupMediaSessionObserver();
             }
 
             @Override
             public void mediaSessionStateChanged(boolean isControllable, boolean isPaused) {
                 if (!isControllable) {
-                    hideNotification();
+                    hideNotificationDelayed();
                     return;
                 }
 
@@ -160,9 +210,7 @@ public class MediaSessionTabHelper implements MediaImageCallback {
                                 .setListener(mControlsListener)
                                 .setMediaSessionActions(mMediaSessionActions);
 
-                MediaNotificationManager.show(ContextUtils.getApplicationContext(),
-                        mNotificationInfoBuilder.build());
-
+                showNotification();
                 Activity activity = getActivityFromTab(mTab);
                 if (activity != null) {
                     activity.setVolumeControlStream(AudioManager.STREAM_MUSIC);
@@ -249,12 +297,11 @@ public class MediaSessionTabHelper implements MediaImageCallback {
             mFavicon = null;
             mPageMediaImage = null;
 
-            if (mNotificationInfoBuilder == null) return;
+            if (isNotificationHiddingOrHidden()) return;
 
             mNotificationInfoBuilder.setOrigin(mOrigin);
             mNotificationInfoBuilder.setLargeIcon(mFavicon);
-            MediaNotificationManager.show(
-                    ContextUtils.getApplicationContext(), mNotificationInfoBuilder.build());
+            showNotification();
         }
 
         @Override
@@ -273,7 +320,7 @@ public class MediaSessionTabHelper implements MediaImageCallback {
 
             cleanupMediaSessionObserver();
 
-            hideNotification();
+            hideNotificationImmediately();
             mTab.removeObserver(this);
             mTab = null;
         }
@@ -290,6 +337,7 @@ public class MediaSessionTabHelper implements MediaImageCallback {
         if (activity != null) {
             mPreviousVolumeControlStream = activity.getVolumeControlStream();
         }
+        mHandler = new Handler();
     }
 
     /**
@@ -362,15 +410,14 @@ public class MediaSessionTabHelper implements MediaImageCallback {
      * |mPageMetadata| or |mFallbackTitle| is changed.
      */
     private void updateNotificationMetadata() {
-        if (mNotificationInfoBuilder == null) return;
+        if (isNotificationHiddingOrHidden()) return;
 
         MediaMetadata newMetadata = getMetadata();
         if (mCurrentMetadata.equals(newMetadata)) return;
 
         mCurrentMetadata = newMetadata;
         mNotificationInfoBuilder.setMetadata(mCurrentMetadata);
-        MediaNotificationManager.show(
-                ContextUtils.getApplicationContext(), mNotificationInfoBuilder.build());
+        showNotification();
     }
 
     /**
@@ -399,11 +446,10 @@ public class MediaSessionTabHelper implements MediaImageCallback {
     }
 
     private void updateNotificationActions() {
-        if (mNotificationInfoBuilder == null) return;
+        if (isNotificationHiddingOrHidden()) return;
 
         mNotificationInfoBuilder.setMediaSessionActions(mMediaSessionActions);
-        MediaNotificationManager.show(
-                ContextUtils.getApplicationContext(), mNotificationInfoBuilder.build());
+        showNotification();
     }
 
     @Override
@@ -418,13 +464,16 @@ public class MediaSessionTabHelper implements MediaImageCallback {
 
         mCurrentMediaImage = newMediaImage;
 
-        if (mNotificationInfoBuilder == null) return;
+        if (isNotificationHiddingOrHidden()) return;
         mNotificationInfoBuilder.setLargeIcon(mCurrentMediaImage);
-        MediaNotificationManager.show(
-                ContextUtils.getApplicationContext(), mNotificationInfoBuilder.build());
+        showNotification();
     }
 
     private Bitmap getNotificationImage() {
         return (mPageMediaImage != null) ? mPageMediaImage : mFavicon;
     }
+
+    private boolean isNotificationHiddingOrHidden() {
+        return mNotificationInfoBuilder == null;
+    }
 }