Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902113004:
Autofill username when the user interacts with the password field. (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. |
DescriptionAutofill username when the user interacts with the password field.
Previously, when the user clicked on a password field, only this field
was autocompleted with their password. This CL ensures that the username
field is autocompleted as well.
If the user interacts with a password field then the username is filled
only if it was empty or *autofilled*, i.e. the user-supplied data is
never overridden.
Note that this works only if the username field was found.
BUG=710374, 708605, 708602
Review-Url: https://codereview.chromium.org/2902113004
Cr-Commit-Position: refs/heads/master@{#475329}
Committed: https://chromium.googlesource.com/chromium/src/+/97b7f62478df1974a2742137afe572f3621464af
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rebase; address comments; refactor tests. #Patch Set 3 : Rebase. #
Total comments: 14
Patch Set 4 : Address comments from kolos@. #
Total comments: 2
Messages
Total messages: 26 (17 generated)
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: + dvadym@chromium.org, kolos@chromium.org
Vadym, Maxim, PTAL. This CL comes instead of crrev/2889393002. It solves the very issue of filling username when interacting with a password field. I will amend the other CL, so that it would simply add a field discovery heuristic. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1165: // Try Filling with a different password. Only password should be changed. selfnit: ... Both fields should be changed. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1259: PreviewSuggestionFromPasswordFieldWithUsernameManuallyFilled) { selfnit: Move this test below PreviewSuggestionFromPasswordField. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1292: // Only password field should be autocompleted. selfnit: // Both fields should be filled with suggestions. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1297: // Try previewing with a different password. Only password should be changed. selfnit: ... Both fields should be changed. https://codereview.chromium.org/2902113004/diff/1/components/autofill/content... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2902113004/diff/1/components/autofill/content... components/autofill/content/renderer/password_autofill_agent.cc:309: bool IsUsernameAmendable(const blink::WebInputElement& username_element, nit: Add comment?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
PTAL at patch set #2. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1165: // Try Filling with a different password. Only password should be changed. On 2017/05/24 10:43:31, pkalinnikov wrote: > selfnit: ... Both fields should be changed. Done. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1259: PreviewSuggestionFromPasswordFieldWithUsernameManuallyFilled) { On 2017/05/24 10:43:31, pkalinnikov wrote: > selfnit: Move this test below PreviewSuggestionFromPasswordField. Done. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1292: // Only password field should be autocompleted. On 2017/05/24 10:43:31, pkalinnikov wrote: > selfnit: // Both fields should be filled with suggestions. Done. https://codereview.chromium.org/2902113004/diff/1/chrome/renderer/autofill/pa... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1297: // Try previewing with a different password. Only password should be changed. On 2017/05/24 10:43:31, pkalinnikov wrote: > selfnit: ... Both fields should be changed. Done.
LGTM thanks for extensive testing and test improvements!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Thanks Pavel for going far beyond the supposed fix. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:452: password, password_autofilled, false, true); Could you please add description to false/true values here and in 2 functions below? e.g. false /* check_suggested_username */ https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1210: for (const auto& element : {username_element_, password_element_}) { Could we change |element| to sth more descriptive? e.g. |selected_username|. The same below. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1225: ClearUsernameAndPasswordFields(); Shall we also call PasswordAutofillAgent::ClearPreview here? https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1256: // sent to the renderer. No sure I am understand what we test here. Please clarify. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2222: FillSuggestionPasswordChangeFormsOnlyPassword) { I believe we should change the name of test.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Thanks for the comments. Maxim, can you take a final look? https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:452: password, password_autofilled, false, true); On 2017/05/26 12:09:11, kolos1 wrote: > Could you please add description to false/true values here and in 2 functions > below? > e.g. false /* check_suggested_username */ Done. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1210: for (const auto& element : {username_element_, password_element_}) { On 2017/05/26 12:09:10, kolos1 wrote: > Could we change |element| to sth more descriptive? e.g. |selected_username|. > The same below. selected_element? Done. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1225: ClearUsernameAndPasswordFields(); On 2017/05/26 12:09:10, kolos1 wrote: > Shall we also call PasswordAutofillAgent::ClearPreview here? ClearPreview is private. We could invoke DidClearAutofillSelection which calls ClearPreview. But I think it would add complexity to these tests, because both FillSuggestion and DidClearAutofillSelection would be tested at the same time. DidClearAutofillSelection method is tested separately in ClearPreview* tests already, so I don't think it's necessary to put it here as well. I changed the ClearUsernameAndPasswordFields() method to reset the state more thoroughly. WDYT? https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1256: // sent to the renderer. On 2017/05/26 12:09:10, kolos1 wrote: > No sure I am understand what we test here. Please clarify. This comment was added when this test was introduced in crrev/208453002. Can I leave it as is? https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2222: FillSuggestionPasswordChangeFormsOnlyPassword) { On 2017/05/26 12:09:10, kolos1 wrote: > I believe we should change the name of test. Seems like I can merge it to the previous test. Done. WDYT?
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.
LGTM. Thx Pavel! https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1210: for (const auto& element : {username_element_, password_element_}) { On 2017/05/26 16:36:52, pkalinnikov wrote: > On 2017/05/26 12:09:10, kolos1 wrote: > > Could we change |element| to sth more descriptive? e.g. |selected_username|. > > The same below. > > selected_element? Done. ah, sure. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1225: ClearUsernameAndPasswordFields(); On 2017/05/26 16:36:52, pkalinnikov wrote: > On 2017/05/26 12:09:10, kolos1 wrote: > > Shall we also call PasswordAutofillAgent::ClearPreview here? > > ClearPreview is private. We could invoke DidClearAutofillSelection which calls > ClearPreview. > > But I think it would add complexity to these tests, because both FillSuggestion > and DidClearAutofillSelection would be tested at the same time. > > DidClearAutofillSelection method is tested separately in ClearPreview* tests > already, so I don't think it's necessary to put it here as well. I changed the > ClearUsernameAndPasswordFields() method to reset the state more thoroughly. > > WDYT? Looks good. Thx. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1256: // sent to the renderer. On 2017/05/26 16:36:52, pkalinnikov wrote: > On 2017/05/26 12:09:10, kolos1 wrote: > > No sure I am understand what we test here. Please clarify. > > This comment was added when this test was introduced in crrev/208453002. Can I > leave it as is? Yes, let's keep as it is. I believe it came from https://codereview.chromium.org/184103016. In https://codereview.chromium.org/208453002/, the comment was copied from similar ones. https://codereview.chromium.org/2902113004/diff/30001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2222: FillSuggestionPasswordChangeFormsOnlyPassword) { On 2017/05/26 16:36:51, pkalinnikov wrote: > On 2017/05/26 12:09:10, kolos1 wrote: > > I believe we should change the name of test. > > Seems like I can merge it to the previous test. Done. WDYT? Looks good. https://codereview.chromium.org/2902113004/diff/50001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/50001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1204: // Try Filling with a different password. Only password should be changed. Thanks for adding a subtest for this.
https://codereview.chromium.org/2902113004/diff/50001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2902113004/diff/50001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1204: // Try Filling with a different password. Only password should be changed. On 2017/05/29 09:17:11, kolos1 wrote: > Thanks for adding a subtest for this. Welcome :)
The CQ bit was checked by pkalinnikov@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/2902113004/#ps50001 (title: "Address comments from kolos@.")
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": 50001, "attempt_start_ts": 1496049667188730,
"parent_rev": "5cf550ea35efdd34ce42848cd00d5608329699c9", "commit_rev":
"97b7f62478df1974a2742137afe572f3621464af"}
Message was sent while issue was closed.
Description was changed from ========== Autofill username when the user interacts with the password field. Previously, when the user clicked on a password field, only this field was autocompleted with their password. This CL ensures that the username field is autocompleted as well. If the user interacts with a password field then the username is filled only if it was empty or *autofilled*, i.e. the user-supplied data is never overridden. Note that this works only if the username field was found. BUG=710374,708605,708602 ========== to ========== Autofill username when the user interacts with the password field. Previously, when the user clicked on a password field, only this field was autocompleted with their password. This CL ensures that the username field is autocompleted as well. If the user interacts with a password field then the username is filled only if it was empty or *autofilled*, i.e. the user-supplied data is never overridden. Note that this works only if the username field was found. BUG=710374,708605,708602 Review-Url: https://codereview.chromium.org/2902113004 Cr-Commit-Position: refs/heads/master@{#475329} Committed: https://chromium.googlesource.com/chromium/src/+/97b7f62478df1974a2742137afe5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/97b7f62478df1974a2742137afe5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
