Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1915083002:
[Android,MediaFling] Create and register the notification object early (Closed)
|
Created:
4 years, 8 months ago by whywhat Modified:
4 years, 8 months ago Reviewers:
aberent CC:
chromium-reviews, feature-media-reviews_chromium.org, 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[Android,MediaFling] Create and register the notification object early
BUG=604564
TEST=manual
This way the CastNotificationContol gets the title change updates before its
shown for the first time.
Committed: https://crrev.com/df9018ebecb9b6588c7e89af9781438d466753e0
Cr-Commit-Position: refs/heads/master@{#389773}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add extra checks and updates #
Total comments: 2
Patch Set 3 : Fix notification not hiding #
Messages
Total messages: 28 (9 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed reviewers: + aberent@chromium.org
PTaL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915083002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915083002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
https://codereview.chromium.org/1915083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1915083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:163: mChromeVideoActivity.get(), controller); Do you really want to show the notification before the MediaRouteDialog has been shown? The user may still decide not to cast at this point by cancelling the MediaRouteDialog. If so, should you be using createNotificationControl here, or is there a good reason for not doing so? Also, is startNotification() still needed?
https://codereview.chromium.org/1915083002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java (right): https://codereview.chromium.org/1915083002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerController.java:163: mChromeVideoActivity.get(), controller); On 2016/04/25 at 16:39:57, aberent wrote: > Do you really want to show the notification before the MediaRouteDialog has been shown? The user may still decide not to cast at this point by cancelling the MediaRouteDialog. > > If so, should you be using createNotificationControl here, or is there a good reason for not doing so? Also, is startNotification() still needed? This doesn't show the notification, it only creates the object if needed and registers it as a UiListener with the controller. In particular, it keeps track of the title. The notification is shown in startNotification() from onCasting() handler in RMPC, which is too late to get onTitleChanged() event causing the original bug in question.
Add extra checks and updates
The CQ bit was checked by avayvod@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/1915083002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915083002/20001
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1915083002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/1915083002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:188: hide(); As discussed this looks strange. From what you said the title is being set to null when the video finishes. The cast code knows when the video finishes, so should be hiding the notification anyway. If there is some other reason why the title is being set to null, then maybe onTitleChanged(null) should simply be ignored, and leave the title as the previous mTitle.
Fix notification not hiding
The CQ bit was checked by avayvod@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/1915083002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915083002/40001
PTaL https://codereview.chromium.org/1915083002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java (right): https://codereview.chromium.org/1915083002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/remote/CastNotificationControl.java:188: hide(); On 2016/04/26 at 12:23:40, aberent wrote: > As discussed this looks strange. From what you said the title is being set to null when the video finishes. The cast code knows when the video finishes, so should be hiding the notification anyway. > > If there is some other reason why the title is being set to null, then maybe onTitleChanged(null) should simply be ignored, and leave the title as the previous mTitle. I changed this to ignore null titles. The notification wasn't hiding because in the case the activity is destroyed, disconnect() is called rather than release() which leaves the notification in place (should it?). The Cast player seems to shut down anyway upon disconnecting so I changed the handler to call release().
lgtm
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 avayvod@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915083002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915083002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Android,MediaFling] Create and register the notification object early BUG=604564 TEST=manual This way the CastNotificationContol gets the title change updates before its shown for the first time. ========== to ========== [Android,MediaFling] Create and register the notification object early BUG=604564 TEST=manual This way the CastNotificationContol gets the title change updates before its shown for the first time. Committed: https://crrev.com/df9018ebecb9b6588c7e89af9781438d466753e0 Cr-Commit-Position: refs/heads/master@{#389773} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/df9018ebecb9b6588c7e89af9781438d466753e0 Cr-Commit-Position: refs/heads/master@{#389773} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
