Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583393002:
Send notification to users upon receiving sms messages (Closed)
DescriptionSend notification to users upon receiving sms messages
After receiving a sms message, instead of creating a notification view bubble,
a notification in notification center containing the sms message is sent to user.
Remove notification view implementation because it is only used to display
sms messages.
BUG=630641
TEST=system_tray_unittest
Patch Set 1 #
Total comments: 83
Patch Set 2 : address comments #
Total comments: 14
Patch Set 3 : address comments #
Total comments: 6
Patch Set 4 : address comments #
Total comments: 1
Patch Set 5 : update return type #Messages
Total messages: 104 (88 generated)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [WIP] 2nd try on remove smsrow BUG= ========== to ========== Send notification to users upon receiving sms messages After receiving a sms message, instead of creating a notification view bubble, a notification in notification center containing the sms message is sent to user. Remove notification view implementation because it is only used to display sms messages, BUG=630641 TEST=system_tray_unittest ==========
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
Patchset #1 (id:1) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Description was changed from ========== Send notification to users upon receiving sms messages After receiving a sms message, instead of creating a notification view bubble, a notification in notification center containing the sms message is sent to user. Remove notification view implementation because it is only used to display sms messages, BUG=630641 TEST=system_tray_unittest ========== to ========== Send notification to users upon receiving sms messages After receiving a sms message, instead of creating a notification view bubble, a notification in notification center containing the sms message is sent to user. Remove notification view implementation because it is only used to display sms messages. BUG=630641 TEST=system_tray_unittest ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Send notification to users upon receiving sms messages After receiving a sms message, instead of creating a notification view bubble, a notification in notification center containing the sms message is sent to user. Remove notification view implementation because it is only used to display sms messages. BUG=630641 TEST=system_tray_unittest ========== to ========== Send notification to users upon receiving sms messages After receiving a sms message, instead of creating a notification view bubble, a notification in notification center containing the sms message is sent to user. Remove notification view implementation because it is only used to display sms messages. BUG=630641 TEST=system_tray_unittest ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
yiyix@chromium.org changed reviewers: + sadrul@chromium.org - tdanderson@chromium.org
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org - sadrul@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@tdanderson, could you please review this change? Thank you
So far so good, but I have some comments below: https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:276: sms_observer_ = new SmsObserver(); Is there a specific reason why you chose to instantiate |sms_observer_| here rather than in the ctor or initializer list? https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:391: sms_observer_->Shutdown(); Is there a reason why you need to introduce a Shutdown() method and call it at this specific place? It seems if you make ShelfWidget own SmsObserver, then de-registering the observer can be done in the SmsObserver dtor rather than needing a separate Shutdown() method. If left as-is, I suggest guarding against a null |sms_observer_|. https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.h:131: SmsObserver* sms_observer_; Who owns |sms_observer_|? (Should the owner be ShelfWidget?) https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:2: // Use of this source code is governed by a BSD-style license that can be nit: update header - no (c) and update year. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:72: message_id_++; This member is initialized to 0 when SmsObserver is created, and then you're incrementing by one each time a message is sent. What happens if you send an SMS to the notification center with ID of 0, SmsObserver is destroyed, SmsObserver is re-instantiated, and then you send a second SMS to the notification center, also with an ID of 0? After a quick search through the codebase I can't find any pattern of generating notification IDs using a counter in this way; do you know of any? It seems like what you might want instead is to generate a GUID like in extension_welcome_notification.cc ? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:79: std::unique_ptr<message_center::Notification> notification; I don't think you need to declare this so early, I suggest moving it to just above line 92 https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:90: std::string notification_id = kNotificationId + std::to_string(message_id_); Looks like you don't need this as a named local variable, so just inline this on line 93 https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:91: LOG(ERROR) << notification_id; Left over from debugging? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:98: message_center::RichNotificationData(), nullptr)); Has this CL received approval from UX in terms of the notification type used, behavior, how the notifications remain in the notification center until explicitly closed, how multiple message from the same number are handled, etc? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: update header - no (c) and update year. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:9: nit: no blank line https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:19: class SmsObserver : public chromeos::NetworkSmsHandler::Observer { Please include some class-level documentation here. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:32: void Update(const base::DictionaryValue* message, Why did you choose to make this protected rather than private? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:37: base::ListValue messages_; Please include a brief description for each member. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:41: static const char kNotificationId[]; I don't think you need to have this in the .h https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:19: using ash::mojom::ThirdPartyVpnProviderPtr; Why are these needed? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:38: for (message_center::NotificationList::Notifications::const_iterator iter = consider using a c++11 range-based for loop instead of an iterator here https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:40: iter != notifications.end(); ++iter) { can you run 'git cl format ash' on this CL? the formatting looks a bit odd in places https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:45: return NULL; nit: nullptr https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:110: TEST_F(SmsObserverTest, TextMessageMissingText) { What's the difference between 'missing' and 'empty' text? The last two tests seem to be doing the same thing https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:119: } Also consider adding a test which sends two notifications, each with the same number, and verifying both have been received. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/tray_sms.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/tray_sms.cc:142: IDR_AURA_UBER_TRAY_SMS_DISMISS)); This resource is no longer used anywhere, so as part of this CL please update the entry in the .grd file and delete the corresponding PNG asset(s). https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/screen_security/screen_tray_item.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/screen_security/screen_tray_item.cc:110: screen_tray_item_->HideNotificationView(); I'm a bit confused about what this line of code was doing here in the first place. Does removing it actually result in any user-visible change? As far as I can tell, screen tray items don't have corresponding notification views. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/syst... File ash/common/system/system_notifier.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/syst... ash/common/system/system_notifier.cc:25: kNotifierSms, Why does this need to be ifdefed for CrOS? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:431: bool SystemTray::IsAnyBubbleVisible() const { Since we now only have one bubble possible, this should be re-named to IsSystemBubbleVisible() and the comment in .h updated accordingly. Surrounding () no longer needed, and please call HasSystemBubble() in place of system_bubble_.get(). https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:93: void SetHideNotifications(bool hidden); Is this still needed? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray_item.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray_item.h:98: virtual void DestroyNotificationView(); I think you also want to remove this method. Note you will have to remove it from test_system_tray_item as well. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/tray_notification_view.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/tray_notification_view.cc:108: void TrayNotificationView::UpdateView(views::View* new_contents) { I'm a bit confused why you're deleting all of this code. Won't it change the behaviour / UX of AccessibilityPopupView?
https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:84: } else { Also, you can land this with just lines 82-83. We haven't been bothering to update the non-md code paths for anything 'new'. I think line 87 is the only place that would have still used IDR_AURA_UBER_TRAY_SMS, so you should remove that asset as well.
@tdanderson, Thank you so much for the detailed review. I have tried my best to answer your concerns. If anything is still unclear to you, I am happy to elaborate more. Thank you. https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:276: sms_observer_ = new SmsObserver(); On 2017/01/26 22:19:06, tdanderson wrote: > Is there a specific reason why you chose to instantiate |sms_observer_| here > rather than in the ctor or initializer list? sorry, it's a mistake. https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:391: sms_observer_->Shutdown(); On 2017/01/26 22:19:06, tdanderson wrote: > Is there a reason why you need to introduce a Shutdown() method and call it at > this specific place? It seems if you make ShelfWidget own SmsObserver, then > de-registering the observer can be done in the SmsObserver dtor rather than > needing a separate Shutdown() method. > > If left as-is, I suggest guarding against a null |sms_observer_|. I originally didn't create the shutdown method, then i keep receive this failure as I exit chrome in my local build: [105378:105378:0202/152205.133528:FATAL:observer_list.h(338)] Check failed: ObserverListBase<ObserverType>::size() == 0U (1 vs. 0) From the stack trace, I believe it calls shutdown first then it calls the dtor. So i added this line to de-register sms_observer properly. I have added the check as you have recommended. https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.h:131: SmsObserver* sms_observer_; On 2017/01/26 22:19:06, tdanderson wrote: > Who owns |sms_observer_|? (Should the owner be ShelfWidget?) I think the owner is ShelfWidget as well. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:2: // Use of this source code is governed by a BSD-style license that can be On 2017/01/26 22:19:06, tdanderson wrote: > nit: update header - no (c) and update year. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:72: message_id_++; On 2017/01/26 22:19:06, tdanderson wrote: > This member is initialized to 0 when SmsObserver is created, and then you're > incrementing by one each time a message is sent. What happens if you send an SMS > to the notification center with ID of 0, SmsObserver is destroyed, SmsObserver > is re-instantiated, and then you send a second SMS to the notification center, > also with an ID of 0? > > After a quick search through the codebase I can't find any pattern of generating > notification IDs using a counter in this way; do you know of any? It seems like > what you might want instead is to generate a GUID like in > extension_welcome_notification.cc ? It is how download notification is generated. https://cs.chromium.org/chromium/src/chrome/browser/download/notification/dow... tag is referring to the notification id in our case. To display all downloading item, tag is a strictly increasing uint32_t and initialized at 0 (https://cs.chromium.org/chromium/src/content/browser/download/download_item_i...). To create a different set of id for SmsObserver, i set the notificationid to chrome://network/sms + number. If SmsObserver is destroyed, then ShelfWidget is destroyed. All notification will be closed. Creating another notification with id=chrome://network/sms0 will not override the previous sms notification, so i don't think it will be a problem. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:79: std::unique_ptr<message_center::Notification> notification; On 2017/01/26 22:19:06, tdanderson wrote: > I don't think you need to declare this so early, I suggest moving it to just > above line 92 Sure. Declare the variable before using it. I was thinking that there is no crash/early return between here and l92, so it will get initialized anyways. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:84: } else { On 2017/01/26 23:12:50, tdanderson wrote: > Also, you can land this with just lines 82-83. We haven't been bothering to > update the non-md code paths for anything 'new'. I think line 87 is the only > place that would have still used IDR_AURA_UBER_TRAY_SMS, so you should remove > that asset as well. Yeah~ Finally, we can land code with md path only! Thanks. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:90: std::string notification_id = kNotificationId + std::to_string(message_id_); On 2017/01/26 22:19:06, tdanderson wrote: > Looks like you don't need this as a named local variable, so just inline this on > line 93 I want to keep it as a class variable so other resources can use this notification id to filter sms notifications in the future. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:91: LOG(ERROR) << notification_id; On 2017/01/26 22:19:06, tdanderson wrote: > Left over from debugging? sorry, i missed this line. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:98: message_center::RichNotificationData(), nullptr)); On 2017/01/26 22:19:06, tdanderson wrote: > Has this CL received approval from UX in terms of the notification type used, > behavior, how the notifications remain in the notification center until > explicitly closed, how multiple message from the same number are handled, etc? Nope, i only talked with sebastien on hangout. how could i get approval from ux? Do you know their ldaps? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/26 22:19:06, tdanderson wrote: > nit: update header - no (c) and update year. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:9: On 2017/01/26 22:19:06, tdanderson wrote: > nit: no blank line Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:19: class SmsObserver : public chromeos::NetworkSmsHandler::Observer { On 2017/01/26 22:19:06, tdanderson wrote: > Please include some class-level documentation here. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:32: void Update(const base::DictionaryValue* message, On 2017/01/26 22:19:06, tdanderson wrote: > Why did you choose to make this protected rather than private? Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:37: base::ListValue messages_; On 2017/01/26 22:19:06, tdanderson wrote: > Please include a brief description for each member. I forget to delete this messages_ variable. It's not needed in the new approach. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:41: static const char kNotificationId[]; On 2017/01/26 22:19:06, tdanderson wrote: > I don't think you need to have this in the .h I don't think i need it, but i see this pattern for other notification id, such as screen_layout_observer.h, tray_supervised_user.h.... https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:19: using ash::mojom::ThirdPartyVpnProviderPtr; On 2017/01/26 22:19:06, tdanderson wrote: > Why are these needed? Removed. It is leftover of some random trials i have. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:38: for (message_center::NotificationList::Notifications::const_iterator iter = On 2017/01/26 22:19:07, tdanderson wrote: > consider using a c++11 range-based for loop instead of an iterator here Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:40: iter != notifications.end(); ++iter) { On 2017/01/26 22:19:06, tdanderson wrote: > can you run 'git cl format ash' on this CL? the formatting looks a bit odd in > places It's weird. I did it... didn't change this format. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:45: return NULL; On 2017/01/26 22:19:06, tdanderson wrote: > nit: nullptr Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:110: TEST_F(SmsObserverTest, TextMessageMissingText) { On 2017/01/26 22:19:06, tdanderson wrote: > What's the difference between 'missing' and 'empty' text? The last two tests > seem to be doing the same thing In MessageReceived method in sms_observer, it checks for 2 different cases: - if the key "text" is missing; and - if the value of "text" key is empty. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:119: } On 2017/01/26 22:19:06, tdanderson wrote: > Also consider adding a test which sends two notifications, each with the same > number, and verifying both have been received. That will be a good test. Added. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/tray_sms.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/tray_sms.cc:142: IDR_AURA_UBER_TRAY_SMS_DISMISS)); On 2017/01/26 22:19:05, tdanderson wrote: > This resource is no longer used anywhere, so as part of this CL please update > the entry in the .grd file and delete the corresponding PNG asset(s). Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/screen_security/screen_tray_item.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/screen_security/screen_tray_item.cc:110: screen_tray_item_->HideNotificationView(); On 2017/01/26 22:19:07, tdanderson wrote: > I'm a bit confused about what this line of code was doing here in the first > place. Does removing it actually result in any user-visible change? As far as I > can tell, screen tray items don't have corresponding notification views. I am not sure what does this line do here. Hide NotificationView if there is one? The only class that uses the NotificationView is SmsTray, as we stop displaying sms messages in the NotificationView, NotificationView should not be used anymore. Therefore, i think i can safely remove this line. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/syst... File ash/common/system/system_notifier.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/syst... ash/common/system/system_notifier.cc:25: kNotifierSms, On 2017/01/26 22:19:07, tdanderson wrote: > Why does this need to be ifdefed for CrOS? No sms will be received for no-CrOS? This block of code has been modified since i made the change. It had something like: #if defined(OS_CHROMEOS) kNotifierNetworkError, #endif (example: the diff in this cl, https://codereview.chromium.org/2487463003) Sorry, I didn't merge it too carefully. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:431: bool SystemTray::IsAnyBubbleVisible() const { On 2017/01/26 22:19:07, tdanderson wrote: > Since we now only have one bubble possible, this should be re-named to > IsSystemBubbleVisible() and the comment in .h updated accordingly. Surrounding > () no longer needed, and please call HasSystemBubble() in place of > system_bubble_.get(). Good Call. Thanks. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray.h:93: void SetHideNotifications(bool hidden); On 2017/01/26 22:19:07, tdanderson wrote: > Is this still needed? You are totally correct, I removed the function in .cc but not in .h. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/system_tray_item.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/system_tray_item.h:98: virtual void DestroyNotificationView(); On 2017/01/26 22:19:07, tdanderson wrote: > I think you also want to remove this method. Note you will have to remove it > from test_system_tray_item as well. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/tray_notification_view.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/tray_notification_view.cc:108: void TrayNotificationView::UpdateView(views::View* new_contents) { On 2017/01/26 22:19:07, tdanderson wrote: > I'm a bit confused why you're deleting all of this code. Won't it change the > behaviour / UX of AccessibilityPopupView? My understanding is that SystemTrayBubble::CreateItemViews calls TrayAccessibility::CreateDetailedView to create a new AccessibilityPopupView. Then, this newly created AccessibilityPopupView is added in TrayBubbleView* bubble_view_ (https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray_bubbl...). As a result, the AccessibilityPopupView is not managed by TrayNotificationView. It is managed by the system_tray_bubble. The current behavior is that it either closed by auto close method after 5 seconds or the user clicks on the close button. In conclusion, deleting these functions in TrayNotificationView don't change the current behavior.
Patchset #2 (id:300001) has been deleted
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:320001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:391: sms_observer_->Shutdown(); On 2017/02/02 20:43:55, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Is there a reason why you need to introduce a Shutdown() method and call it at > > this specific place? It seems if you make ShelfWidget own SmsObserver, then > > de-registering the observer can be done in the SmsObserver dtor rather than > > needing a separate Shutdown() method. > > > > If left as-is, I suggest guarding against a null |sms_observer_|. > > I originally didn't create the shutdown method, then i keep receive this failure > as I exit chrome in my local build: > [105378:105378:0202/152205.133528:FATAL:observer_list.h(338)] Check failed: > ObserverListBase<ObserverType>::size() == 0U (1 vs. 0) > > From the stack trace, I believe it calls shutdown first then it calls the dtor. > So i added this line to de-register sms_observer properly. I have added the > check as you have recommended. Sorry, after analyzing again, I understand that the dtor is not called. So I updated it to unique_ptr and removed the unnecessary shutdown method.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:340001) has been deleted
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yi, please see my next round of comments. https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:276: sms_observer_ = new SmsObserver(); On 2017/02/02 20:43:55, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Is there a specific reason why you chose to instantiate |sms_observer_| here > > rather than in the ctor or initializer list? > > sorry, it's a mistake. Acknowledged. https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.cc:391: sms_observer_->Shutdown(); On 2017/02/02 21:24:19, yiyix wrote: > On 2017/02/02 20:43:55, yiyix wrote: > > On 2017/01/26 22:19:06, tdanderson wrote: > > > Is there a reason why you need to introduce a Shutdown() method and call it > at > > > this specific place? It seems if you make ShelfWidget own SmsObserver, then > > > de-registering the observer can be done in the SmsObserver dtor rather than > > > needing a separate Shutdown() method. > > > > > > If left as-is, I suggest guarding against a null |sms_observer_|. > > > > I originally didn't create the shutdown method, then i keep receive this > failure > > as I exit chrome in my local build: > > [105378:105378:0202/152205.133528:FATAL:observer_list.h(338)] Check failed: > > ObserverListBase<ObserverType>::size() == 0U (1 vs. 0) > > > > From the stack trace, I believe it calls shutdown first then it calls the > dtor. > > So i added this line to de-register sms_observer properly. I have added the > > check as you have recommended. > > Sorry, after analyzing again, I understand that the dtor is not called. So I > updated it to unique_ptr and removed the unnecessary shutdown method. Thanks for looking into this in more detail, what you have in PS2 looks good! https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.h:131: SmsObserver* sms_observer_; On 2017/02/02 20:43:55, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Who owns |sms_observer_|? (Should the owner be ShelfWidget?) > > I think the owner is ShelfWidget as well. Great, thanks for clarifying. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:72: message_id_++; On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > This member is initialized to 0 when SmsObserver is created, and then you're > > incrementing by one each time a message is sent. What happens if you send an > SMS > > to the notification center with ID of 0, SmsObserver is destroyed, SmsObserver > > is re-instantiated, and then you send a second SMS to the notification center, > > also with an ID of 0? > > > > After a quick search through the codebase I can't find any pattern of > generating > > notification IDs using a counter in this way; do you know of any? It seems > like > > what you might want instead is to generate a GUID like in > > extension_welcome_notification.cc ? > > It is how download notification is generated. > https://cs.chromium.org/chromium/src/chrome/browser/download/notification/dow... > tag is referring to the notification id in our case. To display all downloading > item, tag is a strictly increasing uint32_t and initialized at 0 > (https://cs.chromium.org/chromium/src/content/browser/download/download_item_i...). > To create a different set of id for SmsObserver, i set the notificationid to > chrome://network/sms + number. Can you double check that you linked to the right code? Following from the links you've given it looks as though each download item gets a unique ID provided by the underlying DownloadItem object. I don't see anywhere that keeps track of its own counter and increments by 1 each time a notification is sent. > > If SmsObserver is destroyed, then ShelfWidget is destroyed. All notification > will be closed. Creating another notification with id=chrome://network/sms0 will > not override the previous sms notification, so i don't think it will be a > problem. Perhaps the views are destroyed, but I'm not sure that the underlying message_center model is destroyed. In other words, I think there may be the possibility of dispatching an SMS notification with a duplicate ID. Anyhow, I would feel much better if you could ask a message_center owner (such as stevenjb@) to review this portion of the CL once all other comments have been addressed. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:79: std::unique_ptr<message_center::Notification> notification; On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > I don't think you need to declare this so early, I suggest moving it to just > > above line 92 > > Sure. Declare the variable before using it. I was thinking that there is no > crash/early return between here and l92, so it will get initialized anyways. Thanks. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:84: } else { On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 23:12:50, tdanderson wrote: > > Also, you can land this with just lines 82-83. We haven't been bothering to > > update the non-md code paths for anything 'new'. I think line 87 is the only > > place that would have still used IDR_AURA_UBER_TRAY_SMS, so you should remove > > that asset as well. > > Yeah~ Finally, we can land code with md path only! Thanks. :) https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:90: std::string notification_id = kNotificationId + std::to_string(message_id_); On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Looks like you don't need this as a named local variable, so just inline this > on > > line 93 > > I want to keep it as a class variable so other resources can use this > notification id to filter sms notifications in the future. I meant that you don't need |notification_id| as a named variable. I was suggesting that you just put 'kNotificationId + std::to_string(message_id_)' in place of 'notification_id' on line 93. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:91: LOG(ERROR) << notification_id; On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Left over from debugging? > > sorry, i missed this line. Acknowledged. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:98: message_center::RichNotificationData(), nullptr)); On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Has this CL received approval from UX in terms of the notification type used, > > behavior, how the notifications remain in the notification center until > > explicitly closed, how multiple message from the same number are handled, etc? > > Nope, i only talked with sebastien on hangout. how could i get approval from ux? > Do you know their ldaps? Approval from Sebastien is enough in this case, thanks. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:37: base::ListValue messages_; On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Please include a brief description for each member. > > I forget to delete this messages_ variable. It's not needed in the new approach. Acknowledged. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:41: static const char kNotificationId[]; On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > I don't think you need to have this in the .h > > I don't think i need it, but i see this pattern for other notification id, such > as screen_layout_observer.h, tray_supervised_user.h.... It's good to keep the header file as slim as possible, and unless there is a good reason to expose the constant here I'd rather it just be declared in the .cc. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:19: using ash::mojom::ThirdPartyVpnProviderPtr; On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > Why are these needed? > > Removed. It is leftover of some random trials i have. Acknowledged. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:38: for (message_center::NotificationList::Notifications::const_iterator iter = On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:07, tdanderson wrote: > > consider using a c++11 range-based for loop instead of an iterator here > > Done. I'm a bit confused... all of this code seems to have been removed in Patch Set 2. Was that intentional? https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:40: iter != notifications.end(); ++iter) { On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:06, tdanderson wrote: > > can you run 'git cl format ash' on this CL? the formatting looks a bit odd in > > places > > It's weird. I did it... didn't change this format. Acknowledged. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/screen_security/screen_tray_item.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/screen_security/screen_tray_item.cc:110: screen_tray_item_->HideNotificationView(); On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:07, tdanderson wrote: > > I'm a bit confused about what this line of code was doing here in the first > > place. Does removing it actually result in any user-visible change? As far as > I > > can tell, screen tray items don't have corresponding notification views. > > I am not sure what does this line do here. Hide NotificationView if there is > one? The only class that uses the NotificationView is SmsTray, as we stop > displaying sms messages in the NotificationView, NotificationView should not be > used anymore. Therefore, i think i can safely remove this line. Agreed. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/syst... File ash/common/system/system_notifier.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/syst... ash/common/system/system_notifier.cc:25: kNotifierSms, On 2017/02/02 20:43:56, yiyix wrote: > On 2017/01/26 22:19:07, tdanderson wrote: > > Why does this need to be ifdefed for CrOS? > > No sms will be received for no-CrOS? This block of code has been modified since > i made the change. It had something like: > #if defined(OS_CHROMEOS) > kNotifierNetworkError, > #endif > (example: the diff in this cl, https://codereview.chromium.org/2487463003) > Sorry, I didn't merge it too carefully. Thanks, the way you have it in Patch set 2 looks good. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/tray_notification_view.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/tray_notification_view.cc:108: void TrayNotificationView::UpdateView(views::View* new_contents) { On 2017/02/02 20:43:57, yiyix wrote: > On 2017/01/26 22:19:07, tdanderson wrote: > > I'm a bit confused why you're deleting all of this code. Won't it change the > > behaviour / UX of AccessibilityPopupView? > > My understanding is that SystemTrayBubble::CreateItemViews calls > TrayAccessibility::CreateDetailedView to create a new AccessibilityPopupView. > Then, this newly created AccessibilityPopupView is added in TrayBubbleView* > bubble_view_ > (https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray_bubbl...). > As a result, the AccessibilityPopupView is not managed by TrayNotificationView. > It is managed by the system_tray_bubble. The current behavior is that it either > closed by auto close method after 5 seconds or the user clicks on the close > button. > > In conclusion, deleting these functions in TrayNotificationView don't change the > current behavior. AccessibilityPopupView is a subclass of TrayNotificationView. So by removing the *AutoCloseTimer() methods below, I don't think the AccessibilityPopupView will automatically close have 5 seconds. Or am I misunderstanding and is that implemented somewhere else? In the code below please just delete the dead parts and leave the rest intact; we have plans to deprecate AccessibilityPopupView and move it into the notification center as part of crbug.com/685845. https://codereview.chromium.org/2583393002/diff/360001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.h (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.h:131: // Owned by ShelfWidget. nit: This comment is not needed in this case since the ownership is made clear by the fact that you're using a unique_ptr. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:77: gfx::Image image(gfx::Image( You don't need the inner gfx::Image here, just gfx::Image image(CreateVectorIcon()) is sufficient. Furthermore I think you want |image| to be type ImageSkia instead of Image. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:27: return (shelf_widget->sms_observer_).get(); I don't think the () are necessary here. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:432: return (HasSystemBubble() && system_bubble_->bubble()->IsVisible()); nit: outer () not needed. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... ash/common/system/tray/system_tray.h:105: // Returns true if SystemBubble is visible. nit: "the system bubble" instead of the class name SystemBubble. https://codereview.chromium.org/2583393002/diff/360001/ash/resources/ash_reso... File ash/resources/ash_resources.grd (left): https://codereview.chromium.org/2583393002/diff/360001/ash/resources/ash_reso... ash/resources/ash_resources.grd:197: <structure type="chrome_scaled_image" name="IDR_AURA_UBER_TRAY_SMS" file="cros/status/status_sms.png" /> This asset needs to remain in the code base, since it is still used by the notification center. Only the SMS_DISMISS one should be removed.
I will leave the review to tdanderson@, since I haven't worked on this in a while. But I am very glad that this is getting cleaned up. Thank you! https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/stat... ash/common/system/status_area_widget.cc:147: return false; You can remove this too.
@tdanderson, could you please take another look at this change? @stevenjb, I create sms notification id by "chrome://network/sms" + message_id_, where message_id_ is a class variable and it's increased by 1 after receiving a new sms message. Could you review sms_observer.cc to check if it is riget way to create notification ids? Thank you so much! I tried to replicate how download item notification id is created as here: https://cs.chromium.org/chromium/src/chrome/browser/download/chrome_download_... https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:72: message_id_++; On 2017/02/07 00:15:45, tdanderson wrote: > On 2017/02/02 20:43:56, yiyix wrote: > > On 2017/01/26 22:19:06, tdanderson wrote: > > > This member is initialized to 0 when SmsObserver is created, and then you're > > > incrementing by one each time a message is sent. What happens if you send an > > SMS > > > to the notification center with ID of 0, SmsObserver is destroyed, > SmsObserver > > > is re-instantiated, and then you send a second SMS to the notification > center, > > > also with an ID of 0? > > > > > > After a quick search through the codebase I can't find any pattern of > > generating > > > notification IDs using a counter in this way; do you know of any? It seems > > like > > > what you might want instead is to generate a GUID like in > > > extension_welcome_notification.cc ? > > > > It is how download notification is generated. > > > https://cs.chromium.org/chromium/src/chrome/browser/download/notification/dow... > > tag is referring to the notification id in our case. To display all > downloading > > item, tag is a strictly increasing uint32_t and initialized at 0 > > > (https://cs.chromium.org/chromium/src/content/browser/download/download_item_i...). > > To create a different set of id for SmsObserver, i set the notificationid to > > chrome://network/sms + number. > > Can you double check that you linked to the right code? Following from the links > you've given it looks as though each download item gets a unique ID provided by > the underlying DownloadItem object. I don't see anywhere that keeps track of its > own counter and increments by 1 each time a notification is sent. > > > > > If SmsObserver is destroyed, then ShelfWidget is destroyed. All notification > > will be closed. Creating another notification with id=chrome://network/sms0 > will > > not override the previous sms notification, so i don't think it will be a > > problem. > > Perhaps the views are destroyed, but I'm not sure that the underlying > message_center model is destroyed. In other words, I think there may be the > possibility of dispatching an SMS notification with a duplicate ID. > > Anyhow, I would feel much better if you could ask a message_center owner (such > as stevenjb@) to review this portion of the CL once all other comments have been > addressed. As we have discussed offline, The logic where it increases the download id is here: https://cs.chromium.org/chromium/src/chrome/browser/download/chrome_download_... As we have agreed, after addressing all comments from this cl, i will ask @stevenjb to review this particular class. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:90: std::string notification_id = kNotificationId + std::to_string(message_id_); On 2017/02/07 00:15:45, tdanderson wrote: > On 2017/02/02 20:43:56, yiyix wrote: > > On 2017/01/26 22:19:06, tdanderson wrote: > > > Looks like you don't need this as a named local variable, so just inline > this > > on > > > line 93 > > > > I want to keep it as a class variable so other resources can use this > > notification id to filter sms notifications in the future. > > I meant that you don't need |notification_id| as a named variable. I was > suggesting that you just put 'kNotificationId + std::to_string(message_id_)' in > place of 'notification_id' on line 93. Done. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:98: message_center::RichNotificationData(), nullptr)); On 2017/02/07 00:15:45, tdanderson wrote: > On 2017/02/02 20:43:56, yiyix wrote: > > On 2017/01/26 22:19:06, tdanderson wrote: > > > Has this CL received approval from UX in terms of the notification type > used, > > > behavior, how the notifications remain in the notification center until > > > explicitly closed, how multiple message from the same number are handled, > etc? > > > > Nope, i only talked with sebastien on hangout. how could i get approval from > ux? > > Do you know their ldaps? > > Approval from Sebastien is enough in this case, thanks. I will ask him to approve it. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.h (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.h:41: static const char kNotificationId[]; On 2017/02/07 00:15:45, tdanderson wrote: > On 2017/02/02 20:43:56, yiyix wrote: > > On 2017/01/26 22:19:06, tdanderson wrote: > > > I don't think you need to have this in the .h > > > > I don't think i need it, but i see this pattern for other notification id, > such > > as screen_layout_observer.h, tray_supervised_user.h.... > > It's good to keep the header file as slim as possible, and unless there is a > good reason to expose the constant here I'd rather it just be declared in the > .cc. Agree. I will add it in this function as you suggested earlier. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:38: for (message_center::NotificationList::Notifications::const_iterator iter = On 2017/02/07 00:15:45, tdanderson wrote: > On 2017/02/02 20:43:56, yiyix wrote: > > On 2017/01/26 22:19:07, tdanderson wrote: > > > consider using a c++11 range-based for loop instead of an iterator here > > > > Done. > > I'm a bit confused... all of this code seems to have been removed in Patch Set > 2. Was that intentional? Yes, it is intentional. This method only returns the first notification with the given id, so it doesn't really help me with cases where they are multiple notifications. Also I check if notification center is empty before sending the sms message, so there should not be other notification in the notification center, i.e., a filter like this should not be needed. I also believe it is okay to fail the test if more notification is created upon receiving sms messages. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... File ash/common/system/tray/tray_notification_view.cc (left): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/tray... ash/common/system/tray/tray_notification_view.cc:108: void TrayNotificationView::UpdateView(views::View* new_contents) { On 2017/02/07 00:15:45, tdanderson wrote: > On 2017/02/02 20:43:57, yiyix wrote: > > On 2017/01/26 22:19:07, tdanderson wrote: > > > I'm a bit confused why you're deleting all of this code. Won't it change the > > > behaviour / UX of AccessibilityPopupView? > > > > My understanding is that SystemTrayBubble::CreateItemViews calls > > TrayAccessibility::CreateDetailedView to create a new AccessibilityPopupView. > > Then, this newly created AccessibilityPopupView is added in TrayBubbleView* > > bubble_view_ > > > (https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray_bubbl...). > > As a result, the AccessibilityPopupView is not managed by > TrayNotificationView. > > It is managed by the system_tray_bubble. The current behavior is that it > either > > closed by auto close method after 5 seconds or the user clicks on the close > > button. > > > > In conclusion, deleting these functions in TrayNotificationView don't change > the > > current behavior. > > AccessibilityPopupView is a subclass of TrayNotificationView. So by removing the > *AutoCloseTimer() methods below, I don't think the AccessibilityPopupView will > automatically close have 5 seconds. Or am I misunderstanding and is that > implemented somewhere else? > > In the code below please just delete the dead parts and leave the rest intact; > we have plans to deprecate AccessibilityPopupView and move it into the > notification center as part of crbug.com/685845. I have tried and AccessibilityPopupView is auto closed. As I have explained above, AccessibilityPopupView is owned by SystemTrayBubble and this AutoCloseTimer() is called: https://cs.chromium.org/chromium/src/ash/common/system/tray/system_tray_bubbl... https://codereview.chromium.org/2583393002/diff/360001/ash/common/shelf/shelf... File ash/common/shelf/shelf_widget.h (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/shelf/shelf... ash/common/shelf/shelf_widget.h:131: // Owned by ShelfWidget. On 2017/02/07 00:15:45, tdanderson wrote: > nit: This comment is not needed in this case since the ownership is made clear > by the fact that you're using a unique_ptr. Done. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:77: gfx::Image image(gfx::Image( On 2017/02/07 00:15:45, tdanderson wrote: > You don't need the inner gfx::Image here, just gfx::Image > image(CreateVectorIcon()) is sufficient. > > Furthermore I think you want |image| to be type ImageSkia instead of Image. I will address it as we have discussed offline. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:27: return (shelf_widget->sms_observer_).get(); On 2017/02/07 00:15:45, tdanderson wrote: > I don't think the () are necessary here. Done. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/stat... File ash/common/system/status_area_widget.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/stat... ash/common/system/status_area_widget.cc:147: return false; On 2017/02/07 05:42:20, sadrul wrote: > You can remove this too. true, I don't need a separate case for this to return false. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... File ash/common/system/tray/system_tray.cc (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... ash/common/system/tray/system_tray.cc:432: return (HasSystemBubble() && system_bubble_->bubble()->IsVisible()); On 2017/02/07 00:15:46, tdanderson wrote: > nit: outer () not needed. Done. https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... File ash/common/system/tray/system_tray.h (right): https://codereview.chromium.org/2583393002/diff/360001/ash/common/system/tray... ash/common/system/tray/system_tray.h:105: // Returns true if SystemBubble is visible. On 2017/02/07 00:15:46, tdanderson wrote: > nit: "the system bubble" instead of the class name SystemBubble. Done. https://codereview.chromium.org/2583393002/diff/360001/ash/resources/ash_reso... File ash/resources/ash_resources.grd (left): https://codereview.chromium.org/2583393002/diff/360001/ash/resources/ash_reso... ash/resources/ash_resources.grd:197: <structure type="chrome_scaled_image" name="IDR_AURA_UBER_TRAY_SMS" file="cros/status/status_sms.png" /> On 2017/02/07 00:15:46, tdanderson wrote: > This asset needs to remain in the code base, since it is still used by the > notification center. Only the SMS_DISMISS one should be removed. As we have discussed offline, it is safe to remove both.
yiyix@chromium.org changed reviewers: + stevenjb@chromium.org
Thanks for working on this, I've wanted this done for ages! Can we move the accessibility changes to a separate CL to make this one smaller / more manageable? Thanks! https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:69: Update(&message, message_text, message_number); We should pass message_id_ to Update() and rename Update() to something like SendMessage() and make it a non member function in an anonymous namespace since I don't think it uses any other member variables? (Otherwise it is odd to increment it here and use it elsewhere). https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:44: sms->SetString("timestamp", "Fri Jun 8 13:26:04 EDT 2016"); You set up a test message in several places, it would be easier to read with a helper function (also, sms leaks) e.g.: base::unique_ptr<DictionaryValue> sms = CreateMessage(kDefaultNumber, kDefaultMessage, kDefaultTimestamp); https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:147: NOTREACHED(); Use ASSERT_TRUE(false) instead of NOTREACHED in tests. (NOTREACHED crashes in debug test runs only, making the failure difficult to identify).
Thanks for working on this, I've wanted this done for ages! Can we move the accessibility changes to a separate CL to make this one smaller / more manageable? Thanks!
ash/common/system pieces LGTM with stevenjb@'s comments addressed. https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/280001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:38: for (message_center::NotificationList::Notifications::const_iterator iter = On 2017/02/07 22:03:22, yiyix wrote: > On 2017/02/07 00:15:45, tdanderson wrote: > > On 2017/02/02 20:43:56, yiyix wrote: > > > On 2017/01/26 22:19:07, tdanderson wrote: > > > > consider using a c++11 range-based for loop instead of an iterator here > > > > > > Done. > > > > I'm a bit confused... all of this code seems to have been removed in Patch Set > > 2. Was that intentional? > > Yes, it is intentional. This method only returns the first notification with the > given id, so it doesn't really help me with cases where they are multiple > notifications. > > Also I check if notification center is empty before sending the sms message, so > there should not be other notification in the notification center, i.e., a > filter like this should not be needed. I also believe it is okay to fail the > test if more notification is created upon receiving sms messages. Acknowledged.
@stevenjb, Thank you for the reviewing. I have addressed your comments and I will split it into 2 cls: 1. using notification center to display sms messages, 2. removing notification view related deadcode. I will include you in the first cl. https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer.cc (right): https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer.cc:69: Update(&message, message_text, message_number); On 2017/02/07 22:30:25, stevenjb wrote: > We should pass message_id_ to Update() and rename Update() to something like > SendMessage() and make it a non member function in an anonymous namespace since > I don't think it uses any other member variables? (Otherwise it is odd to > increment it here and use it elsewhere). > Done. https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:44: sms->SetString("timestamp", "Fri Jun 8 13:26:04 EDT 2016"); On 2017/02/07 22:30:25, stevenjb wrote: > You set up a test message in several places, it would be easier to read with a > helper function (also, sms leaks) e.g.: > > base::unique_ptr<DictionaryValue> sms = CreateMessage(kDefaultNumber, > kDefaultMessage, kDefaultTimestamp); > > You are right, a helper function would save many line. https://codereview.chromium.org/2583393002/diff/380001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:147: NOTREACHED(); On 2017/02/07 22:30:25, stevenjb wrote: > Use ASSERT_TRUE(false) instead of NOTREACHED in tests. (NOTREACHED crashes in > debug test runs only, making the failure difficult to identify). Good to know. Updated.
https://codereview.chromium.org/2583393002/diff/400001/ash/common/system/chro... File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): https://codereview.chromium.org/2583393002/diff/400001/ash/common/system/chro... ash/common/system/chromeos/network/sms_observer_unittest.cc:34: base::DictionaryValue* CreateMessage(const char* kDefaultNumber, This should return a unique_ptr<> to ensure ownership.
On 2017/02/09 19:12:38, stevenjb wrote: > https://codereview.chromium.org/2583393002/diff/400001/ash/common/system/chro... > File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): > > https://codereview.chromium.org/2583393002/diff/400001/ash/common/system/chro... > ash/common/system/chromeos/network/sms_observer_unittest.cc:34: > base::DictionaryValue* CreateMessage(const char* kDefaultNumber, > This should return a unique_ptr<> to ensure ownership. Done
On 2017/02/09 19:12:38, stevenjb wrote: > https://codereview.chromium.org/2583393002/diff/400001/ash/common/system/chro... > File ash/common/system/chromeos/network/sms_observer_unittest.cc (right): > > https://codereview.chromium.org/2583393002/diff/400001/ash/common/system/chro... > ash/common/system/chromeos/network/sms_observer_unittest.cc:34: > base::DictionaryValue* CreateMessage(const char* kDefaultNumber, > This should return a unique_ptr<> to ensure ownership. As promised, the first cl where "using notification center to display sms messages" is ready for review: https://codereview.chromium.org/2690543003/ |
