Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1161023008:
Suggest to fill password change forms (Closed)
|
Created:
5 years, 6 months ago by dvadym Modified:
5 years, 6 months ago CC:
browser-components-watch_chromium.org, chromium-reviews, darin-cc_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mkwst+watchlist-passwords_chromium.org, mlamouri+watch-content_chromium.org, rouslan+autofillwatch_chromium.org, vabr+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuggest to fill password change forms
This CL allows suggesting for filling password change forms for both username and password fields. We allow to fill password field without filling username to fix cases when a previous to the password text field is not username or invisible (e.g. facebook.com).
Tests will be uploaded in next patchsets
BUG=359315
Committed: https://crrev.com/f78d9b496ce4e04a10374b1bb0cf0013ac5716ba
Cr-Commit-Position: refs/heads/master@{#335019}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added tests, addressed comments #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Renaming and showing password field suggestions rarely #
Total comments: 6
Patch Set 5 : Addressed reviewer comments #Patch Set 6 : Rebase #Patch Set 7 : Added tests for suggestions prompting #Patch Set 8 : Tiny fix #
Total comments: 4
Patch Set 9 : Addressed reviewer's comments #Patch Set 10 : rebase #Patch Set 11 : Rebase #
Messages
Total messages: 25 (7 generated)
dvadym@chromium.org changed reviewers: + gcasto@chromium.org
Hi Garrett, Could you please review this CL? Best regards, Vadym
Looks like this CL isn't complete as |is_password_change_form| is never populated. I'd also like to wait to submit until the tests are done. The general idea looks good though. https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:862: !password_info->fill_data.is_password_change_form && Is this because we don't trust that |username_element| is correct for change password forms? I seem to remember this being a problem somewhere, maybe twitter? Given the other changes this will make it so that a lot of signup forms will show a saved password when you click on them, though that probably isn't a huge deal. https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:147: passwords[0].value() == passwords[1].value()) { You probably want to leave a comment here about why you want to have two empty password fields count as a change password form not as a signup form.
Thanks Garrett! It was incorrect commit, I hadn't added setting of |is_password_change_form|, now it's fixed. Also I've added tests and fixed a case when SignIn and SignUp are in one form. Please take another look. https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:862: !password_info->fill_data.is_password_change_form && On 2015/06/05 21:22:30, Garrett Casto wrote: > Is this because we don't trust that |username_element| is correct for change > password forms? I seem to remember this being a problem somewhere, maybe > twitter? Given the other changes this will make it so that a lot of signup forms > will show a saved password when you click on them, though that probably isn't a > huge deal. Yeah, we will not trust username element and show suggestions for both username (if present) and password fields. The Facebook password change form has additional invisible field, that we can't distinguish from username. https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1161023008/diff/1/components/autofill/content... components/autofill/content/renderer/password_form_conversion_utils.cc:147: passwords[0].value() == passwords[1].value()) { On 2015/06/05 21:22:30, Garrett Casto wrote: > You probably want to leave a comment here about why you want to have two empty > password fields count as a change password form not as a signup form. Done.
It looks like this change is going to tickle crbug.com/498545, which I noticed when trying this out on a signup form. I think that should be a relatively easy fix, but I should probably land that first before submitting this change. https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:862: !password_info->fill_data.is_password_change_form && The more that I think about this, the more that I'm not sure that this is the right thing to do. This will make it such that for most signup forms we will now trigger the password manager on both the username and password fields, where before hand we would show UI on neither. I do want filling to work on Facebook, but I'm not sure if this is the right approach. Maybe instead we only show UI on password fields if the username field hasn't been focused. What do you think? https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.cc:60: !form_on_page.new_password_element.empty(); It would be really nice if we only had to compute this once. As is this seems reasonably likely to cause bugs at some point in the future when only one of these is updated. https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:80: bool is_password_change_form; Given that PasswordFormManager already has both is_ignoreable_change_password_form and is_change_password_form, perhaps this should be named something else (is_possible_change_password_form)? At the moment it seems pretty strange that |is_password_change_form| can be true but |is_change_password_form| that we compute in PasswordFormManager is false. They are used for different things (saving vs. filling) so I understand why, but it's still confusing. https://codereview.chromium.org/1161023008/diff/20001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:477: // (4) the form has new password field, i.e. it's Sign Up or password change If you are adding to this list it you should drop move the "or" to between (3) and (4) and use a comma after (3) instead of a period.
Thanks Garrett! I've addressed your comments. PTAL. https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:862: !password_info->fill_data.is_password_change_form && On 2015/06/09 23:56:22, Garrett Casto wrote: > The more that I think about this, the more that I'm not sure that this is the > right thing to do. This will make it such that for most signup forms we will now > trigger the password manager on both the username and password fields, where > before hand we would show UI on neither. > > I do want filling to work on Facebook, but I'm not sure if this is the right > approach. Maybe instead we only show UI on password fields if the username field > hasn't been focused. What do you think? Yeah, I'm also not sure that this is the best approach. We will show suggestions more often that it's required. Your solution definitely helps to fix it and also it fixes facebook and it's easy to implement. The only thing I see that in your solution the user will have the problem when we incorrectly detect username field and the user click on it, so he will no be able to fill password. But it seems to be minor problem, what do you think? https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.cc:60: !form_on_page.new_password_element.empty(); On 2015/06/09 23:56:22, Garrett Casto wrote: > It would be really nice if we only had to compute this once. As is this seems > reasonably likely to cause bugs at some point in the future when only one of > these is updated. I've added method IsChangePasswordForm in PasswordForm https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:80: bool is_password_change_form; On 2015/06/09 23:56:22, Garrett Casto wrote: > Given that PasswordFormManager already has both > is_ignoreable_change_password_form and is_change_password_form, perhaps this > should be named something else (is_possible_change_password_form)? At the moment > it seems pretty strange that |is_password_change_form| can be true but > |is_change_password_form| that we compute in PasswordFormManager is false. They > are used for different things (saving vs. filling) so I understand why, but it's > still confusing. Thanks, I didn't know about |is_change_password_form| in PasswordFormManager. I've changed calculation |is_change_password_form| to call PasswordForm::IsChangePasswordForm, it seems to be normal to use the same condition for checking change password form in all places. What do you think? https://codereview.chromium.org/1161023008/diff/20001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:477: // (4) the form has new password field, i.e. it's Sign Up or password change On 2015/06/09 23:56:22, Garrett Casto wrote: > If you are adding to this list it you should drop move the "or" to between (3) > and (4) and use a comma after (3) instead of a period. Done.
https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:862: !password_info->fill_data.is_password_change_form && On 2015/06/10 14:22:03, dvadym wrote: > On 2015/06/09 23:56:22, Garrett Casto wrote: > > The more that I think about this, the more that I'm not sure that this is the > > right thing to do. This will make it such that for most signup forms we will > now > > trigger the password manager on both the username and password fields, where > > before hand we would show UI on neither. > > > > I do want filling to work on Facebook, but I'm not sure if this is the right > > approach. Maybe instead we only show UI on password fields if the username > field > > hasn't been focused. What do you think? > > Yeah, I'm also not sure that this is the best approach. We will show suggestions > more often that it's required. > Your solution definitely helps to fix it and also it fixes facebook and it's > easy to implement. The only thing I see that in your solution the user will have > the problem when we incorrectly detect username field and the user click on it, > so he will no be able to fill password. But it seems to be minor problem, what > do you think? That is true. It is already a problem we could have for login forms, and one that we are trying to solve in general anyway. I would also think that it's not a big problem for change password forms since when they have a text field that could be the username it's pretty rare that it's something unrelated. Have you noticed instances where this would be problematic? https://codereview.chromium.org/1161023008/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/1161023008/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:311: bool is_change_password_form = form.IsChangePasswordForm(); I think that you misunderstood my previous comment. I don't think that we want to change how this is computed. In particular, this change will make it so that we won't save signup like forms because IsChangePasswordForm() will be true on these forms. The problem is in naming. This variable was checking that the submitted form really was a password change form (e.g. contained two passwords that didn't match), whereas PasswordForm::IsChangePasswordForm() returns true for any form that could conceivably be a change password form, including signup forms. IsChangePasswordForm() isn't the really the right name, more like IsPossibleChangePasswordForm(). You probably want to try and explain this more in the comment for the function in PasswordForm as well as update the name in PasswordFormFillData. Note, I'm not sure that IsPossibleChangePasswordForm() is an ideal name, but I'm not sure what a better one is.
Thanks Garrett, I've addressed your comments. PTAL https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/20001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:862: !password_info->fill_data.is_password_change_form && On 2015/06/11 00:07:36, Garrett Casto wrote: > On 2015/06/10 14:22:03, dvadym wrote: > > On 2015/06/09 23:56:22, Garrett Casto wrote: > > > The more that I think about this, the more that I'm not sure that this is > the > > > right thing to do. This will make it such that for most signup forms we will > > now > > > trigger the password manager on both the username and password fields, where > > > before hand we would show UI on neither. > > > > > > I do want filling to work on Facebook, but I'm not sure if this is the right > > > approach. Maybe instead we only show UI on password fields if the username > > field > > > hasn't been focused. What do you think? > > > > Yeah, I'm also not sure that this is the best approach. We will show > suggestions > > more often that it's required. > > Your solution definitely helps to fix it and also it fixes facebook and it's > > easy to implement. The only thing I see that in your solution the user will > have > > the problem when we incorrectly detect username field and the user click on > it, > > so he will no be able to fill password. But it seems to be minor problem, what > > do you think? > > That is true. It is already a problem we could have for login forms, and one > that we are trying to solve in general anyway. I would also think that it's not > a big problem for change password forms since when they have a text field that > could be the username it's pretty rare that it's something unrelated. Have you > noticed instances where this would be problematic? No, I didn't notice any sites where it can be a problem on change forms. I've implemented your proposition, i.e. not to show suggestions on password field if the username field was in focus. Please review these changes https://codereview.chromium.org/1161023008/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/1161023008/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:311: bool is_change_password_form = form.IsChangePasswordForm(); On 2015/06/11 00:07:36, Garrett Casto wrote: > I think that you misunderstood my previous comment. I don't think that we want > to change how this is computed. In particular, this change will make it so that > we won't save signup like forms because IsChangePasswordForm() will be true on > these forms. > > The problem is in naming. This variable was checking that the submitted form > really was a password change form (e.g. contained two passwords that didn't > match), whereas PasswordForm::IsChangePasswordForm() returns true for any form > that could conceivably be a change password form, including signup forms. > IsChangePasswordForm() isn't the really the right name, more like > IsPossibleChangePasswordForm(). You probably want to try and explain this more > in the comment for the function in PasswordForm as well as update the name in > PasswordFormFillData. Note, I'm not sure that IsPossibleChangePasswordForm() is > an ideal name, but I'm not sure what a better one is. Thanks for clarification. I didn't realize that changing way of calculation of |is_change_password_form| will cause problems for SignUp forms. Strictly speaking checking in this way for change forms is not 100% correct, since there are change forms without old password (yahoo.com for example), but here it's not important. I've returned as it was before and changed IsChangePasswordForm to IsPossibleChangePasswordForm
https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:862: (!password_info->fill_data.is_change_password_form || Instead of keeping an additional structure, how about adding a bit to PasswordInfo that keeps track of if the username was changed and then updating that in UpdateStateForTextChange(). I think it's a little simpler since we wouldn't need to add another function and another data structure. It would mean that we possibly show on both if the user focuses the username field, doesn't enter any info, and then focuses the password field, but I don't think that matters much. Also, can you add a test for this behavior. https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:146: if (!passwords[0].value().isEmpty() && Can you add a test for this? https://codereview.chromium.org/1161023008/diff/60001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/1161023008/diff/60001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:80: bool is_change_password_form; Change to |is_possible_change_password_form| to match.
Thanks Garrett! PTAL https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:862: (!password_info->fill_data.is_change_password_form || On 2015/06/12 00:09:09, Garrett Casto wrote: > Instead of keeping an additional structure, how about adding a bit to > PasswordInfo that keeps track of if the username was changed and then updating > that in UpdateStateForTextChange(). I think it's a little simpler since we > wouldn't need to add another function and another data structure. It would mean > that we possibly show on both if the user focuses the username field, doesn't > enter any info, and then focuses the password field, but I don't think that > matters much. > > Also, can you add a test for this behavior. I agree, that it's better to be simpler, so I've removed OnFocusChange handler and implemented your proposition with user typing. Here the question, should we stop showing suggestions for password field after filling username/password? In current implementation we will still show, since filling doesn't raise onChange event. But it seems to be not big deal, since if the user used autofill, probably he will not click on password field. I've realized that I hadn't tested ShowSuggestions, only FillSuggestions. So I've added 3 more tests for testing ShowSuggestion (for username field, for password field and for password field after typing in username field). https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/1161023008/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:146: if (!passwords[0].value().isEmpty() && On 2015/06/12 00:09:09, Garrett Casto wrote: > Can you add a test for this? Done. I've fixed corresponding test in password_form_conversion_utils_browsertest.cc https://codereview.chromium.org/1161023008/diff/60001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/1161023008/diff/60001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:80: bool is_change_password_form; On 2015/06/12 00:09:09, Garrett Casto wrote: > Change to |is_possible_change_password_form| to match. Done.
lgtm https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:9: #include <set> Unneeded. https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... File components/autofill/core/common/password_form.h (right): https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... components/autofill/core/common/password_form.h:275: // We use only client heuristics for this, so probably we are wrong. I wouldn't say "probably we are wrong" here. I would say something like "could be wrong" or "could include signup forms".
Thanks Garrett https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:9: #include <set> On 2015/06/12 22:22:16, Garrett Casto wrote: > Unneeded. Done. https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... File components/autofill/core/common/password_form.h (right): https://codereview.chromium.org/1161023008/diff/140001/components/autofill/co... components/autofill/core/common/password_form.h:275: // We use only client heuristics for this, so probably we are wrong. On 2015/06/12 22:22:16, Garrett Casto wrote: > I wouldn't say "probably we are wrong" here. I would say something like "could > be wrong" or "could include signup forms". I changed text to "We use only client heuristics, so it could include signup forms."
dvadym@chromium.org changed reviewers: + mkwst@chromium.org
Hi Mike, Could you please review changes in autofill_messages.h? Best regards, Vadym
Adding a boolean to IPC LGTM.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gcasto@chromium.org Link to the patchset: https://codereview.chromium.org/1161023008/#ps160001 (title: "Addressed reviewer's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161023008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 mkwst@chromium.org, gcasto@chromium.org Link to the patchset: https://codereview.chromium.org/1161023008/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1161023008/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f78d9b496ce4e04a10374b1bb0cf0013ac5716ba Cr-Commit-Position: refs/heads/master@{#335019} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
