Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1005393003:
[Download Notification] Use NotificationUIManager instead of MessageCenter (Closed)
Description[Download Notification] Use NotificationUIManager instead of MessageCenter
Direct use of message_center::MessageCenter may cause some problems (eg. crbug.com/469622) because it is not aware of profie.
This patch changes the code to use NotificationUIManager, which is a higher layer wrapper and is aware of profile, instead of message_center::MessageCenter, which is on lower layer and isn't aware of profile.
BUG=469622
TEST=manually tested
Committed: https://crrev.com/365ac4c9486e5f9963b06de940809189011d84d3
Cr-Commit-Position: refs/heads/master@{#325404}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Adressed comment. #Patch Set 3 : Use NotificationUIManager instead of message_center::MessageCenter #
Total comments: 7
Patch Set 4 : Addressed the comments #Patch Set 5 : Do not stop ongoing download when closed and Add the "Cancel" button #
Total comments: 4
Patch Set 6 : Remove unused code #
Total comments: 6
Patch Set 7 : Add the detailed context for the temporary image #Patch Set 8 : Addressed the comments #
Total comments: 4
Patch Set 9 : Addressed the comments #
Total comments: 2
Messages
Total messages: 41 (8 generated)
yoshiki@chromium.org changed reviewers: + asanka@chromium.org
Asanka, PTAL. Thanks.
Asanka, PTAL. Thanks.
https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_notification_item_unittest.cc:128: ASSERT_TRUE(profile_manager.SetUp()); Move the TestingProfileManager construction and SetUp() call to DownloadNotificationItemTest::SetUp() above. https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_notification_item_unittest.cc:129: Profile* profile = profile_manager.CreateTestingProfile("test-user"); Can't we at least test whether two notifications created by profiles with different names end up with different profile_id()s?
Asanka, PTAL. Thanks. https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_notification_item_unittest.cc:128: ASSERT_TRUE(profile_manager.SetUp()); On 2015/03/24 18:36:15, asanka wrote: > Move the TestingProfileManager construction and SetUp() call to > DownloadNotificationItemTest::SetUp() above. Done. I moved the construction of Profile as well. https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_notification_item_unittest.cc:129: Profile* profile = profile_manager.CreateTestingProfile("test-user"); On 2015/03/24 18:36:15, asanka wrote: > Can't we at least test whether two notifications created by profiles with > different names end up with different profile_id()s? That's good idea. But I think the multi-user test should be covered by browser test rather than unit test. I'm planning to add a browser test including the multi-user test later.
https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... chrome/browser/download/notification/download_notification_item_unittest.cc:129: Profile* profile = profile_manager.CreateTestingProfile("test-user"); On 2015/03/25 05:30:25, yoshiki wrote: > On 2015/03/24 18:36:15, asanka wrote: > > Can't we at least test whether two notifications created by profiles with > > different names end up with different profile_id()s? > > That's good idea. But I think the multi-user test should be covered by browser > test rather than unit test. I'm planning to add a browser test including the > multi-user test later. It would be great if such a test was part of this CL. I understand that notifications are a work in progress, but unless there are very good reasons, tests should go in along with the code that implements functionality.
On 2015/03/25 21:04:48, asanka (OOO until April 20) wrote: > https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... > File chrome/browser/download/notification/download_notification_item_unittest.cc > (right): > > https://codereview.chromium.org/1005393003/diff/1/chrome/browser/download/not... > chrome/browser/download/notification/download_notification_item_unittest.cc:129: > Profile* profile = profile_manager.CreateTestingProfile("test-user"); > On 2015/03/25 05:30:25, yoshiki wrote: > > On 2015/03/24 18:36:15, asanka wrote: > > > Can't we at least test whether two notifications created by profiles with > > > different names end up with different profile_id()s? > > > > That's good idea. But I think the multi-user test should be covered by browser > > test rather than unit test. I'm planning to add a browser test including the > > multi-user test later. > > It would be great if such a test was part of this CL. I understand that > notifications are a work in progress, but unless there are very good reasons, > tests should go in along with the code that implements functionality. Hi Asanka, I'm writing a browsertest but it gets bigger than expected so I splited it into the separated patch: https://codereview.chromium.org/1047243002/. The patch is almost ready and sent you a review after some cleanups. And I changed the code to use NotificationUIManager, which is higher layer wrapper and is aware of profile, instead of message_center::MessageCenter, which is on lower layer and isn't aware of profile. I think using NotificationUIManager is better because a download notification is per-profile notificaiton. Could you take a look at the code again? Thanks.
yoshiki@chromium.org changed reviewers: + benjhayden@chromium.org, mukai@chromium.org
benjhayden: PTAL at the change under C/B/downloads, on behalf of asanka? mukai: PTAL at the change under C/B/notifications? Thanks.
Sorry, I forgot to update the subject and description of this patch along with the new design of the new patch. I've just updated them.
https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/message_center_notification_manager.cc:205: // Update |notification| with the latest information from the message center. This is weird. Is there some case where the state is inconsistent among message center and the MessageCenterNotificationManager? We should fix the inconsistency.
https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/message_center_notification_manager.cc:205: // Update |notification| with the latest information from the message center. On 2015/03/31 19:59:47, Jun Mukai wrote: > This is weird. Is there some case where the state is inconsistent among message > center and the MessageCenterNotificationManager? > We should fix the inconsistency. "shown_as_popup" and "read" flags are changed in the message center (message_center::NotificationList) internally, and we need to sync them at least.
https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_notification_item.cc:125: reshow_after_remove = !current_notification->shown_as_popup(); You don't have to check through this. Notification's delegate has a callback Display() which is called when shown as a popup IIRC. https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/message_center_notification_manager.cc:205: // Update |notification| with the latest information from the message center. On 2015/04/01 04:10:49, yoshiki wrote: > On 2015/03/31 19:59:47, Jun Mukai wrote: > > This is weird. Is there some case where the state is inconsistent among > message > > center and the MessageCenterNotificationManager? > > We should fix the inconsistency. > > "shown_as_popup" and "read" flags are changed in the message center > (message_center::NotificationList) internally, and we need to sync them at > least. Commented on the other part. I believe FindById should not update anything on each notification, it's a method to lookup existing ones. Another suggestion is: return message_center_->FindVisibleNotificationById(iter->second->notification().id());
Mukai-san, thank you for comments and sorry for late. I was busy with the M43 bugs. https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_notification_item.cc:125: reshow_after_remove = !current_notification->shown_as_popup(); On 2015/04/01 18:02:11, Jun Mukai wrote: > You don't have to check through this. Notification's delegate has a callback > Display() which is called when shown as a popup IIRC. Actually, I'd like to check if the notification is currently shown as a popup or closed after timeout. Is the |shown_as_popup| is true, we can know the popup is closed after timeout. https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/notifica... chrome/browser/notifications/message_center_notification_manager.cc:205: // Update |notification| with the latest information from the message center. On 2015/04/01 18:02:11, Jun Mukai wrote: > On 2015/04/01 04:10:49, yoshiki wrote: > > On 2015/03/31 19:59:47, Jun Mukai wrote: > > > This is weird. Is there some case where the state is inconsistent among > > message > > > center and the MessageCenterNotificationManager? > > > We should fix the inconsistency. > > > > "shown_as_popup" and "read" flags are changed in the message center > > (message_center::NotificationList) internally, and we need to sync them at > > least. > > Commented on the other part. > I believe FindById should not update anything on each notification, it's a > method to lookup existing ones. > > Another suggestion is: > return > message_center_->FindVisibleNotificationById(iter->second->notification().id()); Thank you for the suggestion but we can't do it. FindVisibleNotificationById returns the parent class (message_center::Notification), but FindById need to return the child class (Notification defined in chrome/browser).
https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... File chrome/browser/download/notification/download_notification_item.cc (right): https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... chrome/browser/download/notification/download_notification_item.cc:125: reshow_after_remove = !current_notification->shown_as_popup(); On 2015/04/06 02:22:18, yoshiki wrote: > On 2015/04/01 18:02:11, Jun Mukai wrote: > > You don't have to check through this. Notification's delegate has a callback > > Display() which is called when shown as a popup IIRC. > > Actually, I'd like to check if the notification is currently shown as a popup or > closed after timeout. Is the |shown_as_popup| is true, we can know the popup is > closed after timeout. What will happen if the user explicitly toggles the message center? I believe in that case shown_as_popup will be marked. If it needs to be suppress the timeout, just use SYSTEM priority. # possibly deprioritizing from SYSTEM priority may not restart the timeout, which could be a bug of message center.
On 2015/04/06 08:20:06, Jun Mukai wrote: > https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... > File chrome/browser/download/notification/download_notification_item.cc (right): > > https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... > chrome/browser/download/notification/download_notification_item.cc:125: > reshow_after_remove = !current_notification->shown_as_popup(); > On 2015/04/06 02:22:18, yoshiki wrote: > > On 2015/04/01 18:02:11, Jun Mukai wrote: > > > You don't have to check through this. Notification's delegate has a > callback > > > Display() which is called when shown as a popup IIRC. > > > > Actually, I'd like to check if the notification is currently shown as a popup > or > > closed after timeout. Is the |shown_as_popup| is true, we can know the popup > is > > closed after timeout. > > What will happen if the user explicitly toggles the message center? I believe > in that case shown_as_popup will be marked. > If it needs to be suppress the timeout, just use SYSTEM priority. > # possibly deprioritizing from SYSTEM priority may not restart the timeout, > which could be a bug of message center. Let me explain what we want to do: case #1) Pushing [x] button when the notification is popped up -> Add the notification in the message center silently (don't popup it). case #2) Pushing [x] button in the notification in the message center (not popped up, after timeout) -> Close the notification and cancel downloading. (See. go/cros-download-notification for PRD) Hence, I need to know the notification is currently popped out (#1) or not (#2). I think |shown_as_popup| is the good flag to know it. If you want to know other good way to implement it, let me know. Thanks!
mukai@chromium.org changed reviewers: + dewittj@chromium.org
On 2015/04/06 08:59:24, yoshiki wrote: > On 2015/04/06 08:20:06, Jun Mukai wrote: > > > https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... > > File chrome/browser/download/notification/download_notification_item.cc > (right): > > > > > https://codereview.chromium.org/1005393003/diff/40001/chrome/browser/download... > > chrome/browser/download/notification/download_notification_item.cc:125: > > reshow_after_remove = !current_notification->shown_as_popup(); > > On 2015/04/06 02:22:18, yoshiki wrote: > > > On 2015/04/01 18:02:11, Jun Mukai wrote: > > > > You don't have to check through this. Notification's delegate has a > > callback > > > > Display() which is called when shown as a popup IIRC. > > > > > > Actually, I'd like to check if the notification is currently shown as a > popup > > or > > > closed after timeout. Is the |shown_as_popup| is true, we can know the popup > > is > > > closed after timeout. > > > > What will happen if the user explicitly toggles the message center? I believe > > in that case shown_as_popup will be marked. > > If it needs to be suppress the timeout, just use SYSTEM priority. > > # possibly deprioritizing from SYSTEM priority may not restart the timeout, > > which could be a bug of message center. > > Let me explain what we want to do: > case #1) Pushing [x] button when the notification is popped up -> Add the > notification in the message center silently (don't popup it). > case #2) Pushing [x] button in the notification in the message center (not > popped up, after timeout) -> Close the notification and cancel downloading. > (See. go/cros-download-notification for PRD) > > Hence, I need to know the notification is currently popped out (#1) or not (#2). > I think |shown_as_popup| is the good flag to know it. > > If you want to know other good way to implement it, let me know. Thanks! That UX doesn't sound good to me. Please consult with dewittj@ for further discussion (and ask him for the UX in message center side), but I'm against the idea of changing the semantics of the close button to something else. I recommend to change the user flow: - close button closes the notification, which cancels ongoing download - click on the notification (NotificationDelegate::Click()) will mark the notification as read and moves to the message center bubble. - add notification buttons for more verbose functions, such like 'cancel' or something. Why do you want to change the meaning of the close button in the popup? If you want to suppress the cancel from the popup, simply disabling the close button in the popup would be better.
By the way, if you want to check and update the notification status from delegate, please consider: - MessageCenter::GetPopupNotifications() and find by id - MessageCenter::MarkSinglePopupAsShown() instead of setting the state directly
Thank you for kind explanation. I agreed with you and your recommendation seems better to me. I'll ask the PM about it. (and add you and dweittj@ as CC)
Patchset #5 (id:80001) has been deleted
@mukai, I updated the patch set along with the new way we discussed in the mail. - Close button just closes the notification. - Add "Cancel" button to the notification. Could you take a look again? Thanks.
https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... chrome/browser/notifications/message_center_notification_manager.cc:209: iter->second->notification().id())); I believe this is not necessary anymore. https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/profile_notification.h (right): https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... chrome/browser/notifications/profile_notification.h:34: } Similarly this isn't necessary anymore. # state out-of-sync-ness sounds like a bug indeed, but I think that should be reported separately.
Patchset #6 (id:120001) has been deleted
@mukai, Thanks. PTAL https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/message_center_notification_manager.cc (right): https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... chrome/browser/notifications/message_center_notification_manager.cc:209: iter->second->notification().id())); On 2015/04/08 00:13:19, Jun Mukai wrote: > I believe this is not necessary anymore. Right. Removed. https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... File chrome/browser/notifications/profile_notification.h (right): https://codereview.chromium.org/1005393003/diff/100001/chrome/browser/notific... chrome/browser/notifications/profile_notification.h:34: } On 2015/04/08 00:13:19, Jun Mukai wrote: > Similarly this isn't necessary anymore. > # state out-of-sync-ness sounds like a bug indeed, but I think that should be > reported separately. Removed.
c/b/notifications lgtm
@benjhayden, could you take a look? Thanks.
yoshiki@chromium.org changed reviewers: + phajdan.jr@chromium.org, rdsmith@chromium.org
phajdan.jr@, rdsmith@, could you take a look at the changes in C/B/downloads, since asanka@ is ooo?
Quick high level question: Are you asking for a review for all files in this CL, or just the ones in chrome/browser/downloads (i.e. excluding the files in chrome/browser/downloads/notifications)? I've never seen the notifications code before, so despite the fact that I'm listed as a downloads owner, it would take me a while to come up to speed on it. https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:46: // TODO(yoshiki): This is temporary. Change the asset when prepared. Could you give me context for this comment, preferably by fleshing it out? If I ran into this in landed source, I would have no idea what it meant. https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:66: static StubNotificationUIManager* stub_notification_ui_manager_for_testing_; According to the style guide, all data members should be after all methods. https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:68: friend class test::DownloadNotificationItemTest; I'm used to friending being the first thing in each section (i.e. I'd expect this line to be at the beginning of the private section). The style guide doesn't specifically mention where friend decls should go in the order, but it does imply that all things of a type (constants, methods, data members) should be grouped together.
@rdsmith, thank you for reviewing this. I'd like to ask you reviewing the whole change under C/B/downloads including "notification" directory. The download notification code has introduced lately, and asanka@ reviewed the code. But he is ooo now so I'd like to review the code another reviewer on behalf of him. https://codereview.chromium.org/852043002/ The owner of chrome notification (mukai@) kindly saw the change in C/B/downloads/notifications, so I think this patch don't have major problems form the point of chrome notification. I'd like you to review the code just from the point of download owner. Thanks. https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... File chrome/browser/download/download_commands.cc (right): https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... chrome/browser/download/download_commands.cc:46: // TODO(yoshiki): This is temporary. Change the asset when prepared. On 2015/04/10 21:43:19, rdsmith wrote: > Could you give me context for this comment, preferably by fleshing it out? If I > ran into this in landed source, I would have no idea what it meant. I changed the context in the comment. The Download Notification feature is currently under development and behind the flag. Currently, we are waiting for the proper asset images from the UX guys. Meanwhile we'd like to use temporary images to develop and improve the functionality. I will replace the temporary with them soon after I get them. https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:66: static StubNotificationUIManager* stub_notification_ui_manager_for_testing_; On 2015/04/10 21:43:19, rdsmith wrote: > According to the style guide, all data members should be after all methods. Done. https://codereview.chromium.org/1005393003/diff/140001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:68: friend class test::DownloadNotificationItemTest; On 2015/04/10 21:43:19, rdsmith wrote: > I'm used to friending being the first thing in each section (i.e. I'd expect > this line to be at the beginning of the private section). The style guide > doesn't specifically mention where friend decls should go in the order, but it > does imply that all things of a type (constants, methods, data members) should > be grouped together. I agree that. Done.
Two more high level questions, and then I hope I'll be ready to finish the review: * Can you explain the differences in behavior between the old notification interface and the new one that lead to the removal of reshow_after_remove_, the redirect of NotificationWatcher::Close(), and the OnNotificationClose() and OnNotificationRemoved() calls? I suspect these are all related, but I don't get the big picture. * Why is this CL adding the cancel command to the notification menu? That seems like an orthogonal UI change to the change of notification manager. Thanks in advance. https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:25: : public base::SupportsWeakPtr<DownloadNotificationItem>, Ok, I'll bite. What is this used for? After poking around the source for a while, I can't figure it out. https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:85: static StubNotificationUIManager* stub_notification_ui_manager_for_testing_; nit: The style guide specifies that routines need to be before data members (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...).
On 2015/04/14 00:02:00, rdsmith wrote: > Two more high level questions, and then I hope I'll be ready to finish the > review: > * Can you explain the differences in behavior between the old notification > interface and the new one that lead to the removal of reshow_after_remove_, the > redirect of NotificationWatcher::Close(), and the OnNotificationClose() and > OnNotificationRemoved() calls? I suspect these are all related, but I don't get > the big picture. Old behavior: #1) Pushing [x] button when the notification is popped up -> Add the notification in the message center silently (don't popup it). #2) Pushing [x] button in the notification in the message center (not popped up, after timeout) -> Close the notification and cancel downloading. New behavior: Pushing [x] button just closes the notification and doesn't cancel the download. The chrome notification guys notified me that the old behavior is not match with the UX of common notification. Hence I change the behavior. See the comments #18-21 for the detailed context. > * Why is this CL adding the cancel command to the notification menu? That > seems like an orthogonal UI change to the change of notification manager. It was discussed in the mail with notification guys. Along with the behavior change of [x] button I explained above, we need to prepare another way to cancel the download. The added "Cancel" button is for it. > Thanks in advance. > > https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... > File chrome/browser/download/notification/download_notification_item.h (right): > > https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... > chrome/browser/download/notification/download_notification_item.h:25: : public > base::SupportsWeakPtr<DownloadNotificationItem>, > Ok, I'll bite. What is this used for? After poking around the source for a > while, I can't figure it out. > > https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... > chrome/browser/download/notification/download_notification_item.h:85: static > StubNotificationUIManager* stub_notification_ui_manager_for_testing_; > nit: The style guide specifies that routines need to be before data members > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...).
@rdsmith, PTAL again? https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:25: : public base::SupportsWeakPtr<DownloadNotificationItem>, On 2015/04/14 00:02:00, rdsmith wrote: > Ok, I'll bite. What is this used for? After poking around the source for a > while, I can't figure it out. That was no longer used in the this patchset. Removed. https://codereview.chromium.org/1005393003/diff/180001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:85: static StubNotificationUIManager* stub_notification_ui_manager_for_testing_; On 2015/04/14 00:02:00, rdsmith wrote: > nit: The style guide specifies that routines need to be before data members > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...). Thank you for guidance. Done.
> It was discussed in the mail with notification guys. Along with the behavior change of [x] button I explained above, we need to prepare another way to cancel the download. The added "Cancel" button is for it. Ok, but that hits a minor button of mine. I'm a bit uncomfortable with making an infrastructure change in one CL for use in a future CL; I think it's to easy to create never-used code that way. If that's what's happening here, could we consider postponing that addition until the CL that actually puts in the ability to access the code? Note that I'm not objecting to the temporary image that will be updated later, or adding behavior that's only available behind a flag; it's the inaccessible functionality that bothers me. And I won't block the CL for this--when it makes sense is a judgement call, and this is a space I don't know well enough to be certain of my judgement. But I wanted to raise the possibility of postponing the addition of the cancel functionality until the CL where the notification center can use it (or after, if it's not appropriate in that CL). Beyond that (and the suggestions below, also not blocking), LGTM. https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item.h (right): https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item.h:62: static StubNotificationUIManager* stub_notification_ui_manager_for_testing_; nit, suggestion: There's no real reason that this has to be a class variable rather than a file static/anonymous namespace variable, and I think of the right place for things as the least visible place from which they could still function. Maybe consider moving this into a file static? https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... File chrome/browser/download/notification/download_notification_item_unittest.cc (right): https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... chrome/browser/download/notification/download_notification_item_unittest.cc:148: return !download_notification_item_->notification_->shown_as_popup(); Why the inversion? I don't see anything in the comments in ui/message_center/notification.h suggesting that its shown_as_popup() is inverted, nor anything in the testing below to suggest that this function is inverted. (Not blocking review as it's a purely notification issue, which means I assume it's correct based on that review.)
On 2015/04/15 19:16:31, rdsmith wrote: > > It was discussed in the mail with notification guys. Along with the behavior > change of [x] button I explained above, we need to prepare another way to cancel > the download. The added "Cancel" button is for it. > > Ok, but that hits a minor button of mine. I'm a bit uncomfortable with making > an infrastructure change in one CL for use in a future CL; I think it's to easy > to create never-used code that way. If that's what's happening here, could we > consider postponing that addition until the CL that actually puts in the ability > to access the code? > > Note that I'm not objecting to the temporary image that will be updated later, > or adding behavior that's only available behind a flag; it's the inaccessible > functionality that bothers me. And I won't block the CL for this--when it makes > sense is a judgement call, and this is a space I don't know well enough to be > certain of my judgement. But I wanted to raise the possibility of postponing > the addition of the cancel functionality until the CL where the notification > center can use it (or after, if it's not appropriate in that CL). Sorry for lacking my words. The change in download_command.cc will be used in this CL. The code about "cancel" is not unused code. Because of change in download_notification_item.cc in l.293, this code will be called to retrieve the image of cancel button. I considered to separate the CL into the changing-notification-framework part and the changing-cancel-semantics-part for simplicity. But changing-framework needs to add some methods to implement the OLD notification behavior, but notification guys don't want them and the code will be no longer used after the next patch is landed. So I think it's simpler to do them in the one CL, rather than to add temporary code. > Beyond that (and the suggestions below, also not blocking), LGTM. Thank you for reviewing the CL. Thanks. > https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... > File chrome/browser/download/notification/download_notification_item.h (right): > > https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... > chrome/browser/download/notification/download_notification_item.h:62: static > StubNotificationUIManager* stub_notification_ui_manager_for_testing_; > nit, suggestion: There's no real reason that this has to be a class variable > rather than a file static/anonymous namespace variable, and I think of the right > place for things as the least visible place from which they could still > function. Maybe consider moving this into a file static? Good idea! Done. > https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... > File chrome/browser/download/notification/download_notification_item_unittest.cc > (right): > > https://codereview.chromium.org/1005393003/diff/200001/chrome/browser/downloa... > chrome/browser/download/notification/download_notification_item_unittest.cc:148: > return !download_notification_item_->notification_->shown_as_popup(); > Why the inversion? I don't see anything in the comments in > ui/message_center/notification.h suggesting that its shown_as_popup() is > inverted, nor anything in the testing below to suggest that this function is > inverted. > > (Not blocking review as it's a purely notification issue, which means I assume > it's correct based on that review.) Thanks for pointing out. |shown_as_popup| flag does no longer have meaning because the mock class doesn't do anything to it. I just removed the test of this flag. Thanks.
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mukai@chromium.org Link to the patchset: https://codereview.chromium.org/1005393003/#ps200001 (title: "Addressed the comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1005393003/200001
Message was sent while issue was closed.
Committed patchset #9 (id:200001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/365ac4c9486e5f9963b06de940809189011d84d3 Cr-Commit-Position: refs/heads/master@{#325404} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
