Autofill: Refactoring to support fetching password after a username is selected

Refactor password autofill to make it possible to delay fetching passwords from the OS password store (e.g., Keychain) until after the user has selected a username from the dropdown. Note that delayed fetching is not yet implemented; this is just a first step to make it possible.

BUG=178358
R=avi@chromium.org, blundell@chromium.org, gcasto@chromium.org, isherman@chromium.org, nkostylev@chromium.org, palmer@chromium.org
TBR=ben@chromium.org (for DEPS change)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265993

[Patrick Dubroy, 2014-03-03 18:18:06]

Garrett, PTAL.

[Patrick Dubroy, 2014-03-04 13:56:35]

Garrett: ping.

[Garrett Casto, 2014-03-05 07:36:08]

I'm not wild about the AutofillDriver -> PasswordManagerDriver conversion as is.
Having better separation between these components would be nice, but at the
moment it's not clear that this IPC belongs on the PasswordManager side of the
divide since it's sent and received in Autofill. It also introduces a harder
dependency that didn't exist before. Before most of the interdependence came
from IPCs which could be dropped to the floor if you only wanted Autofill
without PasswordManager.

Long term I can see us pulling PasswordAutofillAgent, PasswordGenerationAgent,
and possibly PasswordAutofillManager out into the PasswordManager component, but
I don't think that this change helps us much in that regard.

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

https://codereview.chromium.org/184103016/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:364:
SetElementAutofilled(&username_element, false);
The boolean should be "true" here shouldn't it?

https://codereview.chromium.org/184103016/diff/20001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/184103016/diff/20001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.cc:6: #include
"base/strings/utf_string_conversions.h"
Unused?

https://codereview.chromium.org/184103016/diff/20001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.cc:55: base::string16
*password) {
"base::string16* password"

[Patrick Dubroy, 2014-03-05 16:31:50]

Ok, I see your point about the code moved from AutofillDriver ->
PasswordManagerDriver, so I moved it back.

I still need to fix the unit tests, but wanted to make sure you are ok with the
approach first.

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

https://codereview.chromium.org/184103016/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:364:
SetElementAutofilled(&username_element, false);
On 2014/03/05 07:36:09, Garrett Casto wrote:
> The boolean should be "true" here shouldn't it?

Done. Somehow I was confused and thought this parameter meant something else.

https://codereview.chromium.org/184103016/diff/20001/components/autofill/core...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/184103016/diff/20001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.cc:6: #include
"base/strings/utf_string_conversions.h"
On 2014/03/05 07:36:09, Garrett Casto wrote:
> Unused?

Done.

https://codereview.chromium.org/184103016/diff/20001/components/autofill/core...
components/autofill/core/browser/password_autofill_manager.cc:55: base::string16
*password) {
On 2014/03/05 07:36:09, Garrett Casto wrote:
> "base::string16* password"

Done.

https://codereview.chromium.org/184103016/diff/100001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (left):

https://codereview.chromium.org/184103016/diff/100001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:466: // We need to make
sure this is handled here because the browser process
I delete this comment because I'm not sure that it makes sense. I don't see
where "the browser process skipped handling it", but maybe I'm missing
something.

[Garrett Casto, 2014-03-06 00:42:04]

Adding Ilya on in case he has thoughts on the  componentization issues.

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:27: void
PasswordAutofillManager::DidAcceptAutofillSuggestion(
You should change the function name if this is meant to be declarative.
"AcceptAutofillSuggestion" seems fine.

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:58: //
TODO(dubroy): When password access requires some kind of authentication
This still has the same dependence problems that I was thinking about before.
You need an object from the PasswordManager component in order to construct
something in the Autofill component. The alternative that I was thinking of was
to have this call go through AutofillManagerDelegate, so
TabAutofillManagerDelegate would get the PasswordManager from
ChromePasswordManagerClient and load the password from there. The indirection is
a little higher, but it keep the two components more separated.

[Ilya Sherman, 2014-03-06 01:28:36]

On 2014/03/06 00:42:04, Garrett Casto wrote:
> Adding Ilya on in case he has thoughts on the componentization issues.

Do the components have any dependencies on each other currently?  (I expect the
answer is yes.)  If so, which one is the "depender", and which the "dependee"? 
IMO it's fine to have one component depend on the other, as long as we avoid any
cyclic dependencies.

[Garrett Casto, 2014-03-06 23:49:27]

On 2014/03/06 01:28:36, Ilya Sherman wrote:
> On 2014/03/06 00:42:04, Garrett Casto wrote:
> > Adding Ilya on in case he has thoughts on the componentization issues.
> 
> Do the components have any dependencies on each other currently?  (I expect
the
> answer is yes.)  If so, which one is the "depender", and which the "dependee"?

> IMO it's fine to have one component depend on the other, as long as we avoid
any
> cyclic dependencies.

I think that currently the only dependencies are indirect. Specifically the only
case I know where we don't go through the normal channels (e.g. Delegate or
Driver interfaces) are IPCs that are sent from one side of the component
boundary and caught on the other side (PasswordFormsRendered,
PasswordFormsParsed, etc.).

If there were a dependency here, I would also think that it should go from the
PasswordManager to Autofill, since the password manager doesn't do anything
without Autofill code. This change would make it more difficult to use Autofill
if you didn't want to use the PasswordManager.

[Ilya Sherman, 2014-03-06 23:51:53]

On 2014/03/06 23:49:27, Garrett Casto wrote:
> On 2014/03/06 01:28:36, Ilya Sherman wrote:
> > On 2014/03/06 00:42:04, Garrett Casto wrote:
> > > Adding Ilya on in case he has thoughts on the componentization issues.
> > 
> > Do the components have any dependencies on each other currently?  (I expect
> the
> > answer is yes.)  If so, which one is the "depender", and which the
"dependee"?
> 
> > IMO it's fine to have one component depend on the other, as long as we avoid
> any
> > cyclic dependencies.
> 
> I think that currently the only dependencies are indirect. Specifically the
only
> case I know where we don't go through the normal channels (e.g. Delegate or
> Driver interfaces) are IPCs that are sent from one side of the component
> boundary and caught on the other side (PasswordFormsRendered,
> PasswordFormsParsed, etc.).
> 
> If there were a dependency here, I would also think that it should go from the
> PasswordManager to Autofill, since the password manager doesn't do anything
> without Autofill code. This change would make it more difficult to use
Autofill
> if you didn't want to use the PasswordManager.

