Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208583006:
UMA for pin unlock success/failure. (Closed)
|
Created:
4 years, 4 months ago by sammiequon Modified:
4 years, 4 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUMA for pin unlock success/failure.
BUG=621548
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/233d75b83ccd14e44d51cc1a314337467622a67d
Cr-Commit-Position: refs/heads/master@{#413600}
Patch Set 1 #
Total comments: 21
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 12
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 10
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 10
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 1
Patch Set 6 : Rebased. #Patch Set 7 : Nit. #
Total comments: 2
Patch Set 8 : Nit. #Patch Set 9 : Fixed failing test. #
Total comments: 2
Patch Set 10 : Fixed patch set 9 errors. #Messages
Total messages: 40 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== UMA for pin unlock success/failure. BUG=621548 ========== to ========== UMA for pin unlock success/failure. BUG=621548 ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
jdufault@ - Please take a look.
https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:192: if (error.IsPinAttempt()) { This is a lot of plumbing that I think we can do without. There are four things this logs: 1. PIN success 2. PIN failure 3. Password success 4. Password failure PIN success and failure can both be done inside of Authenticate[1], since the PIN check is synchronous. Password success can be done inside of OnPasswordAuthSuccess[2], which does not get called if PIN was used to unlock the device. OnAuthFailure already has a metric [3]. 1: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... 2: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... 3: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { use enum class https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { Just UnlockSuccess https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:55: AUTH_SUCCESS_TYPE_NUM AUTH_SUCCESS_COUNT or AUTH_SUCCESS_MAX_VALUE https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:58: enum UnlockFailureType { enum class https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:58: enum UnlockFailureType { Just UnlockFailure https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:61: AUTH_FAILURE_TYPE_NUM AUTH_FAILURE_COUNT or AUTH_FAILURE_MAX_VALUE https://codereview.chromium.org/2208583006/diff/20001/chromeos/login/auth/aut... File chromeos/login/auth/auth_status_consumer.h (right): https://codereview.chromium.org/2208583006/diff/20001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:43: : is_pin_attempt_(false), Initialize in declaration (line 107) instead of in the ctor for constant values. https://codereview.chromium.org/2208583006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2208583006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:93875: + <int value="0" label="Password"/> Update these labels
https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:192: if (error.IsPinAttempt()) { On 2016/08/04 19:42:04, jdufault wrote: > This is a lot of plumbing that I think we can do without. > > There are four things this logs: > 1. PIN success > 2. PIN failure > 3. Password success > 4. Password failure > > PIN success and failure can both be done inside of Authenticate[1], since the > PIN check is synchronous. Password success can be done inside of > OnPasswordAuthSuccess[2], which does not get called if PIN was used to unlock > the device. OnAuthFailure already has a metric [3]. > > 1: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... > 2: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... > 3: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/lock/scree... As per our offline chat there may be some issues. OnAuthFailure is reached by both PIN failure and pwd failure and has no way of differentiating. https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { On 2016/08/04 19:42:04, jdufault wrote: > Just UnlockSuccess Done. https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { On 2016/08/04 19:42:04, jdufault wrote: > use enum class Im not sure if UMA_HISTOGRAM_ENUM is compatible with enum class. At least the handful of histograms i looked at all use enum. https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:55: AUTH_SUCCESS_TYPE_NUM On 2016/08/04 19:42:04, jdufault wrote: > AUTH_SUCCESS_COUNT or AUTH_SUCCESS_MAX_VALUE Done. https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:58: enum UnlockFailureType { On 2016/08/04 19:42:04, jdufault wrote: > Just UnlockFailure Done. https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:61: AUTH_FAILURE_TYPE_NUM On 2016/08/04 19:42:04, jdufault wrote: > AUTH_FAILURE_COUNT or AUTH_FAILURE_MAX_VALUE Done. https://codereview.chromium.org/2208583006/diff/20001/chromeos/login/auth/aut... File chromeos/login/auth/auth_status_consumer.h (right): https://codereview.chromium.org/2208583006/diff/20001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:43: : is_pin_attempt_(false), On 2016/08/04 19:42:04, jdufault wrote: > Initialize in declaration (line 107) instead of in the ctor for constant values. Done. https://codereview.chromium.org/2208583006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2208583006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:93875: + <int value="0" label="Password"/> On 2016/08/04 19:42:04, jdufault wrote: > Update these labels Done.
https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { On 2016/08/05 01:46:00, sammiequon wrote: > On 2016/08/04 19:42:04, jdufault wrote: > > use enum class > > Im not sure if UMA_HISTOGRAM_ENUM is compatible with enum class. At least the > handful of histograms i looked at all use enum. Why not? Does something like this work? enum class Foo : int { /* ... */ } https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:228: int{UnlockSuccess::AUTH_SUCCESS_PIN}, Do you need these casts? You don't have them above. https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:316: if (base::StringToInt(pin, &dummy_value)) { Also check if user_context.IsUsingPin() is set to true before attempting PIN. https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1069: UserContext user_context(account_id); I don't think we need to pass authenticated_by_pin since IsPinAuthenticationAvailable should always equal it. But maybe pass it and add a DCHECK with chromeos::PinStorage* pin_storage = chromeos::PinStorageFactory::GetForAccountId(user); DCHECK(pin_storage || (authenticated_by_pin == pin_storage->IsPinAuthenticationAvailable())); https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... File chromeos/login/auth/auth_status_consumer.h (right): https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:98: void SetIsPinAttempt(bool tried_pin) { is_pin_attempt_ = tried_pin; } set_used_pin(bool used_pin) { used_pin_ = used_pin; } https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:99: bool IsPinAttempt() const { return is_pin_attempt_; } bool used_pin() const { return used_pin_; } https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:105: bool is_pin_attempt_ = false; Instead of plumbing through AuthStatusConsumer we can just move this variable to ScreenLocker, similar to incorrect_password_count_.
https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { On 2016/08/08 18:54:25, jdufault wrote: > On 2016/08/05 01:46:00, sammiequon wrote: > > On 2016/08/04 19:42:04, jdufault wrote: > > > use enum class > > > > Im not sure if UMA_HISTOGRAM_ENUM is compatible with enum class. At least the > > handful of histograms i looked at all use enum. > > Why not? Does something like this work? > > enum class Foo : int { /* ... */ } That does not work either. It doesn't build with enum class i tried to cast and that stuff. https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:228: int{UnlockSuccess::AUTH_SUCCESS_PIN}, On 2016/08/08 18:54:26, jdufault wrote: > Do you need these casts? You don't have them above. Done. https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:316: if (base::StringToInt(pin, &dummy_value)) { On 2016/08/08 18:54:26, jdufault wrote: > Also check if user_context.IsUsingPin() is set to true before attempting PIN. Done. https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2208583006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1069: UserContext user_context(account_id); On 2016/08/08 18:54:26, jdufault wrote: > I don't think we need to pass authenticated_by_pin since > IsPinAuthenticationAvailable should always equal it. But maybe pass it and add a > DCHECK with > > chromeos::PinStorage* pin_storage = > chromeos::PinStorageFactory::GetForAccountId(user); > DCHECK(pin_storage || > (authenticated_by_pin == > pin_storage->IsPinAuthenticationAvailable())); Done. https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... File chromeos/login/auth/auth_status_consumer.h (right): https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:98: void SetIsPinAttempt(bool tried_pin) { is_pin_attempt_ = tried_pin; } On 2016/08/08 18:54:26, jdufault wrote: > set_used_pin(bool used_pin) { used_pin_ = used_pin; } Done. https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:99: bool IsPinAttempt() const { return is_pin_attempt_; } On 2016/08/08 18:54:26, jdufault wrote: > bool used_pin() const { return used_pin_; } Done. https://codereview.chromium.org/2208583006/diff/40001/chromeos/login/auth/aut... chromeos/login/auth/auth_status_consumer.h:105: bool is_pin_attempt_ = false; On 2016/08/08 18:54:26, jdufault wrote: > Instead of plumbing through AuthStatusConsumer we can just move this variable to > ScreenLocker, similar to incorrect_password_count_. Done.
I think using the member variable ended up with a much cleaner approach, thanks for switching to it. https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { On 2016/08/09 00:31:25, sammiequon wrote: > On 2016/08/08 18:54:25, jdufault wrote: > > On 2016/08/05 01:46:00, sammiequon wrote: > > > On 2016/08/04 19:42:04, jdufault wrote: > > > > use enum class > > > > > > Im not sure if UMA_HISTOGRAM_ENUM is compatible with enum class. At least > the > > > handful of histograms i looked at all use enum. > > > > Why not? Does something like this work? > > > > enum class Foo : int { /* ... */ } > > That does not work either. It doesn't build with enum class i tried to cast and > that stuff. Ok; do you happen to recall the error message? https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:143: is_pin_attempt_(false), Since you're already modifying this, please move all of these initializations (where possible) to the header. Replace the NULL with nullptr. https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:195: UnlockFailure::AUTH_FAILURE_PIN, Can we replace this with UMA(..., is_pin_attempt_ ? UnlockFailure::AUTH_FAILURE_PIN : UnlockFailure::AUTH_FAILURE_PASSWORD, ...) https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:230: UnlockSuccess::AUTH_SUCCESS_COUNT); See above UMA comment https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1070: DCHECK(pin_storage || Thinking about it more, pin_storage should always be valid. Let's split this up into two DCHECKs. DCHECK(pin_storage); DCHECK(authenticated_...); Is this the formatting given by git cl format? https://codereview.chromium.org/2208583006/diff/60001/chromeos/login/auth/ext... File chromeos/login/auth/extended_authenticator_impl.cc (right): https://codereview.chromium.org/2208583006/diff/60001/chromeos/login/auth/ext... chromeos/login/auth/extended_authenticator_impl.cc:332: if (return_code != cryptohome::MOUNT_ERROR_NONE) { ?
https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccessType { On 2016/08/12 21:57:33, jdufault wrote: > On 2016/08/09 00:31:25, sammiequon wrote: > > On 2016/08/08 18:54:25, jdufault wrote: > > > On 2016/08/05 01:46:00, sammiequon wrote: > > > > On 2016/08/04 19:42:04, jdufault wrote: > > > > > use enum class > > > > > > > > Im not sure if UMA_HISTOGRAM_ENUM is compatible with enum class. At least > > the > > > > handful of histograms i looked at all use enum. > > > > > > Why not? Does something like this work? > > > > > > enum class Foo : int { /* ... */ } > > > > That does not work either. It doesn't build with enum class i tried to cast > and > > that stuff. > > Ok; do you happen to recall the error message? Invalid operands to binary expression ('chromeos::ScreenLocker::UnlockSuccess' and 'int') and cannot initialize a parameter of type 'Sample' (aka 'int') with an rvalue of type 'chromeos::ScreenLocker::UnlockSuccess' https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:143: is_pin_attempt_(false), On 2016/08/12 21:57:33, jdufault wrote: > Since you're already modifying this, please move all of these initializations > (where possible) to the header. Replace the NULL with nullptr. Done. https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:195: UnlockFailure::AUTH_FAILURE_PIN, On 2016/08/12 21:57:33, jdufault wrote: > Can we replace this with > > UMA(..., is_pin_attempt_ ? UnlockFailure::AUTH_FAILURE_PIN : > UnlockFailure::AUTH_FAILURE_PASSWORD, ...) Done. https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:230: UnlockSuccess::AUTH_SUCCESS_COUNT); On 2016/08/12 21:57:33, jdufault wrote: > See above UMA comment Done. https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2208583006/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1070: DCHECK(pin_storage || On 2016/08/12 21:57:33, jdufault wrote: > Thinking about it more, pin_storage should always be valid. Let's split this up > into two DCHECKs. > > DCHECK(pin_storage); > DCHECK(authenticated_...); > > Is this the formatting given by git cl format? Done. https://codereview.chromium.org/2208583006/diff/60001/chromeos/login/auth/ext... File chromeos/login/auth/extended_authenticator_impl.cc (right): https://codereview.chromium.org/2208583006/diff/60001/chromeos/login/auth/ext... chromeos/login/auth/extended_authenticator_impl.cc:332: if (return_code != cryptohome::MOUNT_ERROR_NONE) { On 2016/08/12 21:57:34, jdufault wrote: > ? Done.
https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:139: start_time_(base::Time::Now()), Move initialization to header https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:140: auth_status_consumer_(nullptr), Move initialization to header https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccess { Is it possible to make these enums private? https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:58: enum UnlockFailure { Do these need to be separate enums? Is it possible to just have enum UnlockType { AUTH_PASSWORD = 0, AUTH_PIN, AUTH_COUNT }; https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1071: DCHECK(authenticated_by_pin == pin_storage->IsPinAuthenticationAvailable()); Will authenticated_by_pin be true if the user types their password using the virtual keyboard when the PIN keyboard is not shown? If not, we should change this to DCHECK(pin_storage->IsPinAuthenticationAvailable() || !authenticated_by_pin) Ie, if pin storage is not available, authenticated by PIN must be false. I'd add a comment as well since that DCHECK is non-trivial to read. Sorry for so many iterations here.
https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.cc (right): https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:139: start_time_(base::Time::Now()), On 2016/08/16 17:55:02, jdufault wrote: > Move initialization to header Done. https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.cc:140: auth_status_consumer_(nullptr), On 2016/08/16 17:55:02, jdufault wrote: > Move initialization to header Done. https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:52: enum UnlockSuccess { On 2016/08/16 17:55:02, jdufault wrote: > Is it possible to make these enums private? Done. https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/lock/screen_locker.h:58: enum UnlockFailure { On 2016/08/16 17:55:02, jdufault wrote: > Do these need to be separate enums? Is it possible to just have > > enum UnlockType { > AUTH_PASSWORD = 0, > AUTH_PIN, > AUTH_COUNT > }; Done. https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/2208583006/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1071: DCHECK(authenticated_by_pin == pin_storage->IsPinAuthenticationAvailable()); On 2016/08/16 17:55:02, jdufault wrote: > Will authenticated_by_pin be true if the user types their password using the > virtual keyboard when the PIN keyboard is not shown? > > If not, we should change this to > > DCHECK(pin_storage->IsPinAuthenticationAvailable() || !authenticated_by_pin) > > Ie, if pin storage is not available, authenticated by PIN must be false. I'd add > a comment as well since that DCHECK is non-trivial to read. > > Sorry for so many iterations here. Done. Authenticated by pin will be true if the pin-keyboard was visible.
lgtm https://codereview.chromium.org/2208583006/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/lock/screen_locker.h (right): https://codereview.chromium.org/2208583006/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/lock/screen_locker.h:134: enum UnlockType { AUTH_PASSWORD = 0, AUTH_PIN, AUTH_COUNT }; There's a semi-standard comment block that is put on enums used in UMA warning readers not to modify order/etc. Can you add it here as well?
sammiequon@chromium.org changed reviewers: + alemate@chromium.org, isherman@chromium.org
On 2016/08/18 17:48:50, jdufault wrote: > lgtm > > https://codereview.chromium.org/2208583006/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/lock/screen_locker.h (right): > > https://codereview.chromium.org/2208583006/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/login/lock/screen_locker.h:134: enum UnlockType { > AUTH_PASSWORD = 0, AUTH_PIN, AUTH_COUNT }; > There's a semi-standard comment block that is put on enums used in UMA warning > readers not to modify order/etc. Can you add it here as well? alemate@ - Please take a look. Thanks! isherman@ - Please take a look. histograms.xml. Thanks!
histograms lgtm % a nit: https://codereview.chromium.org/2208583006/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2208583006/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95425: +</enum> nit: Any reason not to combine these two enums into a single one?
lgtm with other reviewers nits.
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, alemate@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2208583006/#ps180001 (title: "Nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2208583006/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2208583006/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95425: +</enum> On 2016/08/18 21:42:20, Ilya Sherman wrote: > nit: Any reason not to combine these two enums into a single one? Done.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Description was changed from ========== UMA for pin unlock success/failure. BUG=621548 ========== to ========== UMA for pin unlock success/failure. BUG=621548 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/08/19 17:17:59, commit-bot: I haz the power wrote: > 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_...) alemate@ - I changed some files to deal with the failing test. Please take a look. Thanks!
https://codereview.chromium.org/2208583006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): https://codereview.chromium.org/2208583006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:118: bool using_pin) { If this takes a param it should pass it using the string. I'd eliminate the function param all-together though.
https://codereview.chromium.org/2208583006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): https://codereview.chromium.org/2208583006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/login/signin/device_id_browsertest.cc:118: bool using_pin) { On 2016/08/19 22:03:42, jdufault wrote: > If this takes a param it should pass it using the string. I'd eliminate the > function param all-together though. Done.
The CQ bit was checked by sammiequon@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...
On 2016/08/22 17:15:32, sammiequon wrote: > https://codereview.chromium.org/2208583006/diff/200001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/signin/device_id_browsertest.cc (right): > > https://codereview.chromium.org/2208583006/diff/200001/chrome/browser/chromeo... > chrome/browser/chromeos/login/signin/device_id_browsertest.cc:118: bool > using_pin) { > On 2016/08/19 22:03:42, jdufault wrote: > > If this takes a param it should pass it using the string. I'd eliminate the > > function param all-together though. > > Done. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2208583006/#ps220001 (title: "Fixed patch set 9 errors.")
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 #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== UMA for pin unlock success/failure. BUG=621548 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== UMA for pin unlock success/failure. BUG=621548 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/233d75b83ccd14e44d51cc1a314337467622a67d Cr-Commit-Position: refs/heads/master@{#413600} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/233d75b83ccd14e44d51cc1a314337467622a67d Cr-Commit-Position: refs/heads/master@{#413600} |
