Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1304573004:
Added UMA statistics for change passwords in the Password Manager (Closed)
|
Created:
5 years, 4 months ago by dvadym Modified:
5 years, 4 months ago CC:
chromium-reviews, tfarina, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, asvitkine+watch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded UMA statistics for change passwords in the Password Manager
Details what statistics is sent and when:
There are 4 interesting cases related to Update password or processing of change password forms:
1.Change password form submission, no accounts stored in the Password Manager, the save bubble is shown.
2.Change password form submission, 1 account stored in the Password Manager, the update bubble without combobox is shown
3.Change password form submission, 1 account stored in the Password Manager, the update bubble with combobox for choosing correct credentials is shown
4.Sign-in form submission, and there is stored credential with submitted username but with password different to stored one. This case is called PASSWORD_OVERRIDEN.
In each of these cases we want to know user behavior, namely the user clicked Yes, Nope or No interaction happened. That's why we have 12 cases to report.
Since in case 1 Save bubble is shown we need to check this case in save button code. Conditions when to report difference cases the following:
1.Save bubble is shown && IsPossibleChangePasswordFormWithoutUsername() == true
2.Update bubble is shown && password_overridden_ == false && !ShouldShowMultipleAccountUpdateUI()
3.Update bubble is shown && password_overridden_ == false && ShouldShowMultipleAccountUpdateUI()
4.Update bubble is shown && password_overridden_ == true
BUG=359315
Committed: https://crrev.com/a84862ae4b048158be5c5155aa257b1a065e2c65
Cr-Commit-Position: refs/heads/master@{#344890}
Patch Set 1 #
Total comments: 34
Patch Set 2 : Addressed reviewers comments #
Total comments: 7
Patch Set 3 : Comments addressed #Patch Set 4 : Rebase #
Total comments: 18
Patch Set 5 : Comments addressed #
Total comments: 10
Patch Set 6 : Comments addressed #Patch Set 7 : Fix unittests #
Total comments: 1
Patch Set 8 : Added DCHECK #Patch Set 9 : Fix interactive tests #Patch Set 10 : Mac fixed #Patch Set 11 : fix #Messages
Total messages: 38 (13 generated)
https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:165: case password_manager::ui::PENDING_PASSWORD_STATE: It's added display disposition calculation for update bubble, that was missed before and made refactoring with replacing if to switch https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1030: DCHECK_EQ(1u, parent->model()->local_credentials().size()); Since https://codereview.chromium.org/1297963002/ it's legitimate case when he have more than 1 local_credentials and 1 account bubble because in case when we show overridden bubble it's always 1 account bubble. So these checks are not true anymore
dvadym@chromium.org changed reviewers: + vasilii@chromium.org
Hi Vasilii, Could you please review this CL? Best regards, Vadym
Overall concern: you introduce a new histogram but at the same time the update bubble reports the dismissal reason. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:166: display_disposition_ = metrics_util::MANUAL_WITH_PASSWORD_PENDING; indent 2 spaces https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:240: } else if (state_ == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE || It's the same condition as above. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:254: if (pending_password_.IsPossibleChangePasswordFormWithoutUsername()) How is it possible to get into that function from the Update bubble? https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:261: update_password_submission_event_ = optional: "if () else if () else" seems more readable to me. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:298: if (pending_password_.IsPossibleChangePasswordFormWithoutUsername()) { How is it possible to get into that function from the Update bubble? https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1032: parent->model()->pending_password().username_value); If this is untrue then there is a bug in the line # 1091. Ideally you should summarize everything in a comment here. https://codereview.chromium.org/1304573004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager_metrics_util.h (right): https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:105: UPDATE_PASSWORD_EVENT_COUNT, It should be the last one. https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:106: NO_UPDATE_SUBMISSION What is this case?
https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:166: display_disposition_ = metrics_util::MANUAL_WITH_PASSWORD_PENDING; On 2015/08/19 13:01:18, vasilii wrote: > indent 2 spaces Done. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:240: } else if (state_ == password_manager::ui::PENDING_PASSWORD_UPDATE_STATE || On 2015/08/19 13:01:17, vasilii wrote: > It's the same condition as above. Thanks, PENDING_PASSWORD_UPDATE_STATE ->PENDING_PASSWORD_STATE https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:254: if (pending_password_.IsPossibleChangePasswordFormWithoutUsername()) On 2015/08/19 13:01:18, vasilii wrote: > How is it possible to get into that function from the Update bubble? We want to log not only UpdateBubble, but any event related to change password. When we have no account we show Save Bubble (for saving with empty password). https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:261: update_password_submission_event_ = At first I tried with If else, but it was awful, with ?: less symbols not related to conditions and results https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/passwords... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:298: if (pending_password_.IsPossibleChangePasswordFormWithoutUsername()) { On 2015/08/19 13:01:17, vasilii wrote: > How is it possible to get into that function from the Update bubble? We want to log not only UpdateBubble, but any event related to change password. When we have no account we show Save Bubble (for saving with empty password).
dvadym@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, Could you please review changes in histograms.xml? Best regards, Vadym
https://codereview.chromium.org/1304573004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager_metrics_util.h (right): https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:35: AUTOMATIC_WITH_PASSWORD_PENDING_UPDATE, Should histograms.xml be updated correspondingly? https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:104: PASSWORD_OVERRIDEN_NO_INTERACTION, nit: "riden" -> "ridden" https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:29364: + Which password update event was happened and what did the user do with the nit: "was happened" -> "happened" https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:71966: + <int value="0" label="NO_ACCOUNTS_CLICKED_UPDATE"/> What does this case mean? What accounts are in scope for this measurement, and how does one update data if there aren't any accounts? https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:71977: + <int value="11" label="PASSWORD_OVERRIDEN_NO_INTERACTION"/> nit: "riden" -> "ridden"
Thanks Ilya and Vasilii! I've addressed your comments in PatchSet 2. PTAL. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1032: parent->model()->pending_password().username_value); On 2015/08/19 13:01:18, vasilii wrote: > If this is untrue then there is a bug in the line # 1091. > > Ideally you should summarize everything in a comment here. Thanks, there was a bug in the line 1091. Fixed. I've added comment // |pending_password()| is a form that should be updated, so it should be used when the user clicks "Update". I think that the main point which can cause errors here, it's that pending_password() should be used for Updating (i.e.error in the line #1091), so I've added this to comment. https://codereview.chromium.org/1304573004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager_metrics_util.h (right): https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:35: AUTOMATIC_WITH_PASSWORD_PENDING_UPDATE, On 2015/08/20 00:30:51, Ilya Sherman wrote: > Should histograms.xml be updated correspondingly? Thanks, Done https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:104: PASSWORD_OVERRIDEN_NO_INTERACTION, On 2015/08/20 00:30:51, Ilya Sherman wrote: > nit: "riden" -> "ridden" Done. https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:106: NO_UPDATE_SUBMISSION On 2015/08/19 13:01:18, vasilii wrote: > What is this case? I've introduce it in order to have default value for convenience of checking that nothing happened. In UIDismissalReason the same pattern is used. Probably it's worth to make comment. What do you think? https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:29364: + Which password update event was happened and what did the user do with the On 2015/08/20 00:30:51, Ilya Sherman wrote: > nit: "was happened" -> "happened" Done. https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:71966: + <int value="0" label="NO_ACCOUNTS_CLICKED_UPDATE"/> On 2015/08/20 00:30:51, Ilya Sherman wrote: > What does this case mean? What accounts are in scope for this measurement, and > how does one update data if there aren't any accounts? It means that there is no accounts stored in the Password Manager, but the user has an account on some site and changed password for it. In this case currently we are going to propose to save password without username and then save username on the first successful sign-in. https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:71977: + <int value="11" label="PASSWORD_OVERRIDEN_NO_INTERACTION"/> On 2015/08/20 00:30:51, Ilya Sherman wrote: > nit: "riden" -> "ridden" Done.
Before proceeding with manage_passwords_bubble_view.cc review I want you to write down a doc or a paragraph in this CL about which [web form, stored passwords] trigger which bubble. It's terribly complicated now and I'm pretty sure that https://codereview.chromium.org/1297963002/ introduced a bug (see the comments inline). You can devote this CL only to histograms if you prefer. Then you need to make sure that the Save and the Update bubbles live in the different histograms as discussed in the chat. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1030: DCHECK_EQ(1u, parent->model()->local_credentials().size()); On 2015/08/19 11:47:56, dvadym wrote: > Since https://codereview.chromium.org/1297963002/ it's legitimate case when he > have more than 1 local_credentials and 1 account bubble because in case when we > show overridden bubble it's always 1 account bubble. So these checks are not > true anymore You are talking about password_overriden case? It seems like a bad bug that we end up in this bubble then. PasswordManager::OnLoginSuccessful contains a condition that password_overriden means "Update". But the password store can be empty. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1032: parent->model()->pending_password().username_value); On 2015/08/20 11:10:11, dvadym wrote: > On 2015/08/19 13:01:18, vasilii wrote: > > If this is untrue then there is a bug in the line # 1091. > > > > Ideally you should summarize everything in a comment here. > > Thanks, there was a bug in the line 1091. Fixed. > > I've added comment > // |pending_password()| is a form that should be updated, so it should be used > when the user clicks "Update". > I think that the main point which can cause errors here, it's that > pending_password() should be used for Updating (i.e.error in the line #1091), so > I've added this to comment. I'm concerned about this change. It violates the comment to PasswordFormManager::Update. I totally admit that PasswordStore::UpdateLogin may fail silently. https://codereview.chromium.org/1304573004/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager_metrics_util.h (right): https://codereview.chromium.org/1304573004/diff/1/components/password_manager... components/password_manager/core/browser/password_manager_metrics_util.h:106: NO_UPDATE_SUBMISSION On 2015/08/20 11:10:11, dvadym wrote: > On 2015/08/19 13:01:18, vasilii wrote: > > What is this case? > > I've introduce it in order to have default value for convenience of checking > that nothing happened. In UIDismissalReason the same pattern is used. Probably > it's worth to make comment. What do you think? Sure. Note that there is always the dismissal reason. So ManagePasswordsBubbleModel::OnBubbleHidden should always report something.
https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:235: password_overridden_ If you separate this construction into a method then the readability will improve. Here you can call update_password_submission_event_ = GetUpdateDismissalReason(NO_INTERACTION); Below update_password_submission_event_ = GetUpdateDismissalReason(NO_CLICKED); update_password_submission_event_ = GetUpdateDismissalReason(UPDATE_CLICKED); https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:247: if (update_password_submission_event_ != metrics_util::NO_UPDATE_SUBMISSION) I think here there is always a reason why the bubble was closed. And this reason should be reported. https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:299: update_password_submission_event_ = As discussed, the save bubble shouldn't participate in the update bubble stats
Metrics LGTM, with some suggested clarifications: https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:71966: + <int value="0" label="NO_ACCOUNTS_CLICKED_UPDATE"/> On 2015/08/20 11:10:11, dvadym wrote: > On 2015/08/20 00:30:51, Ilya Sherman wrote: > > What does this case mean? What accounts are in scope for this measurement, > and > > how does one update data if there aren't any accounts? > > It means that there is no accounts stored in the Password Manager, but the user > has an account on some site and changed password for it. In this case currently > we are going to propose to save password without username and then save username > on the first successful sign-in. This might be a case where I'd actually recommend using the expanded form for an enum, with descriptions of each enum value, e.g. <int value="0" label="NO_ACCOUNTS_CLICKED_UPDATE">The user has no password saved in Chrome for foo.com, but just changed her password on foo.com, and chose to save the update password in Chrome's password manager.</int> It can be a little shorter (or longer, if you'd prefer), but something like that would really help me -- and probably other viewers -- to understand the semantics of these labels on the dashboard. (Admittedly, these descriptions are only shown on hover, so they're still not the most discoverable. It would be even better to simply have clearer labels... but I think that might be tricky.) You might also just update the histogram description to clarify what the multiplied out event space is. Suggested addition to the histogram description: """The password update bubble is shown when the user changes her password on a website, say foo.com. This histogram measures whether the user has any passwords saved in the Chrome password manager for foo.com, and what action the user performed on the offered bubble. It's recorded when the bubble is closed""" https://codereview.chromium.org/1304573004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1304573004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:174: event, UPDATE_PASSWORD_EVENT_COUNT); If you keep the enum element that's past UPDATE_PASSWORD_EVENT_COUNT, please add a DCHECK to this method to verify that it's never called with that enum value.
Thanks Ilya and Vasilii. I've addressed all your comments in PatchSet 4. Vasilii PTAL https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (left): https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1030: DCHECK_EQ(1u, parent->model()->local_credentials().size()); In case of password_overridden, password store can't be empty, since the user overrode previously stored password. See description of CL for more details. https://codereview.chromium.org/1304573004/diff/1/chrome/browser/ui/views/pas... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:1032: parent->model()->pending_password().username_value); Strictly speaking using |pending_password| for argument of call of PasswordStore doesn't violates UpdateLogin, because |pending_password| is one of |best_matches| in this case (https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...). I agree that this is not obvious. So I'm going to change comment for UpdateLogin to the following: ...|credentials_to_update| should be one of |best_matches_| or |pending_credentials|. I've added comments to PasswordManagerClient::PromptUserToSaveOrUpdatePassword https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1304573004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:71966: + <int value="0" label="NO_ACCOUNTS_CLICKED_UPDATE"/> On 2015/08/21 01:24:23, Ilya Sherman wrote: > On 2015/08/20 11:10:11, dvadym wrote: > > On 2015/08/20 00:30:51, Ilya Sherman wrote: > > > What does this case mean? What accounts are in scope for this measurement, > > and > > > how does one update data if there aren't any accounts? > > > > It means that there is no accounts stored in the Password Manager, but the > user > > has an account on some site and changed password for it. In this case > currently > > we are going to propose to save password without username and then save > username > > on the first successful sign-in. > > This might be a case where I'd actually recommend using the expanded form for an > enum, with descriptions of each enum value, e.g. > > <int value="0" label="NO_ACCOUNTS_CLICKED_UPDATE">The user has no password > saved in Chrome for http://foo.com, but just changed her password on http://foo.com, and chose > to save the update password in Chrome's password manager.</int> > > It can be a little shorter (or longer, if you'd prefer), but something like that > would really help me -- and probably other viewers -- to understand the > semantics of these labels on the dashboard. (Admittedly, these descriptions are > only shown on hover, so they're still not the most discoverable. It would be > even better to simply have clearer labels... but I think that might be tricky.) > > You might also just update the histogram description to clarify what the > multiplied out event space is. Suggested addition to the histogram description: > > """The password update bubble is shown when the user changes her password on a > website, say http://foo.com. This histogram measures whether the user has any > passwords saved in the Chrome password manager for http://foo.com, and what action the > user performed on the offered bubble. It's recorded when the bubble is > closed""" Thanks for such comprehensive answer. I've chosen the second option to write down histogram description in more details: The password submission event happened when the user changes her password on a website, say foo.com or types new password on a sign-in form for saved credential (which might mean that the password was changed in a different browser). The later case called password overriding. This histogram measures whether the user has any passwords saved in the Chrome password manager for foo.com or the password was overriden on sign-in form, and what action the user performed on the offered bubble. It's recorded when the bubble is closed. I hope this explains what this metrics was introduced for. https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:235: password_overridden_ On 2015/08/20 15:23:03, vasilii wrote: > If you separate this construction into a method then the readability will > improve. > Here you can call > update_password_submission_event_ = GetUpdateDismissalReason(NO_INTERACTION); > > Below > update_password_submission_event_ = GetUpdateDismissalReason(NO_CLICKED); > update_password_submission_event_ = GetUpdateDismissalReason(UPDATE_CLICKED); Agree,it make perfect sense. Done https://codereview.chromium.org/1304573004/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:247: if (update_password_submission_event_ != metrics_util::NO_UPDATE_SUBMISSION) On 2015/08/20 15:23:03, vasilii wrote: > I think here there is always a reason why the bubble was closed. And this reason > should be reported. It can be not update bubble, this is simply verification that this is update bubble. https://codereview.chromium.org/1304573004/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1304573004/diff/20001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:174: event, UPDATE_PASSWORD_EVENT_COUNT); On 2015/08/21 01:24:23, Ilya Sherman wrote: > If you keep the enum element that's past UPDATE_PASSWORD_EVENT_COUNT, please add > a DCHECK to this method to verify that it's never called with that enum value. Done.
https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:233: state_ == password_manager::ui::PENDING_PASSWORD_STATE) Make this 'if' the outer one. https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:244: update_password_submission_event_ = GetUpdateDismissalReason(NOPE_CLICKED); This is 100% error. https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:181: bool is_update_bubble_; The comment is totally wrong. I guess you need this variable because |state_| may change. I suggest that you remove this memeber and all assignments to |state_| in methods like OnChooseCredentials(). It's not needed by production code. Some ManagePasswordsBubbleModelTest tests will fail but you can safely remove the checks. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_client.h (right): https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:77: // 1.Change password form was submitted and the user has only one stored "A change password form" here and below. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:81: // credential. Then we shouldn't expact anything from expect https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:83: // know which credentials should be update. updated https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:84: // 3.Sign-in password form was submitted with a password different from A sign-in https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:85: // stored one. In this case form_to_save.password_overridden() == true and the stored one https://codereview.chromium.org/1304573004/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:173: DCHECK(event < UPDATE_PASSWORD_EVENT_COUNT); DCHECK_LT or DCHECK_NE
Thanks Vasilii! PTAL https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:233: state_ == password_manager::ui::PENDING_PASSWORD_STATE) On 2015/08/21 14:59:22, vasilii wrote: > Make this 'if' the outer one. Done. https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:244: update_password_submission_event_ = GetUpdateDismissalReason(NOPE_CLICKED); On 2015/08/21 14:59:22, vasilii wrote: > This is 100% error. Thanks, moved to OnNeverForThisSiteClicked() https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/1304573004/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:181: bool is_update_bubble_; On 2015/08/21 14:59:22, vasilii wrote: > The comment is totally wrong. > I guess you need this variable because |state_| may change. I suggest that you > remove this memeber and all assignments to |state_| in methods like > OnChooseCredentials(). It's not needed by production code. Some > ManagePasswordsBubbleModelTest tests will fail but you can safely remove the > checks. Done. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_client.h (right): https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:77: // 1.Change password form was submitted and the user has only one stored On 2015/08/21 14:59:22, vasilii wrote: > "A change password form" here and below. Done. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:81: // credential. Then we shouldn't expact anything from On 2015/08/21 14:59:22, vasilii wrote: > expect Done. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:83: // know which credentials should be update. On 2015/08/21 14:59:22, vasilii wrote: > updated Done. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:84: // 3.Sign-in password form was submitted with a password different from On 2015/08/21 14:59:22, vasilii wrote: > A sign-in Done. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_client.h:85: // stored one. In this case form_to_save.password_overridden() == true and On 2015/08/21 14:59:22, vasilii wrote: > the stored one Done. https://codereview.chromium.org/1304573004/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1304573004/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:173: DCHECK(event < UPDATE_PASSWORD_EVENT_COUNT); On 2015/08/21 14:59:22, vasilii wrote: > DCHECK_LT or DCHECK_NE Done.
https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:231: GetUpdateDismissalReason(NO_INTERACTION); You fall into this block for normal save bubble. But we don't want to report NO_INTERACTION for those cases. https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:158: enum UserBehaviorOnUpdateBubble { The enum should go before the method. https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:164: GetUpdateDismissalReason(UserBehaviorOnUpdateBubble behavior); Should be a const method. https://codereview.chromium.org/1304573004/diff/80001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1304573004/diff/80001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:173: DCHECK_LE(event, UPDATE_PASSWORD_EVENT_COUNT); It can't be equal to UPDATE_PASSWORD_EVENT_COUNT. https://codereview.chromium.org/1304573004/diff/80001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.h (right): https://codereview.chromium.org/1304573004/diff/80001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.h:106: NO_UPDATE_SUBMISSION A line break would help.
Thanks Vasilii! PTAL https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:231: GetUpdateDismissalReason(NO_INTERACTION); On 2015/08/21 15:58:34, vasilii wrote: > You fall into this block for normal save bubble. But we don't want to report > NO_INTERACTION for those cases. Removed state changes https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/manage_passwords_bubble_model.h (right): https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:158: enum UserBehaviorOnUpdateBubble { On 2015/08/21 15:58:34, vasilii wrote: > The enum should go before the method. Done. https://codereview.chromium.org/1304573004/diff/80001/chrome/browser/ui/passw... chrome/browser/ui/passwords/manage_passwords_bubble_model.h:164: GetUpdateDismissalReason(UserBehaviorOnUpdateBubble behavior); On 2015/08/21 15:58:34, vasilii wrote: > Should be a const method. Done. https://codereview.chromium.org/1304573004/diff/80001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/1304573004/diff/80001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:173: DCHECK_LE(event, UPDATE_PASSWORD_EVENT_COUNT); On 2015/08/21 15:58:34, vasilii wrote: > It can't be equal to UPDATE_PASSWORD_EVENT_COUNT. Done. https://codereview.chromium.org/1304573004/diff/80001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.h (right): https://codereview.chromium.org/1304573004/diff/80001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.h:106: NO_UPDATE_SUBMISSION On 2015/08/21 15:58:34, vasilii wrote: > A line break would help. Done.
lgtm https://codereview.chromium.org/1304573004/diff/120001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right): https://codereview.chromium.org/1304573004/diff/120001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:389: } DCHECK_EQ(UPDATE, state);
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, vasilii@chromium.org Link to the patchset: https://codereview.chromium.org/1304573004/#ps140001 (title: "Added DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304573004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304573004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1304573004/#ps160001 (title: "Fix interactive tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304573004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304573004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1304573004/#ps180001 (title: "Max fixed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304573004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304573004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1304573004/#ps200001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304573004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304573004/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/a84862ae4b048158be5c5155aa257b1a065e2c65 Cr-Commit-Position: refs/heads/master@{#344890} |
