Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1924213002:
ARC: Support Android Notification Priority (Closed)
DescriptionARC: Support Android Notification Priority
Previously Android notification priority was just ignored. This patch supports it and Chrome notification reflects Android notification priority.
BUG=b/28409430
Committed: https://crrev.com/6397aeb42297802661ff1f3ea73d735d881a73cc
Cr-Commit-Position: refs/heads/master@{#390898}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Addressed comments #Messages
Total messages: 18 (9 generated)
Description was changed from ========== w BUG=28409430 ========== to ========== ARC: Change the notification priority mapping table Android's DEFAULT priority doesn't trigger heads-up notifications, but Chrome's priority 0 triggers pop-up. So it looks better to map Android's DEFAULT to Chrome's -1. BUG=b/28409430 ==========
yoshiki@chromium.org changed reviewers: + hidehiko@chromium.org
Hidehiko-san, PTAL. Thanks.
Description was changed from ========== ARC: Change the notification priority mapping table Android's DEFAULT priority doesn't trigger heads-up notifications, but Chrome's priority 0 triggers pop-up. So it looks better to map Android's DEFAULT to Chrome's -1. BUG=b/28409430 ========== to ========== ARC: Support Android Notification Priority Previously Android notification priority was just ignored. This patch supports it and Chrome notification reflects Android notification priority. BUG=b/28409430 ==========
FYI: I've just updated the title and the description of the patch.
https://codereview.chromium.org/1924213002/diff/1/ui/arc/notification/arc_not... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1924213002/diff/1/ui/arc/notification/arc_not... ui/arc/notification/arc_notification_item.cc:42: int convertAndroidPriority(const int androidPriority) { Could you comment your intention of this mapping? https://codereview.chromium.org/1924213002/diff/1/ui/arc/notification/arc_not... ui/arc/notification/arc_notification_item.cc:55: return 0; Maybe -1 not to show the popup?
Patchset #3 (id:40001) has been deleted
Hidehiko-san, PTAL. Thanks. https://codereview.chromium.org/1924213002/diff/1/ui/arc/notification/arc_not... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1924213002/diff/1/ui/arc/notification/arc_not... ui/arc/notification/arc_notification_item.cc:42: int convertAndroidPriority(const int androidPriority) { On 2016/04/28 17:07:48, hidehiko wrote: > Could you comment your intention of this mapping? Done. https://codereview.chromium.org/1924213002/diff/1/ui/arc/notification/arc_not... ui/arc/notification/arc_notification_item.cc:55: return 0; On 2016/04/28 17:07:48, hidehiko wrote: > Maybe -1 not to show the popup? I think the default priority (= 0) should be used since this is invalid android priority.
LGTM with nitpick. https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:42: // Convert from Android notification priority to Chrome notification priority. nit: s/Convert/Converts/ https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:43: int convertAndroidPriority(const int androidPriority) { nit: s/androidPriority/android_priority/ for chromium style. https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:49: // Mapped to Chrome's -1, not to pop up the notification. How about; On Android, PRIORITY_DEFAULT does not pop up, so this maps PRIORITY_DEFAULT to Chrome's -1 to adapt that behavior. Also, this maps PRIORITY_LOW and _HIGH to -2 and 0 respectively to adjust the value with keeping the order among _LOW, _DEFAULT and _HIGH. or something in function comment? https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:56: NOTREACHED() << "Invalid Priority"; nit: NOTREACHED() << "Invalid Priority: " << android_priority; would help debugging on some mysterious error case.
https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:42: // Convert from Android notification priority to Chrome notification priority. On 2016/05/01 17:13:36, hidehiko wrote: > nit: s/Convert/Converts/ Done. https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:43: int convertAndroidPriority(const int androidPriority) { On 2016/05/01 17:13:35, hidehiko wrote: > nit: s/androidPriority/android_priority/ for chromium style. Thanks, I confused. Fixed. https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:49: // Mapped to Chrome's -1, not to pop up the notification. On 2016/05/01 17:13:36, hidehiko wrote: > How about; > > On Android, PRIORITY_DEFAULT does not pop up, so this maps PRIORITY_DEFAULT to > Chrome's -1 to adapt that behavior. Also, this maps PRIORITY_LOW and _HIGH to -2 > and 0 respectively to adjust the value with keeping the order among _LOW, > _DEFAULT and _HIGH. > > or something in function comment? Thank you for suggestion. Done. https://codereview.chromium.org/1924213002/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_notification_item.cc:56: NOTREACHED() << "Invalid Priority"; On 2016/05/01 17:13:35, hidehiko wrote: > nit: > > NOTREACHED() << "Invalid Priority: " << android_priority; > > would help debugging on some mysterious error case. It should be useful. Done.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/1924213002/#ps80001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924213002/80001
Message was sent while issue was closed.
Description was changed from ========== ARC: Support Android Notification Priority Previously Android notification priority was just ignored. This patch supports it and Chrome notification reflects Android notification priority. BUG=b/28409430 ========== to ========== ARC: Support Android Notification Priority Previously Android notification priority was just ignored. This patch supports it and Chrome notification reflects Android notification priority. BUG=b/28409430 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== ARC: Support Android Notification Priority Previously Android notification priority was just ignored. This patch supports it and Chrome notification reflects Android notification priority. BUG=b/28409430 ========== to ========== ARC: Support Android Notification Priority Previously Android notification priority was just ignored. This patch supports it and Chrome notification reflects Android notification priority. BUG=b/28409430 Committed: https://crrev.com/6397aeb42297802661ff1f3ea73d735d881a73cc Cr-Commit-Position: refs/heads/master@{#390898} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6397aeb42297802661ff1f3ea73d735d881a73cc Cr-Commit-Position: refs/heads/master@{#390898} |
