Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889393002:
Add username field discovery heuristic. (Closed)
|
Created:
3 years, 7 months ago by pkalinnikov Modified:
3 years, 6 months ago 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd username field discovery heuristic.
There are some cases when autofill can't find a username field for
autocompletion. For example, if the field is created dynamically in JavaScript.
This CL introduces a heuristic which is used as a fallback in such cases to
find a username field.
When password form autocompletion is about to happen with unknown username
field, the heuristic searches for the closest visible autocompletable
non-password text element preceding the password element. If one found, it is
believed to be the username field.
BUG=710374, 606736, 586465, 710003
Review-Url: https://codereview.chromium.org/2889393002
Cr-Commit-Position: refs/heads/master@{#475481}
Committed: https://chromium.googlesource.com/chromium/src/+/4afdcc3b649fd9e056ccfe078890c39419e62d08
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase onto crrev/2902113004 #Patch Set 3 : Rebase. #
Total comments: 11
Patch Set 4 : Address comments; add tests. #
Total comments: 2
Messages
Total messages: 41 (25 generated)
Description was changed from ========== Autofill username when the user interacts with a password field. Previously, when the user clicked on password field, only this field was autocompleted with their password. This CL ensures that the username field is also autocompleted with a login. BUG=710374 ========== to ========== Autofill username when the user interacts with the password field. Previously, when the user clicked on password field, only this field was autocompleted with their password. This CL ensures that the username field is also autocompleted with a login. BUG=710374 ==========
The CQ bit was checked by pkalinnikov@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...
pkalinnikov@chromium.org changed reviewers: + kolos@chromium.org, mathp@chromium.org
Maxim, this is a fork of your crrev/2836653002 with supported suggestions and browsertests fixes. As discussed offline, you can go ahead and close that CL. Everyone, please take an initial look at this change. I will add more tests later. Can you suggest testing scenarios?
https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:57: using autofill::form_util::IsWebElementVisible; selfnit: Remove this.
kolos@chromium.org changed reviewers: + dvadym@chromium.org
Add Vadym as a reviewer. Let Mathieu review tests when they are ready. I would suggest rogerm@ or sebsg@ as reviewers for tests since Mathieu is working on another project. I will think of tests. Pavel: did you test the code on the sites listed in the bug or the blocked bugs?
kolos@chromium.org changed reviewers: - mathp@chromium.org
dvadym@ has ownership for browsertests. Removing autofill folks.
Thanks for implementing this Pavel! In general it looks good. Some comments below https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:646: !input->IsEnabled() || !form_util::IsWebElementVisible(*input)) { It looks like we can do it more optimally by finding at first |password_element| in |elements| and then traversing vertor backwards. We wouldn't need to do this for all elements before |password_element|, as far as I understand the most expensive could be IsWebElementVisible. https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:838: username_element = FindUsernameElementBasedOnPasswordElement( Why don't put call to FindUsernameElementBasedOnPasswordElement inside FindPasswordInfoForElement. FindPasswordInfoForElement is looking for username also and it would allow to have only 1 call site instead of 3.
On 2017/05/19 15:03:18, kolos1 wrote: > Add Vadym as a reviewer. Let Mathieu review tests when they are ready. I would > suggest rogerm@ or sebsg@ as reviewers for tests since Mathieu is working on > another project. > > I will think of tests. > > Pavel: did you test the code on the sites listed in the bug or the blocked bugs? Yes, I have tested it with the sites listed in all the blocking bugs, it works. However, it doesn't work with casino.bet365.com (both when clicking on username and on password). Do you think it can be fixed by this CL? Seems like a different issue (because it wasn't working in the other direction as well), but I will investigate.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Autofill username when the user interacts with the password field. Previously, when the user clicked on password field, only this field was autocompleted with their password. This CL ensures that the username field is also autocompleted with a login. BUG=710374 ========== to ========== Add username field discovery heuristic. TODO: Description. BUG=710374,606736,586465,710003 ==========
The CQ bit was checked by pkalinnikov@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pkalinnikov@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...
Addressed comments, PTAL. https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:57: using autofill::form_util::IsWebElementVisible; On 2017/05/19 14:44:00, pkalinnikov wrote: > selfnit: Remove this. Done. https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:646: !input->IsEnabled() || !form_util::IsWebElementVisible(*input)) { On 2017/05/19 15:36:34, dvadym wrote: > It looks like we can do it more optimally by finding at first |password_element| > in |elements| and then traversing vertor backwards. We wouldn't need to do this > for all elements before |password_element|, as far as I understand the most > expensive could be IsWebElementVisible. SGTM. Done. https://codereview.chromium.org/2889393002/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:838: username_element = FindUsernameElementBasedOnPasswordElement( On 2017/05/19 15:36:34, dvadym wrote: > Why don't put call to FindUsernameElementBasedOnPasswordElement inside > FindPasswordInfoForElement. FindPasswordInfoForElement is looking for username > also and it would allow to have only 1 call site instead of 3. Done. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:611: blink::WebInputElement FindUsernameElementPrecedingPasswordElement( selfnit: Add comment. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:615: DCHECK(form_util::IsWebElementVisible(password_element)); selfnit: This check seems not guaranteed to hold (at least it's not trivial to prove so), remove it. Moreover, the checked condition is not crucial for this function to work properly. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:637: return input->Value().IsEmpty() || input->IsAutofilled() selfnit: It is sufficient to unconditionally return *input, because the ability to amend the username field is verified with IsUsernameAmendable by the call sites.
It looks good. It's left just to add some test. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:611: blink::WebInputElement FindUsernameElementPrecedingPasswordElement( On 2017/05/24 16:09:49, pkalinnikov wrote: > selfnit: Add comment. Acknowledged! https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:615: DCHECK(form_util::IsWebElementVisible(password_element)); On 2017/05/24 16:09:48, pkalinnikov wrote: > selfnit: This check seems not guaranteed to hold (at least it's not trivial to > prove so), remove it. Moreover, the checked condition is not crucial for this > function to work properly. Agree, that's hard to guarantee and not necessary. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:637: return input->Value().IsEmpty() || input->IsAutofilled() On 2017/05/24 16:09:49, pkalinnikov wrote: > selfnit: It is sufficient to unconditionally return *input, because the ability > to amend the username field is verified with IsUsernameAmendable by the call > sites. Acknowledged! https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:948: // Otherwise |username_element| has been set above. Interesting style of comments, in place of imaginary else :).
This situation could be reproduces in tests: 1.HTML with a change password form is loaded (2 password fields) 2.A text field is inserted with JavaScript before the first password field.
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 pkalinnikov@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...
Description was changed from ========== Add username field discovery heuristic. TODO: Description. BUG=710374,606736,586465,710003 ========== to ========== Add username field discovery heuristic. There are some cases when autofill can't find a username field for autocompletion. For example, if the field is created dynamically in JavaScript. This CL introduces a heuristic which is used as a fallback in such cases to find a username field. When password form autocompletion is about to happen with unknown username field, the heuristic searches for the closest visible autocompletable non-password text element preceding the password element. If one found, it is believed to be the username field. BUG=710374,606736,586465,710003 ==========
Maxim, Vadym, PTAL. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:611: blink::WebInputElement FindUsernameElementPrecedingPasswordElement( On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > On 2017/05/24 16:09:49, pkalinnikov wrote: > > selfnit: Add comment. > > Acknowledged! Done. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:615: DCHECK(form_util::IsWebElementVisible(password_element)); On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > On 2017/05/24 16:09:48, pkalinnikov wrote: > > selfnit: This check seems not guaranteed to hold (at least it's not trivial to > > prove so), remove it. Moreover, the checked condition is not crucial for this > > function to work properly. > > Agree, that's hard to guarantee and not necessary. Done. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:637: return input->Value().IsEmpty() || input->IsAutofilled() On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > On 2017/05/24 16:09:49, pkalinnikov wrote: > > selfnit: It is sufficient to unconditionally return *input, because the > ability > > to amend the username field is verified with IsUsernameAmendable by the call > > sites. > > Acknowledged! Done. https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:948: // Otherwise |username_element| has been set above. On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > Interesting style of comments, in place of imaginary else :). Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/29 15:02:21, pkalinnikov wrote: > Maxim, Vadym, PTAL. > > https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:611: > blink::WebInputElement FindUsernameElementPrecedingPasswordElement( > On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > > On 2017/05/24 16:09:49, pkalinnikov wrote: > > > selfnit: Add comment. > > > > Acknowledged! > > Done. > > https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:615: > DCHECK(form_util::IsWebElementVisible(password_element)); > On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > > On 2017/05/24 16:09:48, pkalinnikov wrote: > > > selfnit: This check seems not guaranteed to hold (at least it's not trivial > to > > > prove so), remove it. Moreover, the checked condition is not crucial for > this > > > function to work properly. > > > > Agree, that's hard to guarantee and not necessary. > > Done. > > https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:637: return > input->Value().IsEmpty() || input->IsAutofilled() > On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > > On 2017/05/24 16:09:49, pkalinnikov wrote: > > > selfnit: It is sufficient to unconditionally return *input, because the > > ability > > > to amend the username field is verified with IsUsernameAmendable by the call > > > sites. > > > > Acknowledged! > > Done. > > https://codereview.chromium.org/2889393002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:948: // > Otherwise |username_element| has been set above. > On 2017/05/24 16:30:19, dvadym (OOO till May 29) wrote: > > Interesting style of comments, in place of imaginary else :). > > Acknowledged. LGTM
https://codereview.chromium.org/2889393002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:948: // Otherwise |username_element| has been set above. Optional: } else { // Otherwise |username_element| has been set above. } To be sure, it is else-branch of the last if.
LGTM
Thanks! Submitting. https://codereview.chromium.org/2889393002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2889393002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:948: // Otherwise |username_element| has been set above. On 2017/05/30 05:44:55, kolos1 wrote: > Optional: > } else { > // Otherwise |username_element| has been set above. > } > > To be sure, it is else-branch of the last if. I will leave this as is for compactness.
The CQ bit was checked by pkalinnikov@chromium.org
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": 60001, "attempt_start_ts": 1496137679500440,
"parent_rev": "44f1ff3ef09322690900c00580c90b841a52d73a", "commit_rev":
"4afdcc3b649fd9e056ccfe078890c39419e62d08"}
Message was sent while issue was closed.
Description was changed from ========== Add username field discovery heuristic. There are some cases when autofill can't find a username field for autocompletion. For example, if the field is created dynamically in JavaScript. This CL introduces a heuristic which is used as a fallback in such cases to find a username field. When password form autocompletion is about to happen with unknown username field, the heuristic searches for the closest visible autocompletable non-password text element preceding the password element. If one found, it is believed to be the username field. BUG=710374,606736,586465,710003 ========== to ========== Add username field discovery heuristic. There are some cases when autofill can't find a username field for autocompletion. For example, if the field is created dynamically in JavaScript. This CL introduces a heuristic which is used as a fallback in such cases to find a username field. When password form autocompletion is about to happen with unknown username field, the heuristic searches for the closest visible autocompletable non-password text element preceding the password element. If one found, it is believed to be the username field. BUG=710374,606736,586465,710003 Review-Url: https://codereview.chromium.org/2889393002 Cr-Commit-Position: refs/heads/master@{#475481} Committed: https://chromium.googlesource.com/chromium/src/+/4afdcc3b649fd9e056ccfe078890... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4afdcc3b649fd9e056ccfe078890... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