Fair enough.  I guess keeping indirect dependencies for now sounds good.  At
some point, we should rethink the dependencies between the components more
deeply, as the existing ones developed organically rather than through some
well-thought-out planning.

[Garrett Casto, 2014-03-06 23:59:38]

On 2014/03/06 23:51:53, Ilya Sherman wrote:
> On 2014/03/06 23:49:27, Garrett Casto wrote:
> > On 2014/03/06 01:28:36, Ilya Sherman wrote:
> > > On 2014/03/06 00:42:04, Garrett Casto wrote:
> > > > Adding Ilya on in case he has thoughts on the componentization issues.
> > > 
> > > Do the components have any dependencies on each other currently?  (I
expect
> > the
> > > answer is yes.)  If so, which one is the "depender", and which the
> "dependee"?
> > 
> > > IMO it's fine to have one component depend on the other, as long as we
avoid
> > any
> > > cyclic dependencies.
> > 
> > I think that currently the only dependencies are indirect. Specifically the
> only
> > case I know where we don't go through the normal channels (e.g. Delegate or
> > Driver interfaces) are IPCs that are sent from one side of the component
> > boundary and caught on the other side (PasswordFormsRendered,
> > PasswordFormsParsed, etc.).
> > 
> > If there were a dependency here, I would also think that it should go from
the
> > PasswordManager to Autofill, since the password manager doesn't do anything
> > without Autofill code. This change would make it more difficult to use
> Autofill
> > if you didn't want to use the PasswordManager.
> 
> Fair enough.  I guess keeping indirect dependencies for now sounds good.  At
> some point, we should rethink the dependencies between the components more
> deeply, as the existing ones developed organically rather than through some
> well-thought-out planning.

Yeah, I'm not sure how high a priority it is, but it should probably happen
eventually.

[Patrick Dubroy, 2014-03-10 14:29:56]

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:58: //
TODO(dubroy): When password access requires some kind of authentication
On 2014/03/06 00:42:05, Garrett Casto wrote:
> This still has the same dependence problems that I was thinking about before.
> You need an object from the PasswordManager component in order to construct
> something in the Autofill component. The alternative that I was thinking of
was
> to have this call go through AutofillManagerDelegate, so
> TabAutofillManagerDelegate would get the PasswordManager from
> ChromePasswordManagerClient and load the password from there. The indirection
is
> a little higher, but it keep the two components more separated.

Yeah, that might be slightly better, though it still means a direct dependency
from the autofill component to the password manager component.

Alternatively, it looks like it should be possible to just move all the
password-related IPCs into the password manager component. What do you think
about doing that?

[Patrick Dubroy, 2014-03-10 14:48:49]

On 2014/03/06 23:51:53, Ilya Sherman wrote:
> On 2014/03/06 23:49:27, Garrett Casto wrote:
> > On 2014/03/06 01:28:36, Ilya Sherman wrote:
> > > On 2014/03/06 00:42:04, Garrett Casto wrote:
> > > > Adding Ilya on in case he has thoughts on the componentization issues.
> > > 
> > > Do the components have any dependencies on each other currently?  (I
expect
> > the
> > > answer is yes.)  If so, which one is the "depender", and which the
> "dependee"?
> > 
> > > IMO it's fine to have one component depend on the other, as long as we
avoid
> > any
> > > cyclic dependencies.
> > 
> > I think that currently the only dependencies are indirect. Specifically the
> only
> > case I know where we don't go through the normal channels (e.g. Delegate or
> > Driver interfaces) are IPCs that are sent from one side of the component
> > boundary and caught on the other side (PasswordFormsRendered,
> > PasswordFormsParsed, etc.).
> > 
> > If there were a dependency here, I would also think that it should go from
the
> > PasswordManager to Autofill, since the password manager doesn't do anything
> > without Autofill code. This change would make it more difficult to use
> Autofill
> > if you didn't want to use the PasswordManager.
> 
> Fair enough.  I guess keeping indirect dependencies for now sounds good.  At
> some point, we should rethink the dependencies between the components more
> deeply, as the existing ones developed organically rather than through some
> well-thought-out planning.

