[Media>UI] Apply throttling if the notification is updated too frequently

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 90bf8e871c92724051e5446e11afaf8f8586f295..26dec43e5746597945544712688f339e77334640 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
@@ -21,7 +21,9 @@ import android.graphics.drawable.BitmapDrawable;
 import android.graphics.drawable.Drawable;
 import android.media.AudioManager;
 import android.os.Build;
+import android.os.Handler;
 import android.os.IBinder;
+import android.support.annotation.NonNull;
 import android.support.v4.app.NotificationManagerCompat;
 import android.support.v4.media.MediaMetadataCompat;
 import android.support.v4.media.session.MediaSessionCompat;
@@ -33,6 +35,7 @@ import android.util.SparseArray;
 import android.view.KeyEvent;
 
 import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
 import org.chromium.base.SysUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.blink.mojom.MediaSessionAction;
@@ -129,6 +132,97 @@ public class MediaNotificationManager {
     @VisibleForTesting
     MediaSessionCompat mMediaSession;
 
+    @VisibleForTesting
+    Throttler mThrottler;
+
+    @VisibleForTesting
+    static class Throttler {
+        @VisibleForTesting
+        static final int THROTTLE_MILLIS = 500;
+
+        private final MediaNotificationManager mManager;
+        private final Handler mHandler;
+
+        @VisibleForTesting
+        Throttler(@NonNull MediaNotificationManager manager) {
+            mManager = manager;
+            mHandler = new Handler();
+        }
+
+        // When |mTask| is non-null, it will always be queued in mHandler. When |mTask| is non-null,
+        // all notification updates will be throttled and their info will be stored as
+        // mQueuedNotificationInfo. When |mTask| fires, it will call {@link showNotification()} with
+        // the latest queued notification info.
+        @VisibleForTesting
+        Runnable mTask;
+
+        // The queued notification info. If non-null, it will be the latest notification info.

[whywhat, 2017/04/20 19:34:59]

nit: I don't like the term queued used here as there's no actual queue :)
lastKnownInfo maybe?

[Zhiqiang Zhang (Slow), 2017/04/20 21:40:44]

lastPendingInfo should be more accurate :)

+        // Otherwise, the latest notification info will be |mManager.mMediaNotificationInfo|.
+        @VisibleForTesting
+        MediaNotificationInfo mQueuedNotificationInfo;
+
+        /**
+         * Queue |mediaNotificationInfo| for update. In unthrottled state (i.e. |mTask| != null),
+         * the notification will be updated immediately and enter the throttled state. In
+         * unthrottled state, the method will only update the queued notification info, which will
+         * be used for updating the notification when |mTask| is fired.
+         *
+         * @param mediaNotificationInfo The notification info to be queued.
+         */
+        public void queueNotification(MediaNotificationInfo mediaNotificationInfo) {
+            assert mediaNotificationInfo != null;
+
+            MediaNotificationInfo latestMediaNotificationInfo = mQueuedNotificationInfo != null
+                    ? mQueuedNotificationInfo
+                    : mManager.mMediaNotificationInfo;

[whywhat, 2017/04/20 19:34:59]

So what are the three notification info's here?
1) mediaNotificationInfo - is the one Chrome wants to show at the moment (due to
some state update, like a new title).
2) mManager.mMediaNotificationInfo - is what's being shown right now
3) mQueuedNotificationInfo is what Chrome wanted to show before that's waiting
for the task

How are these updated over time depending on whether the calls are coming within
the throttling window or not?

[Zhiqiang Zhang (Slow), 2017/04/20 21:40:44]

1) mediaNotificationInfo is the newly posted info.
2) mManager.mMediaNotificationInfo is what the user sees.
3) mQueuedNotificationInfo is the last posted info if it was throttled.
4) latestMediaNotificationInfo is used to decide whether the newly posted info
can be accepted, i.e. whether it is from a different tab, etc..

The way they updates are as follows:
* In unthrottled state, mQueuedNotificationInfo will always be null. When
posting a new notification, it will be shown immediately, and we will enter
throttled state.
* In throttled state, mQueuedNotificationInfo will allows to be changed. Any new
posting will replace it.

