Fill on account select in the password manager

Fill on account select in the password manager

This implements, behind a flag, fill on account select in the password
manager. When the --enable-fill-on-account-select flag is used (or set
in chrome://flags), instead of autofilling on page load, the password
manager will mark fields as "autofilled" that it believes it can
appropriately autofill, but it will wait until the user has selected
those fields manually and chooses the appropriate account before filling
them in.

There are two main advantages to this approach. The first is that it
raises the bar for attackers with XSS's to sites in harvesting
passwords. These attackers will not be able to rely on a mere user
gesture before the field is filled; the user now must explicitly choose
the account before it fills. Secondly, it has the possibility of
improving the browsing experience on sites where the password manager
fails to fill correctly, as it won't actually fill until the user
chooses to do so, so if it tries to fill a wrong field, they user will
simply not choose an account.

The most basic part of this CL is quite straightforward in that if the
flag is enabled, it simply stops, marks the fields as autofilled, and
does not actually fill the fields with the credentials. However, there
is a more complicated corner case: forms where the username is not
editable, but the password field is, such as google.com/accounts after a
user has logged in once. For these cases, it was necessary to add
additional logic so that the *password* field can be clicked and the
credentials can be selected. This requires a new IPC and some new
autofill plumbing to change the dropdown menu.

BUG=410963

[jww, 2014-08-24 00:47:38]

Hey Garrett. This is an implementation of fill on account select behind a flag.
It still needs tests, but would you mind taking a look to make sure it looks
sane overall? Thanks!

[Garrett Casto, 2014-09-08 21:27:20]

https://codereview.chromium.org/492043003/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:933: const int
options) {
So I think that the behavior if the username if already filled out is slightly
strange. If the username is one that we have data for, the user will see the
username with autofill highlighting but not password. If they click on the
username field the dropdown should appear and if they select that they should
get a password. If we don't have data for the particular username it's also less
likely that the user will understand what is going on, as it looks like the
username field was just autofilled, but at least the password will fill if they
try to change the username.

I would probably just avoid these cases for now. If the user sees a field that
is highlighted with Autofill but doesn't have any data, I'm not sure what they
are going to think, but if they see a field with data that is highlighted they
will probably just think that it was autofilled. If we decide to go down this
route and have a better indicator we should show it in all cases, but for now I
would just fill normally if the username if prefilled. I also know that this
comes up on accounts.google.com where the username field is not only prefilled
but hidden, so if we don't do this the password manager will just break.

This also makes the logic much simpler, just add an additional if statement in
FillFormOnPasswordRecieved()

if (fill_on_account_select) {
  username_element.setAutofilled(true);
  return;
} else {
  username_element.setValue(...);
}

https://codereview.chromium.org/492043003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:938:
switches::kEnableFillOnAccountSelect);
You should pull this out into a function so that we can easily add a finch
experiment for this. Also you are currently ignoring the disable flag that you
added.

[jww, 2014-09-09 01:40:34]

gcasto@, I think I addressed your concerns. Let me know.

https://codereview.chromium.org/492043003/diff/1/components/autofill/content/...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:933: const int
options) {
I was admittedly a bit confused by your comment, but I think I got the gist of
it and have addressed the issues, but you'll definitely want to double check my
latest CL :-)
On 2014/09/08 21:27:20, Garrett Casto wrote:
> So I think that the behavior if the username if already filled out is slightly
> strange. If the username is one that we have data for, the user will see the
> username with autofill highlighting but not password. If they click on the
> username field the dropdown should appear and if they select that they should
> get a password. If we don't have data for the particular username it's also
less
> likely that the user will understand what is going on, as it looks like the
> username field was just autofilled, but at least the password will fill if
they
> try to change the username.
> 
> I would probably just avoid these cases for now. If the user sees a field that
> is highlighted with Autofill but doesn't have any data, I'm not sure what they
> are going to think, but if they see a field with data that is highlighted they
> will probably just think that it was autofilled. If we decide to go down this
> route and have a better indicator we should show it in all cases, but for now
I
> would just fill normally if the username if prefilled. I also know that this
> comes up on http://accounts.google.com where the username field is not only
prefilled
> but hidden, so if we don't do this the password manager will just break.
> 
> This also makes the logic much simpler, just add an additional if statement in
> FillFormOnPasswordRecieved()
> 
> if (fill_on_account_select) {
>   username_element.setAutofilled(true);
>   return;
> } else {
>   username_element.setValue(...);
> }

