Do not haul suggestions back to browser in AutofillHostMsg_ShowPasswordSuggestions

Do not haul suggestions back to browser in AutofillHostMsg_ShowPasswordSuggestions

Currently, PasswordAutofillAgent (in renderer) prepares password autofill suggestions for PasswordAutofillManager (in browser) and sends them via IPC.

But the manager has the data to generate the suggestions itself, so this CL makes it derive the suggestions from that data instead of hauling it through IPC.

This CL also adds one more test case to the PasswordAutofillManager unittest, to compensate for the coverage lost in the PasswordAutofillAgent browsertest by moving some logic out of the agent to the manager.

BUG=400186, 377422, 118601
Committed: https://crrev.com/86cc798cc2947dd88aa73505d716f93538dffa5b
Cr-Commit-Position: refs/heads/master@{#302245}

[vabr (Chromium), 2014-10-30 18:32:10]

vabr@chromium.org changed reviewers:
+ gcasto@chromium.org, tsepez@chromium.org

[vabr (Chromium), 2014-10-30 18:33:35]

Hi Garret,

Could you please take a look at the whole CL?

Hi Tom,

Could you please check components/autofill/content/common/autofill_messages.h?

Thanks!
Vaclav

[Tom Sepez, 2014-10-30 18:45:16]

Messages LGTM.

[Garrett Casto, 2014-10-30 22:33:29]

LGTM

Bug 118601 is also related here, looks like a dupe of 377422. 

We are going to want to put together a design doc on this soon if we really want
to seriously pursue removing knowledge of passwords from the renderer. Certainly
this is one way, perhaps the best way, of accomplishing the goal of slimming
down and focusing PasswordAutofillAgent, but I think that there are going to be
some complications. The biggest one that I can think of is how would password
autofill suppress Autofill? The obvious parallel would be to have AutofillAgent
ask AutofillManager to show suggestions which delegates to
PasswordAutofillManager, but that introduces a new cross component dependency
(currently password_manager depends on autofill, not the other way around). We
could introduce an interface to get around this, but one doesn't exist at the
moment.

I think that this CL moves us in the right direction, but only if we are
actually going to follow through and start removing complexity from
PasswordAutofillAgent. As is, because of the suppression problem, this CL
increases complexity in the short term.

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

https://codereview.chromium.org/688633004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:264: // No other
possible usernames, but perhaps at least some suggestion?
Nit: Personally I don't think that these comments are necessary given the
simplicity of the code and the function comment, but if you want to keep it I
would change it to be a statement not a question.

https://codereview.chromium.org/688633004/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/688633004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_autofill_manager.cc:117:
LoginToPasswordInfoMap::const_iterator fill_data =
nit: Normally iterators are named in such a way that makes them easy to
distinguish (_iter, _it, etc.). |fill_data| is slightly confusing, especially as
you use |fill_data| above for an actual PasswordFormFillData object.

[vabr (Chromium), 2014-10-31 08:53:56]

Thanks Garrett and Tom for your quick reviews!

More answers for Garrett::
On 2014/10/30 22:33:29, Garrett Casto wrote:
> LGTM
> 
> Bug 118601 is also related here, looks like a dupe of 377422. 
Thanks, noted in the description. I did not dupe those bugs yet, because 377422
seems more specific to me.

> 
> We are going to want to put together a design doc on this soon if we really
want
> to seriously pursue removing knowledge of passwords from the renderer.
Certainly
> this is one way, perhaps the best way, of accomplishing the goal of slimming
> down and focusing PasswordAutofillAgent, but I think that there are going to
be
> some complications. The biggest one that I can think of is how would password
> autofill suppress Autofill? The obvious parallel would be to have
AutofillAgent
> ask AutofillManager to show suggestions which delegates to
> PasswordAutofillManager, but that introduces a new cross component dependency
> (currently password_manager depends on autofill, not the other way around). We
> could introduce an interface to get around this, but one doesn't exist at the
> moment.
I'm afraid I should have been clear on my motives for this. While this CL is
relevant to the task of removing (parts of) password knowledge from the
renderer, my main motivation was http://crbug.com/400186, the refactoring I
still plan for Q4 where the PasswordAutofillAgent should be converted to observe
RenderFrame, split into two classes based on function (form detection vs.
filling), and if possible, slimmed down. While for the first two steps
(RenderFrame + split in two) I am still working on a design doc, I did start
(with this CL) the work on removing unnecessary stuff from the IPC interface.
This removing seemed like a good idea independently of the rest, and I'd like to
do more around the code which moves PasswordFormFillData from
ContentPasswordManagerDriver to PasswordAutofillManager unnecessarily through
the renderer code.


> 
> I think that this CL moves us in the right direction, but only if we are
> actually going to follow through and start removing complexity from
> PasswordAutofillAgent. As is, because of the suppression problem, this CL
> increases complexity in the short term.
I'm not sure I understand you 100% here, but I do plan to remove more complexity
from PasswordAutofillAgent, so hopefully I'm not causing damage to the code
structure. I'll loop you in the CLs following up directly on this one, and I
hope to advance on the design doc for http://crbug.com/400186 (which I'll link
there soon). Happy to have a chat with you if you'd like to speak about any of
this.

