Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
497783002:
Disable lock if no credentials are present (Closed)
|
Created:
6 years, 4 months ago by Mike Lerman Modified:
6 years, 3 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionDisable lock if no credentials are present
BUG=335086
TEST=Ensure new-profile-management and account-consistency are not
enabled. Then sign in a profile. Then enable new-profile-management.
The "lock" button of the avatar menu should be present but disabled.
Committed: https://crrev.com/866872d0b9bc0c42005bd937c51b93e7e14e1faa
Cr-Commit-Position: refs/heads/master@{#291896}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Better variable name #
Total comments: 33
Patch Set 3 : Address reviewer comments (msw / shess) #Patch Set 4 : Return [false] if profile_index = ::npos #
Messages
Total messages: 19 (0 generated)
Hi Roger and Hui, Can I get your reviews on this before I send this out to cocoa and views owners? Thanks! Mike
lgtm
lgtm https://codereview.chromium.org/497783002/diff/20001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.h (right): https://codereview.chromium.org/497783002/diff/20001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.h:36: bool LocalAuthCredentialsExist(size_t info_index); Please provide comment to describe arg.
Hi Mike and Scott, Could I get reviews on cocoa and views implementation of the ProfileChooser-[bubble]? Thanks!
lgtm for cocoa with minor nits. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:456: SignedInProfileLockDisabled) { Alignment. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:485: EXPECT_FALSE([lockButton isEnabled]); Suggest an explicit test that lockButton is not nil. The target and action tests implicitly test that, but it is a precondition for this last line to be valid. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:489: SignedInProfileLockEnabled) { Alignment https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:518: EXPECT_TRUE([lockButton isEnabled]); This case is failsafe against nil, but it wouldn't hurt to explicitly test non-nil here, too.
https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:217: if (info_index == std::string::npos) { Should this logic be in the |profile_info_index| function? Ditto for ValidateLocalAuthCredential and SetLocalAuthCredentials. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:222: nit: remove blank line. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1701: bool enableLock = chrome::LocalAuthCredentialsExist(browser_->profile()); nit: move down to if statement below (and inline). https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:456: SignedInProfileLockDisabled) { On 2014/08/22 20:51:27, Scott Hess wrote: > Alignment. nit: this fits on the line above. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:462: cache->SetLocalAuthCredentialsOfProfileAtIndex(0, ""); nit: use std::string() instead of empty constant. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:472: // TODO(noms): Enforcing 5U fails on the waterfall debug bots, but it's not nit q: Is there a bug filed for this? It seems quite odd, and with these new cases, it might be easier to narrow down the culprit. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:480: // There should be an lock button. nit: "a lock" https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:513: // There should be an lock button. ditto nit: "a lock" https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1274: bool enable_lock = chrome::LocalAuthCredentialsExist(browser_->profile()); ditto nit: move down to if statement below (and inline).
Thank you Mike and thank you Scott! https://codereview.chromium.org/497783002/diff/20001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.h (right): https://codereview.chromium.org/497783002/diff/20001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.h:36: bool LocalAuthCredentialsExist(size_t info_index); On 2014/08/22 19:33:45, Roger Tawa wrote: > Please provide comment to describe arg. Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:217: if (info_index == std::string::npos) { On 2014/08/22 21:46:48, msw wrote: > Should this logic be in the |profile_info_index| function? > Ditto for ValidateLocalAuthCredential and SetLocalAuthCredentials. I turned the NOTREACHED into a DCHECK and put it on top of each of the other functions. I also refactored the other common code from the methods that take a Profile* into an anonymous helper called GetProfileInfoIndexOfProfile. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:222: On 2014/08/22 21:46:48, msw wrote: > nit: remove blank line. Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1701: bool enableLock = chrome::LocalAuthCredentialsExist(browser_->profile()); On 2014/08/22 21:46:48, msw wrote: > nit: move down to if statement below (and inline). Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:456: SignedInProfileLockDisabled) { On 2014/08/22 20:51:27, Scott Hess wrote: > Alignment. Done and done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:462: cache->SetLocalAuthCredentialsOfProfileAtIndex(0, ""); On 2014/08/22 21:46:49, msw wrote: > nit: use std::string() instead of empty constant. Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:472: // TODO(noms): Enforcing 5U fails on the waterfall debug bots, but it's not On 2014/08/22 21:46:48, msw wrote: > nit q: Is there a bug filed for this? It seems quite odd, and with these new > cases, it might be easier to narrow down the culprit. Hey, chatted with noms. She didn't file a bug for this, partially because the issue only appears on Waterfall bots (can't be reproduced anywhere else) and partially because the issue will go away once the NewAvatarMenu is fully rolled out. Let me know if you want me to open a bug, but the likely resolution will be to wait until NewAvatarMenu is fully rolled out. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:480: // There should be an lock button. On 2014/08/22 21:46:49, msw wrote: > nit: "a lock" Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:485: EXPECT_FALSE([lockButton isEnabled]); On 2014/08/22 20:51:27, Scott Hess wrote: > Suggest an explicit test that lockButton is not nil. The target and action > tests implicitly test that, but it is a precondition for this last line to be > valid. Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:489: SignedInProfileLockEnabled) { On 2014/08/22 20:51:27, Scott Hess wrote: > Alignment Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:513: // There should be an lock button. On 2014/08/22 21:46:49, msw wrote: > ditto nit: "a lock" Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:518: EXPECT_TRUE([lockButton isEnabled]); On 2014/08/22 20:51:27, Scott Hess wrote: > This case is failsafe against nil, but it wouldn't hurt to explicitly test > non-nil here, too. Done. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1274: bool enable_lock = chrome::LocalAuthCredentialsExist(browser_->profile()); On 2014/08/22 21:46:49, msw wrote: > ditto nit: move down to if statement below (and inline). Done, though... I've always liked this form of essentially documenting why I'm doing this check by assigning it to a local variable. This documents that I want to enable the lock only if the Auth Credentials exist, and subsequently state if I don't want to enable lock, then disable it. The compiler will likely make an optimization anyways that will lead to a result similar to your suggestion. What's the reason for removing the variable and inlining the condition?
https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:217: if (info_index == std::string::npos) { On 2014/08/25 14:44:30, Mike Lerman wrote: > On 2014/08/22 21:46:48, msw wrote: > > Should this logic be in the |profile_info_index| function? > > Ditto for ValidateLocalAuthCredential and SetLocalAuthCredentials. > > I turned the NOTREACHED into a DCHECK and put it on top of each of the other > functions. > > I also refactored the other common code from the methods that take a Profile* > into an anonymous helper called GetProfileInfoIndexOfProfile. I appreciate the cleanup, but you've removed the "fail safely" mechanisms. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:472: // TODO(noms): Enforcing 5U fails on the waterfall debug bots, but it's not On 2014/08/25 14:44:31, Mike Lerman wrote: > On 2014/08/22 21:46:48, msw wrote: > > nit q: Is there a bug filed for this? It seems quite odd, and with these new > > cases, it might be easier to narrow down the culprit. > > Hey, chatted with noms. She didn't file a bug for this, partially because the > issue only appears on Waterfall bots (can't be reproduced anywhere else) and > partially because the issue will go away once the NewAvatarMenu is fully rolled > out. Let me know if you want me to open a bug, but the likely resolution will be > to wait until NewAvatarMenu is fully rolled out. Filing a bug would help track that information and the cleanup work here. https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1274: bool enable_lock = chrome::LocalAuthCredentialsExist(browser_->profile()); On 2014/08/25 14:44:31, Mike Lerman wrote: > On 2014/08/22 21:46:49, msw wrote: > > ditto nit: move down to if statement below (and inline). > > Done, though... I've always liked this form of essentially documenting why I'm > doing this check by assigning it to a local variable. This documents that I want > to enable the lock only if the Auth Credentials exist, and subsequently state if > I don't want to enable lock, then disable it. The compiler will likely make an > optimization anyways that will lead to a result similar to your suggestion. > > What's the reason for removing the variable and inlining the condition? If you only use something in an if statement, there's no inherent reason to define a local variable for that value. Sometimes named local variables can impart additional information to the reader, and that's great. In this case, |enable_lock| just gates setting STATE_DISABLED on |lock_button_|, so I don't think it adds any information as-is.
https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:217: if (info_index == std::string::npos) { On 2014/08/25 17:39:54, msw wrote: > On 2014/08/25 14:44:30, Mike Lerman wrote: > > On 2014/08/22 21:46:48, msw wrote: > > > Should this logic be in the |profile_info_index| function? > > > Ditto for ValidateLocalAuthCredential and SetLocalAuthCredentials. > > > > I turned the NOTREACHED into a DCHECK and put it on top of each of the other > > functions. > > > > I also refactored the other common code from the methods that take a Profile* > > into an anonymous helper called GetProfileInfoIndexOfProfile. > > I appreciate the cleanup, but you've removed the "fail safely" mechanisms. Did I? The DCHECK(profile) is now in the common method, and info_index == std::string::npos will be caught by DCHECK_NE(info_index, ::npos) in each of the three |profile_info_index| methods. Which fail safety is gone? https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm:472: // TODO(noms): Enforcing 5U fails on the waterfall debug bots, but it's not On 2014/08/25 17:39:55, msw wrote: > On 2014/08/25 14:44:31, Mike Lerman wrote: > > On 2014/08/22 21:46:48, msw wrote: > > > nit q: Is there a bug filed for this? It seems quite odd, and with these new > > > cases, it might be easier to narrow down the culprit. > > > > Hey, chatted with noms. She didn't file a bug for this, partially because the > > issue only appears on Waterfall bots (can't be reproduced anywhere else) and > > partially because the issue will go away once the NewAvatarMenu is fully > rolled > > out. Let me know if you want me to open a bug, but the likely resolution will > be > > to wait until NewAvatarMenu is fully rolled out. > > Filing a bug would help track that information and the cleanup work here. crbug.com/407227 https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1274: bool enable_lock = chrome::LocalAuthCredentialsExist(browser_->profile()); On 2014/08/25 17:39:55, msw wrote: > On 2014/08/25 14:44:31, Mike Lerman wrote: > > On 2014/08/22 21:46:49, msw wrote: > > > ditto nit: move down to if statement below (and inline). > > > > Done, though... I've always liked this form of essentially documenting why I'm > > doing this check by assigning it to a local variable. This documents that I > want > > to enable the lock only if the Auth Credentials exist, and subsequently state > if > > I don't want to enable lock, then disable it. The compiler will likely make an > > optimization anyways that will lead to a result similar to your suggestion. > > > > What's the reason for removing the variable and inlining the condition? > > If you only use something in an if statement, there's no inherent reason to > define a local variable for that value. Sometimes named local variables can > impart additional information to the reader, and that's great. In this case, > |enable_lock| just gates setting STATE_DISABLED on |lock_button_|, so I don't > think it adds any information as-is. Okie. Already done.
https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:217: if (info_index == std::string::npos) { On 2014/08/25 18:11:37, Mike Lerman wrote: > On 2014/08/25 17:39:54, msw wrote: > > On 2014/08/25 14:44:30, Mike Lerman wrote: > > > On 2014/08/22 21:46:48, msw wrote: > > > > Should this logic be in the |profile_info_index| function? > > > > Ditto for ValidateLocalAuthCredential and SetLocalAuthCredentials. > > > > > > I turned the NOTREACHED into a DCHECK and put it on top of each of the other > > > functions. > > > > > > I also refactored the other common code from the methods that take a > Profile* > > > into an anonymous helper called GetProfileInfoIndexOfProfile. > > > > I appreciate the cleanup, but you've removed the "fail safely" mechanisms. > > Did I? The DCHECK(profile) is now in the common method, and info_index == > std::string::npos will be caught by DCHECK_NE(info_index, ::npos) in each of the > three |profile_info_index| methods. Which fail safety is gone? > The early return [false] statements. The old code would NOTREACHED then early return [false]. The new code does as DCHECK and then continues function execution, potentially crashing or employing undefined behavior.
https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... File chrome/browser/signin/local_auth.cc (right): https://codereview.chromium.org/497783002/diff/40001/chrome/browser/signin/lo... chrome/browser/signin/local_auth.cc:217: if (info_index == std::string::npos) { On 2014/08/25 18:54:03, msw wrote: > On 2014/08/25 18:11:37, Mike Lerman wrote: > > On 2014/08/25 17:39:54, msw wrote: > > > On 2014/08/25 14:44:30, Mike Lerman wrote: > > > > On 2014/08/22 21:46:48, msw wrote: > > > > > Should this logic be in the |profile_info_index| function? > > > > > Ditto for ValidateLocalAuthCredential and SetLocalAuthCredentials. > > > > > > > > I turned the NOTREACHED into a DCHECK and put it on top of each of the > other > > > > functions. > > > > > > > > I also refactored the other common code from the methods that take a > > Profile* > > > > into an anonymous helper called GetProfileInfoIndexOfProfile. > > > > > > I appreciate the cleanup, but you've removed the "fail safely" mechanisms. > > > > Did I? The DCHECK(profile) is now in the common method, and info_index == > > std::string::npos will be caught by DCHECK_NE(info_index, ::npos) in each of > the > > three |profile_info_index| methods. Which fail safety is gone? > > > > The early return [false] statements. The old code would NOTREACHED then early > return [false]. The new code does as DCHECK and then continues function > execution, potentially crashing or employing undefined behavior. Gotcha, done.
lgtm
Thanks, Mike!
The CQ bit was checked by mlerman@chromium.org
The CQ bit was unchecked by mlerman@chromium.org
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/497783002/80001
Message was sent while issue was closed.
Committed patchset #4 (80001) as ecd6aec58070af310d7c06acb3a4202d560d78ce
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/866872d0b9bc0c42005bd937c51b93e7e14e1faa Cr-Commit-Position: refs/heads/master@{#291896} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
