Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716873002:
More robust fill password fields on click. (Closed)
|
Created:
3 years, 10 months ago by dvadym Modified:
3 years, 9 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMore robust fill password fields on click.
Originally, Chrome would only offer suggestions for password fields, when no username were found. Since https://codereview.chromium.org/1814193002, it also offers suggestions
when the user clicks on any password field.
However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions.
This CL changes PasswordAutofillAgent to use password field data even if fields names were changed. That allows Chrome to show suggestions even in the above mentioned problematic case.
BUG=597304
Review-Url: https://codereview.chromium.org/2716873002
Cr-Commit-Position: refs/heads/master@{#453193}
Committed: https://chromium.googlesource.com/chromium/src/+/a6ea9dfa43640a82f3c7d07da387d67d485db1c8
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update comment #
Messages
Total messages: 17 (12 generated)
Description was changed from ========== More robust fill password fields on click. BUG=597304 ========== to ========== More robust fill password fields on click. On https://codereview.chromium.org/1814193002 it was implemented a fallback mechanism for filling of passwords fields on click, when autofill is failed. But it can be done even better, in the first implementation if fields for filling are not found, the fallback wouldn't work (that could be for example when JavaScript changes field names immediately after load, i.e.ping about form load from renderer to browser sent with old names, but when fill data comes back to renderer, names are changed). This CL fixes this. BUG=597304 ==========
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, Could you please review this CL? Regards, Vadym
Hi Vadym, The code LGTM. I still think the CL description needs some work. It should describe: (1) what is the problem with the current status, and (2) what this CL does about fixing it. While the current description attempts (1), it is not very clear -- "fallback mechanism for (...) when autofill is failed" does not say what that mechanism is. Part (2) is summarised as "This CL fixes this" which is only slightly better than saying nothing. Also, please try to use short, simple sentences. The description could have a structure like this: ------- Originally, Chrome would only offer suggestions for password fields, when ... Since https://codereview.chromium.org/1814193002, it also offers suggestions when ... However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions. This CL changes PasswordAutofillAgent to use password field data independently of field names, if ... That allows Chrome to show suggestions even in the above mentioned problematic case. ------- Cheers, Vaclav https://codereview.chromium.org/2716873002/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2716873002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:1444: web_input_to_password_info_[blink::WebInputElement()] = password_info; Could you explain in a code comment, how the |password_info| is retrieved if the key is a non-existing WebInputElement? (I suppose it is because PasswordAutofillAgent::FindPasswordInfoForElement will take the first pair in web_input_to_password_info_ in case there is no username associated with the password?)
Description was changed from ========== More robust fill password fields on click. On https://codereview.chromium.org/1814193002 it was implemented a fallback mechanism for filling of passwords fields on click, when autofill is failed. But it can be done even better, in the first implementation if fields for filling are not found, the fallback wouldn't work (that could be for example when JavaScript changes field names immediately after load, i.e.ping about form load from renderer to browser sent with old names, but when fill data comes back to renderer, names are changed). This CL fixes this. BUG=597304 ========== to ========== More robust fill password fields on click. On https://codereview.chromium.org/1814193002 it was implemented a fallback mechanism for filling of passwords fields on click, when autofill is failed. But it can be done even better, in the first implementation if fields for filling are not found, the fallback wouldn't work (that could be for example when JavaScript changes field names immediately after load, i.e.ping about form load from renderer to browser sent with old names, but when fill data comes back to renderer, names are changed). This CL fixes this. Originally, Chrome would only offer suggestions for password fields, when no username were found. Since https://codereview.chromium.org/1814193002, it also offers suggestions when the user clicks on any password field. However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions. This CL changes PasswordAutofillAgent to use password field data even if fields names were changed. That allows Chrome to show suggestions even in the above mentioned problematic case. BUG=597304 ==========
Description was changed from ========== More robust fill password fields on click. On https://codereview.chromium.org/1814193002 it was implemented a fallback mechanism for filling of passwords fields on click, when autofill is failed. But it can be done even better, in the first implementation if fields for filling are not found, the fallback wouldn't work (that could be for example when JavaScript changes field names immediately after load, i.e.ping about form load from renderer to browser sent with old names, but when fill data comes back to renderer, names are changed). This CL fixes this. Originally, Chrome would only offer suggestions for password fields, when no username were found. Since https://codereview.chromium.org/1814193002, it also offers suggestions when the user clicks on any password field. However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions. This CL changes PasswordAutofillAgent to use password field data even if fields names were changed. That allows Chrome to show suggestions even in the above mentioned problematic case. BUG=597304 ========== to ========== More robust fill password fields on click. Originally, Chrome would only offer suggestions for password fields, when no username were found. Since https://codereview.chromium.org/1814193002, it also offers suggestions when the user clicks on any password field. However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions. This CL changes PasswordAutofillAgent to use password field data even if fields names were changed. That allows Chrome to show suggestions even in the above mentioned problematic case. BUG=597304 ==========
Thanks Vaclav for review! I've addressed your comments https://codereview.chromium.org/2716873002/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2716873002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:1444: web_input_to_password_info_[blink::WebInputElement()] = password_info; On 2017/02/24 20:10:36, vabr (Chromium) wrote: > Could you explain in a code comment, how the |password_info| is retrieved if the > key is a non-existing WebInputElement? > (I suppose it is because PasswordAutofillAgent::FindPasswordInfoForElement will > take the first pair in web_input_to_password_info_ in case there is no username > associated with the password?) Yeah, your explanation is correct, I've updated the comment
The CQ bit was checked by dvadym@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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2716873002/#ps20001 (title: "Update comment")
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": 20001, "attempt_start_ts": 1488195784883310,
"parent_rev": "b2491de32a51aa8847f46bbc498a62313285d3f1", "commit_rev":
"a6ea9dfa43640a82f3c7d07da387d67d485db1c8"}
Message was sent while issue was closed.
Description was changed from ========== More robust fill password fields on click. Originally, Chrome would only offer suggestions for password fields, when no username were found. Since https://codereview.chromium.org/1814193002, it also offers suggestions when the user clicks on any password field. However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions. This CL changes PasswordAutofillAgent to use password field data even if fields names were changed. That allows Chrome to show suggestions even in the above mentioned problematic case. BUG=597304 ========== to ========== More robust fill password fields on click. Originally, Chrome would only offer suggestions for password fields, when no username were found. Since https://codereview.chromium.org/1814193002, it also offers suggestions when the user clicks on any password field. However, if JavaScript changes field names immediately after load, before password fill data reach the renderer, Chrome still fails to display suggestions. This CL changes PasswordAutofillAgent to use password field data even if fields names were changed. That allows Chrome to show suggestions even in the above mentioned problematic case. BUG=597304 Review-Url: https://codereview.chromium.org/2716873002 Cr-Commit-Position: refs/heads/master@{#453193} Committed: https://chromium.googlesource.com/chromium/src/+/a6ea9dfa43640a82f3c7d07da387... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a6ea9dfa43640a82f3c7d07da387... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