https://codereview.chromium.org/492043003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:938:
switches::kEnableFillOnAccountSelect);
On 2014/09/08 21:27:20, Garrett Casto wrote:
> You should pull this out into a function so that we can easily add a finch
> experiment for this. Also you are currently ignoring the disable flag that you
> added.

Done.

[Garrett Casto, 2014-09-12 19:08:29]

Apologies if my previous comments were long winded. It is sometime easy to
mistake length with descriptive power :). Hopefully this is clearer.

https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:959: }
This is going to break sites that hide the username field, like
accounts.google.com. If the username is hidden and prefilled we don't want to
delay filling passwords. I would think that this is still pretty safe in the
face of XSS as the attacker would need to prefill the username field with your
username, which seems reasonably difficult. Even if we want to introduce some
other way around this, for the moment I would put this clause inside the next
one, so that we only  abort filling if we would have filled in the username
ourselves.

[jww, 2014-09-15 22:48:00]

https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:959: }
On 2014/09/12 19:08:28, Garrett Casto wrote:
> This is going to break sites that hide the username field, like
> http://accounts.google.com. If the username is hidden and prefilled we don't
want to
> delay filling passwords. I would think that this is still pretty safe in the
> face of XSS as the attacker would need to prefill the username field with your
> username, which seems reasonably difficult. Even if we want to introduce some
> other way around this, for the moment I would put this clause inside the next
> one, so that we only  abort filling if we would have filled in the username
> ourselves.

Hm, this is a tough one. I don't think this is safe at all in the XSS case.
Generally, we consider usernames public knowledge, and especially in the case
where an attacker has an XSS to lots of sites (which almost certainly have
usernames accessible and there's likely overlap).

I feel pretty strongly we should come up with a way around this. Why don't we
provide the accounts popup menu when the user clicks on the password field
anyway? Is that something we can do?

[Garrett Casto, 2014-09-19 08:41:30]

On 2014/09/15 22:48:00, jww wrote:
>
https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
> components/autofill/content/renderer/password_autofill_agent.cc:959: }
> On 2014/09/12 19:08:28, Garrett Casto wrote:
> > This is going to break sites that hide the username field, like
> > http://accounts.google.com. If the username is hidden and prefilled we don't
> want to
> > delay filling passwords. I would think that this is still pretty safe in the
> > face of XSS as the attacker would need to prefill the username field with
your
> > username, which seems reasonably difficult. Even if we want to introduce
some
> > other way around this, for the moment I would put this clause inside the
next
> > one, so that we only  abort filling if we would have filled in the username
> > ourselves.
> 
> Hm, this is a tough one. I don't think this is safe at all in the XSS case.
> Generally, we consider usernames public knowledge, and especially in the case
> where an attacker has an XSS to lots of sites (which almost certainly have
> usernames accessible and there's likely overlap).
> 
> I feel pretty strongly we should come up with a way around this. Why don't we
> provide the accounts popup menu when the user clicks on the password field
> anyway? Is that something we can do?

Technically I'm sure it's possible to do, but we need to think through all the
implications. It's not reliably possible to determine if a input field is hidden
or not so we would need to add this to all password fields. Things off the top
of my head that I can think of

1) Should we show all usernames on click in the password field, or just the the
ones that share the prefix with the text in the input field: In the hidden
username field case, changing the username seems like a bad idea. Not showing
these additional usernames won't be consistent with the username dropdown, but
I'm not sure if that matters.
2) What do we show to the user: Does the dropdown just contain the username? I
believe in all other cases what we show in the dropdown is what we fill in the
field. Is this going to be confusing?
3) Do we want to try suppressing the password dropdown if the username dropdown
has already shown. Doesn't seem like it's useful in this case and may be
confusing to users. Might be a little too magical though.

