Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
56653002:
[Password Autofill] Make better use of prefilled usernames. (Closed)
|
Created:
7 years, 1 month ago by Garrett Casto Modified:
7 years, 1 month ago Reviewers:
Ilya Sherman 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, Ilya Sherman, rouslan+autofillwatch_chromium.org, vabr (Chromium), Patrick Dubroy Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Password Autofill] Make better use of prefilled usernames.
If a username field is prefilled, make sure to check all possible usernames
to see if we can fill instead of only filling with the preferred username if
it matches.
BUG=309817
R=isherman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232928
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 27
Patch Set 4 : Comments #
Total comments: 4
Patch Set 5 : more comments #
Total comments: 7
Patch Set 6 : #
Messages
Total messages: 11 (0 generated)
Sorry, I've been in M32 crunch mode today. I should have time to take a look at this on Monday -- feel free to ping the heck out of me if I don't :)
On 2013/11/02 02:05:53, Ilya Sherman wrote: > Sorry, I've been in M32 crunch mode today. I should have time to take a look at > this on Monday -- feel free to ping the heck out of me if I don't :) Yeah, this is also a change I'm trying to get into M32. Hopefully it should be a relatively short review.
https://codereview.chromium.org/56653002/diff/50001/chrome/renderer/autofill/... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/56653002/diff/50001/chrome/renderer/autofill/... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:358: // we know about, we leave the field untouched. Please write separate tests cases for the two behaviors you're testing here. (This is a recommendation from a recent doc on testing that I read, which I think is actually a fantastic idea, as it makes for shorter and clearer individual tests.) https://codereview.chromium.org/56653002/diff/50001/chrome/renderer/autofill/... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:378: // having a matching username will allow autofilling. Please write a separate test case for having a matching username. (How is this test different from the one above?) https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:500: username_element, password_element); nit: Please use curly braces to enclose the if-stmt body, since it spans multiple lines. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:613: username_element.setValue(fill_data.basic_data.fields[0].value); What happened to setAutofilled(), dispatchFormControlChangeEvent(), and tkent's TODO? (This applies to both the username, which is being set here, and the password, which is being set below. Might be worthwhile to decompose a helper function to autofill an element.) https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:618: true, false); nit: The semantics of "true" and "false" are really hard to make out here. If you're feeling particularly motivated, you could convert the API to accept enumerated values for these parameters rather than booleans. But if not, please at least document them inline as "true /* exact_usernames_match */" or by using a local named variable like "bool exact_usernames_match = true;". https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:673: username_element->setValue(username); Is this safe for a potentially non-modifiable element? Doesn't seem like it ought to be. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:680: SetElementAutofilled(username_element, true); Likewise, is this safe for a potentially non-modifiable element? https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.h:112: // prefilled value we will try and fill the password for that username nit: "try and" -> "try to", or just "fill", depending on whether the filling is certain or not. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.h:113: // instead. nit: Please try to rewrite this comment without using the pronoun "we", which is discouraged as the referent can be hard to infer.
https://codereview.chromium.org/56653002/diff/50001/chrome/renderer/autofill/... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/56653002/diff/50001/chrome/renderer/autofill/... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:358: // we know about, we leave the field untouched. On 2013/11/04 23:10:48, Ilya Sherman wrote: > Please write separate tests cases for the two behaviors you're testing here. > (This is a recommendation from a recent doc on testing that I read, which I > think is actually a fantastic idea, as it makes for shorter and clearer > individual tests.) Done. https://codereview.chromium.org/56653002/diff/50001/chrome/renderer/autofill/... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:378: // having a matching username will allow autofilling. On 2013/11/04 23:10:48, Ilya Sherman wrote: > Please write a separate test case for having a matching username. (How is this > test different from the one above?) The first is different in that the field is prefilled instead of readonly. The second is just a subset of the prefilled and readonly case above, so we can probably ditch it. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:500: username_element, password_element); On 2013/11/04 23:10:48, Ilya Sherman wrote: > nit: Please use curly braces to enclose the if-stmt body, since it spans > multiple lines. Do you know where this is spelled out? I was actually looking to try and figure out the rule for this, and I couldn't find it in either the Chrome or Google style guides. Fixed regardless, I just can never remember if the rule is to use braces if the actual clause is more than one line, or if the body is more than one line. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:613: username_element.setValue(fill_data.basic_data.fields[0].value); On 2013/11/04 23:10:48, Ilya Sherman wrote: > What happened to setAutofilled(), dispatchFormControlChangeEvent(), and tkent's > TODO? (This applies to both the username, which is being set here, and the > password, which is being set below. Might be worthwhile to decompose a helper > function to autofill an element.) The TODO got lost, I'll add it back somewhere. The setAutofilled and dispatchFormControlChangeEvent still happens in FillUserNameAndPassword(). Do you want me to add a comment to this affect? https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:618: true, false); On 2013/11/04 23:10:48, Ilya Sherman wrote: > nit: The semantics of "true" and "false" are really hard to make out here. If > you're feeling particularly motivated, you could convert the API to accept > enumerated values for these parameters rather than booleans. But if not, please > at least document them inline as "true /* exact_usernames_match */" or by using > a local named variable like "bool exact_usernames_match = true;". Done with comments for now. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:673: username_element->setValue(username); On 2013/11/04 23:10:48, Ilya Sherman wrote: > Is this safe for a potentially non-modifiable element? Doesn't seem like it > ought to be. Depends on what you mean by safe. There is nothing in the API that prevents you from changing the value of an WebElement that has a readonly set. Obviously we don't want to change such a value, but since we have exact_username_match set, we shouldn't. I just added a test to verify this. I did notice this while I was making the change, and ended up not guarding it. I wanted to make sure that this cl would stick and it seemed like a slight additional risk. There are some corner cases that I wasn't sure about. For instance, if the user is choosing from the autocomplete dropdown and the field becomes readonly during that time, what do we want to have happen? If the field is readonly and we can't fill the username and password, do we return false? Looks like the only consuming code is DidAcceptAutofillSuggestion which assumes this will succeed, so no? I can either change it in this CL, change in a different CL, or leave it. Sounds like you don't want the latter. Do you have any preference on the other two? https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:680: SetElementAutofilled(username_element, true); On 2013/11/04 23:10:48, Ilya Sherman wrote: > Likewise, is this safe for a potentially non-modifiable element? Similar to above, though it looks like this currently sets the password autofilled to true even if the value isn't autofilled. This seems really strange, but given that they check if the element is editable literally the line before, I wonder if there is a reason that they did it that way. Likely just an oversight, but it gives me pause. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.h:112: // prefilled value we will try and fill the password for that username On 2013/11/04 23:10:48, Ilya Sherman wrote: > nit: "try and" -> "try to", or just "fill", depending on whether the filling is > certain or not. Done. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.h:113: // instead. On 2013/11/04 23:10:48, Ilya Sherman wrote: > nit: Please try to rewrite this comment without using the pronoun "we", which is > discouraged as the referent can be hard to infer. Done.
https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:500: username_element, password_element); On 2013/11/05 00:40:35, Garrett Casto wrote: > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > nit: Please use curly braces to enclose the if-stmt body, since it spans > > multiple lines. > > Do you know where this is spelled out? I was actually looking to try and figure > out the rule for this, and I couldn't find it in either the Chrome or Google > style guides. Fixed regardless, I just can never remember if the rule is to use > braces if the actual clause is more than one line, or if the body is more than > one line. The style guide is pretty flexible: "In general, curly braces are not required for single-line statements, but they are allowed if you like them; conditional or loop statements with complex conditions or statements may be more readable with curly braces. Some projects require that an if must always always have an accompanying brace." In practice, people generally tend to ask for curly braces when the body spans multiple lines. Some reviewers also ask for curly braces when the condition spans multiple lines. Autofill code is pretty consistent about using curly braces around multi-line bodies; less so with multi-line conditions. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:613: username_element.setValue(fill_data.basic_data.fields[0].value); On 2013/11/05 00:40:35, Garrett Casto wrote: > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > What happened to setAutofilled(), dispatchFormControlChangeEvent(), and > tkent's > > TODO? (This applies to both the username, which is being set here, and the > > password, which is being set below. Might be worthwhile to decompose a helper > > function to autofill an element.) > > The TODO got lost, I'll add it back somewhere. The setAutofilled and > dispatchFormControlChangeEvent still happens in FillUserNameAndPassword(). Do > you want me to add a comment to this affect? Ah, sorry, I didn't read the SetElementAutofilled calls closely enough. Nevermind :) https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:618: true, false); On 2013/11/05 00:40:35, Garrett Casto wrote: > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > nit: The semantics of "true" and "false" are really hard to make out here. If > > you're feeling particularly motivated, you could convert the API to accept > > enumerated values for these parameters rather than booleans. But if not, > please > > at least document them inline as "true /* exact_usernames_match */" or by > using > > a local named variable like "bool exact_usernames_match = true;". > > Done with comments for now. Thanks :) https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:673: username_element->setValue(username); On 2013/11/05 00:40:35, Garrett Casto wrote: > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > Is this safe for a potentially non-modifiable element? Doesn't seem like it > > ought to be. > > Depends on what you mean by safe. There is nothing in the API that prevents you > from changing the value of an WebElement that has a readonly set. Obviously we > don't want to change such a value, but since we have exact_username_match set, > we shouldn't. I just added a test to verify this. I'm not convinced that it's safe to assume that the Blink API is a no-op if the value is unchanged. There could easily be side-effects that are observable in JavaScript, which IMO is not ok. > I did notice this while I was making the change, and ended up not guarding it. I > wanted to make sure that this cl would stick and it seemed like a slight > additional risk. There are some corner cases that I wasn't sure about. For > instance, if the user is choosing from the autocomplete dropdown and the field > becomes readonly during that time, what do we want to have happen? If the field > is readonly and we can't fill the username and password, do we return false? > Looks like the only consuming code is DidAcceptAutofillSuggestion which assumes > this will succeed, so no? In the Autofill code, we skip over any fields that happen to have become readonly between when the page loaded and when the user attempted to autofill the form. Fortunately, this is a rare event, so it's not likely to cause much surprise to real users. > I can either change it in this CL, change in a different CL, or leave it. Sounds > like you don't want the latter. Do you have any preference on the other two? I'd prefer to change it in this CL, since it's pretty directly related to the purpose of the CL, i.e. better supporting prefilled usernames, which I'd expect to often be packaged into readonly fields. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:680: SetElementAutofilled(username_element, true); On 2013/11/05 00:40:35, Garrett Casto wrote: > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > Likewise, is this safe for a potentially non-modifiable element? > > Similar to above, though it looks like this currently sets the password > autofilled to true even if the value isn't autofilled. This seems really > strange, but given that they check if the element is editable literally the line > before, I wonder if there is a reason that they did it that way. Likely just an > oversight, but it gives me pause. It looks like an oversight to me too. And it definitely seems wrong to fire off a JavaScript onChange event for a readonly field. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.h:113: // instead. On 2013/11/05 00:40:35, Garrett Casto wrote: > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > nit: Please try to rewrite this comment without using the pronoun "we", which > is > > discouraged as the referent can be hard to infer. > > Done. Hmm, I think the phrasing is still a little confusing. How about something like the following? """Attempts to fill |username_element| and |password_element| with the |fill_data|. Will use the data corresponding to the preferred username, unless the |username_element| already has a value set. In that case, attempts to fill the password matching the already filled username, if such a password exists.""" Incidentally, I'm not really sure why we're passing the |form_element| separately into this method -- shouldn't it be deducible from the username_element and password_element params? Can we drop it entirely? https://codereview.chromium.org/56653002/diff/140001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/56653002/diff/140001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:393: // Don't try and complete a prefilled value even if it's a partial match nit: "try and" -> "try to". https://codereview.chromium.org/56653002/diff/140001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/140001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:689: // TODO(tkent): Check maxlength and pattern This should apply to the username element as well, both here and on line 618.
https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:673: username_element->setValue(username); On 2013/11/05 01:25:11, Ilya Sherman wrote: > On 2013/11/05 00:40:35, Garrett Casto wrote: > > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > > Is this safe for a potentially non-modifiable element? Doesn't seem like it > > > ought to be. > > > > Depends on what you mean by safe. There is nothing in the API that prevents > you > > from changing the value of an WebElement that has a readonly set. Obviously we > > don't want to change such a value, but since we have exact_username_match set, > > we shouldn't. I just added a test to verify this. > > I'm not convinced that it's safe to assume that the Blink API is a no-op if the > value is unchanged. There could easily be side-effects that are observable in > JavaScript, which IMO is not ok. > > > I did notice this while I was making the change, and ended up not guarding it. > I > > wanted to make sure that this cl would stick and it seemed like a slight > > additional risk. There are some corner cases that I wasn't sure about. For > > instance, if the user is choosing from the autocomplete dropdown and the field > > becomes readonly during that time, what do we want to have happen? If the > field > > is readonly and we can't fill the username and password, do we return false? > > Looks like the only consuming code is DidAcceptAutofillSuggestion which > assumes > > this will succeed, so no? > > In the Autofill code, we skip over any fields that happen to have become > readonly between when the page loaded and when the user attempted to autofill > the form. Fortunately, this is a rare event, so it's not likely to cause much > surprise to real users. > > > I can either change it in this CL, change in a different CL, or leave it. > Sounds > > like you don't want the latter. Do you have any preference on the other two? > > I'd prefer to change it in this CL, since it's pretty directly related to the > purpose of the CL, i.e. better supporting prefilled usernames, which I'd expect > to often be packaged into readonly fields. Fixed. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.cc:680: SetElementAutofilled(username_element, true); On 2013/11/05 01:25:11, Ilya Sherman wrote: > On 2013/11/05 00:40:35, Garrett Casto wrote: > > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > > Likewise, is this safe for a potentially non-modifiable element? > > > > Similar to above, though it looks like this currently sets the password > > autofilled to true even if the value isn't autofilled. This seems really > > strange, but given that they check if the element is editable literally the > line > > before, I wonder if there is a reason that they did it that way. Likely just > an > > oversight, but it gives me pause. > > It looks like an oversight to me too. And it definitely seems wrong to fire off > a JavaScript onChange event for a readonly field. Fixed. https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/56653002/diff/50001/components/autofill/conte... components/autofill/content/renderer/password_autofill_agent.h:113: // instead. On 2013/11/05 01:25:11, Ilya Sherman wrote: > On 2013/11/05 00:40:35, Garrett Casto wrote: > > On 2013/11/04 23:10:48, Ilya Sherman wrote: > > > nit: Please try to rewrite this comment without using the pronoun "we", > which > > is > > > discouraged as the referent can be hard to infer. > > > > Done. > > Hmm, I think the phrasing is still a little confusing. How about something like > the following? > > """Attempts to fill |username_element| and |password_element| with the > |fill_data|. Will use the data corresponding to the preferred username, unless > the |username_element| already has a value set. In that case, attempts to fill > the password matching the already filled username, if such a password exists.""" > > Incidentally, I'm not really sure why we're passing the |form_element| > separately into this method -- shouldn't it be deducible from the > username_element and password_element params? Can we drop it entirely? Updated comment, and deduced form_element from username_element. https://codereview.chromium.org/56653002/diff/140001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/56653002/diff/140001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:393: // Don't try and complete a prefilled value even if it's a partial match On 2013/11/05 01:25:12, Ilya Sherman wrote: > nit: "try and" -> "try to". Done. https://codereview.chromium.org/56653002/diff/140001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/140001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:689: // TODO(tkent): Check maxlength and pattern On 2013/11/05 01:25:12, Ilya Sherman wrote: > This should apply to the username element as well, both here and on line 618. Done.
Thanks, LGTM with the remaining comments addressed. https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:603: if (!username_element.form().autoComplete()) nit: Theoretically, it's possible for username_element.form().isNull() to be true. Unless you're sure this code is not reachable in such a case, please add a null check. The simple HTML to try to repro this with is <body> <input id="username"> <input id="password" type="password"> </body> (with the <form> element intentionally omitted) on a domain for which you already have passwords saved. https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:681: if (IsElementEditable(*username_element)) { Please check username_element.autoComplete() as well. https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:695: if (IsElementEditable(*password_element)) { Please check password_element.autoComplete() as well.
https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:603: if (!username_element.form().autoComplete()) On 2013/11/05 02:21:42, Ilya Sherman wrote: > nit: Theoretically, it's possible for username_element.form().isNull() to be > true. Unless you're sure this code is not reachable in such a case, please add > a null check. The simple HTML to try to repro this with is > > <body> > <input id="username"> > <input id="password" type="password"> > </body> > > (with the <form> element intentionally omitted) on a domain for which you > already have passwords saved. Added a test to verify this, but we only get forms by calling WebDocument::forms(), so it doesn't look like this is possible. https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:681: if (IsElementEditable(*username_element)) { On 2013/11/05 02:21:42, Ilya Sherman wrote: > Please check username_element.autoComplete() as well. Done. https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:695: if (IsElementEditable(*password_element)) { On 2013/11/05 02:21:42, Ilya Sherman wrote: > Please check password_element.autoComplete() as well. Added, though I actually moved this check up before the username gets filled, so that we won't autofill the username if we can't fill the password.
https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/56653002/diff/210001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:695: if (IsElementEditable(*password_element)) { On 2013/11/05 05:56:11, Garrett Casto wrote: > On 2013/11/05 02:21:42, Ilya Sherman wrote: > > Please check password_element.autoComplete() as well. > > Added, though I actually moved this check up before the username gets filled, so > that we won't autofill the username if we can't fill the password. Yeah, that's a good call. Thanks :)
Message was sent while issue was closed.
Committed patchset #6 manually as r232928 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
