Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1645943003:
Add selection and poster support to MediaNotificationManager (Closed)
|
Created:
4 years, 10 months ago by aberent Modified:
4 years, 10 months ago Reviewers:
mlamouri (slow - plz ping) CC:
whywhat, chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, media-router+watch_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. |
DescriptionAdd selection and poster support to MediaNotificationManager
These are needed to allow Clank cast to create its notification and
lock screen using MediaNotificationManager, rather than using its own
notification class and its own lock screen classes based on the
deprecated RemoteControlClient API.
BUG=579919
Committed: https://crrev.com/dbb5e35d29c1ca2e09bea8bb3bb1232b6b08a00d
Cr-Commit-Position: refs/heads/master@{#373495}
Patch Set 1 #Patch Set 2 : Fix null poster handling #
Total comments: 10
Patch Set 3 : Respond to comments #
Total comments: 16
Patch Set 4 : Reply to comments #
Total comments: 2
Messages
Total messages: 32 (13 generated)
aberent@chromium.org changed reviewers: + mlamouri@chromium.org
The CQ bit was checked by aberent@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/1645943003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645943003/1
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 aberent@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/1645943003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645943003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:38: public static final int ACTION_SELECT = 1 << 3; Why don't you simply pass the intent to start instead? All notifications will fire an intent. We just have a default behaviour (open tab) and a non-default (open some activity). https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (left): https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:523: nit: do not remove this line https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:548: if (mMediaNotificationInfo.poster != null) wearIcon = mMediaNotificationInfo.poster; What you call 'wearIcon' should be called 'defaultIcon'. Also, I don't think "poster" is the best name here. What about "icon" simply? https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:555: metadataBuilder.putBitmap(MediaMetadataCompat.METADATA_KEY_DISPLAY_ICON, wearIcon); Should that only happen if there is no icon specified? https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:568: metadataBuilder.putBitmap(MediaMetadataCompat.METADATA_KEY_ART, wearIcon); You don't want the poster on the wear device?
https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:38: public static final int ACTION_SELECT = 1 << 3; On 2016/02/02 15:39:20, Mounir Lamouri wrote: > Why don't you simply pass the intent to start instead? All notifications will > fire an intent. We just have a default behaviour (open tab) and a non-default > (open some activity). Done. https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (left): https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:523: On 2016/02/02 15:39:20, Mounir Lamouri wrote: > nit: do not remove this line Done. https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:548: if (mMediaNotificationInfo.poster != null) wearIcon = mMediaNotificationInfo.poster; On 2016/02/02 15:39:20, Mounir Lamouri wrote: > What you call 'wearIcon' should be called 'defaultIcon'. Also, I don't think > "poster" is the best name here. What about "icon" simply? Done. https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:555: metadataBuilder.putBitmap(MediaMetadataCompat.METADATA_KEY_DISPLAY_ICON, wearIcon); On 2016/02/02 15:39:20, Mounir Lamouri wrote: > Should that only happen if there is no icon specified? No, if a poster is provided then it should be used as the icon. https://codereview.chromium.org/1645943003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:568: metadataBuilder.putBitmap(MediaMetadataCompat.METADATA_KEY_ART, wearIcon); On 2016/02/02 15:39:20, Mounir Lamouri wrote: > You don't want the poster on the wear device? As discussed, this gives you one.
The CQ bit was checked by aberent@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/1645943003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645943003/40001
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by aberent@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/1645943003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645943003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can you check that the quality on Wear is good? https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:53: private Intent mSelectIntent = null; nit: call this mContentIntent to be consistent with the Android Notification name. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:54: private Bitmap mPoster; nit: call this mImage, mPoster is a bit too specific. It could be the album art or anything. Also, `= null`; https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:180: * The URl of the poster, if any nit: this is missing a dot. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:222: * @param poster The poster to use on the lock screen ditto. Also, do not mention the lock screen, this is going to be used on various other places like a Wear device. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:379: private final Bitmap mDefaultIconImage; nit: mDefaultMediaSessionImage; https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:538: Bitmap iconImage = mDefaultIconImage; Bitmap mediaSessionImage = mMediaNotificationInfo.image ? mMediaNotificationInfo.image : mDefaultMediaSessionImage; https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:547: // If we have a poster use it as the lock screen background nit: "// METADATA_KEY_ART is optional and should only be used if we can provide something better than the default image." https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:558: // background. no need for this comment.
https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java (right): https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:53: private Intent mSelectIntent = null; On 2016/02/03 14:40:32, Mounir Lamouri wrote: > nit: call this mContentIntent to be consistent with the Android Notification > name. Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:54: private Bitmap mPoster; On 2016/02/03 14:40:32, Mounir Lamouri wrote: > nit: call this mImage, mPoster is a bit too specific. It could be the album art > or anything. > > Also, `= null`; Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:180: * The URl of the poster, if any On 2016/02/03 14:40:32, Mounir Lamouri wrote: > nit: this is missing a dot. Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationInfo.java:222: * @param poster The poster to use on the lock screen On 2016/02/03 14:40:32, Mounir Lamouri wrote: > ditto. > > Also, do not mention the lock screen, this is going to be used on various other > places like a Wear device. Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:379: private final Bitmap mDefaultIconImage; On 2016/02/03 14:40:32, Mounir Lamouri wrote: > nit: mDefaultMediaSessionImage; Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:538: Bitmap iconImage = mDefaultIconImage; On 2016/02/03 14:40:32, Mounir Lamouri wrote: > Bitmap mediaSessionImage = mMediaNotificationInfo.image ? > mMediaNotificationInfo.image : mDefaultMediaSessionImage; Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:547: // If we have a poster use it as the lock screen background On 2016/02/03 14:40:32, Mounir Lamouri wrote: > nit: "// METADATA_KEY_ART is optional and should only be used if we can provide > something better than the default image." Done. https://codereview.chromium.org/1645943003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:558: // background. On 2016/02/03 14:40:32, Mounir Lamouri wrote: > no need for this comment. Done.
The CQ bit was checked by aberent@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/1645943003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645943003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for your patience, this lgtm! :) https://codereview.chromium.org/1645943003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1645943003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:553: } FWIW, when the regular media notification will start using this, it will likely be bound to some size restrictions. Not sure from where the poster is coming but I assume it's big enough.
The CQ bit was checked by aberent@chromium.org
https://codereview.chromium.org/1645943003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/1645943003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:553: } On 2016/02/04 08:01:11, Mounir Lamouri (slow) wrote: > FWIW, when the regular media notification will start using this, it will likely > be bound to some size restrictions. Not sure from where the poster is coming but > I assume it's big enough. It comes from the video element's poster attribute, so we actually have very little control of its size. We do give it a maximum size of 1M pixels to reduce the danger of OOM errors when we read it and copy it from a C++ bitmap to a Java bitmap, see https://code.google.com/p/chromium/issues/detail?id=529871.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645943003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645943003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add selection and poster support to MediaNotificationManager These are needed to allow Clank cast to create its notification and lock screen using MediaNotificationManager, rather than using its own notification class and its own lock screen classes based on the deprecated RemoteControlClient API. BUG=579919 ========== to ========== Add selection and poster support to MediaNotificationManager These are needed to allow Clank cast to create its notification and lock screen using MediaNotificationManager, rather than using its own notification class and its own lock screen classes based on the deprecated RemoteControlClient API. BUG=579919 Committed: https://crrev.com/dbb5e35d29c1ca2e09bea8bb3bb1232b6b08a00d Cr-Commit-Position: refs/heads/master@{#373495} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/dbb5e35d29c1ca2e09bea8bb3bb1232b6b08a00d Cr-Commit-Position: refs/heads/master@{#373495} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