I'll send an e-mail to Rebecca and Sabine to discuss this.

[jww, 2014-09-19 21:35:13]

On 2014/09/19 08:41:30, Garrett Casto wrote:
> On 2014/09/15 22:48:00, jww wrote:
> >
>
https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
> > File components/autofill/content/renderer/password_autofill_agent.cc
(right):
> > 
> >
>
https://codereview.chromium.org/492043003/diff/40001/components/autofill/cont...
> > components/autofill/content/renderer/password_autofill_agent.cc:959: }
> > On 2014/09/12 19:08:28, Garrett Casto wrote:
> > > This is going to break sites that hide the username field, like
> > > http://accounts.google.com. If the username is hidden and prefilled we
don't
> > want to
> > > delay filling passwords. I would think that this is still pretty safe in
the
> > > face of XSS as the attacker would need to prefill the username field with
> your
> > > username, which seems reasonably difficult. Even if we want to introduce
> some
> > > other way around this, for the moment I would put this clause inside the
> next
> > > one, so that we only  abort filling if we would have filled in the
username
> > > ourselves.
> > 
> > Hm, this is a tough one. I don't think this is safe at all in the XSS case.
> > Generally, we consider usernames public knowledge, and especially in the
case
> > where an attacker has an XSS to lots of sites (which almost certainly have
> > usernames accessible and there's likely overlap).
> > 
> > I feel pretty strongly we should come up with a way around this. Why don't
we
> > provide the accounts popup menu when the user clicks on the password field
> > anyway? Is that something we can do?
> 
> Technically I'm sure it's possible to do, but we need to think through all the
> implications. It's not reliably possible to determine if a input field is
hidden
> or not so we would need to add this to all password fields. Things off the top
> of my head that I can think of
Yup, my intuition is that we'd need to add it to all password fields.
Personally, this wouldn't bother me that much, as I've always liked that when I
click on password fields, LastPass gives me my account options there.
> 
> 1) Should we show all usernames on click in the password field, or just the
the
> ones that share the prefix with the text in the input field: In the hidden
> username field case, changing the username seems like a bad idea. Not showing
> these additional usernames won't be consistent with the username dropdown, but
> I'm not sure if that matters.
I agree that if the username is hidden, we shouldn't modify it. I actually see
that as consistent. I think for the password field in general we should only
provide options based on the username field's live contents, just as we do
today. I think the only difference is that we won't provide all accounts on a
second click.
> 2) What do we show to the user: Does the dropdown just contain the username? I
> believe in all other cases what we show in the dropdown is what we fill in the
> field. Is this going to be confusing?
I like palmer's suggestion in our email thread that we add to all dropdown's
"Log into foo.com as:" (or "Log into foo.com with the credentials for:", etc.)
> 3) Do we want to try suppressing the password dropdown if the username
dropdown
> has already shown. Doesn't seem like it's useful in this case and may be
> confusing to users. Might be a little too magical though.
Yeah, I'd lean against doing that to start. Sounds like, at best, it's an
optimization (if not too magical, as you put it).
> 
> I'll send an e-mail to Rebecca and Sabine to discuss this.

[jww, 2014-11-01 01:25:08]

Garrett, this implementation I think finally addresses the hidden username case.
Can you take a look at it? Obviously, still need some unit tests, and I think
there's a Mac corner case or two, but I'd love to make sure my basic CL looks
okay/isn't totally crazy first. Thanks!

[Garrett Casto, 2014-11-04 23:35:30]

