Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1475583002:
Add IPC messages for ARC notification (Closed)
DescriptionAdd IPC messages for ARC notification
This change comes along with:
- http://crrev.com/1475583002 (ArcBridge part, this patch)
- http://crrev.com/1477733002 (UI part)
- http://ag/824594
- http://ag/824724
- http://ag/818913
These patch adds minimal support of new ARC notification:
- Showing notifications
- Closing ARC notifications from either side
This chromium-side change should do nothing until the android side change
is landed.
BUG=560682
Committed: https://crrev.com/89ba62b6ed3836eca44b5e7f13a178453f1f2693
Cr-Commit-Position: refs/heads/master@{#362987}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed the comments and splitted the patch #
Total comments: 6
Patch Set 3 : Addressed the comments #
Total comments: 12
Patch Set 4 : rebase #Patch Set 5 : Addressed comments #
Total comments: 18
Patch Set 6 : Addressed the comments #
Total comments: 6
Patch Set 7 : Addressed the comments #
Total comments: 6
Patch Set 8 : Addressed the comments #Patch Set 9 : Rebased #
Messages
Total messages: 60 (34 generated)
Description was changed from ========== wip: add arc-notification BUG= ========== to ========== Minimal support of new ARC notification (chromium-side) This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/817945/ https://googleplex-android-review.git.corp.google.com/#/c/818913/ These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
yoshiki@chromium.org changed reviewers: + khmel@chromium.org
Description was changed from ========== Minimal support of new ARC notification (chromium-side) This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/817945/ https://googleplex-android-review.git.corp.google.com/#/c/818913/ These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (chromium-side) This CL comes along with: - crbug.com/1475583002 - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Yury, could you take a look? Thanks.
khmel@google.com changed reviewers: + khmel@google.com
I reviewed you CL and it is look OK for me in general. However please add right reviewers from the OWNERS files. I would recommend to split your review off. One is for ArcBridgeService and second is for UI Items in Chrome. https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:70: ImageDecoder::Cancel(this); // just in case It is automatically canceled on DTOR https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:18: ArcNotificationManager::~ArcNotificationManager() {} No need to RemoveObserver(this) ? https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:24: items_.insert(make_pair(data.key, item)); What will be in case if manager deleted with pending items? It seems potential memory leak. Is any scoped ptr solution better? https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:42: if (it != items_.end()) { Log error in case not found? https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:56: LOG(ERROR) << "CLIOCKE: " << key; Debug print? https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/chr... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/chr... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:402: arc_notification_manager_.reset(new ArcNotificationManager()); Would it be better to pass ArcBridgeService in CTOR once you create your manager here? https://codereview.chromium.org/1475583002/diff/1/chrome/chrome_browser_chrom... File chrome/chrome_browser_chromeos.gypi (right): https://codereview.chromium.org/1475583002/diff/1/chrome/chrome_browser_chrom... chrome/chrome_browser_chromeos.gypi:55: 'browser/chromeos/arc/notification/arc_notification_item.cc', They must be under define enable_arc https://codereview.chromium.org/1475583002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1475583002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service.cc:260: FOR_EACH_OBSERVER(Observer, observer_list_, DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()) ? and below https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_h... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_h... components/arc/common/arc_host_messages.h:18: IPC_MESSAGE_CONTROL1(ArcInstanceHostMsg_NotificationPosted, Comments are expected here, please take a look at ArcInstanceMsg_RegisterInputDevice for example https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_i... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_i... components/arc/common/arc_instance_messages.h:29: IPC_ENUM_TRAITS_MAX_VALUE(arc::NotificationEvent, Comments
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Minimal support of new ARC notification (chromium-side) This CL comes along with: - crbug.com/1475583002 - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (arcbridge ) This CL comes along with: - crrev.com/1475583002 (arc-bridge change) - crrev.com/1477733002 (UI change) - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Description was changed from ========== Minimal support of new ARC notification (arcbridge ) This CL comes along with: - crrev.com/1475583002 (arc-bridge change) - crrev.com/1477733002 (UI change) - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (arcbridge part) This CL comes along with: - crrev.com/1475583002 (arc-bridge change) - crrev.com/1477733002 (UI change) - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Patchset #2 (id:40001) has been deleted
yoshiki@chromium.org changed reviewers: + elijahtaylor@chromium.org
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Minimal support of new ARC notification (arcbridge part) This CL comes along with: - crrev.com/1475583002 (arc-bridge change) - crrev.com/1477733002 (UI change) - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (ArcBridge part) This CL comes along with: - crrev.com/1475583002 (ArcBridge part) - crrev.com/1477733002 (UI part) - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
yoshiki@chromium.org changed reviewers: - elijahtaylor@chromium.org
I removed the UI part from this patch, and will send it review later: crrev.com/1477733002. I just wanted you to review generally as a local reviewer before asking the owner. Thanks! https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:70: ImageDecoder::Cancel(this); // just in case On 2015/11/25 00:30:10, khmel1 wrote: > It is automatically canceled on DTOR Done. https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:18: ArcNotificationManager::~ArcNotificationManager() {} On 2015/11/25 00:30:10, khmel1 wrote: > No need to RemoveObserver(this) ? Done. https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:24: items_.insert(make_pair(data.key, item)); On 2015/11/25 00:30:10, khmel1 wrote: > What will be in case if manager deleted with pending items? It seems potential > memory leak. Is any scoped ptr solution better? Done. https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:42: if (it != items_.end()) { On 2015/11/25 00:30:10, khmel1 wrote: > Log error in case not found? This method may be called during OnNotificationRemovedFromAndroid(), and in such case the item is always not found. Of cause we can detect such false removal. But I don't think we need to add a logic only for debug messages here. https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:56: LOG(ERROR) << "CLIOCKE: " << key; On 2015/11/25 00:30:10, khmel1 wrote: > Debug print? Done. https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/chr... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1475583002/diff/1/chrome/browser/chromeos/chr... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:402: arc_notification_manager_.reset(new ArcNotificationManager()); On 2015/11/25 00:30:10, khmel1 wrote: > Would it be better to pass ArcBridgeService in CTOR once you create your manager > here? Done. https://codereview.chromium.org/1475583002/diff/1/chrome/chrome_browser_chrom... File chrome/chrome_browser_chromeos.gypi (right): https://codereview.chromium.org/1475583002/diff/1/chrome/chrome_browser_chrom... chrome/chrome_browser_chromeos.gypi:55: 'browser/chromeos/arc/notification/arc_notification_item.cc', On 2015/11/25 00:30:10, khmel1 wrote: > They must be under define enable_arc Done. https://codereview.chromium.org/1475583002/diff/1/components/arc/arc_bridge_s... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1475583002/diff/1/components/arc/arc_bridge_s... components/arc/arc_bridge_service.cc:260: FOR_EACH_OBSERVER(Observer, observer_list_, On 2015/11/25 00:30:10, khmel1 wrote: > DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()) ? > and below Done. https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_h... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_h... components/arc/common/arc_host_messages.h:18: IPC_MESSAGE_CONTROL1(ArcInstanceHostMsg_NotificationPosted, On 2015/11/25 00:30:10, khmel1 wrote: > Comments are expected here, please take a look at > ArcInstanceMsg_RegisterInputDevice for example Done. https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_i... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475583002/diff/1/components/arc/common/arc_i... components/arc/common/arc_instance_messages.h:29: IPC_ENUM_TRAITS_MAX_VALUE(arc::NotificationEvent, On 2015/11/25 00:30:10, khmel1 wrote: > Comments Done.
yoshiki@chromium.org changed reviewers: + elijahtaylor@chromium.org
Elijah, could you take a look, or add any appropriate reviewers? Thanks.
Description was changed from ========== Minimal support of new ARC notification (ArcBridge part) This CL comes along with: - crrev.com/1475583002 (ArcBridge part) - crrev.com/1477733002 (UI part) - ag/817945 - ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (ArcBridge part) This CL comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Description was changed from ========== Minimal support of new ARC notification (ArcBridge part) This CL comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (ArcBridge part) This CL comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Description was changed from ========== Minimal support of new ARC notification (ArcBridge part) This CL comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Minimal support of new ARC notification (ArcBridge part) This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
yoshiki@chromium.org changed reviewers: + hidehiko@chromium.org
+hidehiko-san PTAL.
I'd recommend you to add more descriptive title. E.g., "Add IPC messages for ARC notification." or something? https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... File components/arc/common/arc_message_param_traits.h (right): https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... components/arc/common/arc_message_param_traits.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. IPC_STRUCT_TRAITS_{BEGIN,END,MEMBER} macros are your friends. https://code.google.com/p/chromium/codesearch#chromium/src/ipc/param_traits_m... Also, you may be interested in IPC_ENUM_TRAITS* family. You can use https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/co... as a reference, I think. https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... components/arc/common/arc_notification_types.h:41: virtual ~ArcNotificationData(); s/virtual //
One more thing. https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... components/arc/common/arc_notification_types.h:25: NOTIFICATION_EVENT_CLOSED = 6, If the number of BUTTONs can be grown, how about move CLOSED to 1, and shift all BUTTON events? Note: CLOSED event in your Java code in another CL is different from this.
Description was changed from ========== Minimal support of new ARC notification (ArcBridge part) This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Hidehiko-san, PTAL. Thanks. https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... File components/arc/common/arc_message_param_traits.h (right): https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... components/arc/common/arc_message_param_traits.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/11/26 03:39:41, hidehiko wrote: > IPC_STRUCT_TRAITS_{BEGIN,END,MEMBER} macros are your friends. > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/param_traits_m... > > Also, you may be interested in IPC_ENUM_TRAITS* family. > > You can use > https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/co... > as a reference, I think. Thank you for letting me know! I updated the code with macros. https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... components/arc/common/arc_notification_types.h:25: NOTIFICATION_EVENT_CLOSED = 6, On 2015/11/26 05:48:28, hidehiko wrote: > If the number of BUTTONs can be grown, how about move CLOSED to 1, and shift all > BUTTON events? Done. > Note: CLOSED event in your Java code in another CL is different from this. Thanks. I'll fix the patch of android side. https://codereview.chromium.org/1475583002/diff/80001/components/arc/common/a... components/arc/common/arc_notification_types.h:41: virtual ~ArcNotificationData(); On 2015/11/26 03:39:41, hidehiko wrote: > s/virtual // Done.
Almost looks good. Now, arc_bridge_service's refactoring is landed. Could you rebase on ToT? https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service.h:126: // Notifys a notification event to Android side. nit: Notifies.
https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:137: bool ArcBridgeService::NotifyNotificationEvent(const std::string& key, "NotifyNotification" is kind of confusing to put together, and it's not clear from this method name the direction of things. Maybe, SendNotificationEventToAndroid? https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:141: LOG(ERROR) << "Called RegisterInputDevice when the service is not ready"; copy/paste error with RegisterInputDevice Maybe we should wait for the mojo switch, but we could probably reduce some of the boilerplate here by centralizing state/method checking to make sure we're in a consistent state https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... components/arc/common/arc_host_messages.h:35: // Notifies that a notification is posted (created or updated) on the container s/on the container side/from Android/ https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... components/arc/common/arc_host_messages.h:41: // Notifies that a notification is removed on the container side. same https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... components/arc/common/arc_instance_messages.h:33: // Sends an event from Chrome notification UI to the container. s/the container/Android
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Hidehiko-san, Elijah, PTAL. Thanks. https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:137: bool ArcBridgeService::NotifyNotificationEvent(const std::string& key, On 2015/11/27 05:42:49, elijahtaylor1 wrote: > "NotifyNotification" is kind of confusing to put together, and it's not clear > from this method name the direction of things. Maybe, > SendNotificationEventToAndroid? Done. https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service.cc:141: LOG(ERROR) << "Called RegisterInputDevice when the service is not ready"; On 2015/11/27 05:42:49, elijahtaylor1 wrote: > copy/paste error with RegisterInputDevice > > Maybe we should wait for the mojo switch, but we could probably reduce some of > the boilerplate here by centralizing state/method checking to make sure we're in > a consistent state I created a SendMessageIfReady(Message*) method, which sends a message only if it's ready. How about this? https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service.h:126: // Notifys a notification event to Android side. On 2015/11/26 10:47:09, hidehiko wrote: > nit: Notifies. Done. https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... components/arc/common/arc_host_messages.h:35: // Notifies that a notification is posted (created or updated) on the container On 2015/11/27 05:42:49, elijahtaylor1 wrote: > s/on the container side/from Android/ Done. https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... components/arc/common/arc_host_messages.h:41: // Notifies that a notification is removed on the container side. On 2015/11/27 05:42:49, elijahtaylor1 wrote: > same Done. https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475583002/diff/100001/components/arc/common/... components/arc/common/arc_instance_messages.h:33: // Sends an event from Chrome notification UI to the container. On 2015/11/27 05:42:49, elijahtaylor1 wrote: > s/the container/Android Done.
lgtm. Defer to elijahtaylor@ and khmel@. You'll also need a code review from the security team for IPC message change. https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:140: // Notifies a notification event to Android side. nit: s/Notifies/Sends/ https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:39: // Notifies that a notification is posted (created or updated) on Android with nit: Similar to the method name, it'd be probably easier to understand if you can avoid using a word "notify" under two context. Maybe "Tells the Chrome that a notification is posted (created or updated) on Android. |notification_data| is the contents and metadata of the notification." etc.? https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:46: // |key| is the identifier of notification. nit: "the notification"
https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:81: virtual void OnNotificationPostedFromAndroid( Please pay attention that I added AppsObserver in my CL (still under review). Probably it makes sens to add NotifyObserver. Having one huge observer might be inconvenient and have performance implication. https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:26: IPC_STRUCT_TRAITS_BEGIN(arc::ArcNotificationData) In my CL I put similar code into arc_messages_traits.h, however not sure what is standard in Chromium? https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_notification_types.h:38: class ArcNotificationData { Just question, do we really need arc::ArcNotificationData. Could arc::NotificationData be simpler?
lgtm but you'll need a chrome security reviewer for messages.h https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:111: LOG(ERROR) << "NotifyNotificationEvent failed: Wrong parameter"; method rename in log message https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:115: new ArcInstanceMsg_NotifyNotificationEvent(key, event)); I think the method name change should go all the way through to the message, ArcInstanceMsg_SendNotificationEventToAndroid, or if you think that's too redundant, get rid of ToAndroid. https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:205: LOG(ERROR) << "Tried to send a message but the service is not ready"; too bad we lose the actual method name here, is there any printable information from |message| we could add to this log?
Thank you for reviewing! https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:81: virtual void OnNotificationPostedFromAndroid( On 2015/12/01 06:44:51, khmel1 wrote: > Please pay attention that I added AppsObserver in my CL (still under review). > Probably it makes sens to add NotifyObserver. Having one huge observer might be > inconvenient and have performance implication. Done. I added NotificationObserver. https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:140: // Notifies a notification event to Android side. On 2015/12/01 06:35:43, hidehiko wrote: > nit: s/Notifies/Sends/ Done. https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:111: LOG(ERROR) << "NotifyNotificationEvent failed: Wrong parameter"; On 2015/12/01 07:02:36, elijahtaylor1 wrote: > method rename in log message Done. https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:115: new ArcInstanceMsg_NotifyNotificationEvent(key, event)); On 2015/12/01 07:02:36, elijahtaylor1 wrote: > I think the method name change should go all the way through to the message, > ArcInstanceMsg_SendNotificationEventToAndroid, or if you think that's too > redundant, get rid of ToAndroid. Done. https://codereview.chromium.org/1475583002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:205: LOG(ERROR) << "Tried to send a message but the service is not ready"; On 2015/12/01 07:02:36, elijahtaylor1 wrote: > too bad we lose the actual method name here, is there any printable information > from |message| we could add to this log? Let me revert back here to the original. I think creating this method is too much abstraction as for now. Let's do this when necessary. https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:26: IPC_STRUCT_TRAITS_BEGIN(arc::ArcNotificationData) On 2015/12/01 06:44:51, khmel1 wrote: > In my CL I put similar code into arc_messages_traits.h, however not sure what is > standard in Chromium? I think this can be in this file, since there is only one struct trait and this is only used by the host messages. Please feel free to move this when you add more traits. https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:39: // Notifies that a notification is posted (created or updated) on Android with On 2015/12/01 06:35:43, hidehiko wrote: > nit: Similar to the method name, it'd be probably easier to understand if you > can avoid using a word "notify" under two context. Maybe > > "Tells the Chrome that a notification is posted (created or updated) on Android. > |notification_data| is the contents and metadata of the notification." > > etc.? Done. https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:46: // |key| is the identifier of notification. On 2015/12/01 06:35:43, hidehiko wrote: > nit: "the notification" Done. https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/180001/components/arc/common/... components/arc/common/arc_notification_types.h:38: class ArcNotificationData { On 2015/12/01 06:44:51, khmel1 wrote: > Just question, do we really need arc::ArcNotificationData. Could > arc::NotificationData be simpler? I use "Arc" because "ArcBridgeService" has "Arc".
yoshiki@chromium.org changed reviewers: + dcheng@chromium.org, kenrb@chromium.org
dcheng, kenrb: Can I get a review on my changes to the IPC message in components/arc/common?
https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... components/arc/common/arc_notification_types.h:17: enum ArcNotificationEvent { enum class for both these enums, since they don't appear to be bitmasks. https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... components/arc/common/arc_notification_types.h:48: std::vector<uint8_t> icon_data; Where will this image be decoded? https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... components/arc/common/arc_notification_types.h:50: base::Time timestamp; base::Time can go backwards. What's the intended purpose of this timestamp?
dcheng, PTAL. https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... components/arc/common/arc_notification_types.h:17: enum ArcNotificationEvent { On 2015/12/02 02:42:58, dcheng wrote: > enum class for both these enums, since they don't appear to be bitmasks. Done. https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... components/arc/common/arc_notification_types.h:48: std::vector<uint8_t> icon_data; On 2015/12/02 02:42:58, dcheng wrote: > Where will this image be decoded? These data are used by the UI part patch: http://crrev.com/1477733002. https://codereview.chromium.org/1475583002/diff/200001/components/arc/common/... components/arc/common/arc_notification_types.h:50: base::Time timestamp; On 2015/12/02 02:42:58, dcheng wrote: > base::Time can go backwards. What's the intended purpose of this timestamp? Changed the name to just "time". There was no strong reason. That was because we have a property named "timestamp" in message_center::RichNotification class and I reused that.
lgtm for IPC I'm still vaguely uncomfortable with image decoding happening 'at a distance'. I'd prefer to see it moved into the IPC message filter here and for https://codereview.chromium.org/1475563002 https://codereview.chromium.org/1475583002/diff/220001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service.h:83: class NotificationObserver { With https://codereview.chromium.org/1475563002, there will now be 3 different observer types for ARC... https://codereview.chromium.org/1475583002/diff/220001/components/arc/common/... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/220001/components/arc/common/... components/arc/common/arc_notification_types.h:18: NOTIFICATION_EVENT_BODY_CLICKED = 0, Since these are enum class, you don't have to namespace the enum values. Just BODY_CLICKED, CLOSED, BUTTON1_CLICKED, etc. https://codereview.chromium.org/1475583002/diff/220001/components/arc/common/... components/arc/common/arc_notification_types.h:32: NOTIFICATION_TYPE_BASIC = 0, Ditto.
Patchset #8 (id:240001) has been deleted
Patchset #9 (id:280001) has been deleted
Patchset #9 (id:300001) has been deleted
> I'm still vaguely uncomfortable with image decoding happening 'at a distance'. > I'd prefer to see it moved into the IPC message filter here and for > https://codereview.chromium.org/1475563002 The icon image related may be changed in near feature. Let me keep as is, and do that later. Thanks for giving me advice!
https://codereview.chromium.org/1475583002/diff/220001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475583002/diff/220001/components/arc/arc_bri... components/arc/arc_bridge_service.h:83: class NotificationObserver { On 2015/12/02 18:26:53, dcheng wrote: > With https://codereview.chromium.org/1475563002, there will now be 3 different > observer types for ARC... Yes, and we'll have more type of observers, but I think it is better rather than one shared giant observer class in terms of performance. https://codereview.chromium.org/1475583002/diff/220001/components/arc/common/... File components/arc/common/arc_notification_types.h (right): https://codereview.chromium.org/1475583002/diff/220001/components/arc/common/... components/arc/common/arc_notification_types.h:18: NOTIFICATION_EVENT_BODY_CLICKED = 0, On 2015/12/02 18:26:54, dcheng wrote: > Since these are enum class, you don't have to namespace the enum values. Just > BODY_CLICKED, CLOSED, BUTTON1_CLICKED, etc. Done. https://codereview.chromium.org/1475583002/diff/220001/components/arc/common/... components/arc/common/arc_notification_types.h:32: NOTIFICATION_TYPE_BASIC = 0, On 2015/12/02 18:26:54, dcheng wrote: > Ditto. Done.
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, elijahtaylor@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1475583002/#ps320001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475583002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475583002/320001
Description was changed from ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/817945 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
The CQ bit was unchecked by yoshiki@chromium.org
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475583002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475583002/320001
Description was changed from ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824594 - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
The CQ bit was unchecked by yoshiki@chromium.org
The CQ bit was checked by yoshiki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475583002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475583002/320001
Message was sent while issue was closed.
Description was changed from ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824594 - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824594 - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824594 - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 ========== to ========== Add IPC messages for ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part, this patch) - http://crrev.com/1477733002 (UI part) - http://ag/824594 - http://ag/824724 - http://ag/818913 These patch adds minimal support of new ARC notification: - Showing notifications - Closing ARC notifications from either side This chromium-side change should do nothing until the android side change is landed. BUG=560682 Committed: https://crrev.com/89ba62b6ed3836eca44b5e7f13a178453f1f2693 Cr-Commit-Position: refs/heads/master@{#362987} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/89ba62b6ed3836eca44b5e7f13a178453f1f2693 Cr-Commit-Position: refs/heads/master@{#362987} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
