Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2542093002:
[Password Generation] Fixes sending votes about the usage of the password generation popup (Closed)
Description[Password Generation] Fixes sending votes about the usage of the password generation popup
If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response.
This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote.
BUG=552420
Committed: https://crrev.com/7209820b1bdfa1f86eaaa0e14b2c370ebf0064cd
Cr-Commit-Position: refs/heads/master@{#439763}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changes addressed to reviewer comments #
Total comments: 16
Patch Set 3 : Changes addressed to reviewer comments 2 #
Total comments: 6
Patch Set 4 : Changes addressed to reviewer comments 3 #Patch Set 5 : Rebase #Patch Set 6 : Fix for the failed Mac test #
Total comments: 1
Messages
Total messages: 55 (37 generated)
kolos@chromium.org changed reviewers: + dvadym@chromium.org
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by kolos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kolos@chromium.org
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by kolos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kolos@chromium.org
Thanks Maxim for refactoring and fixing issue with sending votes! I've checked logic of votes sending and it looks good. Could you please add some tests for verifying a new behavior? https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:734: is_cpf_form It looks that is_cpf_form == password_type == autofill::NEW_PASSWORD || password_type == autofill::PROBABLY_NEW_PASSWORD || password_type == autofill::NOT_NEW_PASSWORD Then 1. So we might skip passing |is_cpf_form| to this function. 2. has_autofill_vote = password_type != UNKNOWN Is this true? https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:743: if (!has_autofill_vote and !has_password_generation_vote) and -> && https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:752: : pending_credentials_.form_data); Could you please add a comment why |form_data| is chosen in such way
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #1 (id:60001) has been deleted
https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:734: is_cpf_form On 2016/12/02 15:10:27, dvadym wrote: > It looks that is_cpf_form == password_type == autofill::NEW_PASSWORD || > password_type == autofill::PROBABLY_NEW_PASSWORD || > password_type == autofill::NOT_NEW_PASSWORD > > Then > 1. So we might skip passing |is_cpf_form| to this function. > 2. has_autofill_vote = password_type != UNKNOWN > > Is this true? Good point. Have removed the parameter |is_cpf_form|, have changed checking of the password type. https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:743: if (!has_autofill_vote and !has_password_generation_vote) On 2016/12/02 15:10:27, dvadym wrote: > and -> && Done. https://codereview.chromium.org/2542093002/diff/100001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:752: : pending_credentials_.form_data); On 2016/12/02 15:10:27, dvadym wrote: > Could you please add a comment why |form_data| is chosen in such way Done.
Patchset #3 (id:200001) has been deleted
Patchset #5 (id:260001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
kolos@chromium.org changed reviewers: + vasilii@chromium.org
vasilii@chromium.org: Please review this CL. Regards, Maxim
Honestly I am not familiar with the votes in password_form_manager.cc. I can only rubber stamp it. https://codereview.chromium.org/2542093002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/2542093002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:388: form_manager->OnNeverClicked(); Why can't PermanentlyBlacklist() call OnNeverClicked() internally? It seems to be the only place where PermanentlyBlacklist is called. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:690: LabelFieldsForChangePasswordForm(password_type, &form_structure, What if I login with a new password. Is it also considered "change password form"? https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:728: password_type == autofill::NOT_NEW_PASSWORD); Printing |password_type| with "<< password_type" may help. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:221: // Called when the user clicked "Never" button. ... in the "save password" prompt? https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:366: // Tries to label password fields and upload |form_data|. Returns true on There is no |form_data|. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:369: // not assume that |form| is being uploaded during the same browsing session There is no |form| parameter either. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:372: // doesn't log the same quality UMA metrics. As a newbie I don't understand the comment at all. It's good to know that it's different from AutofillManager::OnFormSubmitted which I also don't know. It would be more helpful to describe what it really does instead of doesn't ;-) https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager_unittest.cc:648: default: I think if you remove "default" then you'll get a compilation error if not all the enum values are processed. If it's true then I'd prefer to debug the compilation error.
Description was changed from ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. BUG=552420 ========== to ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merge two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 ==========
Description was changed from ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merge two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 ========== to ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 ==========
Patchset #3 (id:300001) has been deleted
Thanks Vasilii for review. Vadym will make one more pass on votes specific code. https://codereview.chromium.org/2542093002/diff/280001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/2542093002/diff/280001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:388: form_manager->OnNeverClicked(); On 2016/12/08 16:58:14, vasilii wrote: > Why can't PermanentlyBlacklist() call OnNeverClicked() internally? It seems to > be the only place where PermanentlyBlacklist is called. Done. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:690: LabelFieldsForChangePasswordForm(password_type, &form_structure, On 2016/12/08 16:58:14, vasilii wrote: > What if I login with a new password. Is it also considered "change password > form"? Renamed Label* methods. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.cc:728: password_type == autofill::NOT_NEW_PASSWORD); On 2016/12/08 16:58:14, vasilii wrote: > Printing |password_type| with "<< password_type" may help. Done. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:221: // Called when the user clicked "Never" button. On 2016/12/08 16:58:14, vasilii wrote: > ... in the "save password" prompt? Done. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:366: // Tries to label password fields and upload |form_data|. Returns true on On 2016/12/08 16:58:14, vasilii wrote: > There is no |form_data|. Done. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:369: // not assume that |form| is being uploaded during the same browsing session On 2016/12/08 16:58:14, vasilii wrote: > There is no |form| parameter either. Done. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:372: // doesn't log the same quality UMA metrics. On 2016/12/08 16:58:14, vasilii wrote: > As a newbie I don't understand the comment at all. It's good to know that it's > different from AutofillManager::OnFormSubmitted which I also don't know. It > would be more helpful to describe what it really does instead of doesn't ;-) This is a part of very old comment to UploadPasswordForm. But actually, nobody is interested in the difference between UploadPasswordVote and OnFormSubmitted, I believe. So, I deleted that. Now the description of the method is brief and describe only high-level purpose of the method. https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2542093002/diff/280001/components/password_ma... components/password_manager/core/browser/password_form_manager_unittest.cc:648: default: On 2016/12/08 16:58:14, vasilii wrote: > I think if you remove "default" then you'll get a compilation error if not all > the enum values are processed. If it's true then I'd prefer to debug the > compilation error. Good point. Fixed.
Only nits left. https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:368: // TODO(crbug.com/) TODO what? https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:429: const autofill::ServerFieldType& password_type, It's better to pass the enum by value. Same below. https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... components/password_manager/core/browser/password_form_manager_unittest.cc:648: NOTREACHED(); remove it.
https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:368: // TODO(crbug.com/) On 2016/12/12 18:57:14, vasilii wrote: > TODO what? ooops. Removed that. https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... components/password_manager/core/browser/password_form_manager.h:429: const autofill::ServerFieldType& password_type, On 2016/12/12 18:57:14, vasilii wrote: > It's better to pass the enum by value. Same below. Done. https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... File components/password_manager/core/browser/password_form_manager_unittest.cc (right): https://codereview.chromium.org/2542093002/diff/320001/components/password_ma... components/password_manager/core/browser/password_form_manager_unittest.cc:648: NOTREACHED(); On 2016/12/12 18:57:14, vasilii wrote: > remove it. Done.
LGTM
lgtm
The CQ bit was checked by dvadym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
You forgot to update ManagePasswordsUIControllerMock with the new method. Once you overwrite it the error should disappear.
Patchset #7 (id:400001) has been deleted
Patchset #6 (id:380001) has been deleted
The CQ bit was checked by kolos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2542093002/diff/420001/chrome/browser/ui/pass... File chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h (right): https://codereview.chromium.org/2542093002/diff/420001/chrome/browser/ui/pass... chrome/browser/ui/passwords/manage_passwords_ui_controller_mock.h:33: MOCK_METHOD0(OnNoInteraction, void()); vasilii@: Thanks.
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 kolos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org Link to the patchset: https://codereview.chromium.org/2542093002/#ps420001 (title: "Fix for the failed Mac test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1482230213503560,
"parent_rev": "79749f80a169c2e9f7ed50caebf821e7e79efb50", "commit_rev":
"8216efa3353a8e0d0207c4da804f756eb436cfdd"}
Message was sent while issue was closed.
Description was changed from ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 ========== to ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 Review-Url: https://codereview.chromium.org/2542093002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 Review-Url: https://codereview.chromium.org/2542093002 ========== to ========== [Password Generation] Fixes sending votes about the usage of the password generation popup If the password generation popup was accepted by the user, the vote (ACCEPTED) will be always sent (because of automatic saving for generated passwords). If the popup was ignored, the vote (IGNORED) will be sent only if the user presses "Save" or "Update", otherwise the vote wouldn't be sent. This distorts the data about the usage of generation popup. This CL enables sending the votes regardless user's response. This CL also merges two similar functions UploadPasswordForm and UploadChangePasswordForm into UploadPasswordVote. BUG=552420 Committed: https://crrev.com/7209820b1bdfa1f86eaaa0e14b2c370ebf0064cd Cr-Commit-Position: refs/heads/master@{#439763} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7209820b1bdfa1f86eaaa0e14b2c370ebf0064cd Cr-Commit-Position: refs/heads/master@{#439763} |