To be clear, we currently have direct dependencies (in the form of includes)
from the password_manager component to the autofill_component. This CL would
make it a circular dependency.

[Ilya Sherman, 2014-03-10 21:44:06]

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
File components/autofill/core/browser/password_autofill_manager.cc (right):

https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
components/autofill/core/browser/password_autofill_manager.cc:58: //
TODO(dubroy): When password access requires some kind of authentication
On 2014/03/10 14:29:57, Patrick Dubroy wrote:
> On 2014/03/06 00:42:05, Garrett Casto wrote:
> > This still has the same dependence problems that I was thinking about
before.
> > You need an object from the PasswordManager component in order to construct
> > something in the Autofill component. The alternative that I was thinking of
> was
> > to have this call go through AutofillManagerDelegate, so
> > TabAutofillManagerDelegate would get the PasswordManager from
> > ChromePasswordManagerClient and load the password from there. The
indirection
> is
> > a little higher, but it keep the two components more separated.
> 
> Yeah, that might be slightly better, though it still means a direct dependency
> from the autofill component to the password manager component.
> 
> Alternatively, it looks like it should be possible to just move all the
> password-related IPCs into the password manager component. What do you think
> about doing that?

Yes, if we can do that, definitely let's! :)

[Garrett Casto, 2014-03-10 22:30:17]

On 2014/03/10 14:29:56, Patrick Dubroy wrote:
>
https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
> File components/autofill/core/browser/password_autofill_manager.cc (right):
> 
>
https://codereview.chromium.org/184103016/diff/100001/components/autofill/cor...
> components/autofill/core/browser/password_autofill_manager.cc:58: //
> TODO(dubroy): When password access requires some kind of authentication
> On 2014/03/06 00:42:05, Garrett Casto wrote:
> > This still has the same dependence problems that I was thinking about
before.
> > You need an object from the PasswordManager component in order to construct
> > something in the Autofill component. The alternative that I was thinking of
> was
> > to have this call go through AutofillManagerDelegate, so
> > TabAutofillManagerDelegate would get the PasswordManager from
> > ChromePasswordManagerClient and load the password from there. The
indirection
> is
> > a little higher, but it keep the two components more separated.
> 
> Yeah, that might be slightly better, though it still means a direct dependency
> from the autofill component to the password manager component.
> 

How is there still a direct dependency? Assuming that AutofillManagerDelegate
had a function like GetPasswordForUsername(), then the embedder would need to
know how to get this information (e.g. TabAutofillManagerDelegate would have to
know about ChromePasswordManagerClient) but it wouldn't be part of core code.
That is, if you wanted to use Autofill but not the PasswordManager, you could
just have your delegate return an empty string for this call. Did you mean
something else by direct dependency?

> Alternatively, it looks like it should be possible to just move all the
> password-related IPCs into the password manager component. What do you think
> about doing that?

By this do you mean that it's possible to move the IPCs over as well as the
autofill code that's using it (PasswordAutofillAgent, PasswordGenerationAgent,
and PasswordAutofillManager)? Just moving the IPCs without the rest of it
doesn't seem like it's obviously better as it would just mean that Autofill code
would depend on PasswordManager IPCs. I was assuming that moving all of these
classes would be relatively painful (need to define interfaces for "external
autofill agents" and something similar for PasswordAutofillManager) but it does
seem nice to do at some point. Is this what you were planning on doing?

[Garrett Casto, 2014-03-10 22:34:44]

On 2014/03/10 14:48:49, Patrick Dubroy wrote:
> On 2014/03/06 23:51:53, Ilya Sherman wrote:
> > On 2014/03/06 23:49:27, Garrett Casto wrote:
> > > On 2014/03/06 01:28:36, Ilya Sherman wrote:
> > > > On 2014/03/06 00:42:04, Garrett Casto wrote:
> > > > > Adding Ilya on in case he has thoughts on the componentization issues.
> > > > 
> > > > Do the components have any dependencies on each other currently?  (I
> expect
> > > the
> > > > answer is yes.)  If so, which one is the "depender", and which the
> > "dependee"?
> > > 
> > > > IMO it's fine to have one component depend on the other, as long as we
> avoid
> > > any
> > > > cyclic dependencies.
> > > 
> > > I think that currently the only dependencies are indirect. Specifically
the
> > only
> > > case I know where we don't go through the normal channels (e.g. Delegate
or
> > > Driver interfaces) are IPCs that are sent from one side of the component
> > > boundary and caught on the other side (PasswordFormsRendered,
> > > PasswordFormsParsed, etc.).
> > > 
> > > If there were a dependency here, I would also think that it should go from
> the
> > > PasswordManager to Autofill, since the password manager doesn't do
anything
> > > without Autofill code. This change would make it more difficult to use
> > Autofill
> > > if you didn't want to use the PasswordManager.
> > 
> > Fair enough.  I guess keeping indirect dependencies for now sounds good.  At
> > some point, we should rethink the dependencies between the components more
> > deeply, as the existing ones developed organically rather than through some
> > well-thought-out planning.
> 
> To be clear, we currently have direct dependencies (in the form of includes)
> from the password_manager component to the autofill_component. This CL would
> make it a circular dependency.

