Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446273002:
Delay the signal for "MediaSession goes uncontrollable" message (Closed)
|
Created:
4 years, 1 month ago by Zhiqiang Zhang (Slow) Modified:
4 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDelay the signal for "MediaSession goes uncontrollable" message
This CL make the notification dismiss delayed in order to avoid
notification flickering when MediaSession goes inactive and then active
in a short period.
BUG=515143, 649336
Committed: https://crrev.com/1c90bda2a54336143cbc014b95db28ad2ee80892
Cr-Commit-Position: refs/heads/master@{#429875}
Patch Set 1 #Patch Set 2 : implement in Java #Patch Set 3 : nits #
Total comments: 6
Patch Set 4 : addressed comments #
Total comments: 6
Patch Set 5 : more fixes #Patch Set 6 : rebased #
Total comments: 5
Messages
Total messages: 28 (13 generated)
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
Wouldn't that be easier to move to the UI layer? The MediaSessionTabHelper could delay hiding the notification until a second after the playback stopped and recycle it if it gets asked to show a new notification. WDYT?
On 2016/10/26 15:08:38, mlamouri wrote: > Wouldn't that be easier to move to the UI layer? The MediaSessionTabHelper could > delay hiding the notification until a second after the playback stopped and > recycle it if it gets asked to show a new notification. WDYT? Yeah, it should be cleaner to do in MSTH. But I somehow remember that there was a reason to do it here in our discussion before, but I cannot remember what is it. Bad memory... I'll try do it in MSTH and see if it works.
PTAL :) Implemented in Java.
https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:42: private static final int HIDE_NOTIFICATION_DELAY_MILLIS = 2000; I would prefer 1000. Is there a reason why you picked 2000? I think 1000 is better because it will be short enough that it wouldn't be noticeable but still avoid flickering during transitions. https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:71: if (mNotificationInfoBuilder == null) return; Can you have a method like `isNotificationHiddingOrHidden()`? It would be better than having an odd condition that needs a comment ;) https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:145: private void updateNotification() { Maybe this should be called `showNotification()`? :)
PTAL :) https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:42: private static final int HIDE_NOTIFICATION_DELAY_MILLIS = 2000; On 2016/10/28 10:31:47, mlamouri wrote: > I would prefer 1000. Is there a reason why you picked 2000? I think 1000 is > better because it will be short enough that it wouldn't be noticeable but still > avoid flickering during transitions. I'm a bit worried about if the network speed is slow, or have a high latency. Let's try 1000 first and test it on various network conditions. https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:71: if (mNotificationInfoBuilder == null) return; On 2016/10/28 10:31:47, mlamouri wrote: > Can you have a method like `isNotificationHiddingOrHidden()`? It would be better > than having an odd condition that needs a comment ;) Done. https://codereview.chromium.org/2446273002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:145: private void updateNotification() { On 2016/10/28 10:31:47, mlamouri wrote: > Maybe this should be called `showNotification()`? :) Done.
https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:46: private static final int HIDE_NOTIFICATION_DELAY_MILLIS = 2000; You said let's try with 1000 but you kept 2000. Did I missunderstood or did you forgot to upload the new CL? https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:103: hideNotificationImmediately(); Why are you doing this here? The frontend wouldn't hide itself but wait for the MediaSession code to request it. Is it because we would otherwise delay? https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:116: // Do nothing when a hiding task has already queued. nit: the comment is just paraphrasing the code.
PTAL https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:46: private static final int HIDE_NOTIFICATION_DELAY_MILLIS = 2000; On 2016/10/31 10:31:14, mlamouri wrote: > You said let's try with 1000 but you kept 2000. Did I missunderstood or did you > forgot to upload the new CL? Sorry, I forgot to fix this and somehow thought I did. https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:103: hideNotificationImmediately(); On 2016/10/31 10:31:14, mlamouri wrote: > Why are you doing this here? The frontend wouldn't hide itself but wait for the > MediaSession code to request it. Is it because we would otherwise delay? I thought it would affect cast but actually won't, removed this. https://codereview.chromium.org/2446273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:116: // Do nothing when a hiding task has already queued. On 2016/10/31 10:31:14, mlamouri wrote: > nit: the comment is just paraphrasing the code. Removed.
lgtm. Thanks! :)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2446273002/#ps100001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Delay the signal for "MediaSession goes uncontrollable" message This CL make the notification dismiss delayed in order to avoid notification flickering when MediaSession goes inactive and then active in a short period. BUG=515143,649336 ========== to ========== Delay the signal for "MediaSession goes uncontrollable" message This CL make the notification dismiss delayed in order to avoid notification flickering when MediaSession goes inactive and then active in a short period. BUG=515143,649336 Committed: https://crrev.com/1c90bda2a54336143cbc014b95db28ad2ee80892 Cr-Commit-Position: refs/heads/master@{#429875} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1c90bda2a54336143cbc014b95db28ad2ee80892 Cr-Commit-Position: refs/heads/master@{#429875}
Message was sent while issue was closed.
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:125: if (mHideNotificationDelayedTask != null) return; shouldn't you post a new task and cancel the current one in this case? https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:136: mNotificationInfoBuilder = null; 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?
Message was sent while issue was closed.
https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:125: if (mHideNotificationDelayedTask != null) return; On 2016/11/04 14:34:44, whywhat wrote: > shouldn't you post a new task and cancel the current one in this case? 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. https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:136: mNotificationInfoBuilder = null; On 2016/11/04 14:34:44, whywhat wrote: > 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? 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.
Message was sent while issue was closed.
https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java (right): https://codereview.chromium.org/2446273002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java:136: mNotificationInfoBuilder = null; On 2016/11/04 14:34:44, whywhat wrote: > 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? 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.) |
