Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1477733002:
Add ArcNotificationManager for new ARC notification (Closed)
|
Created:
5 years ago by yoshiki Modified:
4 years, 11 months ago Reviewers:
elijahtaylor1, stevenjb, Roger Tawa OOO till Jul 10th, sky, xiyuan, Luis Héctor Chávez, jam, reed2, dcheng, danakj, hidehiko, reed1 CC:
davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ArcNotificationManager for new ARC notification
This change comes along with:
- http://crrev.com/1475583002 (ArcBridge part)
- http://crrev.com/1477733002 (UI part, this patch)
- 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/5d4f913c38b5a225caf110f3c280cb426f424f24
Cr-Commit-Position: refs/heads/master@{#368924}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Add OWNERS file #Patch Set 4 : Fix comment #
Total comments: 24
Patch Set 5 : Rebase to mojo and addressed comments #Patch Set 6 : Add a TODO comment about ArcServiceManager #
Total comments: 24
Patch Set 7 : Addressed comments #Patch Set 8 : Moved the notification stuffs to components/arc. #Patch Set 9 : . #
Total comments: 38
Patch Set 10 : Addressed comments #
Total comments: 4
Patch Set 11 : Addressed comments #Patch Set 12 : Rebased and formatted #
Total comments: 26
Patch Set 13 : Addressed comments #Patch Set 14 : Rebase #
Total comments: 10
Patch Set 15 : rebase #Patch Set 16 : Moved to ui/arc and Addressed comments #
Total comments: 19
Patch Set 17 : Addressed comments #
Total comments: 8
Patch Set 18 : Addressed comments #
Total comments: 10
Patch Set 19 : Addressed comments (#85) #Patch Set 20 : Rebased #
Total comments: 4
Patch Set 21 : Addressed comments (#88) #Patch Set 22 : Rebased #Patch Set 23 : Sync dependencies #Messages
Total messages: 110 (50 generated)
Description was changed from ========== wip BUG= ========== to ========== wip: Minimal support of new ARC notification (chromium UI side) This CL comes along with: - crrev.com/1475583002 - crrev.com/1477733002 - 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 ========== wip: Minimal support of new ARC notification (chromium UI side) This CL comes along with: - crrev.com/1475583002 - crrev.com/1477733002 - 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 ========== wip: Minimal support of new ARC notification (chromium UI side) 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 ========== wip: Minimal support of new ARC notification (chromium UI side) 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 ========== wip: Minimal support of new ARC notification (chromium UI side) This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== wip: Minimal support of new ARC notification (chromium UI side) This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ========== wip: Minimal support of new ARC notification (chromium UI side) This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ==========
Patchset #2 (id:40001) has been deleted
Description was changed from ========== wip: Minimal support of new ARC notification (chromium UI side) This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ArcNotificationManager for new ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ==========
yoshiki@chromium.org changed reviewers: + elijahtaylor@chromium.org, hidehiko@chromium.org
Elijah, Hidehiko-san, PTAL. Thanks.
https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:25: const char kArcNotificationOrigin[] = "chrome://arc"; what is the implication of using something like this? I'm concerned because chrome://arc isn't a real origin (and I could imagine maybe creating it some day). Is there any other dummy or null data we could pass in? https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:38: bool HasClickedListener() override { return true; } what is this for? maybe a comment? https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:46: // The destractor is private since this classes is ref-counted. s/classes/class/ https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:67: DCHECK(arc::ArcBridgeService::Get()); nothing in this class uses ArcBridgeService, what is this protecting against? https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:103: 0.5); what is the 0.5 for? https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:18: DCHECK(arc::ArcBridgeService::Get()); comments on https://codereview.chromium.org/1495723004/ suggest avoiding getters like this, instead preferring to maintain proper relationships and lifetimes between objects. What are you testing anyway with this, do we expect the bridge service to change? It's also weird because you do have an arc_bridge_ member but below use the global getter instead https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:25: // This should be free'd before ArcBridgeService. is this actually guaranteed? https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:34: // TODO(yoshiki): Consider multi-user. I don't know how multi-user works but I would assume a different profile (and maybe browser process) so maybe nothing needs to be done? Please talk to someone before putting these comments in https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:50: VLOG(3) << "Android requested to remove the notification (key: " << key s/the/a/ https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:51: << "), but it has already gone."; s/has/is/ https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.h:32: void NotifyNotificationRemovedFromChrome(const std::string& key); same with the other CL, I think NotifyNotification is confusing, would prefer SendNotification https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:650: new ArcNotificationManager(arc_bridge_service_.get(), profile())); is the main reason ArcNotificationManager lives in chrome/browser/chromeos/arc instead of components/arc because we need the profile for its initialization? Or is there a deeper integration to chrome/browser that might cause a layering/dependency violation from components/ ? We've been consolidating ARC related services in components/arc/arc_service_manager (see https://codereview.chromium.org/1496193002/), maybe we just need a post-profile initialization there as well and we can have this live there too.
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:25: const char kArcNotificationOrigin[] = "chrome://arc"; On 2015/12/09 06:41:25, elijahtaylor1 wrote: > what is the implication of using something like this? I'm concerned because > chrome://arc isn't a real origin (and I could imagine maybe creating it some > day). Is there any other dummy or null data we could pass in? As I read the code, empty URL is acceptable. So I removed this. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:38: bool HasClickedListener() override { return true; } On 2015/12/09 06:41:25, elijahtaylor1 wrote: > what is this for? maybe a comment? Done. This changes the cursor on hover. I'll refine in future. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:46: // The destractor is private since this classes is ref-counted. On 2015/12/09 06:41:25, elijahtaylor1 wrote: > s/classes/class/ Done. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:67: DCHECK(arc::ArcBridgeService::Get()); On 2015/12/09 06:41:25, elijahtaylor1 wrote: > nothing in this class uses ArcBridgeService, what is this protecting against? Rdmoved. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:103: 0.5); On 2015/12/09 06:41:25, elijahtaylor1 wrote: > what is the 0.5 for? I intended just "round up". https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:18: DCHECK(arc::ArcBridgeService::Get()); On 2015/12/09 06:41:25, elijahtaylor1 wrote: > comments on https://codereview.chromium.org/1495723004/ suggest avoiding getters > like this, instead preferring to maintain proper relationships and lifetimes > between objects. > > What are you testing anyway with this, do we expect the bridge service to > change? It's also weird because you do have an arc_bridge_ member but below use > the global getter instead I want to ensure that this is initialized after the bridge service. So I changed this to check the given instance. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:25: // This should be free'd before ArcBridgeService. On 2015/12/09 06:41:26, elijahtaylor1 wrote: > is this actually guaranteed? It's guaranteed in the definition order in chrome_browser_main_chromeos.cc. The below DCHECK checks that. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:34: // TODO(yoshiki): Consider multi-user. On 2015/12/09 06:41:26, elijahtaylor1 wrote: > I don't know how multi-user works but I would assume a different profile (and > maybe browser process) so maybe nothing needs to be done? Please talk to > someone before putting these comments in I meant that "multi-user" is simultaneous multi-user login feature on Chrome OS. We must pass a profile ID, which is generated from profile, to a notification instance, to specify the target desktop. I'm not sure if ARC++ supports multi-user, but as for now, a notification should be shown only on the primary user's desktop. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:50: VLOG(3) << "Android requested to remove the notification (key: " << key On 2015/12/09 06:41:25, elijahtaylor1 wrote: > s/the/a/ Done. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:51: << "), but it has already gone."; On 2015/12/09 06:41:26, elijahtaylor1 wrote: > s/has/is/ Done. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.h:32: void NotifyNotificationRemovedFromChrome(const std::string& key); On 2015/12/09 06:41:26, elijahtaylor1 wrote: > same with the other CL, I think NotifyNotification is confusing, would prefer > SendNotification Done. https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:650: new ArcNotificationManager(arc_bridge_service_.get(), profile())); On 2015/12/09 06:41:26, elijahtaylor1 wrote: > is the main reason ArcNotificationManager lives in chrome/browser/chromeos/arc > instead of components/arc because we need the profile for its initialization? > Or is there a deeper integration to chrome/browser that might cause a > layering/dependency violation from components/ ? Currently notification needs Profile to determine a desktop to be shown on, as I wrote in the previous comment. And, to extract an image, the current ArcNotificationManager depends chrome::ImageDecoder, which decode images in sandbox (renderer process). So it's difficult to move it to under component/arc. > We've been consolidating ARC related services in > components/arc/arc_service_manager (see > https://codereview.chromium.org/1496193002/), maybe we just need a post-profile > initialization there as well and we can have this live there too. That's good. I'll make ArcNotificationManager managed by ArcServiceManager.
> > We've been consolidating ARC related services in > > components/arc/arc_service_manager (see > > https://codereview.chromium.org/1496193002/), maybe we just need a > post-profile > > initialization there as well and we can have this live there too. > > That's good. I'll make ArcNotificationManager managed by ArcServiceManager. I've just find ArcServiceManager can't manipulate ArcNotificationManager directly due to dependency violation. Let me do it later after a manager for services under C/B/chromeos/arc is introduced.
Elijah, Hidehiko-san, PTAL. Thanks.
I do not have a strong preference if the code should put into components/arc in this CL or later (defer to Elijah). However, if you choose later, could you file a bug at least? https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:37: // TODO(yoshiki): Return the corerct value according to the content intent nit: s/corerct/correct/ https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:67: notification_key_ = data.key; Please move them to the initializer list. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:70: UpdateWithArcNotificationData(data); IMHO, you can defer the responsibility to call this method to the caller. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:78: message_center::NotificationType type = I prefer move this to default:, as you set it in the "case arc::ARC_NOTIFICATION_TYPE_PROGRESS". https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:87: 0.5); // round off nit: std::round is your friend. BTW, the values, |progress_current| and |progress_max|, can be trusted? Otherwise, probably you may want to clamp with [0, 100] ? https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:137: message_center_->AddNotification(make_scoped_ptr( Looks race? - UpdateWithArcNotificationData(...). - Before decode is done OnClosedFromAndroid()/ Also, - UpdateWithArcNotificationData(...). - Before decode is done, UpdateWithArcNotificationData(...). https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:138: new message_center::Notification(*notification_))); // copy You do not need a copy here, instead you can move the |notification_|. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.h:40: ArcNotificationManager* const manager_; nit: Personally, I rarely see "const" after "*" in Chromium. Is this common manner around here? https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:38: items_.insert(std::make_pair(data.key, item)); emplace? https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:48: ArcNotificationItem* item = it->second; Let's use scoped_ptr. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:67: ArcNotificationItem* item = it->second; Ditto. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.h:38: typedef std::map<std::string, ArcNotificationItem*> Items; the value can be scoped_ptr?
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:37: // TODO(yoshiki): Return the corerct value according to the content intent On 2015/12/11 05:18:00, hidehiko wrote: > nit: s/corerct/correct/ Done. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:67: notification_key_ = data.key; On 2015/12/11 05:18:00, hidehiko wrote: > Please move them to the initializer list. Done. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:70: UpdateWithArcNotificationData(data); On 2015/12/11 05:18:00, hidehiko wrote: > IMHO, you can defer the responsibility to call this method to the caller. Done. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:78: message_center::NotificationType type = On 2015/12/11 05:18:00, hidehiko wrote: > I prefer move this to default:, as you set it in the "case > arc::ARC_NOTIFICATION_TYPE_PROGRESS". I moved them into the switch block, since I removed the default. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:87: 0.5); // round off On 2015/12/11 05:18:00, hidehiko wrote: > nit: std::round is your friend. > > BTW, the values, |progress_current| and |progress_max|, can be trusted? > Otherwise, probably you may want to clamp with [0, 100] ? Thank you for letting me know round(), done. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:137: message_center_->AddNotification(make_scoped_ptr( On 2015/12/11 05:18:00, hidehiko wrote: > Looks race? > > - UpdateWithArcNotificationData(...). > - Before decode is done OnClosedFromAndroid()/ It shouldn't be occurred. Because this instance is free'd soon after OnClosedFromAndroid(). But I agree that it's too complex to understand that. > Also, > - UpdateWithArcNotificationData(...). > - Before decode is done, UpdateWithArcNotificationData(...). Yes it should be race. I added some logic to prevent multiple decodes at the same time. So if UpdateWithArcNotificationData(...) is called during decoding, the updating is delayed. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.cc:138: new message_center::Notification(*notification_))); // copy On 2015/12/11 05:18:00, hidehiko wrote: > You do not need a copy here, instead you can move the |notification_|. I thought I wanted to have the data stored to skip the image decoding when the image is not changed. But that logic is not implemented yet, so I just pass it. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_item.h:40: ArcNotificationManager* const manager_; On 2015/12/11 05:18:00, hidehiko wrote: > nit: Personally, I rarely see "const" after "*" in Chromium. > Is this common manner around here? As I grep'd, it is used many time in Chromium. I think it can be used. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:38: items_.insert(std::make_pair(data.key, item)); On 2015/12/11 05:18:00, hidehiko wrote: > emplace? I changed std::map to ScopedPtrHashMap and use set(...) method here. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:48: ArcNotificationItem* item = it->second; On 2015/12/11 05:18:00, hidehiko wrote: > Let's use scoped_ptr. Done. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:67: ArcNotificationItem* item = it->second; On 2015/12/11 05:18:00, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/notification/arc_notification_manager.h:38: typedef std::map<std::string, ArcNotificationItem*> Items; On 2015/12/11 05:18:00, hidehiko wrote: > the value can be scoped_ptr? Done with ScopedPtrHashMap.
Hidehiko-san, Elijah, PTAL. Thanks On 2015/12/11 05:18:01, hidehiko wrote: > I do not have a strong preference if the code should put into components/arc in > this CL or later (defer to Elijah). However, if you choose later, could you file > a bug at least? Filed: http://crbug.com/569562 > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/notification/arc_notification_item.cc (right): > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:37: // > TODO(yoshiki): Return the corerct value according to the content intent > nit: s/corerct/correct/ > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:67: > notification_key_ = data.key; > Please move them to the initializer list. > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:70: > UpdateWithArcNotificationData(data); > IMHO, you can defer the responsibility to call this method to the caller. > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:78: > message_center::NotificationType type = > I prefer move this to default:, as you set it in the "case > arc::ARC_NOTIFICATION_TYPE_PROGRESS". > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:87: 0.5); // > round off > nit: std::round is your friend. > > BTW, the values, |progress_current| and |progress_max|, can be trusted? > Otherwise, probably you may want to clamp with [0, 100] ? > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:137: > message_center_->AddNotification(make_scoped_ptr( > Looks race? > > - UpdateWithArcNotificationData(...). > - Before decode is done OnClosedFromAndroid()/ > > Also, > - UpdateWithArcNotificationData(...). > - Before decode is done, UpdateWithArcNotificationData(...). > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.cc:138: new > message_center::Notification(*notification_))); // copy > You do not need a copy here, instead you can move the |notification_|. > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/notification/arc_notification_item.h (right): > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_item.h:40: > ArcNotificationManager* const manager_; > nit: Personally, I rarely see "const" after "*" in Chromium. > Is this common manner around here? > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/notification/arc_notification_manager.cc > (right): > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:38: > items_.insert(std::make_pair(data.key, item)); > emplace? > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:48: > ArcNotificationItem* item = it->second; > Let's use scoped_ptr. > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_manager.cc:67: > ArcNotificationItem* item = it->second; > Ditto. > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/notification/arc_notification_manager.h > (right): > > https://codereview.chromium.org/1477733002/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/notification/arc_notification_manager.h:38: typedef > std::map<std::string, ArcNotificationItem*> Items; > the value can be scoped_ptr?
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
Hi, I've just moved the notification stuffs to components/arc. PTAL again? Thanks.
lgtm, but please check with hidehiko in case he wanted to take another look. I think you may also need an OWNER for chrome/browser/chromeos https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:646: // ArcServiceManager, when it can manage a service under C/B/chromeos seems this comment is out of date now? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:84: if (notification_) { I'm glad there's a comment here, but you could be more explicit about why checking notification_ is a good proxy for decode being in progress https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:110: message_center::NotifierId notifier_id( I don't have any idea what this is/does, can you add some comments to clarify? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:121: GURL(), // origin url can you state here why empty is ok? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:57: // OnNotificationRemovedFromAndroid(), since that method should removes s/removes/remove https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:58: // the notification on Chrome UI. s/on/in
lgtm, but please check with hidehiko in case he wanted to take another look. I think you may also need an OWNER for chrome/browser/chromeos
https://codereview.chromium.org/1477733002/diff/320001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/arc_ser... components/arc/arc_service_manager.h:34: void PostProfileInit(const AccountId& account_id); Could you add comments, specifically when this will fire? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:57: base::WeakPtr<arc::ArcNotificationItem> item_; nit: Oops, I overlooked. DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:75: // This must be initialized after ArcBridgeService. This comment looks stale...? Or, maybe taking ArcNotificationManger implies it? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:85: newer_data_ = data.Clone(); This will keep only newest data here, and (intentionally) discard older ones. Could you add a brief comment, too? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:133: // TODO(yoshiki): Remove decoding by passing a bitmap directly from Android. Thank you for offline chat. I thought, the simpler way is passing scoped_ptr<message_center::Notification> to OnImageDecoded as an argument, but it may have a problem about ordering depending on the duration of image decoding. After mojo refactoring (hopefully done somehow soon), you'll move the image decoding to Android side, so it will not be necessary to PostTask here, right? Then, it is not necessary to be worried about ordering problem, as creating a Notification message can be synchronously done here. It means, we can remove |notification_| and |newer_data_| fields and simplify the structure. Could you add brief comment about that stuff, too? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:161: message_center_->AddNotification(notification_.Pass()); Maybe: std::move(notification_) ? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:162: DCHECK(!notification_); // |notification_| becomes null. nit: To be honest, this is redundant. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:164: if (newer_data_) { Could you add brief comment what this is for? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:13: arc::ArcBridgeService* arc_bridge, Maybe, it's time to remove "arc::" from this CL :-). Ditto for others. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:31: if (items_.get(data.key) == nullptr) { nit: contains()? Or, if you do not want to search twice: ArcNotificationItem* item = items_.get(data.key); if (!item) { item = new ArcNotificationItem(...); bool added = items_.add(data.key, scoped_ptr<...>(item)); DCHECK(added); } item->UpdateWithArcNotificationData(...); etc.? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:66: } nit: maybe logging on error? https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:74: } Ditto.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:646: // ArcServiceManager, when it can manage a service under C/B/chromeos On 2015/12/16 02:34:37, elijahtaylor1 wrote: > seems this comment is out of date now? components/arc cannot have any dependency to chrome/, so this will never be posible. https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:651: #if defined(USE_ASH) Why not gate the whole chunk with USE_ASH instead of just this line?
Patchset #10 (id:340001) has been deleted
Patchset #10 (id:360001) has been deleted
Patchset #10 (id:380001) has been deleted
Patchset #10 (id:400001) has been deleted
Patchset #10 (id:420001) has been deleted
Patchset #10 (id:440001) has been deleted
PTAL. Thanks. https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:646: // ArcServiceManager, when it can manage a service under C/B/chromeos On 2015/12/17 17:43:09, Luis Héctor Chávez wrote: > On 2015/12/16 02:34:37, elijahtaylor1 wrote: > > seems this comment is out of date now? > > components/arc cannot have any dependency to chrome/, so this will never be > posible. Yes, removed. https://codereview.chromium.org/1477733002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:651: #if defined(USE_ASH) On 2015/12/17 17:43:09, Luis Héctor Chávez wrote: > Why not gate the whole chunk with USE_ASH instead of just this line? multi_user_util.[h|cc] is in ash/ directory so I thought we needed this guard. But I found these files are always complied on chromeos, so I removed it. https://codereview.chromium.org/1477733002/diff/320001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/arc_ser... components/arc/arc_service_manager.h:34: void PostProfileInit(const AccountId& account_id); On 2015/12/17 07:47:51, hidehiko wrote: > Could you add comments, specifically when this will fire? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:57: base::WeakPtr<arc::ArcNotificationItem> item_; On 2015/12/17 07:47:51, hidehiko wrote: > nit: Oops, I overlooked. DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:75: // This must be initialized after ArcBridgeService. On 2015/12/17 07:47:51, hidehiko wrote: > This comment looks stale...? > Or, maybe taking ArcNotificationManger implies it? It was slate. Removed. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:84: if (notification_) { On 2015/12/16 02:34:37, elijahtaylor1 wrote: > I'm glad there's a comment here, but you could be more explicit about why > checking notification_ is a good proxy for decode being in progress Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:85: newer_data_ = data.Clone(); On 2015/12/17 07:47:51, hidehiko wrote: > This will keep only newest data here, and (intentionally) discard older ones. > Could you add a brief comment, too? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:110: message_center::NotifierId notifier_id( On 2015/12/16 02:34:37, elijahtaylor1 wrote: > I don't have any idea what this is/does, can you add some comments to clarify? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:121: GURL(), // origin url On 2015/12/16 02:34:37, elijahtaylor1 wrote: > can you state here why empty is ok? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:133: // TODO(yoshiki): Remove decoding by passing a bitmap directly from Android. On 2015/12/17 07:47:51, hidehiko wrote: > Thank you for offline chat. > > I thought, the simpler way is passing scoped_ptr<message_center::Notification> > to OnImageDecoded as an argument, but it may have a problem about ordering > depending on the duration of image decoding. > > After mojo refactoring (hopefully done somehow soon), you'll move the image > decoding to Android side, so it will not be necessary to PostTask here, right? Correct, we'll be able to omit the async task from here after I do mojo refactoring. > Then, it is not necessary to be worried about ordering problem, as creating a > Notification message can be synchronously done here. It means, we can remove > |notification_| and |newer_data_| fields and simplify the structure. Could you > add brief comment about that stuff, too? Correct, we will be able to remove them at that time. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:161: message_center_->AddNotification(notification_.Pass()); On 2015/12/17 07:47:51, hidehiko wrote: > Maybe: std::move(notification_) ? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:162: DCHECK(!notification_); // |notification_| becomes null. On 2015/12/17 07:47:51, hidehiko wrote: > nit: To be honest, this is redundant. Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_item.cc:164: if (newer_data_) { On 2015/12/17 07:47:51, hidehiko wrote: > Could you add brief comment what this is for? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:13: arc::ArcBridgeService* arc_bridge, On 2015/12/17 07:47:51, hidehiko wrote: > Maybe, it's time to remove "arc::" from this CL :-). > Ditto for others. Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:31: if (items_.get(data.key) == nullptr) { On 2015/12/17 07:47:51, hidehiko wrote: > nit: contains()? > > Or, if you do not want to search twice: > > ArcNotificationItem* item = items_.get(data.key); > if (!item) { > item = new ArcNotificationItem(...); > bool added = items_.add(data.key, scoped_ptr<...>(item)); > DCHECK(added); > } > item->UpdateWithArcNotificationData(...); > > etc.? Done. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:57: // OnNotificationRemovedFromAndroid(), since that method should removes On 2015/12/16 02:34:37, elijahtaylor1 wrote: > s/removes/remove Thanks, but I removed the comment. https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:66: } On 2015/12/17 07:47:51, hidehiko wrote: > nit: maybe logging on error? Done. And to make the logging code simpler, I added a flag |being_removed_by_manager_| to ArcNotificationItem, to prevent this method from being called after the notification is already removed (I meant the case I had described in the removed comment) https://codereview.chromium.org/1477733002/diff/320001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:74: } On 2015/12/17 07:47:51, hidehiko wrote: > Ditto. Done.
lgtm
Wait, does this pass trybots? It looks like this will most likely break the GN bots since I don't see the new files being added to components/arc/BUILD.gn.
https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:61: ArcBridgeService::Get()->SendNotificationEventToAndroid( You already have |arc_bridge_|, any reason why you are not using it? Same in the method below.
More nits from the linter. By the way, can you run `git cl lint` and `git cl format` before you resubmit? There are a few things that need formatting. https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... components/arc/notification/arc_notification_item.cc:6: lint nit: include <algorithm> and <vector> https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:80: } // namespace components nit: namespace arc.
Patchset #12 (id:500001) has been deleted
Patchset #11 (id:480001) has been deleted
Modified BUILD.gn as well and did "git cl lint" and "git cl format. https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/460001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:61: ArcBridgeService::Get()->SendNotificationEventToAndroid( On 2015/12/17 22:49:50, Luis Héctor Chávez wrote: > You already have |arc_bridge_|, any reason why you are not using it? Same in the > method below. Done.
yoshiki@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan, PTAL at the changes in chrome_browser_main_chromeos.cc? Thanks.
Patchset #12 (id:540001) has been deleted
https://codereview.chromium.org/1477733002/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:649: multi_user_util::GetAccountIdFromProfile(profile())); IIRC, this code path is usually executed on the login screen (except for the browser restart case). The profile() is most likely to be the signin profile thus would not give your the correct AccountId for user. Have you tried with "--login-manager" or on a real device? On dev box, the default is to run with a stub user so the code here may look like giving you a user profile but that is not the case on real devices. If you need user profile, maybe we can add the code to UserSessionManager::FinalizePrepareProfile [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
lgtm with nitpicks. Please re-run trybots after your update, again. https://codereview.chromium.org/1477733002/diff/560001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/arc_ser... components/arc/arc_service_manager.h:35: // Called the main profile is initialized after user logs-in. nit: "// Called when the main profile is ..." ? https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:61: // The destractor is private since this class is ref-counted. nit: destructor. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:87: // Checks if a decode task is on-going or not. If |notification_| is non-null, nit: s/Checks/Check/. In code comment, let's use declarative form. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:92: // Stores the latest data to the |newer_data_| property and returns, if the ditto. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:94: // If old |newer_daa_| has been stored, discard the old one. nit: s/daa_/data_/ https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:117: break; nit: you may want to something like "default: LOG(ERROR) << data.type" here, in order to future debugging. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:132: base::UTF8ToUTF16("arc"), // display source nit: maybe "Display source." (for making a sentence?) https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:139: OnImageDecoded(SkBitmap()); // Passes an empty bitmap. ditto. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:167: // This instance may be destroied during this call. nit: s/destroied/destroyed/ https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:168: // Don't use any properties in this method after this call. How about: "Because this instance will be destroyed in the following call, do not touch its any members afterwards." or something? https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... File components/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.h:43: std::string notification_key_; nit: maybe "const" for consistency? https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:46: nit: unnecessary an empty line?
Patchset #13 (id:580001) has been deleted
Xiyuan PTAL. Thanks. https://codereview.chromium.org/1477733002/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:649: multi_user_util::GetAccountIdFromProfile(profile())); On 2015/12/21 22:56:36, xiyuan wrote: > IIRC, this code path is usually executed on the login screen (except for the > browser restart case). The profile() is most likely to be the signin profile > thus would not give your the correct AccountId for user. > > Have you tried with "--login-manager" or on a real device? On dev box, the > default is to run with a stub user so the code here may look like giving you a > user profile but that is not the case on real devices. > > If you need user profile, maybe we can add the code to > UserSessionManager::FinalizePrepareProfile [1]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > I checked that the previous code (hooking is in PostProfileInit) worked well in the real device. But what I want is the primary user profile, so I move the code to the login manager. Thanks. https://codereview.chromium.org/1477733002/diff/560001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/arc_ser... components/arc/arc_service_manager.h:35: // Called the main profile is initialized after user logs-in. On 2015/12/22 08:51:26, hidehiko wrote: > nit: "// Called when the main profile is ..." ? Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:61: // The destractor is private since this class is ref-counted. On 2015/12/22 08:51:26, hidehiko wrote: > nit: destructor. Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:87: // Checks if a decode task is on-going or not. If |notification_| is non-null, On 2015/12/22 08:51:27, hidehiko wrote: > nit: s/Checks/Check/. In code comment, let's use declarative form. Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:92: // Stores the latest data to the |newer_data_| property and returns, if the On 2015/12/22 08:51:27, hidehiko wrote: > ditto. Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:94: // If old |newer_daa_| has been stored, discard the old one. On 2015/12/22 08:51:27, hidehiko wrote: > nit: s/daa_/data_/ Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:117: break; On 2015/12/22 08:51:27, hidehiko wrote: > nit: you may want to something like "default: LOG(ERROR) << data.type" here, in > order to future debugging. I want to avoid "default" case, since we want to ensure to handle all the values in enum. Instead, I added the DCHECK to check the range. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:132: base::UTF8ToUTF16("arc"), // display source On 2015/12/22 08:51:27, hidehiko wrote: > nit: maybe "Display source." (for making a sentence?) No, I meant this is a noun phrase. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:139: OnImageDecoded(SkBitmap()); // Passes an empty bitmap. On 2015/12/22 08:51:26, hidehiko wrote: > ditto. Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.cc:168: // Don't use any properties in this method after this call. On 2015/12/22 08:51:27, hidehiko wrote: > How about: > "Because this instance will be destroyed in the following call, > do not touch its any members afterwards." > or something? Thanks. Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... File components/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_item.h:43: std::string notification_key_; On 2015/12/22 08:51:27, hidehiko wrote: > nit: maybe "const" for consistency? Done. https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... File components/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/560001/components/arc/notific... components/arc/notification/arc_notification_manager.cc:46: On 2015/12/22 08:51:28, hidehiko wrote: > nit: unnecessary an empty line? Done.
user_session_manager.cc LGTM https://codereview.chromium.org/1477733002/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1477733002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:649: multi_user_util::GetAccountIdFromProfile(profile())); On 2015/12/22 10:13:53, yoshiki wrote: > On 2015/12/21 22:56:36, xiyuan wrote: > > IIRC, this code path is usually executed on the login screen (except for the > > browser restart case). The profile() is most likely to be the signin profile > > thus would not give your the correct AccountId for user. > > > > Have you tried with "--login-manager" or on a real device? On dev box, the > > default is to run with a stub user so the code here may look like giving you a > > user profile but that is not the case on real devices. > > > > If you need user profile, maybe we can add the code to > > UserSessionManager::FinalizePrepareProfile [1]. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > I checked that the previous code (hooking is in PostProfileInit) worked well in > the real device. But what I want is the primary user profile, so I move the code > to the login manager. Thanks. Interesting. That means the AccountId of profile is probably not really used since it would be always empty for signin profile.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, lhchavez@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/1477733002/#ps600001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477733002/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477733002/600001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #14 (id:620001) 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, elijahtaylor@chromium.org, lhchavez@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1477733002/#ps640001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477733002/640001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477733002/640001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yoshiki@chromium.org changed reviewers: + danakj@chromium.org, dcheng@chromium.org, jam@chromium.org, reed@chromium.org, stevenjb@chromium.org
Hi, could you approve the changes? Thanks. dcheng@: changes in components/arc/common/notifications.mojom jam@: dependency on content/public reed@: dependency on third_party/skia danakj@: dependency on ui/gfx stevenjb@: dependency on ui/message_center
https://codereview.chromium.org/1477733002/diff/640001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/DEPS#ne... components/arc/DEPS:12: "+ui/message_center", I would suggest that at this point, components/arc is exceeding what should be considered a single "component". I think that instead we should put the UI under ui/arc with a dependency on components/arc.
On 2015/12/29 05:55:50, yoshiki wrote: > Hi, could you approve the changes? Thanks. > > dcheng@: changes in components/arc/common/notifications.mojom > jam@: dependency on content/public > reed@: dependency on third_party/skia > danakj@: dependency on ui/gfx > stevenjb@: dependency on ui/message_center do you really want to add a depenency on content/ just for the two lines that image decoding class does? why not just do https://code.google.com/p/chromium/codesearch#chromium/src/content/child/imag... directly? i think it's better to avoid depending on content.
https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_item.cc:25: static const char* kNotifierId = "ARC_NOTIFICATION"; Nit: const char kNotifierId[]. Similar for the constant below. (Note that as currently defined, kNotifiedId can be reassigned as well) https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_item.cc:35: // in ARC and should be safe. "generated in ARC": does this mean the ARC host itself generated it, or the app inside the host? https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... File components/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_manager.h:24: explicit ArcNotificationManager(ArcBridgeService* bridge_service, Nit: no explicit
DEPS lgtm
On 2015/12/29 20:14:34, jam wrote: > On 2015/12/29 05:55:50, yoshiki wrote: > > Hi, could you approve the changes? Thanks. > > > > dcheng@: changes in components/arc/common/notifications.mojom > > jam@: dependency on content/public > > reed@: dependency on third_party/skia > > danakj@: dependency on ui/gfx > > stevenjb@: dependency on ui/message_center > > do you really want to add a depenency on content/ just for the two lines that > image decoding class does? why not just do > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/imag... > directly? > > i think it's better to avoid depending on content. Thanks, now image decoding can be done directly without dependency on content/. But I still want to add a dependency on content/ to use blocking pool. I think we can remove this dependency in near feature after image decoding is moved to Android-side. So can we keep this dependency as for now?
hidehiko@, elijah@, I moved the arc notification stuffs to ui/arc as stevenjb@ suggested. Is it ok for you? danakj@, stevenjb@, jam@: PTAL at dependency again? Thanks. https://codereview.chromium.org/1477733002/diff/640001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/DEPS#ne... components/arc/DEPS:12: "+ui/message_center", On 2015/12/29 17:21:15, stevenjb wrote: > I would suggest that at this point, components/arc is exceeding what should be > considered a single "component". I think that instead we should put the UI under > ui/arc with a dependency on components/arc. Thanks for suggestion. Done. https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_item.cc:25: static const char* kNotifierId = "ARC_NOTIFICATION"; On 2015/12/30 19:19:35, dcheng wrote: > Nit: const char kNotifierId[]. Similar for the constant below. > > (Note that as currently defined, kNotifiedId can be reassigned as well) Done. https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_item.cc:35: // in ARC and should be safe. On 2015/12/30 19:19:35, dcheng wrote: > "generated in ARC": does this mean the ARC host itself generated it, or the app > inside the host? Done. https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... File components/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_manager.h:24: explicit ArcNotificationManager(ArcBridgeService* bridge_service, On 2015/12/30 19:19:35, dcheng wrote: > Nit: no explicit Done.
Could you run trybots? I'm fine to move the code to ui/arc. Defer to elijahtaylor@. https://codereview.chromium.org/1477733002/diff/680001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/680001/components/arc/arc_ser... components/arc/arc_service_manager.h:25: ArcServiceManager(scoped_ptr<ArcSettingsBridge> settings_bridge); explicit needs to be kept? https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:47: // This property will be removed after removing async task of image decoding. nit: s/property/field/ https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:59: // This property will be removed after removing async task of image decoding. ditto. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:24: explicit ArcNotificationManager(ArcBridgeService* bridge_service, nit: no explicit. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:29: // void OnStateChanged(ArcBridgeService::State state) override; nit: Remove this line?
On 2016/01/05 10:09:07, yoshiki wrote: > On 2015/12/29 20:14:34, jam wrote: > > On 2015/12/29 05:55:50, yoshiki wrote: > > > Hi, could you approve the changes? Thanks. > > > > > > dcheng@: changes in components/arc/common/notifications.mojom > > > jam@: dependency on content/public > > > reed@: dependency on third_party/skia > > > danakj@: dependency on ui/gfx > > > stevenjb@: dependency on ui/message_center > > > > do you really want to add a depenency on content/ just for the two lines that > > image decoding class does? why not just do > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/imag... > > directly? > > > > i think it's better to avoid depending on content. > > Thanks, now image decoding can be done directly without dependency on content/. > But I still want to add a dependency on content/ to use blocking pool. I think > we can remove this dependency in near feature after image decoding is moved to > Android-side. So can we keep this dependency as for now? please use dependency injection for this; i.e. have whatever code in src/chrome which constructs this object pass in a pointer to the blocking pool.
DEPS lgtm, thanks. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:35: static const gfx::Size kNotificationIconSize( extra space https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:63: } nit: invert logic and early exit if not found. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:77: } same here and below. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:45: Items; nit: ItemMap would be more clear.
https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/DEPS File ui/arc/notification/DEPS (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/DE... ui/arc/notification/DEPS:6: "+ui/gfx", This LGTM
Patchset #17 (id:700001) has been deleted
yoshiki@chromium.org changed reviewers: + rogerta@chromium.org, sky@chromium.org
elijahtaylor@: Is it ok for you to move the notification stuff to ui/arc? Hidehiko deferred to you (see comment #70). sky@: could you approve to use ui/arc? reed@: could you approve the dependency on third_party/skia? rogerta@: could you approve the dependency on components/signin? https://codereview.chromium.org/1477733002/diff/680001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/680001/components/arc/arc_ser... components/arc/arc_service_manager.h:25: ArcServiceManager(scoped_ptr<ArcSettingsBridge> settings_bridge); On 2016/01/05 11:41:32, hidehiko wrote: > explicit needs to be kept? Done. I had removed wrongly... https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:35: static const gfx::Size kNotificationIconSize( On 2016/01/05 16:59:06, stevenjb wrote: > extra space Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:47: // This property will be removed after removing async task of image decoding. On 2016/01/05 11:41:32, hidehiko wrote: > nit: s/property/field/ Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:59: // This property will be removed after removing async task of image decoding. On 2016/01/05 11:41:32, hidehiko wrote: > ditto. Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:63: } On 2016/01/05 16:59:06, stevenjb wrote: > nit: invert logic and early exit if not found. Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:77: } On 2016/01/05 16:59:06, stevenjb wrote: > same here and below. Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:24: explicit ArcNotificationManager(ArcBridgeService* bridge_service, On 2016/01/05 11:41:33, hidehiko wrote: > nit: no explicit. Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:29: // void OnStateChanged(ArcBridgeService::State state) override; On 2016/01/05 11:41:33, hidehiko wrote: > nit: Remove this line? Done. https://codereview.chromium.org/1477733002/diff/680001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:45: Items; On 2016/01/05 16:59:06, stevenjb wrote: > nit: ItemMap would be more clear. Done.
reed@google.com changed reviewers: + reed@google.com
deps on skia: lgtm
lgtm, I have no issue moving this code to ui/arc if it's been requested https://codereview.chromium.org/1477733002/diff/720001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/720001/components/arc/arc_ser... components/arc/arc_service_manager.h:36: // Called when the main profile is initialized after user logs-in. nit: logs in, no hyphen
lgtm deps in account_id
I'm having a hard time seeing how all this fits together. Do you have a design doc or something you could point me at? https://codereview.chromium.org/1477733002/diff/720001/ui/arc/BUILD.gn File ui/arc/BUILD.gn (right): https://codereview.chromium.org/1477733002/diff/720001/ui/arc/BUILD.gn#newcode19 ui/arc/BUILD.gn:19: "//third_party/WebKit/public:blink_headers", Having this code depend upon webkit is unfortunate. Is there a way to structure it so that you don't depend upon webkit?
On 2016/01/06 22:02:36, sky wrote: > I'm having a hard time seeing how all this fits together. Do you have a design > doc or something you could point me at? Design Doc is: http://go/arc-notification The current files under /ui/arc are mostly UI-related code: it'll show notifications which are passed from Android via ARC++ IPC. So I think /ui/arc is good place. WDYT? > https://codereview.chromium.org/1477733002/diff/720001/ui/arc/BUILD.gn > File ui/arc/BUILD.gn (right): > > https://codereview.chromium.org/1477733002/diff/720001/ui/arc/BUILD.gn#newcode19 > ui/arc/BUILD.gn:19: "//third_party/WebKit/public:blink_headers", > Having this code depend upon webkit is unfortunate. Is there a way to structure > it so that you don't depend upon webkit? I will remove the dependency in near future. Currently it is used to decode images on Chrome side. But it'll be done on Android side.
https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_item.cc:35: // in ARC and should be safe. On 2016/01/05 at 10:12:14, yoshiki wrote: > On 2015/12/30 19:19:35, dcheng wrote: > > "generated in ARC": does this mean the ARC host itself generated it, or the app > > inside the host? > > Done. I don't feel like I got an answer to my question. https://codereview.chromium.org/1477733002/diff/720001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/720001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:38: message_center::kNotificationIconSize); Won't this generate a static initializer? https://codereview.chromium.org/1477733002/diff/720001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:46: const blink::WebImage& image = blink::WebImage::fromData( Why not use //ui/gfx/codec? Using this directly in the browser process is weird.
@sky, I added the permission to the doc, and removed the dependency on webkit. PTAL again? @dcheng, Thank you! PTAL again? https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... File components/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/640001/components/arc/notific... components/arc/notification/arc_notification_item.cc:35: // in ARC and should be safe. On 2016/01/07 19:24:21, dcheng wrote: > On 2016/01/05 at 10:12:14, yoshiki wrote: > > On 2015/12/30 19:19:35, dcheng wrote: > > > "generated in ARC": does this mean the ARC host itself generated it, or the > app > > > inside the host? > > > > Done. > > I don't feel like I got an answer to my question. Sorry. I meant I update the comment. The ArcNotificationListener which runs in Android generates images. https://codereview.chromium.org/1477733002/diff/720001/components/arc/arc_ser... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1477733002/diff/720001/components/arc/arc_ser... components/arc/arc_service_manager.h:36: // Called when the main profile is initialized after user logs-in. On 2016/01/06 17:45:28, elijahtaylor1 wrote: > nit: logs in, no hyphen Done. https://codereview.chromium.org/1477733002/diff/720001/ui/arc/BUILD.gn File ui/arc/BUILD.gn (right): https://codereview.chromium.org/1477733002/diff/720001/ui/arc/BUILD.gn#newcode19 ui/arc/BUILD.gn:19: "//third_party/WebKit/public:blink_headers", On 2016/01/06 22:02:36, sky wrote: > Having this code depend upon webkit is unfortunate. Is there a way to structure > it so that you don't depend upon webkit? I removed the dependency on webkit. https://codereview.chromium.org/1477733002/diff/720001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/720001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:38: message_center::kNotificationIconSize); On 2016/01/07 19:24:21, dcheng wrote: > Won't this generate a static initializer? You're right, this was wrong. This is removed in the latest patch. https://codereview.chromium.org/1477733002/diff/720001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:46: const blink::WebImage& image = blink::WebImage::fromData( On 2016/01/07 19:24:21, dcheng wrote: > Why not use //ui/gfx/codec? > > Using this directly in the browser process is weird. Thank you for letting me know. I'm using it!
https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:154: data.icon_data.storage().end()); // copy AFAICT you aren't using icon_data_str. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:158: base::Bind(&DecodeImage, data.icon_data.storage()), I believe this makes a copy. Is it worth making this take ArcNotificationDataPtr so that it can take owernship of the array and pass it to the background thread? If you expect the images to be small, it likely isn't worth it. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:196: // There is the newer data, so re-updates again. re-updates -> update https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:20: class ArcNotificationItem { Please add a description. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:43: typedef base::ScopedPtrHashMap<std::string, scoped_ptr<ArcNotificationItem>> typedef->using
sky@: PTAL, Thanks. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:154: data.icon_data.storage().end()); // copy On 2016/01/08 17:17:30, sky wrote: > AFAICT you aren't using icon_data_str. Removed. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:158: base::Bind(&DecodeImage, data.icon_data.storage()), On 2016/01/08 17:17:29, sky wrote: > I believe this makes a copy. Is it worth making this take ArcNotificationDataPtr > so that it can take owernship of the array and pass it to the background thread? > If you expect the images to be small, it likely isn't worth it. Images must be small (80x80px), so let me keep it as it is. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:196: // There is the newer data, so re-updates again. On 2016/01/08 17:17:29, sky wrote: > re-updates -> update Done. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.h (right): https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.h:20: class ArcNotificationItem { On 2016/01/08 17:17:30, sky wrote: > Please add a description. Done. https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.h (right): https://codereview.chromium.org/1477733002/diff/740001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.h:43: typedef base::ScopedPtrHashMap<std::string, scoped_ptr<ArcNotificationItem>> On 2016/01/08 17:17:30, sky wrote: > typedef->using Done.
LGTM
lgtm https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:27: // static Nit: // static is redundant here and line 30 https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:49: *data, main_profile_id_); Nit: I think it would be slightly clearer to only pass data->key
Thank you all! https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:27: // static On 2016/01/12 01:25:48, dcheng wrote: > Nit: // static is redundant here and line 30 Done. https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/1477733002/diff/780001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:49: *data, main_profile_id_); On 2016/01/12 01:25:48, dcheng wrote: > Nit: I think it would be slightly clearer to only pass data->key Done.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, hidehiko@chromium.org, stevenjb@chromium.org, danakj@chromium.org, elijahtaylor@chromium.org, rogerta@chromium.org, reed@google.com, sky@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1477733002/#ps800001 (title: "Addressed comments (#88)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477733002/800001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477733002/800001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/arc/arc_service_manager.cc:
While running git apply --index -3 -p1;
error: patch failed: components/arc/arc_service_manager.cc:13
Falling back to three-way merge...
Applied patch to 'components/arc/arc_service_manager.cc' with conflicts.
U components/arc/arc_service_manager.cc
Patch: components/arc/arc_service_manager.cc
Index: components/arc/arc_service_manager.cc
diff --git a/components/arc/arc_service_manager.cc
b/components/arc/arc_service_manager.cc
index
9b8bd81b0922b3e939b56bdb22b936e2f95fc4bc..f7cf276b2974c66d8416c5a3f0a7f78897885359
100644
--- a/components/arc/arc_service_manager.cc
+++ b/components/arc/arc_service_manager.cc
@@ -13,6 +13,7 @@
#include "components/arc/input/arc_input_bridge.h"
#include "components/arc/power/arc_power_bridge.h"
#include "components/arc/settings/arc_settings_bridge.h"
+#include "ui/arc/notification/arc_notification_manager.h"
namespace arc {
@@ -58,4 +59,12 @@ ArcBridgeService* ArcServiceManager::arc_bridge_service() {
return arc_bridge_service_.get();
}
+void ArcServiceManager::OnPrimaryUserProfilePrepared(
+ const AccountId& account_id) {
+ DCHECK(thread_checker_.CalledOnValidThread());
+
+ arc_notification_manager_.reset(
+ new ArcNotificationManager(arc_bridge_service(), account_id));
+}
+
} // namespace arc
The CQ bit was unchecked by commit-bot@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/1477733002/800001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477733002/800001
The CQ bit was unchecked by yoshiki@chromium.org
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, stevenjb@chromium.org, rogerta@chromium.org, sky@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org, dcheng@chromium.org, danakj@chromium.org, hidehiko@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1477733002/#ps820001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477733002/820001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477733002/820001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, stevenjb@chromium.org, rogerta@chromium.org, sky@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org, dcheng@chromium.org, danakj@chromium.org, hidehiko@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1477733002/#ps840001 (title: "Sync dependencies")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477733002/840001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477733002/840001
Message was sent while issue was closed.
Description was changed from ========== Add ArcNotificationManager for new ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ArcNotificationManager for new ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 #23 (id:840001)
Message was sent while issue was closed.
Description was changed from ========== Add ArcNotificationManager for new ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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 ArcNotificationManager for new ARC notification This change comes along with: - http://crrev.com/1475583002 (ArcBridge part) - http://crrev.com/1477733002 (UI part, this patch) - 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/5d4f913c38b5a225caf110f3c280cb426f424f24 Cr-Commit-Position: refs/heads/master@{#368924} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/5d4f913c38b5a225caf110f3c280cb426f424f24 Cr-Commit-Position: refs/heads/master@{#368924} |