Yeah, forgot about the fact that all the types live in autofill code. I'm
generally okay with this as I mentioned since I'm not sure if the password
manager will ever be useful without Autofill UI.

[Patrick Dubroy, 2014-03-11 17:38:21]

Ok, the latest patch set attempts to move PasswordAutofillManager and its
related IPCs into the password_manager component. I think I could it should be
possible to do something similar with PasswordAutofillAgent, though it would
likely require implementing some kind of AutofillAgent interface in order to
call from autofill into password_manager.

What do you guys think? Is this worth doing?

Alternatively, the simpler solution is to do what Garrett suggested, and keep
the autofill => password_manager dependency in chrome/browser/, I guess similar
to what was done with the PasswordGenerationManager.

[Garrett Casto, 2014-03-12 01:09:22]

It looks like this patch is in an in between state (e.g.
autofill::PasswordAutofillManager hasn't been deleted yet, IPCs haven't been
moved) so I didn't get into to much detail yet on the review.  In general this
split seems reasonable. My original thought was to try and just move
PasswordAutofillManager and introduce a new interface for registering a handler
with AutofillExternalDelegate. I like your solution of having another
AutofillPopupDelegate better though. Looks like slightly more code, but one less
level of indirection is worth it.

Agent migration seems reasonable as well, though I wouldn't do it in this CL.

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:11: #include
"components/password_manager/core/browser/password_autofill_manager.h"
Is this necessary?

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/content_password_manager_driver.cc (right):

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
chrome/browser/password_manager/content_password_manager_driver.cc:54:
ContentPasswordManagerDriver::AcceptPasswordAutofillSuggestion(
It looks like the "void" could be on the same line.

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/content_password_manager_driver.h (right):

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
chrome/browser/password_manager/content_password_manager_driver.h:18: class
AutofillManagerDelegate;
Is this necessary?

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.cc:234:
driver_->GetPasswordAutofillManager()->Reset();
This should only happen if the navigation is not in page.

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.cc:317: void
PasswordManager::OnAddPasswordFormMapping(
Any reason why you are doing Driver -> PasswordManager ->
PasswordAutofillManager instead of just Driver->PasswordAutofillManager?

[Patrick Dubroy, 2014-03-12 16:47:08]

> It looks like this patch is in an in between state (e.g.
> autofill::PasswordAutofillManager hasn't been deleted yet,
> IPCs haven't been moved) so I didn't get into to much detail
> yet on the review.

I've cleaned things up a bit more, so I think it's ready for a closer look.

What do you mean by "IPCs" haven't been moved? You mean moved out of
autofill_messages.h? I left them there because the other IPCs that are
handled/sent from the password_manager component are also still there. However,
I can move and rename them all if you'd like.

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/chrome_password_manager_client.h (right):

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
chrome/browser/password_manager/chrome_password_manager_client.h:11: #include
"components/password_manager/core/browser/password_autofill_manager.h"
On 2014/03/12 01:09:22, Garrett Casto wrote:
> Is this necessary?

Whoops, fixed.

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/content_password_manager_driver.cc (right):

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
chrome/browser/password_manager/content_password_manager_driver.cc:54:
ContentPasswordManagerDriver::AcceptPasswordAutofillSuggestion(
On 2014/03/12 01:09:22, Garrett Casto wrote:
> It looks like the "void" could be on the same line.

Yeah, not sure what happened. Fixed.

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
File chrome/browser/password_manager/content_password_manager_driver.h (right):

https://codereview.chromium.org/184103016/diff/110001/chrome/browser/password...
chrome/browser/password_manager/content_password_manager_driver.h:18: class
AutofillManagerDelegate;
On 2014/03/12 01:09:22, Garrett Casto wrote:
> Is this necessary?

Fixed.

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.cc:234:
driver_->GetPasswordAutofillManager()->Reset();
On 2014/03/12 01:09:22, Garrett Casto wrote:
> This should only happen if the navigation is not in page.

Done.

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.cc:317: void
PasswordManager::OnAddPasswordFormMapping(
On 2014/03/12 01:09:22, Garrett Casto wrote:
> Any reason why you are doing Driver -> PasswordManager ->
> PasswordAutofillManager instead of just Driver->PasswordAutofillManager?

Do you mean why is this function defined in PasswordManager, rather than
directly in ContentPasswordManagerDriver?

I put it here to directly parallel the old code, and also because all the other
IPC handlers are here. However, if you think it's better to just put it in the
driver, I can do that too.

[Garrett Casto, 2014-03-14 08:25:21]

Looks good. Just a few minor nits. Keeping the IPCs definitions in autofill::
for now makes sense, I just misunderstood your comment about moving the IPCs to
mean moving the definitions as well as the handlers.

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.cc:317: void
PasswordManager::OnAddPasswordFormMapping(
On 2014/03/12 16:47:09, Patrick Dubroy wrote:
> On 2014/03/12 01:09:22, Garrett Casto wrote:
> > Any reason why you are doing Driver -> PasswordManager ->
> > PasswordAutofillManager instead of just Driver->PasswordAutofillManager?
> 
> Do you mean why is this function defined in PasswordManager, rather than
> directly in ContentPasswordManagerDriver?
> 
> I put it here to directly parallel the old code, and also because all the
other
> IPC handlers are here. However, if you think it's better to just put it in the
> driver, I can do that too.
> 

I guess it depends on how you think the code should be organized. I was thinking
of the *Manager classes as peers, not the PasswordManager as the primary manager
that delegated to the others. That being said, the fact that we are going to
only monitor for navigation in this class leans towards having the
PasswordManager as primary. Seems like the indirection isn't really getting you
anything, but I'll accept it either way.

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

https://codereview.chromium.org/184103016/diff/190001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:154: } else if
(identifier == POPUP_ITEM_ID_PASSWORD_ENTRY) {
Seems like eventually this should be removed, but I'm fine leaving it for extra
verification for now.

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

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:76: /*
FIXME: Do we need this? It's only used for metrics logging.
Having some logging here might eventually be interesting, but we certainly don't
need it now.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:87: //
FIXME: Can we get rid of this?
If you mean this interface function, I don't think so. It's used by
AutofillPopupControllerImpl.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:111: // 
driver_->RendererShouldClearPreviewedForm(); // FIXME: Do we need this?
No, password autofill doesn't preview at the moment. There is a longstanding bug
for this (crbug.com/63421).

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:13: //
http://crbug.com/51644
I can't tell if this should be updated or removed.
chrome/renderer/autofill/password_autofill_manager obviously doesn't exist
anymore, and the bug referenced here is closed. It actually seems like the
description of the bug more closely matches 118601, which is referenced below.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:65: //
TODO(csharp): Modify the AutofillExternalDeletegate code so that it can
Update comment.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
File
components/password_manager/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:34:
// TODO(dubroy): Implement a TestPasswordManagerDriver that can be shared by
Want to file a bug for this? Having a canonical TestPasswordManagerDriver and
TestPasswordManagerClient would be very useful.

[Patrick Dubroy, 2014-03-14 12:07:18]

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/184103016/diff/110001/components/password_man...
components/password_manager/core/browser/password_manager.cc:317: void
PasswordManager::OnAddPasswordFormMapping(
On 2014/03/14 08:25:22, Garrett Casto wrote:
> On 2014/03/12 16:47:09, Patrick Dubroy wrote:
> > On 2014/03/12 01:09:22, Garrett Casto wrote:
> > > Any reason why you are doing Driver -> PasswordManager ->
> > > PasswordAutofillManager instead of just Driver->PasswordAutofillManager?
> > 
> > Do you mean why is this function defined in PasswordManager, rather than
> > directly in ContentPasswordManagerDriver?
> > 
> > I put it here to directly parallel the old code, and also because all the
> other
> > IPC handlers are here. However, if you think it's better to just put it in
the
> > driver, I can do that too.
> > 
> 
> I guess it depends on how you think the code should be organized. I was
thinking
> of the *Manager classes as peers, not the PasswordManager as the primary
manager
> that delegated to the others. That being said, the fact that we are going to
> only monitor for navigation in this class leans towards having the
> PasswordManager as primary. Seems like the indirection isn't really getting
you
> anything, but I'll accept it either way.

Makes sense. Fixed to be as you described.

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

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:76: /*
FIXME: Do we need this? It's only used for metrics logging.
On 2014/03/14 08:25:22, Garrett Casto wrote:
> Having some logging here might eventually be interesting, but we certainly
don't
> need it now.

Done.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:87: //
FIXME: Can we get rid of this?
On 2014/03/14 08:25:22, Garrett Casto wrote:
> If you mean this interface function, I don't think so. It's used by
> AutofillPopupControllerImpl.

Ah, I see. That's kind of unfortunate, but what can you do.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:111: // 
driver_->RendererShouldClearPreviewedForm(); // FIXME: Do we need this?
On 2014/03/14 08:25:22, Garrett Casto wrote:
> No, password autofill doesn't preview at the moment. There is a longstanding
bug
> for this (crbug.com/63421).

Done.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:13: //
http://crbug.com/51644
On 2014/03/14 08:25:22, Garrett Casto wrote:
> I can't tell if this should be updated or removed.
> chrome/renderer/autofill/password_autofill_manager obviously doesn't exist
> anymore, and the bug referenced here is closed. It actually seems like the
> description of the bug more closely matches 118601, which is referenced below.

Removed, because I'm pretty sure this is obsolete.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:65: //
TODO(csharp): Modify the AutofillExternalDeletegate code so that it can
On 2014/03/14 08:25:22, Garrett Casto wrote:
> Update comment.

I think this is also obsolete -- removed.

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
File
components/password_manager/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:34:
// TODO(dubroy): Implement a TestPasswordManagerDriver that can be shared by
On 2014/03/14 08:25:22, Garrett Casto wrote:
> Want to file a bug for this? Having a canonical TestPasswordManagerDriver and
> TestPasswordManagerClient would be very useful.

Done.

[Patrick Dubroy, 2014-03-14 12:11:14]

+blundell for owner review of the components/components_tests.gyp change.

[blundell, 2014-03-14 12:15:18]

components_tests.gyp LGTM

[Garrett Casto, 2014-03-14 23:24:21]

LGTM

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/190001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:13: //
http://crbug.com/51644
On 2014/03/14 12:07:19, Patrick Dubroy wrote:
> On 2014/03/14 08:25:22, Garrett Casto wrote:
> > I can't tell if this should be updated or removed.
> > chrome/renderer/autofill/password_autofill_manager obviously doesn't exist
> > anymore, and the bug referenced here is closed. It actually seems like the
> > description of the bug more closely matches 118601, which is referenced
below.
> 
> Removed, because I'm pretty sure this is obsolete.

FWIW, the explanatory text and crbug.com/118601 describe an issue that still
exists. That is, we have the exact same mapping stored in a 
LoginToPasswordInfoMap in both the renderer and the browser. It probably doesn't
deserve a top level comment like this though.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_manager.h:31: namespace gfx {
Unneeded.

[Ilya Sherman, 2014-03-18 00:14:27]

I only skimmed the new //components/password_manager files.  Let me know if
you'd like me to look at those more closely.

I mostly just have nits, but I do have a question about functionality changes in
the password_autofill_agent.cc code:

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/common/autofill_messages.h:141: base::string16 /*
password value */)
nit: The commented out bits should be variable names.  (This file is currently
not consistent about that, but that's the desired state.)

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:364:
SetElementAutofilled(&username_element, true);
Hmm, this is missing IsElementAutocompletable() checks, and fails to set the
selection for the username field.  Are these intentional changes in behavior?

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:42: // the
username and password fields were filled, false otherwise.
nit: Please update this comment.

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:43: bool
AcceptAutofillSuggestionWithPassword(const blink::WebNode& node,
Optional nit: I'd shorten this name to just "AcceptSuggestion", but the longer
name is fine too if you prefer it.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:56:
DCHECK(web_contents());
nit: This seems redundant with calling a method on web_contents() below, and is
surprising here given that the other similar methods don't have such a check.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:45: void
OnShowPasswordSuggestions(
nit: Docs, please.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:56:
AcceptAutofillSuggestion);
Please avoid friending tests.  Use protected methods overridden by test-specific
subclasses instead.  This reduces the degree to which tests can mess with
internal state, which helps keep tests more sane.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:86:
autofill::FormFieldData form_field_;
nit: Docs.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:61:
void(const base::string16&, const base::string16&));
nit: Why add these mock methods if they're never used?

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File
components/password_manager/core/browser/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_generation_manager_unittest.cc:64:
}
nit: Do you need to add these methods?  It doesn't look like they're ever used.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:71:
MOCK_METHOD0(GetPasswordAutofillManager, PasswordAutofillManager*());
nit: Unneeded?

[Patrick Dubroy, 2014-03-28 15:44:21]

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/common/autofill_messages.h:141: base::string16 /*
password value */)
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: The commented out bits should be variable names.  (This file is currently
> not consistent about that, but that's the desired state.)

Ok, fixed here.

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:364:
SetElementAutofilled(&username_element, true);
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> Hmm, this is missing IsElementAutocompletable() checks, and fails to set the
> selection for the username field.  Are these intentional changes in behavior?

No, that was an oversight. Fixed.

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:42: // the
username and password fields were filled, false otherwise.
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:43: bool
AcceptAutofillSuggestionWithPassword(const blink::WebNode& node,
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> Optional nit: I'd shorten this name to just "AcceptSuggestion", but the longer
> name is fine too if you prefer it.

I shortened to just AcceptAutofillSuggestion, if that's ok. Otherwise I think
AcceptSuggestion, in the context of passwords, is ambiguous -- it could mean an
autofill suggestion, or it could mean a generated password. At least, Vaclav and
I both felt it could be confusing.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:56:
DCHECK(web_contents());
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: This seems redundant with calling a method on web_contents() below, and
is
> surprising here given that the other similar methods don't have such a check.

Well, some have it, some don't. But, I agree that there's little point, so I
removed it.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:45: void
OnShowPasswordSuggestions(
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:45: void
OnShowPasswordSuggestions(
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:56:
AcceptAutofillSuggestion);
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> Please avoid friending tests.  Use protected methods overridden by
test-specific
> subclasses instead.  This reduces the degree to which tests can mess with
> internal state, which helps keep tests more sane.

How about adding a public ...ForTest() method? I think that's much nicer than
making the method virtual and protected, and having to create a subclass in the
test file just to change the visibility of the method.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:86:
autofill::FormFieldData form_field_;
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Docs.

Done.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:61:
void(const base::string16&, const base::string16&));
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Why add these mock methods if they're never used?

They are currently pure virtuals and need to be overridden in all mocks. I
already opened a bug to fix this by implementing a TestPasswordManagerDriver
that contains empty implementations of all the pure virtual functions:
http://crbug.com/352566.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File
components/password_manager/core/browser/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_generation_manager_unittest.cc:64:
}
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Do you need to add these methods?  It doesn't look like they're ever
used.

See previous comment.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_manager.h:31: namespace gfx {
On 2014/03/14 23:24:22, Garrett Casto wrote:
> Unneeded.

Done.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:71:
MOCK_METHOD0(GetPasswordAutofillManager, PasswordAutofillManager*());
On 2014/03/18 00:14:27, Ilya Sherman wrote:
> nit: Unneeded?

Yes, pure virtuals. Will fix in a future CL as mentioned above.

[Ilya Sherman, 2014-03-28 21:33:33]

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:43: bool
AcceptAutofillSuggestionWithPassword(const blink::WebNode& node,
On 2014/03/28 15:44:22, Patrick Dubroy wrote:
> On 2014/03/18 00:14:27, Ilya Sherman wrote:
> > Optional nit: I'd shorten this name to just "AcceptSuggestion", but the
longer
> > name is fine too if you prefer it.
> 
> I shortened to just AcceptAutofillSuggestion, if that's ok. Otherwise I think
> AcceptSuggestion, in the context of passwords, is ambiguous -- it could mean
an
> autofill suggestion, or it could mean a generated password. At least, Vaclav
and
> I both felt it could be confusing.

Since this class is the password *autofill* agent, I honestly don't think it's
especially helpful to include "Autofill" in method names -- especially since
this class doesn't actually deal with generated password suggestions and since
there's a method named "ShowSuggestions()" below.  But, this also isn't cleanup
that needs to happen as part of this CL, especially if you and Vaclav both think
it provides some clarity.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:56:
AcceptAutofillSuggestion);
On 2014/03/28 15:44:22, Patrick Dubroy wrote:
> On 2014/03/18 00:14:27, Ilya Sherman wrote:
> > Please avoid friending tests.  Use protected methods overridden by
> test-specific
> > subclasses instead.  This reduces the degree to which tests can mess with
> > internal state, which helps keep tests more sane.
> 
> How about adding a public ...ForTest() method? I think that's much nicer than
> making the method virtual and protected, and having to create a subclass in
the
> test file just to change the visibility of the method.
> 

I personally prefer protected methods, but a public ...ForTest method is also ok
if the active owners of this component prefer it.  Both are IMO much better than
friending test code.

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/184103016/diff/200037/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:61:
void(const base::string16&, const base::string16&));
On 2014/03/28 15:44:22, Patrick Dubroy wrote:
> On 2014/03/18 00:14:27, Ilya Sherman wrote:
> > nit: Why add these mock methods if they're never used?
> 
> They are currently pure virtuals and need to be overridden in all mocks. I
> already opened a bug to fix this by implementing a TestPasswordManagerDriver
> that contains empty implementations of all the pure virtual functions:
> http://crbug.com/352566.

I'd prefer that you simply override the methods with empty bodies if they don't
actually need to be mocked, but I won't push super hard for this.

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

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:364:
IsElementAutocompletable(password_info.password_field)) {
nit: I would invert this condition and include an early return stmt, so that the
body of the method can be less indented.

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:376: }
Could you please add test coverage for this method?  Ideally the tests should
have been able to catch the lack of IsElementAutocompletable() calls or the
forgotten setSelectionRange() call.  (I apologize that there wasn't already test
coverage in place for this.)

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

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:42: // Returns
true if the fields were filled, false otherwise.
This doesn't match what the implementation currently does.  Specifically, the
method returns true but the fields remain unfilled if either element is
unfillable.

https://codereview.chromium.org/184103016/diff/220001/components/password_man...
File components/password_manager/core/browser/password_autofill_manager.h
(right):

https://codereview.chromium.org/184103016/diff/220001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:46: //
|suggestions| from the password manager.
What about |bounds| and |realms|?

https://codereview.chromium.org/184103016/diff/220001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.h:60: }
nit: Please don't inline this method body -- only trivial accessors should be
inlined.

[Patrick Dubroy, 2014-04-01 16:08:47]

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

https://codereview.chromium.org/184103016/diff/200037/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:43: bool
AcceptAutofillSuggestionWithPassword(const blink::WebNode& node,
On 2014/03/28 21:33:34, Ilya Sherman wrote:
> On 2014/03/28 15:44:22, Patrick Dubroy wrote:
> > On 2014/03/18 00:14:27, Ilya Sherman wrote:
> > > Optional nit: I'd shorten this name to just "AcceptSuggestion", but the
> longer
> > > name is fine too if you prefer it.
> > 
> > I shortened to just AcceptAutofillSuggestion, if that's ok. Otherwise I
think
> > AcceptSuggestion, in the context of passwords, is ambiguous -- it could mean
> an
> > autofill suggestion, or it could mean a generated password. At least, Vaclav
> and
> > I both felt it could be confusing.
> 
> Since this class is the password *autofill* agent, I honestly don't think it's
> especially helpful to include "Autofill" in method names -- especially since
> this class doesn't actually deal with generated password suggestions and since
> there's a method named "ShowSuggestions()" below.  But, this also isn't
cleanup
> that needs to happen as part of this CL, especially if you and Vaclav both
think
> it provides some clarity.

Ok, I agree. Changed. (In fairness, Vaclav didn't really have the full context
when I asked him the question.)

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

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:364:
IsElementAutocompletable(password_info.password_field)) {
On 2014/03/28 21:33:34, Ilya Sherman wrote:
> nit: I would invert this condition and include an early return stmt, so that
the
> body of the method can be less indented.

Done.

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:376: }
On 2014/03/28 21:33:34, Ilya Sherman wrote:
> Could you please add test coverage for this method?  Ideally the tests should
> have been able to catch the lack of IsElementAutocompletable() calls or the
> forgotten setSelectionRange() call.  (I apologize that there wasn't already
test
> coverage in place for this.)

Done. In doing so, I did noticed some weird behavior of the old code though:

- The username is always filled, even if the username or password is not
autocompletable or the login doesn't match.
- In those situations, the username is not marked as autofilled.

I'm assuming those behaviors weren't intentional, so I've fixed them in my code.
Do you know of a reason that I shouldn't fix them?

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

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.h:42: // Returns
true if the fields were filled, false otherwise.
On 2014/03/28 21:33:34, Ilya Sherman wrote:
> This doesn't match what the implementation currently does.  Specifically, the
> method returns true but the fields remain unfilled if either element is
> unfillable.

Fixed (the implementation, not the comment).

[Garrett Casto, 2014-04-01 17:32:20]

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

https://codereview.chromium.org/184103016/diff/220001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:376: }
On 2014/04/01 16:08:48, Patrick Dubroy wrote:
> On 2014/03/28 21:33:34, Ilya Sherman wrote:
> > Could you please add test coverage for this method?  Ideally the tests
should
> > have been able to catch the lack of IsElementAutocompletable() calls or the
> > forgotten setSelectionRange() call.  (I apologize that there wasn't already
> test
> > coverage in place for this.)
> 
> Done. In doing so, I did noticed some weird behavior of the old code though:
> 
> - The username is always filled, even if the username or password is not
> autocompletable or the login doesn't match.
> - In those situations, the username is not marked as autofilled.
> 
> I'm assuming those behaviors weren't intentional, so I've fixed them in my
code.
> Do you know of a reason that I shouldn't fix them?
> 

I think that this was an oversight, but note that if this happens you are in a
pretty strange situation to begin with. The user has selected their credentials
from the UI, so not filling them is going to be confusing as well.  I'm not sure
if it matters much what we plan on doing at this point as this shouldn't happen
on well behaving sites unless we have a bug.

[Ilya Sherman, 2014-04-02 07:08:08]

LGTM, thanks!

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

https://codereview.chromium.org/184103016/diff/280001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:145: for
(autofill::PasswordFormFillData::LoginCollection::const_iterator iter =
nit: Alignment is slightly off

[Patrick Dubroy, 2014-04-02 08:25:34]

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

https://codereview.chromium.org/184103016/diff/280001/components/password_man...
components/password_manager/core/browser/password_autofill_manager.cc:145: for
(autofill::PasswordFormFillData::LoginCollection::const_iterator iter =
On 2014/04/02 07:08:09, Ilya Sherman wrote:
> nit: Alignment is slightly off

Done.

[Patrick Dubroy, 2014-04-02 15:01:59]

Garrett, can you take another look? I had to make a change to prevent a crash in
one of the unit tests. Easiest thing to do is to compare to patchset 13.

[Patrick Dubroy, 2014-04-02 15:02:46]

Avi, PTAL for owners review of tab_helpers.cc.
palmer - owners review of autofill_messages.h

[Garrett Casto, 2014-04-02 20:50:38]

On 2014/04/02 15:02:46, Patrick Dubroy wrote:
> Avi, PTAL for owners review of tab_helpers.cc.
> palmer - owners review of autofill_messages.h

LGTM

[Patrick Dubroy, 2014-04-03 13:53:00]

avi, palmer: ping.

[Avi (use Gerrit), 2014-04-03 14:59:42]

tab contents lgtm

[Patrick Dubroy, 2014-04-03 15:12:25]

nkostylev, PTAL at simple_web_view_dialog.cc change.

What is the password manager client required for in SimpleWebViewDialog? Is it
for autofilling passwords on captive portals?

[Nikita (slow), 2014-04-03 15:27:35]

On 2014/04/03 15:12:25, Patrick Dubroy wrote:
> nkostylev, PTAL at simple_web_view_dialog.cc change.
> 
> What is the password manager client required for in SimpleWebViewDialog? Is it
> for autofilling passwords on captive portals?

password manager client is not required for this dialog since captive portal
window is only used on Chrome OS login UI where there's no real user profile.

[Nikita (slow), 2014-04-03 15:28:21]

lgtm

[palmer, 2014-04-03 19:02:38]

IPC security LGTM.

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

https://codereview.chromium.org/184103016/diff/400001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:141: // Tells the
renderer that a password autofill suggestion was accepted, and
Nit: Say who does the accepting: "Tells the renderer that the user accepted an
autofill suggestion, ..."

https://codereview.chromium.org/184103016/diff/400001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:142: // that the username
and password should be set to the given values.
...and say who should to the setting: "...that the renderer should set the
username and password to the given values."

[Patrick Dubroy, 2014-04-23 17:57:29]

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

https://codereview.chromium.org/184103016/diff/400001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:141: // Tells the
renderer that a password autofill suggestion was accepted, and
On 2014/04/03 19:02:38, Chromium Palmer wrote:
> Nit: Say who does the accepting: "Tells the renderer that the user accepted an
> autofill suggestion, ..."

Done.

https://codereview.chromium.org/184103016/diff/400001/components/autofill/con...
components/autofill/content/common/autofill_messages.h:142: // that the username
and password should be set to the given values.
On 2014/04/03 19:02:38, Chromium Palmer wrote:
> ...and say who should to the setting: "...that the renderer should set the
> username and password to the given values."

Done.

[Patrick Dubroy, 2014-04-24 21:14:04]

Committed patchset #19 manually as r265993 (presubmit successful).