Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1931263002:
[MediaFling, UI] Always show play/pause in notification (Closed)
|
Created:
4 years, 7 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mlamouri (slow - plz ping), posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MediaFling, UI] Avoid flickering in notification
When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style.
This CL fixes the problem by treating the loading state as a transitional state.
BUG=607862
Committed: https://crrev.com/b95089462ef15efc94c745c84a690020ff19c40e
Cr-Commit-Position: refs/heads/master@{#392578}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : show pause button when loading #
Total comments: 1
Patch Set 4 : addressed comments #
Total comments: 5
Patch Set 5 : addressed comments #
Total comments: 6
Patch Set 6 : addressed avayvod's comments #
Total comments: 3
Patch Set 7 : let CastNotificationControl handle everything #Patch Set 8 : remove unused code #
Total comments: 4
Patch Set 9 : fix nit #
Messages
Total messages: 33 (13 generated)
Description was changed from ========== [Media, UI] Always support play/pause in MediaFling BUG=607862 ========== to ========== [MediaFling, UI] Always show play/pause in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by always showing a play/pause button. BUG=607862 ==========
Patchset #4 (id:60001) has been deleted
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/patch-status/1931263002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931263002/40001
zqzhang@chromium.org changed reviewers: + aberent@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
+avayvod
https://codereview.chromium.org/1931263002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/1931263002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:133: mNotificationBuilder.setPaused(mState == PlayerState.PAUSED); What happens if the user presses the pause button while in loading state? Does the player state switch to Paused or stay in Loading? If it stays in loading then presumably the user gets no immediate feedback from pressing the pause button. What effect does pressing it repeatedly have? If it switches to paused then what happens when loading completes; or when the user presses play before loading completes? I think you need to add tests for these sorts of cases.
So IIUC, the problem is that Chromecast player transitions from the paused to
playing state via the loading state - in this case the nofitication will flicker
from media style to normal and to media style again. Some thoughts on the ideal
behavior:
- when transitioning from paused to loading, don't change the notification to
non media style immediately; when the state switches to playing, there won't be
flickering -- this could be implemented by CastNotificationControl if comparing
the current state (paused) with the new state (loading), in which case just stay
in 'paused' state.
- when switching from loading state to playing takes too long, we'll hopefully
get an error and stop casting or change the state (however, in theory we don't
want the notification stay in paused state forever).
- we could have a "transitional loading state" supported by the media style
notification so we don't have to switch to the small notification but have the
buttons disabled or something.
The current fix is bad for the initiation of casting when the state is 'loading'
for a few seconds before user can actually play or pause.
I think having something like:
public void onPlaybackStateChanged(PlayerState newState) {
if (mState == newState
|| mState == PlayerState.PAUSED && newState == PlayerState.LOADING)
{
return;
}
mState = newState;
updateNotification();
}
Should be a reasonable fix to the actual problem.
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/patch-status/1931263002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931263002/80001
PTAL. The comments have been addressed. Now we show a non-MediaStyle notification when initiating Cast, and always show MediaStyle notification after remote playback started. LOADING is now considered as a transitional state and won't cause the notification to update. There are multiple code paths updating the notification, some of which may be incorrect or redundant. I think this CL is minimal to make the behavior correct. https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:154: if (mState == newState || mState == PlayerState.PAUSED && newState == PlayerState.LOADING) { Look LOADING as a transitional state, and don't update the notification. https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:164: mNotificationControl.show(PlayerState.INVALIDATED); This is for showing a non-MediaStyle notification when initiating MediaFling. https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:292: public void onPlaybackStateChanged(PlayerState newState) { Removed the body of this method since it's duplicate with CastNotificationControl.onPlaybackStateChanged()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You might want to update the change description to reflect the current change. https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:164: mNotificationControl.show(PlayerState.INVALIDATED); On 2016/05/04 at 15:35:13, Zhiqiang Zhang wrote: > This is for showing a non-MediaStyle notification when initiating MediaFling. Do you need this? As I read it, the condition above would not be true (as mState is not PAUSED by default and so LOADING will not be ignored? Also, wouldn't it show the notification too early? Have you tried your change when casting another video and switching between YouTube/non-YouTube videos (need to build Clankium)? You can use avayvod.github.io/mediaflingtest.html to test various scenarios. https://codereview.chromium.org/1931263002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:292: public void onPlaybackStateChanged(PlayerState newState) { On 2016/05/04 at 15:35:13, Zhiqiang Zhang wrote: > Removed the body of this method since it's duplicate with CastNotificationControl.onPlaybackStateChanged()
PTAL. Addressed avayvod@'s comments. Did more clean up to the code, and let RemoteMediaPlayerController have a Boolean to indicate whether the notification has started. If not, it will start the notification for the first time. Then CastNotificationControl will listen to the playback state change and update itself.
https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (left): https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:254: createNotificationControl(); createNotificationControl() can be removed too perhaps? https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:57: private boolean mIsNotificationStarted = false; why do we need it? could we have it exposed by CastNotificationControl instead? https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:284: CastNotificationControl notificationControl = getNotificationControl(); So getNotificationControl can in theory return null, would you still want to set the flag? Or if the notification will never be null, we should perhaps remove the if below?
Description was changed from ========== [MediaFling, UI] Always show play/pause in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by always showing a play/pause button. BUG=607862 ========== to ========== [MediaFling, UI] Avoid flickering in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by treating the loading state as a transitional state. BUG=607862 ==========
PTAL, addressed comments with replies. https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (left): https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:254: createNotificationControl(); On 2016/05/06 12:34:29, whywhat wrote: > createNotificationControl() can be removed too perhaps? I'll keep this method. It is now called at the first time in onPlaybackStateChanged() https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:57: private boolean mIsNotificationStarted = false; On 2016/05/06 12:34:29, whywhat wrote: > why do we need it? could we have it exposed by CastNotificationControl instead? Done. https://codereview.chromium.org/1931263002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:284: CastNotificationControl notificationControl = getNotificationControl(); On 2016/05/06 12:34:29, whywhat wrote: > So getNotificationControl can in theory return null, would you still want to set > the flag? > Or if the notification will never be null, we should perhaps remove the if > below? Use mNotificationControl to indicate whether the notification has started by itself. So now here is the point where we start the notification.
Sorry for more comments but I really think we can make the logic simpler https://codereview.chromium.org/1931263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (left): https://codereview.chromium.org/1931263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:162: mNotificationControl = CastNotificationControl.getOrCreate( Hm, this change actually fixed a recent bug with the notification not getting the tab title in time. Is it still working? https://codereview.chromium.org/1931263002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:179: mNotificationControl = null; So IIUC this will only happen when the user is either trying to cast for the very first time (controller is null) so the control should be null anyway, or trying to cast a YouTube video while casting a src= one or vice versa (covered by the next if below). release() below probably causes the notification to hide. https://codereview.chromium.org/1931263002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:277: if (mNotificationControl != null) return; So we will return every time user casts another video of the same type (YouTube or non-YouTube) as we only set once created notification control to null when the user switches types. Is that intended? 1. user casts video 1, mNotificationControl is null so we show the notification and it registers itself as a ui listener 2. the casting stops, mNotificationControl still points to the notification, however it's hidden as it gets ui listener update 3. user casts video 2, mNotificationControl in not null, so we don't show the notification We should probably only return if the control is not null AND is showing (as long as MediaRouteController updates it w.r.t the new video if user casts video 2 while video 1 is still casting). TBH, it seems like we currently create/update mNotificationControl whenever the mCurrentRouteController changes, so it's always there and listens to UiListener events. Perhaps, it needs to be able to show itself when the state changes to LOADING/PLAYING/PAUSED when it's hidden? Then this method in RMPC needs to do nothing at all.
PTAL, addressed more comments. Now I let CastNotificationControl handle everything.
lgtm % nits https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:157: show(newState); shouldn't you return here? does show() update mState to newState? https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:164: mNotificationControl.setPosterBitmap(getPoster()); not an issue to address in this review but a question: IIRC, poster could be fetched later on. would we update it on the notification control then (do we have onPosterBitmapUpdated event, for example)?
https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:157: show(newState); On 2016/05/09 19:43:23, whywhat wrote: > shouldn't you return here? does show() update mState to newState? Done. Yes. https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1931263002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:164: mNotificationControl.setPosterBitmap(getPoster()); On 2016/05/09 19:43:23, whywhat wrote: > not an issue to address in this review but a question: IIRC, poster could be > fetched later on. would we update it on the notification control then (do we > have onPosterBitmapUpdated event, for example)? Yes, it is CastNotificationControl.onPosterBitmapUpdated(). However for now, it is only called from a test.
The CQ bit was checked by zqzhang@chromium.org
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/1931263002/#ps180001 (title: "fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931263002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931263002/180001
Message was sent while issue was closed.
Description was changed from ========== [MediaFling, UI] Avoid flickering in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by treating the loading state as a transitional state. BUG=607862 ========== to ========== [MediaFling, UI] Avoid flickering in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by treating the loading state as a transitional state. BUG=607862 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [MediaFling, UI] Avoid flickering in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by treating the loading state as a transitional state. BUG=607862 ========== to ========== [MediaFling, UI] Avoid flickering in notification When the remote playback is in loading state, it does not have play/pause button, then MediaNotification will use non-MediaStyle. This causes the notification flicking between media and non-media style. This CL fixes the problem by treating the loading state as a transitional state. BUG=607862 Committed: https://crrev.com/b95089462ef15efc94c745c84a690020ff19c40e Cr-Commit-Position: refs/heads/master@{#392578} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b95089462ef15efc94c745c84a690020ff19c40e Cr-Commit-Position: refs/heads/master@{#392578} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
