|
|
Created:
3 years, 9 months ago by yhanada Modified:
3 years, 8 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, Peter Beverloo, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, awdf+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), mlamouri+watch-notifications_chromium.org, qsr+mojo_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtend notifications.mojom for expandable notifications.
- Add |TOGGLE_EXPANSION| value to ArcNotificationEvent enum.
- Add |expand_state| value to ArcNotificationData.
BUG=703989
Review-Url: https://codereview.chromium.org/2765923003
Cr-Commit-Position: refs/heads/master@{#460062}
Committed: https://chromium.googlesource.com/chromium/src/+/c787104b181d6ca1356c1f0b82257ed5a62ad633
Patch Set 1 #Patch Set 2 #
Total comments: 1
Patch Set 3 : delete message_center::NotificationExpandState #
Total comments: 1
Patch Set 4 : mark as Extensible #Patch Set 5 : add Minversion to the enum #
Messages
Total messages: 38 (27 generated)
Description was changed from ========== Extend notifications.mojom for expandable notifications. - Add |TOGGLE_EXPANSION| value to ArcNotificationEvent enum. - Add |expandable| flag to ArcNotificationData. BUG=703989 ========== to ========== Extend notifications.mojom for expandable notifications. - Add |TOGGLE_EXPANSION| value to ArcNotificationEvent enum. - Add |expand_state| value to ArcNotificationData. BUG=703989 ==========
The CQ bit was checked by yhanada@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...
yhanada@chromium.org changed reviewers: + hidehiko@chromium.org, yoshiki@chromium.org
Hi, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yhanada@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.
https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... File ui/arc/notification/arc_custom_notification_item.cc (right): https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... ui/arc/notification/arc_custom_notification_item.cc:53: message_center::NotificationExpandState ToNotificationExpandState( I think it's enough to use the mojo enum values (mojom::ArcNotificationExpandState) directly than converting values from mojo to chrome (messagece_center::NotificationExpandState). We already use mojo values directly at many places in ui/arc. If you want to having conversion, we should write typemaps and converter (see. components/arc/bluetooth/bluetooth_type_converters.cc for example).
On 2017/03/24 10:47:28, yoshiki wrote: > https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... > File ui/arc/notification/arc_custom_notification_item.cc (right): > > https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... > ui/arc/notification/arc_custom_notification_item.cc:53: > message_center::NotificationExpandState ToNotificationExpandState( > I think it's enough to use the mojo enum values > (mojom::ArcNotificationExpandState) directly than converting values from mojo to > chrome (messagece_center::NotificationExpandState). We already use mojo values > directly at many places in ui/arc. > > If you want to having conversion, we should write typemaps and converter (see. > components/arc/bluetooth/bluetooth_type_converters.cc for example). Thank you for reviewing. Is is ok to make ui/message_center depend on components/arc? If yes, I'll use mojo enums directly.
On 2017/03/24 10:51:05, yhanada wrote: > On 2017/03/24 10:47:28, yoshiki wrote: > > > https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... > > File ui/arc/notification/arc_custom_notification_item.cc (right): > > > > > https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... > > ui/arc/notification/arc_custom_notification_item.cc:53: > > message_center::NotificationExpandState ToNotificationExpandState( > > I think it's enough to use the mojo enum values > > (mojom::ArcNotificationExpandState) directly than converting values from mojo > to > > chrome (messagece_center::NotificationExpandState). We already use mojo values > > directly at many places in ui/arc. > > > > If you want to having conversion, we should write typemaps and converter (see. > > components/arc/bluetooth/bluetooth_type_converters.cc for example). > > Thank you for reviewing. > Is is ok to make ui/message_center depend on components/arc? If yes, I'll use > mojo enums directly. Is it necessary for RichNotificationData to have expand_state? I don't think ui/message_center needs to have expand_state. If you have a plan to use expand_state from ui/message_center in near future, the converting is ok.
The CQ bit was checked by yhanada@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...
On 2017/03/24 13:42:08, yoshiki wrote: > On 2017/03/24 10:51:05, yhanada wrote: > > On 2017/03/24 10:47:28, yoshiki wrote: > > > > > > https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... > > > File ui/arc/notification/arc_custom_notification_item.cc (right): > > > > > > > > > https://codereview.chromium.org/2765923003/diff/20001/ui/arc/notification/arc... > > > ui/arc/notification/arc_custom_notification_item.cc:53: > > > message_center::NotificationExpandState ToNotificationExpandState( > > > I think it's enough to use the mojo enum values > > > (mojom::ArcNotificationExpandState) directly than converting values from > mojo > > to > > > chrome (messagece_center::NotificationExpandState). We already use mojo > values > > > directly at many places in ui/arc. > > > > > > If you want to having conversion, we should write typemaps and converter > (see. > > > components/arc/bluetooth/bluetooth_type_converters.cc for example). > > > > Thank you for reviewing. > > Is is ok to make ui/message_center depend on components/arc? If yes, I'll use > > mojo enums directly. > > Is it necessary for RichNotificationData to have expand_state? I don't think > ui/message_center needs to have expand_state. > > If you have a plan to use expand_state from ui/message_center in near future, > the converting is ok. Ah, I assumed we can't pass data to ArcCustomNotificationItem only through message_center::Notification. Just removed message_center::NotificationExpandState. Thank you for pointing out!
Thanks! It gets much simpler! LGTM. But please get approvals from an ARC owner (hidehiko or someone) and an IPC owner for mojo change.
yhanada@chromium.org changed reviewers: + dcheng@chromium.org
hidehide@: Could you review changes in components/arc as an OWNER? dcheng@: Could you review changes in components/arc/common/notifications.mojom?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2765923003/diff/40001/components/arc/common/n... File components/arc/common/notifications.mojom (right): https://codereview.chromium.org/2765923003/diff/40001/components/arc/common/n... components/arc/common/notifications.mojom:41: enum ArcNotificationExpandState { Is the general rule for [Extensible] that we mark enums that we send to the ARC++ container extensible?
On 2017/03/28 02:29:25, dcheng wrote: > LGTM > > https://codereview.chromium.org/2765923003/diff/40001/components/arc/common/n... > File components/arc/common/notifications.mojom (right): > > https://codereview.chromium.org/2765923003/diff/40001/components/arc/common/n... > components/arc/common/notifications.mojom:41: enum ArcNotificationExpandState { > Is the general rule for [Extensible] that we mark enums that we send to the > ARC++ container extensible? I marked it as Extensible. Thank you for pointing it out!
The CQ bit was checked by yhanada@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 yhanada@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 yhanada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoshiki@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2765923003/#ps80001 (title: "add Minversion to the enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490701817177180, "parent_rev": "ce4d88aebc3c49570ad75ceb0877e74c72aa4a4d", "commit_rev": "c787104b181d6ca1356c1f0b82257ed5a62ad633"}
Message was sent while issue was closed.
Description was changed from ========== Extend notifications.mojom for expandable notifications. - Add |TOGGLE_EXPANSION| value to ArcNotificationEvent enum. - Add |expand_state| value to ArcNotificationData. BUG=703989 ========== to ========== Extend notifications.mojom for expandable notifications. - Add |TOGGLE_EXPANSION| value to ArcNotificationEvent enum. - Add |expand_state| value to ArcNotificationData. BUG=703989 Review-Url: https://codereview.chromium.org/2765923003 Cr-Commit-Position: refs/heads/master@{#460062} Committed: https://chromium.googlesource.com/chromium/src/+/c787104b181d6ca1356c1f0b8225... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/c787104b181d6ca1356c1f0b8225... |