Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
120343003:
Autofill popup should not be presented when autocomplete='off', even if (Closed)
|
Created:
7 years ago by jww Modified:
6 years, 11 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAutofill popup should not be presented when autocomplete='off', even if
there already is information in the autofill DB.
BUG=326679
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243841
Patch Set 1 #Patch Set 2 : Added browser test #
Total comments: 13
Patch Set 3 : Refactoring and fixes as per comments #
Total comments: 11
Patch Set 4 : Test and comment improvements #
Total comments: 12
Patch Set 5 : Fixes of nits from isherman #
Messages
Total messages: 18 (0 generated)
I think this fixes the (very minor) problem that if an autocomplete='off' form is hit, but for whatever reason we have data in the autofill DB for this form, we should *not* offer a suggestion popup for it.
Please add test coverage for this change.
Browser test added.
https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:814: TEST_F(PasswordAutofillAgentTest, SelectUsernameOnAutofillOff) { nit: "On" -> "When" or "With" or something like that, so that it doesn't look like it's contrasting with "Off" https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:817: WebString::fromUTF8("off")); It looks like the code refuses to show suggestions if autocomplete is off for *either* the password field or the username field. Right now, though, you're only testing the case where the password field disables autocomplete. Please add test coverage for the username case as well. https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:850: EXPECT_TRUE(SimulateShowSuggestions()); Please also test that no AutofillHostMsg_ShowPasswordSuggestions message is generated for a known username (and I guess make sure that it had been being generated prior to this CL). That seems like a more direct test of the property you're trying to verify. https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:328: !form_data.passwordShouldAutocomplete)) Why do you need both "!password_form->password_autocomplete_set" and "!form_data.passwordShouldAutocomplete"? It seems like all that matters is the latter one -- whether autocomplete is *currently* disabled for the actual HTML element. If autocomplete had previously been disabled, but then got enabled by the time we get here, I think it should be fine to show the popup. Is there an interesting case that I'm overlooking?
Currently on vacation, so not going to be responding very fast. https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:324: blink::WebPasswordFormData form_data(element.form()); You also need to verify that element.form() is valid. It doesn't look like this is checked previously in the call chain and it's possible to have input elements that aren't in a form. https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:328: !form_data.passwordShouldAutocomplete)) passwordShouldAutocomplete implies password_autocomplete_set, so there is no reason to check both. It seems like the right thing to do is move this seciton after the iterator check. Then you check if both the password field (iter->second.password_field) and the username field are fillable in the same manner that FillUserNameAndPassword does. You might want to factor out the code that checks that into a different function, as it does seem to get a lot of use (IsElementEditable && ShouldIgnore...).
I think I've addressed all of the comments, expect for the one question I've let for isherman. https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:814: TEST_F(PasswordAutofillAgentTest, SelectUsernameOnAutofillOff) { On 2013/12/27 21:43:22, Ilya Sherman (Away thru Jan 6) wrote: > nit: "On" -> "When" or "With" or something like that, so that it doesn't look > like it's contrasting with "Off" Done. https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:817: WebString::fromUTF8("off")); On 2013/12/27 21:43:22, Ilya Sherman (Away thru Jan 6) wrote: > It looks like the code refuses to show suggestions if autocomplete is off for > *either* the password field or the username field. Right now, though, you're > only testing the case where the password field disables autocomplete. Please > add test coverage for the username case as well. Done. https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:850: EXPECT_TRUE(SimulateShowSuggestions()); That's a great idea. I didn't attempt it because I thought there was no way to access the browser process from the browser test to check if an IPC handler fired (or, even harder, if it didn't fire). Your suggestion, though, of checking for the creation (or lack thereof) of the message makes a lot of sense, though. Do you have any pointers on how to do this (or perhaps an example browser test that does this)? On 2013/12/27 21:43:22, Ilya Sherman (Away thru Jan 6) wrote: > Please also test that no AutofillHostMsg_ShowPasswordSuggestions message is > generated for a known username (and I guess make sure that it had been being > generated prior to this CL). That seems like a more direct test of the property > you're trying to verify. https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:324: blink::WebPasswordFormData form_data(element.form()); On 2013/12/27 22:09:48, Garrett Casto wrote: > You also need to verify that element.form() is valid. It doesn't look like this > is checked previously in the call chain and it's possible to have input elements > that aren't in a form. I believe this is irrelevant given my changes per your comment below. Please correct me if I'm wrong. https://codereview.chromium.org/120343003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:328: !form_data.passwordShouldAutocomplete)) On 2013/12/27 22:09:48, Garrett Casto wrote: > passwordShouldAutocomplete implies password_autocomplete_set, so there is no > reason to check both. > > It seems like the right thing to do is move this seciton after the iterator > check. Then you check if both the password field (iter->second.password_field) > and the username field are fillable in the same manner that > FillUserNameAndPassword does. You might want to factor out the code that checks > that into a different function, as it does seem to get a lot of use > (IsElementEditable && ShouldIgnore...). Done.
https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:850: EXPECT_TRUE(SimulateShowSuggestions()); I think something like this ought to work: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au... If that doesn't quite help enough on its own, you might try a search for something like "file:autofill file:renderer ipc sink" on https://code.google.com/p/chromium/codesearch On 2013/12/27 23:04:39, jww wrote: > That's a great idea. I didn't attempt it because I thought there was no way to > access the browser process from the browser test to check if an IPC handler > fired (or, even harder, if it didn't fire). > > Your suggestion, though, of checking for the creation (or lack thereof) of the > message makes a lot of sense, though. Do you have any pointers on how to do this > (or perhaps an example browser test that does this)? > On 2013/12/27 21:43:22, Ilya Sherman (Away thru Jan 6) wrote: > > Please also test that no AutofillHostMsg_ShowPasswordSuggestions message is > > generated for a known username (and I guess make sure that it had been being > > generated prior to this CL). That seems like a more direct test of the > property > > you're trying to verify. > https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:257: void SimulateShowSuggestionsWithAutocompleteOff(WebInputElement& element) { Please document this method. https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:263: SimulateUsernameChange("foobar", true); IMO, lines 258-263 should be kept in the individual test cases, especially since I think it's important to test known usernames as well as unknown ones (known ones, I'd expect, are the ones that would have previously shown a suggestions popup). The remaining lines I would recommend keeping factored out, since the comment is long. For those, I would name the method containing them something starting with "Expect", e.g. "ExpectNoSuggestionsPopup()". https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:853: ClearUsernameAndPasswordFields(); Please make a separate test case (TEST_F invocation), rather than resetting state within the test. Shorter test cases are easier to keep simple and correct, and also tend to be clearer. https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:185: bool IsElementAutocompletable(const blink::WebInputElement& element) { nit: Docs, please. https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:334: // suggestion dialog, so we return "true," prentending we've done so. nit: "prentending" -> "pretending" https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:334: // suggestion dialog, so we return "true," prentending we've done so. nit: Please avoid the pronoun "we" in comments (actually, try to avoid pronouns as much as possible), because the referent can be unclear. In this case, I'd suggest rephrasing the comment to something like """ If autocomplete='off' is set on the form elements, no suggestion dialog should be shown. However, return |true| to indicate that this is a known password form and that the request to show suggestions has been handled (as a no-op). """
https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/60001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:850: EXPECT_TRUE(SimulateShowSuggestions()); On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote: > I think something like this ought to work: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/au... > > If that doesn't quite help enough on its own, you might try a search for > something like "file:autofill file:renderer ipc sink" on > https://code.google.com/p/chromium/codesearch > > On 2013/12/27 23:04:39, jww wrote: > > That's a great idea. I didn't attempt it because I thought there was no way to > > access the browser process from the browser test to check if an IPC handler > > fired (or, even harder, if it didn't fire). > > > > Your suggestion, though, of checking for the creation (or lack thereof) of the > > message makes a lot of sense, though. Do you have any pointers on how to do > this > > (or perhaps an example browser test that does this)? > > On 2013/12/27 21:43:22, Ilya Sherman (Away thru Jan 6) wrote: > > > Please also test that no AutofillHostMsg_ShowPasswordSuggestions message is > > > generated for a known username (and I guess make sure that it had been being > > > generated prior to this CL). That seems like a more direct test of the > > property > > > you're trying to verify. > > > Done. https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:257: void SimulateShowSuggestionsWithAutocompleteOff(WebInputElement& element) { On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote: > Please document this method. Done. https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:263: SimulateUsernameChange("foobar", true); Because of the nature of the test setup, it actually doesn't matter if the usernames are known are unknown; in either case, the show suggestions message will be sent. Just to clean things up, I actually changed this to use known usernames. On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote: > IMO, lines 258-263 should be kept in the individual test cases, especially since > I think it's important to test known usernames as well as unknown ones (known > ones, I'd expect, are the ones that would have previously shown a suggestions > popup). The remaining lines I would recommend keeping factored out, since the > comment is long. For those, I would name the method containing them something > starting with "Expect", e.g. "ExpectNoSuggestionsPopup()". Done. https://codereview.chromium.org/120343003/diff/140001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:853: ClearUsernameAndPasswordFields(); On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote: > Please make a separate test case (TEST_F invocation), rather than resetting > state within the test. Shorter test cases are easier to keep simple and > correct, and also tend to be clearer. Done. https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:185: bool IsElementAutocompletable(const blink::WebInputElement& element) { On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote: > nit: Docs, please. Done. https://codereview.chromium.org/120343003/diff/140001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:334: // suggestion dialog, so we return "true," prentending we've done so. On 2013/12/27 23:17:09, Ilya Sherman (Away thru Jan 6) wrote: > nit: "prentending" -> "pretending" Done.
Thanks. LGTM with nits addressed: https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:262: // password and username. Normally, if a unknown username is enterted, the nit: "a unknown" -> "an unknown" https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:264: // we're expecting "true" here. This sentence doesn't make sense anymore, since the tested username is known, rather than unknown. Please update this comment. (Maybe it's worth testing with an unknown username as well? Up to you.) https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:287: // And as a final sanity check, we also make sure that no "show suggestions" nit: s/"we"// https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:290: AutofillHostMsg_ShowPasswordSuggestions::ID) == NULL); nit: I'd write this as either EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching( AutofillHostMsg_ShowPasswordSuggestions::ID)); or as an EXPECT_EQ. EXPECT_TRUE(a == b) is an unusual pattern. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:844: // The following two tests are regression tests for http://crbug.com/326679 nit: Please have one comment per test, in case tests move around or whatnot. It's fine if both comments are something very brief like "Regression test for http://crbug.com/326679" https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:849: // Set the main password element to autocomplete='off' nit: Please update this comment.
Thanks, isherman! gcasto, I'll still wait for your LGTM to double check this. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:262: // password and username. Normally, if a unknown username is enterted, the On 2014/01/07 00:56:23, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: "a unknown" -> "an unknown" Done. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:264: // we're expecting "true" here. On 2014/01/07 00:56:23, Ilya Sherman (Away Jan11-Feb2) wrote: > This sentence doesn't make sense anymore, since the tested username is known, > rather than unknown. Please update this comment. (Maybe it's worth testing > with an unknown username as well? Up to you.) Done. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:287: // And as a final sanity check, we also make sure that no "show suggestions" On 2014/01/07 00:56:23, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: s/"we"// Done. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:290: AutofillHostMsg_ShowPasswordSuggestions::ID) == NULL); On 2014/01/07 00:56:23, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: I'd write this as either > > EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching( > AutofillHostMsg_ShowPasswordSuggestions::ID)); > > or as an EXPECT_EQ. EXPECT_TRUE(a == b) is an unusual pattern. Done. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:844: // The following two tests are regression tests for http://crbug.com/326679 On 2014/01/07 00:56:23, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please have one comment per test, in case tests move around or whatnot. > It's fine if both comments are something very brief like "Regression test for > http://crbug.com/326679%22 Done. https://codereview.chromium.org/120343003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:849: // Set the main password element to autocomplete='off' On 2014/01/07 00:56:23, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please update this comment. Done.
The only thing that I noticed is that autocomplete=off at the form level won't be respected. I thought that we did check this somewhere, but I must have been mistaken or it must have been removed in a refactoring. I'm not sure if you want to deal with this now or in a separate CL, I think that I'm fine either way.
On 2014/01/08 23:31:25, Garrett Casto wrote: > The only thing that I noticed is that autocomplete=off at the form level won't > be respected. I thought that we did check this somewhere, but I must have been > mistaken or it must have been removed in a refactoring. I'm not sure if you > want to deal with this now or in a separate CL, I think that I'm fine either > way. Elements inherit the autocomplete attributes of their enclosing <form> if they don't specify an autocomplete attribute themselves. Does that address your concern, or is there a codepath that you can think of where autocomplete="off" at the form level would still not be respected?
On 2014/01/08 23:36:21, Ilya Sherman (Away Jan11-Feb2) wrote: > On 2014/01/08 23:31:25, Garrett Casto wrote: > > The only thing that I noticed is that autocomplete=off at the form level won't > > be respected. I thought that we did check this somewhere, but I must have been > > mistaken or it must have been removed in a refactoring. I'm not sure if you > > want to deal with this now or in a separate CL, I think that I'm fine either > > way. > > Elements inherit the autocomplete attributes of their enclosing <form> if they > don't specify an autocomplete attribute themselves. Does that address your > concern, or is there a codepath that you can think of where autocomplete="off" > at the form level would still not be respected? LGTM Nope, that's good.
On 2014/01/09 00:36:01, Garrett Casto wrote: > On 2014/01/08 23:36:21, Ilya Sherman (Away Jan11-Feb2) wrote: > > On 2014/01/08 23:31:25, Garrett Casto wrote: > > > The only thing that I noticed is that autocomplete=off at the form level > won't > > > be respected. I thought that we did check this somewhere, but I must have > been > > > mistaken or it must have been removed in a refactoring. I'm not sure if you > > > want to deal with this now or in a separate CL, I think that I'm fine either > > > way. > > > > Elements inherit the autocomplete attributes of their enclosing <form> if they > > don't specify an autocomplete attribute themselves. Does that address your > > concern, or is there a codepath that you can think of where autocomplete="off" > > at the form level would still not be respected? > > LGTM > > Nope, that's good. Great, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/120343003/240001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/120343003/240001
Message was sent while issue was closed.
Change committed as 243841 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