Cheers,
Vaclav

> 
>
https://codereview.chromium.org/688633004/diff/1/components/autofill/content/...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/688633004/diff/1/components/autofill/content/...
> components/autofill/content/renderer/password_autofill_agent.cc:264: // No
other
> possible usernames, but perhaps at least some suggestion?
> Nit: Personally I don't think that these comments are necessary given the
> simplicity of the code and the function comment, but if you want to keep it I
> would change it to be a statement not a question.
> 
>
https://codereview.chromium.org/688633004/diff/1/components/password_manager/...
> File components/password_manager/core/browser/password_autofill_manager.cc
> (right):
> 
>
https://codereview.chromium.org/688633004/diff/1/components/password_manager/...
> components/password_manager/core/browser/password_autofill_manager.cc:117:
> LoginToPasswordInfoMap::const_iterator fill_data =
> nit: Normally iterators are named in such a way that makes them easy to
> distinguish (_iter, _it, etc.). |fill_data| is slightly confusing, especially
as
> you use |fill_data| above for an actual PasswordFormFillData object.

[vabr (Chromium), 2014-10-31 08:54:12]

Comments addressed, sending to the CQ.

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

https://codereview.chromium.org/688633004/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_autofill_agent.cc:264: // No other
possible usernames, but perhaps at least some suggestion?
On 2014/10/30 22:33:29, Garrett Casto wrote:
> Nit: Personally I don't think that these comments are necessary given the
> simplicity of the code and the function comment, but if you want to keep it I
> would change it to be a statement not a question.

Done, by removing, also the one on line 253.
I'm sometimes prone to over-commenting, so it's good to have another person
assessing the real need for the comment. :)

https://codereview.chromium.org/688633004/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/688633004/diff/1/components/password_manager/...
components/password_manager/core/browser/password_autofill_manager.cc:117:
LoginToPasswordInfoMap::const_iterator fill_data =
On 2014/10/30 22:33:29, Garrett Casto wrote:
> nit: Normally iterators are named in such a way that makes them easy to
> distinguish (_iter, _it, etc.). |fill_data| is slightly confusing, especially
as
> you use |fill_data| above for an actual PasswordFormFillData object.

Done.

[vabr (Chromium), 2014-10-31 08:54:16]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-10-31 08:55:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688633004/20001

[commit-bot: I haz the power, 2014-10-31 09:36:14]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-31 09:36:49]

Patchset 2 (id:??) landed as
https://crrev.com/86cc798cc2947dd88aa73505d716f93538dffa5b
Cr-Commit-Position: refs/heads/master@{#302245}

[jww, 2014-11-01 01:10:03]

jww@chromium.org changed reviewers:
+ jww@chromium.org

[jww, 2014-11-01 01:10:04]

https://codereview.chromium.org/688633004/diff/20001/components/password_mana...
File components/password_manager/core/browser/password_autofill_manager.cc
(right):

https://codereview.chromium.org/688633004/diff/20001/components/password_mana...
components/password_manager/core/browser/password_autofill_manager.cc:124: if
(!autofill::IsValidString16Vector(suggestions) ||
Drive by comment: Seems like this validation isn't really doing much good here;
shouldn't the validation be on |typed_username|, since that's the untrusted
content? By the same token, GetSuggestions() returns trusted data, right, so why
validate it at this point?

[kareng, 2014-11-03 04:14:48]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/695233002/ by kareng@google.com.

The reason for reverting is: caused crashers. see bug 429576.