+
+            if (shouldIgnoreMediaNotificationInfo(
+                        latestMediaNotificationInfo, mediaNotificationInfo)) {
+                return;
+            }
+
+            if (mTask == null) {
+                showNotificationImmediately(mediaNotificationInfo);
+            } else {
+                mQueuedNotificationInfo = mediaNotificationInfo;
+            }
+        }
+
+        /**
+         * Clears the queued notification and enter unthrottled state.
+         */
+        public void clearQueuedNotifications() {
+            mHandler.removeCallbacks(mTask);
+            mQueuedNotificationInfo = null;
+            mTask = null;
+        }
+
+        @VisibleForTesting
+        void showNotificationImmediately(MediaNotificationInfo mediaNotificationInfo) {
+            // If no notification hasn't been updated in the last THROTTLE_MILLIS, update
+            // immediately and queue a task for blocking further updates.
+            mManager.showNotification(mediaNotificationInfo);
+            mTask = new Runnable() {

[whywhat, 2017/04/20 19:34:59]

is that an idiomatic way? shouldn't we rather only post a delay task if the
current timestamp is too close to the previous timestamp of when
queueNotification was called?
Also, shouldn't we throttle after a certain number of updates coming too fast?
If the website does two updates in a row, 500ms is a user visible timeout which
could even be a UI review worthy... Maybe use 200ms (if that's not enough in a
bad case, exponentially increase the timeout until some max value)?

[Zhiqiang Zhang (Slow), 2017/04/20 21:40:44]

I tried to avoid using timestamps. The reason is that the task already contains
the time information as the looper will call it after certain amount of time.
The logic can guarantee two consequent calls cannot happen between one
throttling window.
I guess the logic could be much more complex if we do that (maybe it requires
additional code to evaluate the update frequency, and smoothing probably).

As for the throttling window length, I can do more manual testing to find a
balance. Currently I assume 500ms is fine as toggling play/pause and switching
tracks usually takes roughly that long.

+                @Override
+                public void run() {
+                    if (mQueuedNotificationInfo != null) {
+                        // If any notification info is queued during the throttling time window,
+                        // update the notification.
+                        showNotificationImmediately(mQueuedNotificationInfo);
+                        mQueuedNotificationInfo = null;
+                    } else {
+                        // Otherwise, clear the task so further update is unthrottled.
+                        mTask = null;
+                    }
+                }
+            };
+            if (!mHandler.postDelayed(mTask, THROTTLE_MILLIS)) {
+                Log.w(TAG, "Failed to post the throttler task.");
+                mTask = null;
+            }
+        }
+    }
+
     private final MediaSessionCompat.Callback mMediaSessionCallback =
             new MediaSessionCompat.Callback() {
                 @Override
@@ -468,7 +562,7 @@ public class MediaNotificationManager {
             sManagers.put(notificationInfo.id, manager);
         }
 
-        manager.showNotification(notificationInfo);
+        manager.mThrottler.queueNotification(notificationInfo);
     }
 
     /**
@@ -662,6 +756,8 @@ public class MediaNotificationManager {
                 new MediaButtonInfo(R.drawable.ic_fast_rewind_white_36dp,
                         R.string.accessibility_seek_backward,
                         ListenerService.ACTION_SEEK_BACKWARD));
+
+        mThrottler = new Throttler(this);
     }
 
     /**
@@ -710,9 +806,7 @@ public class MediaNotificationManager {
 
     @VisibleForTesting
     void showNotification(MediaNotificationInfo mediaNotificationInfo) {
-        if (mediaNotificationInfo.equals(mMediaNotificationInfo)) return;
-        if (mediaNotificationInfo.isPaused && mMediaNotificationInfo != null
-                && mediaNotificationInfo.tabId != mMediaNotificationInfo.tabId) {
+        if (shouldIgnoreMediaNotificationInfo(mMediaNotificationInfo, mediaNotificationInfo)) {
             return;
         }
 
@@ -735,7 +829,15 @@ public class MediaNotificationManager {
         updateNotification();
     }
 
-    private void clearNotification() {
+    private static boolean shouldIgnoreMediaNotificationInfo(
+            MediaNotificationInfo oldInfo, MediaNotificationInfo newInfo) {
+        return newInfo.equals(oldInfo)
+                || ((newInfo.isPaused && oldInfo != null && newInfo.tabId != oldInfo.tabId));
+    }
+
+    @VisibleForTesting
+    void clearNotification() {
+        mThrottler.clearQueuedNotifications();
         if (mMediaNotificationInfo == null) return;
 
         NotificationManagerCompat manager = NotificationManagerCompat.from(getContext());