Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2128053002:
Implements the feature notification for Quick Unlock (Closed)
|
Created:
4 years, 5 months ago by malaykeshav Modified:
4 years, 5 months ago CC:
achuith+watch_chromium.org, chromium-reviews, davemoore+watch_chromium.org, dzhioev+watch_chromium.org, kalyank, oshima+watch_chromium.org, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplements the feature notification for Quick Unlock
Displays a feature notification informing the user about quick unlock.
Upon clicking the notification the user is taken to the settings page.
The UI strings used in the notification are still TODO and not final.
BUG=626079
COMPONENT=Chrome OS, Quick Unlock, Preferences, Strings
Committed: https://crrev.com/b2b9f1278e86ff8e05c7d06f8bd315c792da2662
Cr-Commit-Position: refs/heads/master@{#405105}
Patch Set 1 #
Total comments: 15
Patch Set 2 #
Total comments: 2
Patch Set 3 : Implements the feature notification for Quick Unlock #Patch Set 4 : Implements the feature notification for Quick Unlock #Patch Set 5 : Implements the feature notification for Quick Unlock #
Total comments: 26
Patch Set 6 : Implements the feature notification for Quick Unlock #
Total comments: 14
Patch Set 7 : Implements the feature notification for Quick Unlock #
Total comments: 6
Patch Set 8 : Implements the feature notification for Quick Unlock #
Total comments: 1
Patch Set 9 : Implements the feature notification for Quick Unlock #Messages
Total messages: 65 (37 generated)
malaykeshav@chromium.org changed reviewers: + jdufault@chromium.org
The CQ bit was checked by malaykeshav@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_...)
Thanks for doing this! https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.cc (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.cc:46: // Do not show survey if this is a guest session. Update comment (don't mention 'survey') https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.cc:102: Profile* profile) { Drop the profile parameter; just use profile_ directly. https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.h:21: class FeatureNotificationController : public NotificationDelegate { What about naming this just QuickUnlockNotification and then dropping the namespace quickunlock? https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:1749: qu_feature_notification_handler_.insert( The notification should only be launched after the user has unlocked the device (so not after signing in, but after unlocking). This is probably a good place to put the check: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.h:481: scoped_refptr<quickunlock::FeatureNotificationController>, Will std::unique_ptr work here?
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by malaykeshav@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/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.cc (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.cc:46: // Do not show survey if this is a guest session. On 2016/07/06 at 21:06:43, jdufault wrote: > Update comment (don't mention 'survey') Done https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.cc:102: Profile* profile) { On 2016/07/06 at 21:06:43, jdufault wrote: > Drop the profile parameter; just use profile_ directly. Isn't it cleaner and more readable to make the method dependency explicit? https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.h:21: class FeatureNotificationController : public NotificationDelegate { On 2016/07/06 at 21:06:43, jdufault wrote: > What about naming this just QuickUnlockNotification and then dropping the namespace quickunlock? This name is slightly misleading since this is a feature notification for quick unlock. https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.cc:1749: qu_feature_notification_handler_.insert( On 2016/07/06 at 21:06:43, jdufault wrote: > The notification should only be launched after the user has unlocked the device (so not after signing in, but after unlocking). > > This is probably a good place to put the check: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... Done https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.h:481: scoped_refptr<quickunlock::FeatureNotificationController>, On 2016/07/06 at 21:06:43, jdufault wrote: > Will std::unique_ptr work here? unique_ptr would work in place of scoped_ptr but not with scoped_refptr.
https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.cc (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.cc:102: Profile* profile) { On 2016/07/07 21:39:22, malaykeshav wrote: > On 2016/07/06 at 21:06:43, jdufault wrote: > > Drop the profile parameter; just use profile_ directly. > > Isn't it cleaner and more readable to make the method dependency explicit? That's fine - but then you should move this function to an anonymous namespace so there is no implicit `this` dependency. Right now, the profile dependency is available from two sources - the reader has to figure out what/if there is a difference between them is. https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.h:21: class FeatureNotificationController : public NotificationDelegate { On 2016/07/07 21:39:22, malaykeshav wrote: > On 2016/07/06 at 21:06:43, jdufault wrote: > > What about naming this just QuickUnlockNotification and then dropping the > namespace quickunlock? > > This name is slightly misleading since this is a feature notification for quick > unlock. QuickUnlockNotificationController then? I think we can drop Feature because there is only one quick unlock notification type. I think we should drop the quickunlock namespace, since this is only place it would be used. one-off namespaces seem pretty unusual in the codebase, too (but there are probably instances of it). https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/session/user_session_manager.h:481: scoped_refptr<quickunlock::FeatureNotificationController>, On 2016/07/07 21:39:22, malaykeshav wrote: > On 2016/07/06 at 21:06:43, jdufault wrote: > > Will std::unique_ptr work here? > > unique_ptr would work in place of scoped_ptr but not with scoped_refptr. Discussed offline, scoped_refptr is needed. A while back all of the scoped_ptr usages were converted to unique_ptr. https://codereview.chromium.org/2128053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2128053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:199: scoped_refptr<quickunlock::FeatureNotificationController> The screen locker shouldn't own an instance of the controller (especially since the controller is only allocated when the screen locker is about to be destroyed). When the controller is allocated the Notification instance should be the only owner. Please add a note that the controller is owned by the Notification.
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 malaykeshav@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/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.cc (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.cc:102: Profile* profile) { On 2016/07/07 at 22:08:01, jdufault wrote: > On 2016/07/07 21:39:22, malaykeshav wrote: > > On 2016/07/06 at 21:06:43, jdufault wrote: > > > Drop the profile parameter; just use profile_ directly. > > > > Isn't it cleaner and more readable to make the method dependency explicit? > > That's fine - but then you should move this function to an anonymous namespace so there is no implicit `this` dependency. > > Right now, the profile dependency is available from two sources - the reader has to figure out what/if there is a difference between them is. Moving to anonymous namespace. https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/quick_unlock/feature_notification.h (right): https://codereview.chromium.org/2128053002/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/quick_unlock/feature_notification.h:21: class FeatureNotificationController : public NotificationDelegate { On 2016/07/07 at 22:08:01, jdufault wrote: > On 2016/07/07 21:39:22, malaykeshav wrote: > > On 2016/07/06 at 21:06:43, jdufault wrote: > > > What about naming this just QuickUnlockNotification and then dropping the > > namespace quickunlock? > > > > This name is slightly misleading since this is a feature notification for quick > > unlock. > > QuickUnlockNotificationController then? I think we can drop Feature because there is only one quick unlock notification type. > > I think we should drop the quickunlock namespace, since this is only place it would be used. one-off namespaces seem pretty unusual in the codebase, too (but there are probably instances of it). Done https://codereview.chromium.org/2128053002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2128053002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:199: scoped_refptr<quickunlock::FeatureNotificationController> On 2016/07/07 at 22:08:01, jdufault wrote: > The screen locker shouldn't own an instance of the controller (especially since the controller is only allocated when the screen locker is about to be destroyed). When the controller is allocated the Notification instance should be the only owner. > > Please add a note that the controller is owned by the Notification. Done
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 malaykeshav@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by malaykeshav@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/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:9: #include "chrome/browser/chrome_notification_types.h" Can you please verify that all of the includes are needed? https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:52: content::NotificationService::AllSources())) { Move into a helper function, UnregisterNotification(). https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:72: return true; Can you have this always return false for now with a TODO(jdufault): Enable once quick unlock settings land. https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:85: // If notification is not from screen unlock, then do nothing. Either update the comment or the variable name, since they imply different things. https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:90: content::NotificationService::AllSources()); Call helper function UnregisterNotification() https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:93: std::unique_ptr<Notification> notification(CreateNotification()); This should verify that the notification type is NOTIFICATION_SCREEN_LOCK_STATE_CHANGED. https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:107: chrome::NavigateParams params(profile_, GURL(chrome::kChromeUISettingsURL), chrome://settings/quickUnlock/authenticate is the URL that this should open (atm it is probably chrome://md-settings/quickUnlock/authenticate). https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h:10: #include "chrome/browser/notifications/notification.h" Can this class be forward declared? https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h:12: #include "components/signin/core/account_id/account_id.h" Is this needed? https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h:48: std::unique_ptr<content::NotificationRegistrar> registrar_; Does this need to be a unique_ptr, or can it just be content::NotificationRegistrar? https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1743: if (QuickUnlockNotificationController::ShouldShowNotificationToProfile( What about just QuickUnlockNotificationController::ShouldShow? Notification is redundant. I'm not sure that ToProfile adds much either, since profile is in the argument list. https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1747: auto qu_feature_notification_controller = nit: auto* https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:483: qu_feature_notification_handler_; nit: Rename to quick_unlock_notification_handler_
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 malaykeshav@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...
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:9: #include "chrome/browser/chrome_notification_types.h" On 2016/07/08 at 21:15:41, jdufault wrote: > Can you please verify that all of the includes are needed? Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:52: content::NotificationService::AllSources())) { On 2016/07/08 at 21:15:41, jdufault wrote: > Move into a helper function, UnregisterNotification(). Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:72: return true; On 2016/07/08 at 21:15:41, jdufault wrote: > Can you have this always return false for now with a > > TODO(jdufault): Enable once quick unlock settings land. Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:85: // If notification is not from screen unlock, then do nothing. On 2016/07/08 at 21:15:41, jdufault wrote: > Either update the comment or the variable name, since they imply different things. Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:90: content::NotificationService::AllSources()); On 2016/07/08 at 21:15:41, jdufault wrote: > Call helper function UnregisterNotification() Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:93: std::unique_ptr<Notification> notification(CreateNotification()); On 2016/07/08 at 21:15:41, jdufault wrote: > This should verify that the notification type is NOTIFICATION_SCREEN_LOCK_STATE_CHANGED. Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:107: chrome::NavigateParams params(profile_, GURL(chrome::kChromeUISettingsURL), On 2016/07/08 at 21:15:41, jdufault wrote: > chrome://settings/quickUnlock/authenticate is the URL that this should open (atm it is probably chrome://md-settings/quickUnlock/authenticate). Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h:10: #include "chrome/browser/notifications/notification.h" On 2016/07/08 at 21:15:41, jdufault wrote: > Can this class be forward declared? Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h:12: #include "components/signin/core/account_id/account_id.h" On 2016/07/08 at 21:15:41, jdufault wrote: > Is this needed? Not anymore. Removed. https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.h:48: std::unique_ptr<content::NotificationRegistrar> registrar_; On 2016/07/08 at 21:15:41, jdufault wrote: > Does this need to be a unique_ptr, or can it just be content::NotificationRegistrar? Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1743: if (QuickUnlockNotificationController::ShouldShowNotificationToProfile( On 2016/07/08 at 21:15:41, jdufault wrote: > What about just QuickUnlockNotificationController::ShouldShow? > > Notification is redundant. I'm not sure that ToProfile adds much either, since profile is in the argument list. Done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1747: auto qu_feature_notification_controller = On 2016/07/08 at 21:15:42, jdufault wrote: > nit: auto* done https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.h (right): https://codereview.chromium.org/2128053002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.h:483: qu_feature_notification_handler_; On 2016/07/08 at 21:15:42, jdufault wrote: > nit: Rename to quick_unlock_notification_handler_ Done
https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:57: if (profile->IsGuestSession()) Can you also add a check for PinStorage::IsPinSet[1]? This would ensure that if the pin is already enabled the notification is not shown. 1: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo... https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:65: // TODO(jdufault): Enable once quick unlock settings land(crbug.com/291747). nit: newline above comment https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:69: // NotificationDelegate override: Remove this comment https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:85: if (type != chrome::NOTIFICATION_SCREEN_LOCK_STATE_CHANGED) This should come before the is_screen_locked check, right? Otherwise details might not be a boolean value. https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:103: // TODO(http://crbug.com/291747): Redirect to specific checkbox location in Remove this TODO https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:134: // quick unlock feature notiifcation. nit: spelling on notification https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1744: if (quick_unlock_notification_handler_.find(profile) == Instead of two ifs just use an &&
https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:57: if (profile->IsGuestSession()) On 2016/07/11 at 18:03:48, jdufault wrote: > Can you also add a check for PinStorage::IsPinSet[1]? This would ensure that if the pin is already enabled the notification is not shown. > > 1: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo... Done https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:65: // TODO(jdufault): Enable once quick unlock settings land(crbug.com/291747). On 2016/07/11 at 18:03:48, jdufault wrote: > nit: newline above comment Done https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:69: // NotificationDelegate override: On 2016/07/11 at 18:03:49, jdufault wrote: > Remove this comment This is an override though. Why remove it? https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:85: if (type != chrome::NOTIFICATION_SCREEN_LOCK_STATE_CHANGED) On 2016/07/11 at 18:03:49, jdufault wrote: > This should come before the is_screen_locked check, right? Otherwise details might not be a boolean value. Done https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:103: // TODO(http://crbug.com/291747): Redirect to specific checkbox location in On 2016/07/11 at 18:03:48, jdufault wrote: > Remove this TODO Done https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:134: // quick unlock feature notiifcation. On 2016/07/11 at 18:03:49, jdufault wrote: > nit: spelling on notification Done https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2128053002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/session/user_session_manager.cc:1744: if (quick_unlock_notification_handler_.find(profile) == On 2016/07/11 at 18:03:49, jdufault wrote: > Instead of two ifs just use an && Done
lgtm
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org
lgtm with nit and question. https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:65: return false; nit: test this first https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:71: } nit: check this second (less expensive than getting a PinStorage instance) https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:95: Should we check ShouldShow here? It seems conceivable that some state may have changed between when this was constructed and when the event gets fired?
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:65: return false; On 2016/07/11 at 19:55:46, stevenjb wrote: > nit: test this first Done https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:71: } On 2016/07/11 at 19:55:46, stevenjb wrote: > nit: check this second (less expensive than getting a PinStorage instance) Done https://codereview.chromium.org/2128053002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:95: On 2016/07/11 at 19:55:46, stevenjb wrote: > Should we check ShouldShow here? It seems conceivable that some state may have changed between when this was constructed and when the event gets fired? +1 The state may change before the execution reaches this point. Another call to ShouldShow(profile) will have to be added here.
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/2128053002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc (right): https://codereview.chromium.org/2128053002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_notification_controller.cc:100: if (ShouldShow(profile_)) { Shouldn't this be !ShouldShow ?
The CQ bit was checked by malaykeshav@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_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 malaykeshav@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2128053002/#ps220001 (title: "Implements the feature notification for Quick Unlock")
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 malaykeshav@chromium.org
The CQ bit was checked by malaykeshav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Implements the feature notification for Quick Unlock Displays a feature notification informing the user about quick unlock. Upon clicking the notification the user is taken to the settings page. The UI strings used in the notification are still TODO and not final. BUG=626079 COMPONENT=Chrome OS, Quick Unlock, Preferences, Strings ========== to ========== Implements the feature notification for Quick Unlock Displays a feature notification informing the user about quick unlock. Upon clicking the notification the user is taken to the settings page. The UI strings used in the notification are still TODO and not final. BUG=626079 COMPONENT=Chrome OS, Quick Unlock, Preferences, Strings Committed: https://crrev.com/b2b9f1278e86ff8e05c7d06f8bd315c792da2662 Cr-Commit-Position: refs/heads/master@{#405105} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b2b9f1278e86ff8e05c7d06f8bd315c792da2662 Cr-Commit-Position: refs/heads/master@{#405105} |
