Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814193002:
Better filling on suggestion of password fields in Password Manager. (Closed)
|
Created:
4 years, 9 months ago by dvadym Modified:
4 years, 8 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, mlamouri+watch-content_chromium.org, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, jam, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, vabr+watchlistautofill_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. |
DescriptionBetter filling on suggestion of password fields in Password Manager.
There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't.
Precise logic of showing suggestion on password field is the following: A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field.
The latter part is made for decreasing number of false positives. For example if there is a change password form with <old password field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>.
BUG=597304, 591058
Committed: https://crrev.com/20e808ba94aa4a1ff5d3c16155f86bf93747de42
Cr-Commit-Position: refs/heads/master@{#386109}
Patch Set 1 #Patch Set 2 : Continuation of impl #Patch Set 3 : Continuation of impl #Patch Set 4 : Rebase and clean-up #Patch Set 5 : Some fixes #
Total comments: 9
Patch Set 6 : Comments update #Patch Set 7 : Changed condition on suggestion popup on password field showing #Patch Set 8 : Fixes #Patch Set 9 : Tests added #
Total comments: 7
Patch Set 10 : Comments addressed #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Better filling on suggestion of pw field Not ready for review yet BUG= ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store. So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304 ==========
Description was changed from ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store. So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304 ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304 ==========
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Could you please have a look at this CL? Regards, Vadym
Hi Vadym, Looks good so far, I'm waiting with the approval for the tests. Some comments are below. Cheers, Vaclav https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:896: *password_info = &web_input_to_password_info_.begin()->second; nit: Please comment on the choice of the value for |*password_info| here, it looks rather random. https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:949: // suggestions popup. Please update the comment to also speak about not showing the pop-up if the user already accepted a suggestion on some other password field. https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:111: int key; nit: Please explain what kind of key is this. https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:186: // corresponding username WebInputElement and PasswordInfo into nit: Please update the comment to speak about |password_element| as well. Is it always the same as *password_info->password_field?
Description was changed from ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304 ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ==========
Description was changed from ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is a change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ==========
Description was changed from ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. For decreasing number of such false positives as soon as the user chose suggestion for a password field, it's not shown on any other field. For example if there is a change password form with <old passoword field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. Precise logic of showing suggestion on password field is the following: A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field. The latter part is made for decreasing number of false positives. For example if there is a change password form with <old password field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ==========
Hi Vaclav, Finally tests for better filling are ready. Also there are some other changes in logic. The logic of filling of password fields are A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field. PTAL Regards, Vadym https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:896: *password_info = &web_input_to_password_info_.begin()->second; On 2016/04/01 14:07:37, vabr (Chromium) wrote: > nit: Please comment on the choice of the value for |*password_info| here, it > looks rather random. Done. https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:949: // suggestions popup. On 2016/04/01 14:07:37, vabr (Chromium) wrote: > Please update the comment to also speak about not showing the pop-up if the user > already accepted a suggestion on some other password field. I've decided to simplify this condition, the only case when not to show when the user has already accepted it on another password field, that will save us from cases where the username found incorrectly. Please have a look at changes in PatchSet 7. https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:111: int key; On 2016/04/01 14:07:37, vabr (Chromium) wrote: > nit: Please explain what kind of key is this. Done. https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:186: // corresponding username WebInputElement and PasswordInfo into On 2016/04/01 14:07:37, vabr (Chromium) wrote: > nit: Please update the comment to speak about |password_element| as well. Is it > always the same as *password_info->password_field? I've updated. No it's not always equal to *password_info->password_field. ->password_field contains our first guess, but if the user clicks on a password element that is not in ->password_field we select it as |password_element|. https://codereview.chromium.org/1814193002/diff/160001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (left): https://codereview.chromium.org/1814193002/diff/160001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1985: TEST_F(PasswordAutofillAgentTest, Due to these changes this test is not needed anymore, since we always will suggest to fill password field, not depending on input in username field. https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:1421: blink::WebInputElement selected_element = user_input; With current logic we don't need this code anymore.
Thanks, Vadym. LGTM with some comments. Cheers, Vaclav https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1814193002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:949: // suggestions popup. On 2016/04/07 17:30:20, dvadym wrote: > On 2016/04/01 14:07:37, vabr (Chromium) wrote: > > Please update the comment to also speak about not showing the pop-up if the > user > > already accepted a suggestion on some other password field. > > I've decided to simplify this condition, the only case when not to show when the > user has already accepted it on another password field, that will save us from > cases where the username found incorrectly. Please have a look at changes in > PatchSet 7. Acknowledged. https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (left): https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:1421: blink::WebInputElement selected_element = user_input; On 2016/04/07 17:30:20, dvadym wrote: > With current logic we don't need this code anymore. Acknowledged. https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:942: // If the element is a password field, not to show a popup if the user has nit: not to show -> do not show https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:109: int key; Please also initialise the |key| (probably to -1, to mark it as invalid?).
Thanks for review! https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:942: // If the element is a password field, not to show a popup if the user has On 2016/04/08 14:47:31, vabr (Chromium) wrote: > nit: not to show -> do not show Done. https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/1814193002/diff/160001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:109: int key; On 2016/04/08 14:47:31, vabr (Chromium) wrote: > Please also initialise the |key| (probably to -1, to mark it as invalid?). Done.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1814193002/#ps180001 (title: "Comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814193002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814193002/180001
Message was sent while issue was closed.
Description was changed from ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. Precise logic of showing suggestion on password field is the following: A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field. The latter part is made for decreasing number of false positives. For example if there is a change password form with <old password field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. Precise logic of showing suggestion on password field is the following: A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field. The latter part is made for decreasing number of false positives. For example if there is a change password form with <old password field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. Precise logic of showing suggestion on password field is the following: A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field. The latter part is made for decreasing number of false positives. For example if there is a change password form with <old password field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 ========== to ========== Better filling on suggestion of password fields in Password Manager. There are a lot of reasons why PasswordManager could miss a password field for autofilling, for example when there is an invisible dummy password field and it's autofilled instead of correct one. This patch makes that as soon as the user clicks on password field Password Manager will show suggestion dropdown with credentials that are already fetched from store in PasswordFormManager (if this is not enough, we'll need to add fetching credentials, but now it looks as adding non-necessary complexity). So as consequence of this patch Password Manager will show more suggestions on password fields, probably sometimes in cases where it shouldn't. Precise logic of showing suggestion on password field is the following: A suggestion dropdown is shown on any password field until the user chooses one of the suggestion on some password field, then a suggestion is shown only on this field. The latter part is made for decreasing number of false positives. For example if there is a change password form with <old password field> and <new password field>, initially suggestions will be shown on clicking on both of them, but as soon as the user accepts it for <old passoword field> it will be not shown for <new password field>. BUG=597304, 591058 Committed: https://crrev.com/20e808ba94aa4a1ff5d3c16155f86bf93747de42 Cr-Commit-Position: refs/heads/master@{#386109} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/20e808ba94aa4a1ff5d3c16155f86bf93747de42 Cr-Commit-Position: refs/heads/master@{#386109} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
