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

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

There's a issue where the notification is updated too frequently
(especially when the media metadata or fallback media metadata changes),
which can cause the system UI to be very janky or even a system crash.
This CL throttles the notification update if the notification has been
updated during the previous 500ms.

BUG=711730
Review-Url: https://codereview.chromium.org/2833493002
Cr-Commit-Position: refs/heads/master@{#466651}
Committed: https://chromium.googlesource.com/chromium/src/+/bf4103d69a0d7e289266c9671c2b980d585c0d5a

[Zhiqiang Zhang (Slow), 2017-04-19 15:57:30]

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

[Zhiqiang Zhang (Slow), 2017-04-19 15:57:53]

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

[commit-bot: I haz the power, 2017-04-19 15:58:04]

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, 2017-04-19 18:07:01]

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

[commit-bot: I haz the power, 2017-04-19 18:07:02]

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

[whywhat, 2017-04-20 12:12:45]

See my comment on the bug with thoughts on other fixes we can try.

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

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

https://codereview.chromium.org/2833493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:159:
// The queued notification info. If non-null, it will be the latest notification
info.
nit: I don't like the term queued used here as there's no actual queue :)
lastKnownInfo maybe?

https://codereview.chromium.org/2833493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:177:
: mManager.mMediaNotificationInfo;
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?

https://codereview.chromium.org/2833493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:205:
mTask = new Runnable() {
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)?

https://codereview.chromium.org/2833493002/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2833493002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_impl.cc:334: if (!IsSuspended())
Why this change?

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

PTAL w/ replies.

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

https://codereview.chromium.org/2833493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:159:
// The queued notification info. If non-null, it will be the latest notification
info.
On 2017/04/20 at 19:34:59, whywhat wrote:
> nit: I don't like the term queued used here as there's no actual queue :)
lastKnownInfo maybe?

lastPendingInfo should be more accurate :)

https://codereview.chromium.org/2833493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:177:
: mManager.mMediaNotificationInfo;
On 2017/04/20 at 19:34:59, whywhat wrote:
> 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?

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.

https://codereview.chromium.org/2833493002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:205:
mTask = new Runnable() {
On 2017/04/20 at 19:34:59, whywhat wrote:
> 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?

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.

> 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)?

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.

https://codereview.chromium.org/2833493002/diff/1/content/browser/media/sessi...
File content/browser/media/session/media_session_impl.cc (right):

https://codereview.chromium.org/2833493002/diff/1/content/browser/media/sessi...
content/browser/media/session/media_session_impl.cc:334: if (!IsSuspended())
On 2017/04/20 at 19:34:59, whywhat wrote:
> Why this change?

As we now throttle the notifications, the notification is more likely to be
out-of-sync with the latest info. Therefore the notification might send an
action that MediaSession no longer accepts.

[whywhat, 2017-04-20 22:33:02]

lgtm

[Zhiqiang Zhang (Slow), 2017-04-22 11:57:07]

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

[commit-bot: I haz the power, 2017-04-22 11:57: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, 2017-04-22 13:03:58]

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

[commit-bot: I haz the power, 2017-04-22 13:03:59]

Dry run: This issue passed the CQ dry run.

[Zhiqiang Zhang (Slow), 2017-04-24 14:59:18]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2017-04-24 14:59:18]

The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org
Link to the patchset: https://codereview.chromium.org/2833493002/#ps40001
(title: "rebased and fixed tests")

[commit-bot: I haz the power, 2017-04-24 14:59:45]

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, 2017-04-24 16:28:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493045958630610,
"parent_rev": "cfe6442e2558f28417fdcc98c07795f029d1d159", "commit_rev":
"7faa7ba0555d9f3a536343cd441eda52eaa7e07c"}

[commit-bot: I haz the power, 2017-04-24 16:29:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493045958630610,
"parent_rev": "28b213b11b8c5609916e306c9079b194028baabf", "commit_rev":
"bf4103d69a0d7e289266c9671c2b980d585c0d5a"}

[commit-bot: I haz the power, 2017-04-24 16:29:10]

Description was changed from

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

There's a issue where the notification is updated too frequently
(especially when the media metadata or fallback media metadata changes),
which can cause the system UI to be very janky or even a system crash.
This CL throttles the notification update if the notification has been
updated during the previous 500ms.

BUG=711730
==========

to

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

There's a issue where the notification is updated too frequently
(especially when the media metadata or fallback media metadata changes),
which can cause the system UI to be very janky or even a system crash.
This CL throttles the notification update if the notification has been
updated during the previous 500ms.

BUG=711730

Review-Url: https://codereview.chromium.org/2833493002
Cr-Commit-Position: refs/heads/master@{#466651}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf4103d69a0d7e289266c9671c2b...
==========

[commit-bot: I haz the power, 2017-04-24 16:29:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/bf4103d69a0d7e289266c9671c2b...