Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1292693004:
[Password Manager] Autofill forms with field name and id attributes missing. (Closed)
|
Created:
5 years, 4 months ago by Pritam Nikam Modified:
5 years, 2 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Password Manager] Autofill forms with field name and id attributes missing.
Currently, password form (and change password form) having no name or id
attribute specified for password input field, treated as an invalid password
form. And autofilling such forms is ignored.
CL is split into 2 parts:
[A] Allow us storing the password forms having name or id attributes missing
from the fields: https://codereview.chromium.org/1286593003/
[B] Allow us to autofill such password forms [this patch].
With this patch we autofill the password forms having fields that either
ambiguous or lacks names and id fields.
BUG=497630
TEST=PasswordManagerBrowserTestBase.
AutofillSuggetionsForPasswordFormWithoutNameOrIdAttribute
PasswordManagerBrowserTestBase.
AutofillSuggetionsForPasswordFormWithAmbiguousIdAttribute
PasswordManagerBrowserTestBase.
AutofillSuggetionsForChangePwdWithEmptyNames
PasswordManagerBrowserTestBase.
AutofillSuggetionsForChangePwdWithEmptyNamesAndAutocomplete
PasswordManagerBrowserTestBase.
AutofillSuggetionsForChangePwdWithEmptyNamesButOnlyNewPwdField
PasswordAutofillAgentTest.
SuggestionsOnFormContainingAmbiguousOrEmptyNames
Committed: https://crrev.com/7e2a984f49a0511533f2aabbc2e4be5107a8bb91
Cr-Commit-Position: refs/heads/master@{#350795}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Added Tests. #Patch Set 4 : Just a code rebase. #Patch Set 5 : Code restructured. #
Total comments: 22
Patch Set 6 : Incorporated Vaclav's changes. #
Total comments: 2
Patch Set 7 : Fixed browser tests. #
Total comments: 8
Patch Set 8 : Just a code rebase. #Patch Set 9 : Addresses Vadym's inputs. #
Total comments: 7
Patch Set 10 : Addresses Vadym's inputs. #
Total comments: 6
Patch Set 11 : Addresses Vadym's inputs. #Patch Set 12 : Added a browser_tests for suggestion popup. #
Total comments: 13
Patch Set 13 : Incorporated review inputs. #
Total comments: 4
Patch Set 14 : Incorporated Vaclav's Inputs. #
Total comments: 6
Patch Set 15 : Addresses Vadym's inputs. #Messages
Total messages: 50 (19 generated)
pritam.nikam@samsung.com changed reviewers: + vivek.vg@samsung.com, vivekg@chromium.org
On 2015/08/14 16:17:26, Pritam Nikam wrote: > mailto:pritam.nikam@samsung.com changed reviewers: > + mailto:vivek.vg@samsung.com, mailto:vivekg@chromium.org Hi Vivek, This patch is related to autofilling the password form having fields either ambiguous or no name or id attribute specified. Please have a look in spare time. Thanks!
pritam.nikam@samsung.com changed reviewers: + dvadym@chromium.org, vabr@chromium.org
Hi Vadym, with this patch we fill the password form having fields that either ambiguous (i.e. same name/id attributes for username & password fields) or lacks names and id attributes from the fields. Furthermore, it make use of dummy names assigned to fields and works irrespective of user prefer to store such password forms or not. Please have a look. Thanks!
Hi Pritam Nikam, I'm posting a couple of minor comments here. But then I realised that Vadym is already reviewing the related CL, and said there he would like to review both. So I did not finish the review, and will leave it to Vadym (unless he objects). Cheers, Vaclav https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:134: void FillAndSubmitPasswordForm( Please remove this function and use the store directly to save the credential, rather than doing that through JavaScript. (Search for AddLogin in this file to see examples of how to do that.) We should only use JavaScript and HTML interaction to test code, using it to prepare the test state will result in slower and more flaky tests. https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2163: // ambiguity in id attribute gets autofill correctly. nit: autofill -> autofilled https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2187: ASSERT_EQ("myusername", actual_username); EXPECT_EQ seems more appropriate here and on lines 2194, 2222, 2229, there is no need to abort the whole test if this check fails. https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2198: // name and id attribute gets autofill correctly. nit: autofill -> autofilled https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2198: // name and id attribute gets autofill correctly. nit: Please remove extra space between "attribute" and "gets". https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:60: const char kInputTypePassword[] = "password"; nit: You can just inline this constant in the helper function below. The type name will not change frequently, as it is part of the HTML norm. https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:102: bool ambiguousOrNoNameAndIdAttribute) { style: Please use lowercase_with_underscore, not camelCase. (Also below.) https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:116: // Helper function to get the password form control elements. On supplied with Please reformulate the second sentence. You probably mean something like: "If |field_name| is ... then only returns the fields with that name." Also, it is not clear, what is meant by "valid". Do you mean non-empty? https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:117: // valid |field_name| gets the elements matching it. You should comment on what the bool argument is used for (and possibly update the whole comment if it is outdated). https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:123: ambiguousOrNoNameAndIdAttribute Please use if-else here, do not create a value just to throw it out and only use its side-effects.
Patchset #6 (id:100001) has been deleted
Thanks Vaclav for review. I've addressed you inputs, please have a look. Thanks! https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:134: void FillAndSubmitPasswordForm( On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > Please remove this function and use the store directly to save the credential, > rather than doing that through JavaScript. (Search for AddLogin in this file to > see examples of how to do that.) > > We should only use JavaScript and HTML interaction to test code, using it to > prepare the test state will result in slower and more flaky tests. Done. But unfortunately that didn't work for me. With new patch set I've added necessary changes where we do |password_store->AddLogin()| to store passwords. Atop executing a javascript to retrieve form field values. I'm not able to figure out the problem. Any pointers here would be a great help. https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2163: // ambiguity in id attribute gets autofill correctly. On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > nit: autofill -> autofilled Done. https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2187: ASSERT_EQ("myusername", actual_username); On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > EXPECT_EQ seems more appropriate here and on lines 2194, 2222, 2229, there is no > need to abort the whole test if this check fails. Done. https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2198: // name and id attribute gets autofill correctly. On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > nit: autofill -> autofilled Done. https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:2198: // name and id attribute gets autofill correctly. On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > nit: Please remove extra space between "attribute" and "gets". Done. https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:60: const char kInputTypePassword[] = "password"; On 2015/08/26 09:36:01, vabr (slow start past holiday) wrote: > nit: You can just inline this constant in the helper function below. The type > name will not change frequently, as it is part of the HTML norm. Done. https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:102: bool ambiguousOrNoNameAndIdAttribute) { On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > style: Please use lowercase_with_underscore, not camelCase. (Also below.) Done. https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:116: // Helper function to get the password form control elements. On supplied with On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > Please reformulate the second sentence. You probably mean something like: "If > |field_name| is ... then only returns the fields with that name." Also, it is > not clear, what is meant by "valid". Do you mean non-empty? Done. valid -> non-empty and non-ambiguous. https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:117: // valid |field_name| gets the elements matching it. On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > You should comment on what the bool argument is used for (and possibly update > the whole comment if it is outdated). Done. https://codereview.chromium.org/1292693004/diff/80001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:123: ambiguousOrNoNameAndIdAttribute On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > Please use if-else here, do not create a value just to throw it out and only use > its side-effects. Done.
Hi Pritam Nikam. Thanks for addressing the comments. As I said, I'd like to keep this review to Vadym (I only realised that too late, that's why I posted the first comments). Cheers, Vaclav https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:134: void FillAndSubmitPasswordForm( On 2015/08/26 13:25:40, Pritam Nikam wrote: > On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > > Please remove this function and use the store directly to save the credential, > > rather than doing that through JavaScript. (Search for AddLogin in this file > to > > see examples of how to do that.) > > > > We should only use JavaScript and HTML interaction to test code, using it to > > prepare the test state will result in slower and more flaky tests. > > Done. > > But unfortunately that didn't work for me. With new patch set I've added > necessary changes where we do |password_store->AddLogin()| to store passwords. > Atop executing a javascript to retrieve form field values. I'm not able to > figure out the problem. Any pointers here would be a great help. I'm afraid I don't understand your sentence "Atop ... values," so I'm not sure I understand your problem here. If the problem is that the saved passwords are not filled, I suggest debugging the issue, you should be able to figure out if the passwords are not stored, the password forms are not matching enough, or if there is another failed check. https://codereview.chromium.org/1292693004/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:2222: static_cast<password_manager::TestPasswordStore*>( You don't need the cast and the TestPasswordStore interface, AddLogin is already present in PasswordStore.
Thanks Vaclav! I've addressed your review comments in new patch set. Hi Vadym, PTAL. Thanks! https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:134: void FillAndSubmitPasswordForm( On 2015/08/26 13:39:00, vabr (slow start past holiday) wrote: > On 2015/08/26 13:25:40, Pritam Nikam wrote: > > On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > > > Please remove this function and use the store directly to save the > credential, > > > rather than doing that through JavaScript. (Search for AddLogin in this file > > to > > > see examples of how to do that.) > > > > > > We should only use JavaScript and HTML interaction to test code, using it to > > > prepare the test state will result in slower and more flaky tests. > > > > Done. > > > > But unfortunately that didn't work for me. With new patch set I've added > > necessary changes where we do |password_store->AddLogin()| to store passwords. > > Atop executing a javascript to retrieve form field values. I'm not able to > > figure out the problem. Any pointers here would be a great help. > > I'm afraid I don't understand your sentence "Atop ... values," so I'm not sure I > understand your problem here. If the problem is that the saved passwords are not > filled, I suggest debugging the issue, you should be able to figure out if the > passwords are not stored, the password forms are not matching enough, or if > there is another failed check. Acknowledged. Able to debug the problem. We had 2 problems here: 1. |password_store->AddLogin()| asynchronously adds password form to store. 2. If we store login form having only |username_value|, |password_value| and |signon_realm| don't actually prefill fields (Ref [1]). Added fix for both the cases. [1]. https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor... https://codereview.chromium.org/1292693004/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:2222: static_cast<password_manager::TestPasswordStore*>( On 2015/08/26 13:39:01, vabr (slow start past holiday) wrote: > You don't need the cast and the TestPasswordStore interface, AddLogin is already > present in PasswordStore. Done.
On 2015/08/27 11:42:02, Pritam Nikam wrote: > Thanks Vaclav! I've addressed your review comments in new patch set. > > Hi Vadym, PTAL. > > Thanks! > > https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > https://codereview.chromium.org/1292693004/diff/80001/chrome/browser/password... > chrome/browser/password_manager/password_manager_browsertest.cc:134: void > FillAndSubmitPasswordForm( > On 2015/08/26 13:39:00, vabr (slow start past holiday) wrote: > > On 2015/08/26 13:25:40, Pritam Nikam wrote: > > > On 2015/08/26 09:36:00, vabr (slow start past holiday) wrote: > > > > Please remove this function and use the store directly to save the > > credential, > > > > rather than doing that through JavaScript. (Search for AddLogin in this > file > > > to > > > > see examples of how to do that.) > > > > > > > > We should only use JavaScript and HTML interaction to test code, using it > to > > > > prepare the test state will result in slower and more flaky tests. > > > > > > Done. > > > > > > But unfortunately that didn't work for me. With new patch set I've added > > > necessary changes where we do |password_store->AddLogin()| to store > passwords. > > > Atop executing a javascript to retrieve form field values. I'm not able to > > > figure out the problem. Any pointers here would be a great help. > > > > I'm afraid I don't understand your sentence "Atop ... values," so I'm not sure > I > > understand your problem here. If the problem is that the saved passwords are > not > > filled, I suggest debugging the issue, you should be able to figure out if the > > passwords are not stored, the password forms are not matching enough, or if > > there is another failed check. > > Acknowledged. > > Able to debug the problem. We had 2 problems here: > 1. |password_store->AddLogin()| asynchronously adds password form to store. > 2. If we store login form having only |username_value|, |password_value| and > |signon_realm| don't actually prefill fields (Ref [1]). > > Added fix for both the cases. Thanks for figuring out the problem with the test, Pritam Nikam!
Hi Pritam Nikam, 1.After landing your CL with saving of forms with empty username and password fields the name of fields in PasswordFormFillData for such fields would be "anonymous_username" and "anonymous_password" (since here they are set up https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... ) so checking in PasswordFormWithAmbiguousOrNoNameAndIdAttribute doesn't cover empty name/id case. 2.Current matching algo for such forms will cover only forms with not more than 1 username field and not more then 1 password field, so for example it will not cover password change forms (they usually have >= 2 password fields), I think for ambiguous for no id case it's ok to choose the first username and the first password field (ideally it would be to choose the last username field before the first password field, the same way as we do it in password_form_conversion_utils.cc, but I'm afraid to make this code too complex). https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:88: return fill_data.username_field.name == fill_data.password_field.name; It seems this covers only a case of the same name for username and password fields, not a case of absence name and id attributes for username and password fields. https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:108: return element.hasHTMLTagName("input") && element.isTextField() && I think that you can skip checking for "input" html tag. It's already input element (that suggests Input in WebInputElement). And I'd suggest not to make extract this code in separate function. https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:150: input_elements[i].to<blink::WebInputElement>(); You need to check that this is WebInputElement before converting to WebInputElement
Thanks Vadym for inputs :-) I've incorporated review comments along new patch set, ptal. Thanks! > Hi Pritam Nikam, > > 1.After landing your CL with saving of forms with empty username and password > fields the > name of fields in PasswordFormFillData for such fields would be > "anonymous_username" and "anonymous_password" (since here they are set up > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > ) so checking in PasswordFormWithAmbiguousOrNoNameAndIdAttribute doesn't cover > empty name/id case. Actually, plan was to keep it independent of other patch i.e. to store empty name/id attribute patch (https://codereview.chromium.org/1286593003/). And once we land that patch for storing password form having empty name/id field attributes, we can extend logic here. > 2.Current matching algo for such forms will cover only forms with not more than > 1 username field > and not more then 1 password field, so for example it will not cover password > change forms (they usually have >= 2 password fields), I think for ambiguous for > no id case it's ok to choose the first username and the first password field > (ideally it would be to choose the last username field before the first password > field, the same way as we do it in password_form_conversion_utils.cc, but I'm > afraid to make this code too complex). Yes, you are right! It’s difficult to handle it right now. But we can work on it after we start tagging fields with dummy names. https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:88: return fill_data.username_field.name == fill_data.password_field.name; On 2015/08/28 16:06:02, dvadym wrote: > It seems this covers only a case of the same name for username and password > fields, not a case of absence name and id attributes for username and password > fields. Actually, I was planning to keep it independent of other patch i.e. to store empty name/id attribute patch (https://codereview.chromium.org/1286593003/). So, keep it |fill_data.username_field.name == fill_data.password_field.name| shall cover our both cases: 1. Empty name/id -> |empty-string == empty-string| 2. Ambiguous name/id -> |samename == samename| I've added browser tests to cover these scenarios as well: A. AutofillSuggetionsForPasswordFormWithoutNameOrIdAttribute B. AutofillSuggetionsForPasswordFormWithAmbiguousIdAttribute I've added a "TODO:" mentioning to change it once we land patch for storing password form having empty name/id field attributes. https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:108: return element.hasHTMLTagName("input") && element.isTextField() && On 2015/08/28 16:06:02, dvadym wrote: > I think that you can skip checking for "input" html tag. It's already input > element (that suggests Input in WebInputElement). And I'd suggest not to make > extract this code in separate function. Done. https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:150: input_elements[i].to<blink::WebInputElement>(); On 2015/08/28 16:06:02, dvadym wrote: > You need to check that this is WebInputElement before converting to > WebInputElement Done. We can safely ignore this check |control_elements[i].hasHTMLTagName("input")| as with recent base code this has been handled already.
vabr@chromium.org changed reviewers: - vabr@chromium.org
Hi Pritam Nikam, please find below my comments. Thanks, Vadym https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:150: input_elements[i].to<blink::WebInputElement>(); On 2015/09/01 13:40:25, Pritam Nikam wrote: > On 2015/08/28 16:06:02, dvadym wrote: > > You need to check that this is WebInputElement before converting to > > WebInputElement > > Done. > > We can safely ignore this check |control_elements[i].hasHTMLTagName("input")| as > with recent base code this has been handled already. Could you please point out where these changes were made, that now we have only input elements here? https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:93: // http://crbug.com/497630 Please rebase on the top of save patch and implement this method. To make branch B depend on A (so that in the end the changes in B are the difference of the changes in B originally minus changes in A): git checkout B git branch -u A git pull --rebase https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:105: return ambiguous_or_no_name_and_id_attribute Is the case of no-username field considered here? It seems to be that in such case "anonymous_username" will be returned, or did I miss something? https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:135: if (found_input) { Could you please allow case when there are more than 1 password field when |ambiguous_or_no_name_and_id_attribute| == true, it will allow to support change password forms. In this case the first password field should be taken.
Patchset #10 (id:200001) has been deleted
Patchset #10 (id:220001) has been deleted
Thanks! Vadym. I've incorporated inputs along new patch set, ptal. Thanks! https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/140001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:150: input_elements[i].to<blink::WebInputElement>(); On 2015/09/07 15:26:00, dvadym wrote: > On 2015/09/01 13:40:25, Pritam Nikam wrote: > > On 2015/08/28 16:06:02, dvadym wrote: > > > You need to check that this is WebInputElement before converting to > > > WebInputElement > > > > Done. > > > > We can safely ignore this check |control_elements[i].hasHTMLTagName("input")| > as > > with recent base code this has been handled already. > > Could you please point out where these changes were made, that now we have only > input elements here? My mistake, I've missed the check for <textarea> and <select>. Corrected in new patch set. https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:93: // http://crbug.com/497630 On 2015/09/07 15:26:00, dvadym wrote: > Please rebase on the top of save patch and implement this method. > > To make branch B depend on A (so that in the end the changes in B are the > difference of the changes in B originally minus changes in A): > > git checkout B > git branch -u A > git pull --rebase Done. https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:105: return ambiguous_or_no_name_and_id_attribute On 2015/09/07 15:26:00, dvadym wrote: > Is the case of no-username field considered here? It seems to be that in such > case "anonymous_username" will be returned, or did I miss something? For the case of no-username field, |ambiguous_or_no_name_and_id_attribute| will be false. And we will return an empty string instead. https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:135: if (found_input) { On 2015/09/07 15:26:00, dvadym wrote: > Could you please allow case when there are more than 1 password field when > |ambiguous_or_no_name_and_id_attribute| == true, it will allow to support change > password forms. In this case the first password field should be taken. Done.
Hi Pritam Nikam, Please find my nits. Currently CL looks good. Sorry I've realized only today that another one thing left, we need to implement support of no id, no name fields on showing suggestion. Please find a comment in code which points out to "if (element...", in this line we bail out and not to show a suggestion. It should be a small change to allow showing suggestions. Best regards, Vadym https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/180001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:105: return ambiguous_or_no_name_and_id_attribute On 2015/09/08 15:56:19, Pritam Nikam wrote: > On 2015/09/07 15:26:00, dvadym wrote: > > Is the case of no-username field considered here? It seems to be that in such > > case "anonymous_username" will be returned, or did I miss something? > > For the case of no-username field, |ambiguous_or_no_name_and_id_attribute| will > be false. And we will return an empty string instead. Acknowledged https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:139: // For change password form keep only the first password field entry. Could you please test case when we have more than one password and no id/name attributes? https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:140: FormInputElementMap::const_iterator old_entry = result->find(field_name); It seems that we don't need to make look-up in a map. Since if we already found this element then found_input = true and the names of username and password fields should be different since you've implemented this. https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:708: if (element.nameForAutofill().isEmpty()) We need to consider how to correctly show password suggestion in our case. FYI this line moved in another CL to method ShowSuggestions, but it didn't change anything.
Patchset #11 (id:260001) has been deleted
Thanks Vadym for inputs. I've incorporated in new patch set, ptal. Thanks! https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:139: // For change password form keep only the first password field entry. On 2015/09/11 14:15:34, dvadym wrote: > Could you please test case when we have more than one password and no id/name > attributes? Done. Added a test: PasswordManagerBrowserTestBase.AutofillSuggetionsForChangePasswordFormWithoutNameOrIdAttribute https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:140: FormInputElementMap::const_iterator old_entry = result->find(field_name); On 2015/09/11 14:15:34, dvadym wrote: > It seems that we don't need to make look-up in a map. Since if we already found > this element then found_input = true and the names of username and password > fields should be different since you've implemented this. Done. https://codereview.chromium.org/1292693004/diff/240001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:708: if (element.nameForAutofill().isEmpty()) On 2015/09/11 14:15:34, dvadym wrote: > We need to consider how to correctly show password suggestion in our case. FYI > this line moved in another CL to method ShowSuggestions, but it didn't change > anything. Done.
The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run
The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292693004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292693004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Hi Pritam Nikam, CL looks good. Could you please add test that suggestions popup shown? It can be done in password_autofill_agent_browsertest.cc, see for example TEST_F(PasswordAutofillAgentTest, ShowPopupOnEmptyPasswordField) Thanks, Vadym
On 2015/09/17 16:27:19, dvadym wrote: > Hi Pritam Nikam, > > CL looks good. > > Could you please add test that suggestions popup shown? It can be done in > password_autofill_agent_browsertest.cc, see for example > TEST_F(PasswordAutofillAgentTest, ShowPopupOnEmptyPasswordField) > > Thanks, > Vadym + Vaclav Thanks Vadym, As suggested, I've added a browser test, ptal. Hi Vaclav, Have a owner's review @ chrome/renderer/autofill/password_autofill_agent_browsertest.cc Thanks!
pritam.nikam@samsung.com changed reviewers: + vabr@chromium.org
vabr@chromium.org: Please review changes in chrome/renderer/autofill/password_autofill_agent_browsertest.cc Thanks!
Hi Pritaim Nikam! Thank you and Vadym for your work on this. It looks good to me, my only bigger concern is the support of change-password forms: This fills the first password form on such forms. We should at least make it recognise the autocomplete="current-password" attribute, and fill first such field, if given. And we should have tests to document this in particular. Otherwise LGTM. Cheers, Vaclav https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2090: SuggestionsOnSubmittingPasswordFormHavingFieldsWithAmbiguousOrNoNameOrIdAttributes) { nit: This name is a bit too long. :) You might want to shorten it and add a comment with the details about what the test does. https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2102: " <INPUT type='text' placeholder='username'/>" Again, this looks like a sign-up form. I would expect the change-password form to have an old password and a new password. The username is optional and so is the confirmation of the new password. I defer to Vadym to chose the best type (and if he already chose this one, then just ignore me here and in the other test). https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2140: for (size_t i = 0; i < arraysize(test_cases); ++i) { You can spare yourself the arraysize if you write: for (const auto& test_case : test_cases) {...} https://codereview.chromium.org/1292693004/diff/300001/chrome/test/data/passw... File chrome/test/data/password/ambiguous_password_form.html (right): https://codereview.chromium.org/1292693004/diff/300001/chrome/test/data/passw... chrome/test/data/password/ambiguous_password_form.html:18: <label>Username: </label> <input type="text" placeholder="username" /> This looks like a sign-up form, not a change password form. https://codereview.chromium.org/1292693004/diff/300001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/300001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:91: bool PasswordFormWithAmbiguousOrNoNameAndIdAttribute( Usually, predicates are named as the questions they answer. Here, I suggest: DoesFormContainAmbiguousOrEmptyNames https://codereview.chromium.org/1292693004/diff/300001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:105: // Returns the |field|'s autofillable name. If no name or id attribute is The second sentence should reference the bool argument instead of its expected meaning.
Thanks Vaclav, good point about autocomplete attribute. I've checked code, in case when we have change password forms without current-password and username we receive PasswordFormFillData in PasswordAutofllAgent with empty both username and password elements. And PasswordFormWithAmbiguousOrNoNameAndIdAttribute returns true, that isn't correct. So it seems that the only fix that we need in order to make it works correctly to add check in PasswordFormWithAmbiguousOrNoNameAndIdAttribute that password_field.name != "" (but of course software is complex, probably there are other nuances). Pritam Nikam: could you please add test with autocomplete attributes? https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2102: " <INPUT type='text' placeholder='username'/>" On 2015/09/18 14:41:38, vabr (Chromium) wrote: > Again, this looks like a sign-up form. > I would expect the change-password form to have an old password and a new > password. The username is optional and so is the confirmation of the new > password. > > I defer to Vadym to chose the best type (and if he already chose this one, then > just ignore me here and in the other test). A password change form may or may not contain old password. So the current HTML corresponds to the form which we consider to be password change form. But I agree it would be more clear if the fields have name with reference to old and new password.
Thanks Vaclav & Vadym for inputs. I've addressed these along new patch set, PTAL. Thanks! https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2090: SuggestionsOnSubmittingPasswordFormHavingFieldsWithAmbiguousOrNoNameOrIdAttributes) { On 2015/09/18 14:41:38, vabr (Chromium) wrote: > nit: This name is a bit too long. :) You might want to shorten it and add a > comment with the details about what the test does. Done. https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2102: " <INPUT type='text' placeholder='username'/>" On 2015/09/18 15:59:00, dvadym wrote: > On 2015/09/18 14:41:38, vabr (Chromium) wrote: > > Again, this looks like a sign-up form. > > I would expect the change-password form to have an old password and a new > > password. The username is optional and so is the confirmation of the new > > password. > > > > I defer to Vadym to chose the best type (and if he already chose this one, > then > > just ignore me here and in the other test). > > A password change form may or may not contain old password. So the current HTML > corresponds to the form which we consider to be password change form. But I > agree it would be more clear if the fields have name with reference to old and > new password. Done. Added attributes |autocomplete='current-password'| for old password fields and |autocomplete='new-password'| for new password fields. Also added related tests, ptal. https://codereview.chromium.org/1292693004/diff/300001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2140: for (size_t i = 0; i < arraysize(test_cases); ++i) { On 2015/09/18 14:41:38, vabr (Chromium) wrote: > You can spare yourself the arraysize if you write: > > for (const auto& test_case : test_cases) {...} Done. https://codereview.chromium.org/1292693004/diff/300001/chrome/test/data/passw... File chrome/test/data/password/ambiguous_password_form.html (right): https://codereview.chromium.org/1292693004/diff/300001/chrome/test/data/passw... chrome/test/data/password/ambiguous_password_form.html:18: <label>Username: </label> <input type="text" placeholder="username" /> On 2015/09/18 14:41:38, vabr (Chromium) wrote: > This looks like a sign-up form, not a change password form. Done. https://codereview.chromium.org/1292693004/diff/300001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/300001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:91: bool PasswordFormWithAmbiguousOrNoNameAndIdAttribute( On 2015/09/18 14:41:38, vabr (Chromium) wrote: > Usually, predicates are named as the questions they answer. Here, I suggest: > > DoesFormContainAmbiguousOrEmptyNames Done. https://codereview.chromium.org/1292693004/diff/300001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:105: // Returns the |field|'s autofillable name. If no name or id attribute is On 2015/09/18 14:41:38, vabr (Chromium) wrote: > The second sentence should reference the bool argument instead of its expected > meaning. Done.
Patchset #13 (id:320001) has been deleted
Hi Pritam Nikam! I think I need a bit more clarification (see the longer comment below). Thanks, Vaclav https://codereview.chromium.org/1292693004/diff/340001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/340001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2088: // Tests that credential suggestions are getting autofill on a password (and nit: getting autofill -> autofilled (Or did you mean something else?) https://codereview.chromium.org/1292693004/diff/340001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/340001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:168: // For change password form with ambiguous or empty names keep only the There might be a misunderstanding I caused. What I meant was: If there is an autocomplete='current-password' attribute on some password field, return the first such field. If no password field has this attribute set, then continue returning the first password field, as in the patch set before. Do I understand correctly that the code as it is now does not find the password field at all unless it has the autocomplete attribute? In any case, this should be also captured in the tests in that both HTML code with autocomplete attributes, and without them is tested.
Patchset #14 (id:360001) has been deleted
Patchset #14 (id:380001) has been deleted
Thanks Vaclav for review inputs. I've incorporated review comments along new patch set, ptal. Thanks! https://codereview.chromium.org/1292693004/diff/340001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/1292693004/diff/340001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:2088: // Tests that credential suggestions are getting autofill on a password (and On 2015/09/23 11:23:24, vabr (Chromium) wrote: > nit: getting autofill -> autofilled > (Or did you mean something else?) Done. https://codereview.chromium.org/1292693004/diff/340001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/340001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:168: // For change password form with ambiguous or empty names keep only the On 2015/09/23 11:23:24, vabr (Chromium) wrote: > There might be a misunderstanding I caused. What I meant was: > If there is an autocomplete='current-password' attribute on some password field, > return the first such field. > If no password field has this attribute set, then continue returning the first > password field, as in the patch set before. > > Do I understand correctly that the code as it is now does not find the password > field at all unless it has the autocomplete attribute? > > In any case, this should be also captured in the tests in that both HTML code > with autocomplete attributes, and without them is tested. Done.
Hi Pritam Nikam, LGTM with comments. Regards, Vadym https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:107: bool HasAutocompleteCurrentPasswordAttribute( Please use function HasAutocompleteAttributeValue from password_form_conversion_utils.cc (it's needed to add it definition to password_form_conversion_utils.h). https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:114: bool HasAutocompleteNewPasswordAttribute( Please use function HasAutocompleteAttributeValue from password_form_conversion_utils.cc https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:161: HasPasswordWithAutocompleteAttribute(control_elements); Here I'm concerned about perfomance a little bit. We don't need to call this function when ambiguous_or_empty_names == false. Probably it's better to rename this variable something like ambiguous_and_multiple_password_field_with_autocomplete and write something like this bool ambiguous_and_multiple_password_field_with_autocomplete = ambiguous_or_empty_names && HasP...;
Patchset #15 (id:410001) has been deleted
Thanks Vadym for inputs. I've incorporated these in new patch set, ptal. Thanks! https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:107: bool HasAutocompleteCurrentPasswordAttribute( On 2015/09/24 15:43:17, dvadym wrote: > Please use function HasAutocompleteAttributeValue from > password_form_conversion_utils.cc (it's needed to add it definition to > password_form_conversion_utils.h). Done. https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:114: bool HasAutocompleteNewPasswordAttribute( On 2015/09/24 15:43:17, dvadym wrote: > Please use function HasAutocompleteAttributeValue from > password_form_conversion_utils.cc Done. https://codereview.chromium.org/1292693004/diff/400001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:161: HasPasswordWithAutocompleteAttribute(control_elements); On 2015/09/24 15:43:17, dvadym wrote: > Here I'm concerned about perfomance a little bit. We don't need to call this > function when ambiguous_or_empty_names == false. Probably it's better to rename > this variable something like > ambiguous_and_multiple_password_field_with_autocomplete and write something like > this > bool ambiguous_and_multiple_password_field_with_autocomplete = > ambiguous_or_empty_names && HasP...; Done.
The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292693004/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292693004/430001
LGTM Thanks!
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 pritam.nikam@samsung.com
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/1292693004/#ps430001 (title: "Addresses Vadym's inputs.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1292693004/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1292693004/430001
Message was sent while issue was closed.
Committed patchset #15 (id:430001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/7e2a984f49a0511533f2aabbc2e4be5107a8bb91 Cr-Commit-Position: refs/heads/master@{#350795} |