Would you mind adding some screenshots of this UI to the bug? In general it
seems like a reasonable approach, though you are going to need an OK from an
Autofill OWNER.

This CL is also big enough that I would suggest splitting it in half. The first
half can be just the UI changes and PasswordAutofillManager::ShowSuggestions()
would take a parameter to use them but it would always be false. The second CL
would actually introduce the experiment and change PasswordAutofillAgent to ask
for this UI as necessary.

https://codereview.chromium.org/492043003/diff/140001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/492043003/diff/140001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:132: const
base::string16& title) {
Given the way that this works for other labels, it seems like |title| shouldn't
be a part of this function and instead PasswordAutofillManager should explicitly
add the correct values to the other vectors if it wants to add a title.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:297:
IPC_MESSAGE_ROUTED4(AutofillHostMsg_ShowPasswordSuggestionsForPasswordField,
Seems like this should just be an additional parameter to
ShowPasswordSuggestions instead of a separate IPC.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:435: //   (b)
The username element isn't prefilled
Is that last line supposed to be (c)?

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:444: if
(IsElementAutocompletable(username_element)) {
I think that you dropped the form_contains_username_field check. It's possible
that username_element is invalid here.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:726: const
blink::WebInputElement* username_element;
Why does this need to be a pointer?

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:731:
password_to_username_.find(element);
Where is this defined and populated?

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:762:
element.isPasswordField());
Is there any reason why you need the fourth parameter here instead of just
passing |element| as the second param and change behavior in
ShowSuggestionPopup() depending on if element.isPasswordField() is true or not?

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:160: bool
show_on_password_field = false);
Default arguments are verboten.

[jww, 2014-11-06 21:28:57]

Thanks, Garrett. Yes, it definitely makes sense to split this CL. I'll finish up
here and then take out all the flag and experimental portions, as you suggested.

https://codereview.chromium.org/492043003/diff/140001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/492043003/diff/140001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:132: const
base::string16& title) {
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Given the way that this works for other labels, it seems like |title|
shouldn't
> be a part of this function and instead PasswordAutofillManager should
explicitly
> add the correct values to the other vectors if it wants to add a title.

Done.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:297:
IPC_MESSAGE_ROUTED4(AutofillHostMsg_ShowPasswordSuggestionsForPasswordField,
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Seems like this should just be an additional parameter to
> ShowPasswordSuggestions instead of a separate IPC.

Done.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:435: //   (b)
The username element isn't prefilled
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Is that last line supposed to be (c)?

Yup :-)

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:444: if
(IsElementAutocompletable(username_element)) {
On 2014/11/04 23:35:30, Garrett Casto wrote:
> I think that you dropped the form_contains_username_field check. It's possible
> that username_element is invalid here.

Done.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:726: const
blink::WebInputElement* username_element;
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Why does this need to be a pointer?

I was trying to have only return statement, and since the value username_element
is dependent on whether element is a password field, I made it assignable.
However, I've rewritten the method to get rid of the pointer, so let me know
which version you prefer (or if there's another option I'm not thinking of).

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:731:
password_to_username_.find(element);
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Where is this defined and populated?

It's defined in password_autofill_agent.h and it is populated in
OnFillPasswordForm in password_autofill_agent.cc (at the same time and place
that login_to_password_info_ is populated). In case it's not clear, I did not
add this field.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:762:
element.isPasswordField());
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Is there any reason why you need the fourth parameter here instead of just
> passing |element| as the second param and change behavior in
> ShowSuggestionPopup() depending on if element.isPasswordField() is true or
not?

Yes, because we ultimately need the username as the element, not the password
field. Of course, we could pass in the password element and then check and find
the username element, but given that this is a special case, it seems to make
the most sense to abstract out the search for the username element to the one
place it's used. Note the additional search step on line 723 in
FindPasswordInfoForElement; it seems weird to call that again inside
ShowSuggestionPopup (since it's already needed inside ShowSuggestions()).

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:160: bool
show_on_password_field = false);
On 2014/11/04 23:35:30, Garrett Casto wrote:
> Default arguments are verboten.

Done.

[jww, 2014-11-06 21:31:29]

On 2014/11/06 21:28:57, jww wrote:
> Thanks, Garrett. Yes, it definitely makes sense to split this CL. I'll finish
up
> here and then take out all the flag and experimental portions, as you
suggested.
> 
>
https://codereview.chromium.org/492043003/diff/140001/chrome/browser/ui/autof...
> File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/492043003/diff/140001/chrome/browser/ui/autof...
> chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:132: const
> base::string16& title) {
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Given the way that this works for other labels, it seems like |title|
> shouldn't
> > be a part of this function and instead PasswordAutofillManager should
> explicitly
> > add the correct values to the other vectors if it wants to add a title.
> 
> Done.
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> File components/autofill/content/common/autofill_messages.h (right):
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/common/autofill_messages.h:297:
> IPC_MESSAGE_ROUTED4(AutofillHostMsg_ShowPasswordSuggestionsForPasswordField,
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Seems like this should just be an additional parameter to
> > ShowPasswordSuggestions instead of a separate IPC.
> 
> Done.
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.cc:435: //   (b)
> The username element isn't prefilled
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Is that last line supposed to be (c)?
> 
> Yup :-)
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.cc:444: if
> (IsElementAutocompletable(username_element)) {
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > I think that you dropped the form_contains_username_field check. It's
possible
> > that username_element is invalid here.
> 
> Done.
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.cc:726: const
> blink::WebInputElement* username_element;
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Why does this need to be a pointer?
> 
> I was trying to have only return statement, and since the value
username_element
> is dependent on whether element is a password field, I made it assignable.
> However, I've rewritten the method to get rid of the pointer, so let me know
> which version you prefer (or if there's another option I'm not thinking of).
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.cc:731:
> password_to_username_.find(element);
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Where is this defined and populated?
> 
> It's defined in password_autofill_agent.h and it is populated in
> OnFillPasswordForm in password_autofill_agent.cc (at the same time and place
> that login_to_password_info_ is populated). In case it's not clear, I did not
> add this field.
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.cc:762:
> element.isPasswordField());
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Is there any reason why you need the fourth parameter here instead of just
> > passing |element| as the second param and change behavior in
> > ShowSuggestionPopup() depending on if element.isPasswordField() is true or
> not?
> 
> Yes, because we ultimately need the username as the element, not the password
> field. Of course, we could pass in the password element and then check and
find
> the username element, but given that this is a special case, it seems to make
> the most sense to abstract out the search for the username element to the one
> place it's used. Note the additional search step on line 723 in
> FindPasswordInfoForElement; it seems weird to call that again inside
> ShowSuggestionPopup (since it's already needed inside ShowSuggestions()).
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> File components/autofill/content/renderer/password_autofill_agent.h (right):
> 
>
https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.h:160: bool
> show_on_password_field = false);
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Default arguments are verboten.
> 
> Done.

Also, I forgot to mention that I did add screenshots to the bug.

[Garrett Casto, 2014-11-23 07:49:49]

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:444: if
(IsElementAutocompletable(username_element)) {
On 2014/11/06 21:28:56, jww (vacation until 11-24) wrote:
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > I think that you dropped the form_contains_username_field check. It's
possible
> > that username_element is invalid here.
> 
> Done.

Can you add a test so that this would have been caught?

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:762:
element.isPasswordField());
On 2014/11/06 21:28:57, jww (vacation until 11-24) wrote:
> On 2014/11/04 23:35:30, Garrett Casto wrote:
> > Is there any reason why you need the fourth parameter here instead of just
> > passing |element| as the second param and change behavior in
> > ShowSuggestionPopup() depending on if element.isPasswordField() is true or
> not?
> 
> Yes, because we ultimately need the username as the element, not the password
> field. Of course, we could pass in the password element and then check and
find
> the username element, but given that this is a special case, it seems to make
> the most sense to abstract out the search for the username element to the one
> place it's used. Note the additional search step on line 723 in
> FindPasswordInfoForElement; it seems weird to call that again inside
> ShowSuggestionPopup (since it's already needed inside ShowSuggestions()).

I'm not sure that I understand why we need the username in ShowSuggestionPopup
if we are going to show the popup on the password field. As is, it looks like it
just immediately converts the username element to the password element. Assuming
|user_input| was always the element that you wanted the popup to be shown on,
couldn't you just remove the first new if statement in ShowSuggestionPopup?

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2180:
popup_controller_->Show(popup_values, popup_labels, popup_icons, popup_ids);
Seems like you should just revert this change now.

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:264:
SetSelectedLine(GetFirstSelectableLine());
It might not be possible now, but I would think that you wouldn't want to focus
anything that CanAccept() returns false for. It might be clearer to set the
current line to kNoSelection and then call SelectNextLine(). Cuts down on code
duplication a bit.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:295: int /* show all
suggestions and the field is a password field options */,
I think that keeping the two boolean arguments here is clearer. There are times
when bitmasks are better, but I think that you need more arguments than this.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:46:
FILL_PREFERRED_USERNAME = 1 << 2
Looks like FILL_PREFERRED_USERNAME isn't used currently? The only place we
handle the preferred username differently is in FillFormOnPasswordRecieved, not
in FillUserNameAndPassword, so I'm also not sure what the intention was here. If
you're not adding an additional here, I'd also leave this as boolean arguments
for FillUserNameAndPassword.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:138: //
TODO(jww): Add experimental groups check here.
When I said splitting off the CL into two, what I meant was making the changes
in password_autofill_manager and Autofill code in one CL, and adding all the
machinery to this file in another. I think that just holding out just this one
piece doesn't matter much. This change isn't large enough that I think you must
break it up, it was just a suggestion.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/492043003/diff/200001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:165: labels,
icons, ids, GetWeakPtr());
I would revert this now just to cut down on the diff.

https://codereview.chromium.org/492043003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/492043003/diff/200001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:110: void
PasswordAutofillManager::ShowPasswordSuggestions(
Nit: I would just keep this as one function for now. The split doesn't make to
code any clearer to me.

[jww, 2014-11-25 02:46:26]

My two outstanding issues are:
  * unit tests
  * splitting the CL in two

I'll definitely try to get the unit tests done tomorrow, and hopefully the
second part as well.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:444: if
(IsElementAutocompletable(username_element)) {
On 2014/11/23 07:49:48, Garrett Casto wrote:
> On 2014/11/06 21:28:56, jww (vacation until 11-24) wrote:
> > On 2014/11/04 23:35:30, Garrett Casto wrote:
> > > I think that you dropped the form_contains_username_field check. It's
> possible
> > > that username_element is invalid here.
> > 
> > Done.
> 
> Can you add a test so that this would have been caught? 

Yup, unit tests are my next step.

https://codereview.chromium.org/492043003/diff/140001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:762:
element.isPasswordField());
On 2014/11/23 07:49:48, Garrett Casto wrote:
> On 2014/11/06 21:28:57, jww (vacation until 11-24) wrote:
> > On 2014/11/04 23:35:30, Garrett Casto wrote:
> > > Is there any reason why you need the fourth parameter here instead of just
> > > passing |element| as the second param and change behavior in
> > > ShowSuggestionPopup() depending on if element.isPasswordField() is true or
> > not?
> > 
> > Yes, because we ultimately need the username as the element, not the
password
> > field. Of course, we could pass in the password element and then check and
> find
> > the username element, but given that this is a special case, it seems to
make
> > the most sense to abstract out the search for the username element to the
one
> > place it's used. Note the additional search step on line 723 in
> > FindPasswordInfoForElement; it seems weird to call that again inside
> > ShowSuggestionPopup (since it's already needed inside ShowSuggestions()).
> 
> I'm not sure that I understand why we need the username in ShowSuggestionPopup
> if we are going to show the popup on the password field. As is, it looks like
it
> just immediately converts the username element to the password element.
Assuming
> |user_input| was always the element that you wanted the popup to be shown on,
> couldn't you just remove the first new if statement in ShowSuggestionPopup?

I believe it's because the browser process can only look up mappings by
username. Thus, we need to pass to the browser the username that we want the
credential for.

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2180:
popup_controller_->Show(popup_values, popup_labels, popup_icons, popup_ids);
On 2014/11/23 07:49:48, Garrett Casto wrote:
> Seems like you should just revert this change now.

Done.

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/492043003/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:264:
SetSelectedLine(GetFirstSelectableLine());
On 2014/11/23 07:49:48, Garrett Casto wrote:
> It might not be possible now, but I would think that you wouldn't want to
focus
> anything that CanAccept() returns false for. It might be clearer to set the
> current line to kNoSelection and then call SelectNextLine(). Cuts down on code
> duplication a bit.

Done.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:295: int /* show all
suggestions and the field is a password field options */,
On 2014/11/23 07:49:48, Garrett Casto wrote:
> I think that keeping the two boolean arguments here is clearer. There are
times
> when bitmasks are better, but I think that you need more arguments than this.

If you feel strongly about it, I can make the change but I prefer this for two
reasons:
  (a) At the call sites, it seems much clearer to me what is being selected, and
  (b) Since there's no IPC_MESSAGE_ROUTED6, this prevents us from having to
create a much more complicated arguments structure to pass over the IPC.

Let me know what you think.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:46:
FILL_PREFERRED_USERNAME = 1 << 2
On 2014/11/23 07:49:48, Garrett Casto wrote:
> Looks like FILL_PREFERRED_USERNAME isn't used currently? The only place we
> handle the preferred username differently is in FillFormOnPasswordRecieved,
not
> in FillUserNameAndPassword, so I'm also not sure what the intention was here.
If
> you're not adding an additional here, I'd also leave this as boolean arguments
> for FillUserNameAndPassword.

Done.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:138: //
TODO(jww): Add experimental groups check here.
On 2014/11/23 07:49:48, Garrett Casto wrote:
> When I said splitting off the CL into two, what I meant was making the changes
> in password_autofill_manager and Autofill code in one CL, and adding all the
> machinery to this file in another. I think that just holding out just this one
> piece doesn't matter much. This change isn't large enough that I think you
must
> break it up, it was just a suggestion.

Got it. I'll probably still commit without the experimental groups at first just
to keep it cleaner.

https://codereview.chromium.org/492043003/diff/200001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/492043003/diff/200001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:165: labels,
icons, ids, GetWeakPtr());
On 2014/11/23 07:49:48, Garrett Casto wrote:
> I would revert this now just to cut down on the diff.

Done.

https://codereview.chromium.org/492043003/diff/200001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/492043003/diff/200001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:110: void
PasswordAutofillManager::ShowPasswordSuggestions(
On 2014/11/23 07:49:48, Garrett Casto wrote:
> Nit: I would just keep this as one function for now. The split doesn't make to
> code any clearer to me.

Done.

[jww, 2014-11-26 01:58:23]

Garrett, I added unit and browser tests. I'll split this CL up soon, although it
would be great to get your opinion on the tests whenever you get a chance.

[jww, 2014-12-01 21:58:46]

On 2014/11/26 01:58:23, jww wrote:
> Garrett, I added unit and browser tests. I'll split this CL up soon, although
it
> would be great to get your opinion on the tests whenever you get a chance.

This CL is officially deprecated in favor of two others that I've split from
this:
   * https://codereview.chromium.org/767353002/ implements the password manager
half
   * https://codereview.chromium.org/773573004/ implements the renderer half