Generate passwords only for forms that autofill server marks as account creation forms.

Generate passwords only for the forms that autofill server classifies one of its fields as ACCOUNT_CREATION_PASSWORD.

BUG=

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

[zysxqn, 2013-08-26 18:58:34]

Generate passwords only for the forms that autofill server classifies one of its
fields as ACCOUNT_CREATION_FORM.

[Garrett Casto, 2013-08-29 22:45:13]

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_generation_manager.cc (right):

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager.cc:71:
account_creation_form_origins.push_back(form->source_url());
So I don't have an example to point to right now, but I remember us occasionally
triggering on the wrong form on a page. For instance, say the sign up and login
forms are on the same page, and the login page has "hidden" fields which aren't
actually type="hidden". In this case, specifying the exact form you mean would
be useful, so I think that we should probably send a vector of FormData's down
instead. We can switch the actual comparison in a later CL, as soon we should be
able to easily get FormData from PasswordForm.

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager_unittest.cc:215:
form1.fields_.push_back(field);
You should probably construct a FormData and then create a FormStructure from
it. Not only will that remove the need for the friend declaration in
FormStructure, but it's closer to what actually happens. For instance, right now
FormStructure will never parse password forms because of
FormStructure::ShouldSkipField. Ideally I would also like a BrowserTest where we
mock out the servers response, navigate to a page, and make sure that we show
the icon. We don't need to do that in this CL though.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/DEPS (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/DEPS:15:
"!chrome/browser/password_manager/password_generation_manager.h",
I don't think that this is okay, but there should be ways around this.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:104: // they are here primarly
for use by Autocheckout.
Were these changes that had been made on the server side that hadn't been made
in Chrome yet?

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:108: PASSWORD = 75,
These probably deserve a comment. Something like "Field types for password
fields. ACCOUNT_CREATION_PASSWORD fields can trigger password generation."
(wording is pretty rough, feel free to clean it up).

[Ilya Sherman, 2013-08-29 23:08:13]

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_generation_manager.cc (right):

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager.cc:68:
form->begin(); field_it != form->end(); ++field_it) {
nit: Indent this line 5 more spaces.

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager.cc:180: }
nit: Why not just inline this method?

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
File components/autofill/content/renderer/password_generation_manager.cc
(right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.cc:214:
origins.end());
Is it expected to be possible that autofill_acount_creation_form_origins_ might
not be empty when this code is reached?  If so, why?

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.cc:221: // is
not blacklisted by the users, and the autofill server has marked one
nit: "autofill" -> "Autofill"

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.cc:238: }
nit: I'd write this as an early return using a wrapper function around
std::find:

bool ContainsURL(const std::vector<GURL>& urls, const GURL& url) {
  return std::find(urls.begin(), urls.end(), url) != urls.end();
}

// Only show the icon for non-blacklisted forms:
if (!ContainsURL(not_blacklisted_password_form_origins_,
                 account_creation_form_origin_))
  return;

You can write something similar to replace the loop below as well.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
File components/autofill/content/renderer/password_generation_manager.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.h:69: GURL
account_creation_form_origin_;
nit: Perhaps update this name to |possible_account_creation_form_origin_|?  It
currently seems pretty weird to see a loop scanning through
autofill_account_creation_form_origins_ looking for
account_creation_form_origin_ -- at first glance, that looks like that search
should always succeed, because of the semantics implied by the names.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.h:77: // one of
its fields as ACCOUNT_CREATION_PASSWORD.
nit: Several small wording tweaks: "Stores the origin of each password form for
which the Autofill server classifies one of the form's fields as an
ACCOUNT_CREATION_PASSWORD."

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/DEPS (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/DEPS:15:
"!chrome/browser/password_manager/password_generation_manager.h",
This is not an acceptable dependency.  Please find a way to avoid it, possibly
via one of the delegate interfaces like AutofillManagerDelegate.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager.cc:754: }
nit: No need for curly braces.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:106: MERCHANT_PROMO_CODE = 74,
Why are these being added as part of this CL?  They don't seem relevant to the
change that you're making, and Autocheckout is deprecated.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:109: ACCOUNT_CREATION_PASSWORD =
76,
Seems like PASSWORD should be less generic given that another option is the more
specific ACCOUNT_CREATION_PASSWORD.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:109: ACCOUNT_CREATION_PASSWORD =
76,
Have these changes already been made on the server side?

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:193: PASSWORD_FIELD,
nit: This would be a more appropriate place to use the constant name PASSWORD.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/form_structure.h:180:
DetectAccountCreationForms);
What exactly do you need to use from the private section of this class?  It's
better to find other ways to expose that information than friending tests,
especially given that the test that you're friending is defined outside of the
autofill component.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/comm...
File components/autofill/core/common/autofill_messages.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_messages.h:193:
IPC_MESSAGE_ROUTED1(AutofillMsg_AccountCreationFormsDetected,
nit: Please add documentation for this IPC message.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_messages.h:194: std::vector<GURL> /*
form origins */)
nit: The inline comment should be a variable name.

[zysxqn, 2013-09-03 23:00:20]

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_generation_manager.cc (right):

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager.cc:68:
form->begin(); field_it != form->end(); ++field_it) {
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: Indent this line 5 more spaces.

Done.

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager.cc:71:
account_creation_form_origins.push_back(form->source_url());
On 2013/08/29 22:45:13, Garrett Casto wrote:
> So I don't have an example to point to right now, but I remember us
occasionally
> triggering on the wrong form on a page. For instance, say the sign up and
login
> forms are on the same page, and the login page has "hidden" fields which
aren't
> actually type="hidden". In this case, specifying the exact form you mean would
> be useful, so I think that we should probably send a vector of FormData's down
> instead. We can switch the actual comparison in a later CL, as soon we should
be
> able to easily get FormData from PasswordForm.

Done.

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager.cc:180: }
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: Why not just inline this method?

I don't have strong preference on this one but I use a separate method for the
following reasons:

(1) To be consistent with SendStateToRenderer()

(2) It's a bit cleaner since we need to refer to "host" twice in the method.

Are you fine with this?

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/23432002/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_generation_manager_unittest.cc:215:
form1.fields_.push_back(field);
On 2013/08/29 22:45:13, Garrett Casto wrote:
> You should probably construct a FormData and then create a FormStructure from
> it. Not only will that remove the need for the friend declaration in
> FormStructure, but it's closer to what actually happens. For instance, right
now
> FormStructure will never parse password forms because of
> FormStructure::ShouldSkipField. Ideally I would also like a BrowserTest where
we
> mock out the servers response, navigate to a page, and make sure that we show
> the icon. We don't need to do that in this CL though.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
File components/autofill/content/renderer/password_generation_manager.cc
(right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.cc:214:
origins.end());
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> Is it expected to be possible that autofill_acount_creation_form_origins_
might
> not be empty when this code is reached?  If so, why?

I'm not 100% sure about the flow but account_creation_forms_ is only cleared
once the main frame is loaded. So if there are account creation forms in other
frames (rare? I'm not sure) it might not be empty here. I think we use
"push_back" for the same reason in OnFormNotBlacklisted() above.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.cc:221: // is
not blacklisted by the users, and the autofill server has marked one
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill"

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.cc:238: }
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: I'd write this as an early return using a wrapper function around
> std::find:
> 
> bool ContainsURL(const std::vector<GURL>& urls, const GURL& url) {
>   return std::find(urls.begin(), urls.end(), url) != urls.end();
> }
> 
> // Only show the icon for non-blacklisted forms:
> if (!ContainsURL(not_blacklisted_password_form_origins_,
>                  account_creation_form_origin_))
>   return;
> 
> You can write something similar to replace the loop below as well.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
File components/autofill/content/renderer/password_generation_manager.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.h:69: GURL
account_creation_form_origin_;
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: Perhaps update this name to |possible_account_creation_form_origin_|?  It
> currently seems pretty weird to see a loop scanning through
> autofill_account_creation_form_origins_ looking for
> account_creation_form_origin_ -- at first glance, that looks like that search
> should always succeed, because of the semantics implied by the names.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/content/r...
components/autofill/content/renderer/password_generation_manager.h:77: // one of
its fields as ACCOUNT_CREATION_PASSWORD.
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: Several small wording tweaks: "Stores the origin of each password form
for
> which the Autofill server classifies one of the form's fields as an
> ACCOUNT_CREATION_PASSWORD."

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/DEPS (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/DEPS:15:
"!chrome/browser/password_manager/password_generation_manager.h",
On 2013/08/29 22:45:13, Garrett Casto wrote:
> I don't think that this is okay, but there should be ways around this.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/DEPS:15:
"!chrome/browser/password_manager/password_generation_manager.h",
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> This is not an acceptable dependency.  Please find a way to avoid it, possibly
> via one of the delegate interfaces like AutofillManagerDelegate.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_manager.cc:754: }
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: No need for curly braces.

NA now.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/field_types.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:104: // they are here primarly
for use by Autocheckout.
On 2013/08/29 22:45:13, Garrett Casto wrote:
> Were these changes that had been made on the server side that hadn't been made
> in Chrome yet?

Yes.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:106: MERCHANT_PROMO_CODE = 74,
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> Why are these being added as part of this CL?  They don't seem relevant to the
> change that you're making, and Autocheckout is deprecated.

They have been added on the server side but haven't used anywhere in the client
side. To keep them consistent I have to add them here.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:108: PASSWORD = 75,
On 2013/08/29 22:45:13, Garrett Casto wrote:
> These probably deserve a comment. Something like "Field types for password
> fields. ACCOUNT_CREATION_PASSWORD fields can trigger password generation."
> (wording is pretty rough, feel free to clean it up).

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:109: ACCOUNT_CREATION_PASSWORD =
76,
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> Seems like PASSWORD should be less generic given that another option is the
more
> specific ACCOUNT_CREATION_PASSWORD.

I think it is by design. We will vote a password field as PASSWORD every time we
submit a password form. We will then vote a password field (should already be
marked as PASSWORD) as ACCOUNT_CREATION_PASSWORD when we have more evidence that
it is an account creation password (i.e. when it is first used to login). So
PASSWORD type should be more generic.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:109: ACCOUNT_CREATION_PASSWORD =
76,
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> Have these changes already been made on the server side?

Yes.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/field_types.h:193: PASSWORD_FIELD,
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: This would be a more appropriate place to use the constant name PASSWORD.

See my comments above. Are you fine with leave it as is. If later we find better
naming we can submit a CL to change all of them altogether consistently.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/brow...
components/autofill/core/browser/form_structure.h:180:
DetectAccountCreationForms);
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> What exactly do you need to use from the private section of this class?  It's
> better to find other ways to expose that information than friending tests,
> especially given that the test that you're friending is defined outside of the
> autofill component.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/comm...
File components/autofill/core/common/autofill_messages.h (right):

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_messages.h:193:
IPC_MESSAGE_ROUTED1(AutofillMsg_AccountCreationFormsDetected,
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: Please add documentation for this IPC message.

Done.

https://codereview.chromium.org/23432002/diff/1/components/autofill/core/comm...
components/autofill/core/common/autofill_messages.h:194: std::vector<GURL> /*
form origins */)
On 2013/08/29 23:08:13, Ilya Sherman wrote:
> nit: The inline comment should be a variable name.

Done.

[Ilya Sherman, 2013-09-03 23:57:56]

LGTM % nits, but please wait for Garrett's review as well.  Thanks.

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/password_m...
File chrome/browser/password_manager/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/password_m...
chrome/browser/password_manager/password_generation_manager_unittest.cc:255:
const char * const kServerResponse =
nit: "char *" -> "char*"

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:267: }
nit: No need for curly braces.

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:13: 
nit: Spurious newline.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
File components/autofill/content/renderer/password_generation_manager.cc
(right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:76: // TODO:
use more signals than just origin to compare.
nit: TODO(zysxqn):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:86: }
nit: No need for curly braces.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:235:
account_creation_forms_.begin(), forms.begin(), forms.end());
nit: Shouldn't be a big deal in practice, but it's more efficient to append to
the end of a vector than to insert at the front.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
File components/autofill/content/renderer/password_generation_manager.h (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.h:82: // Stores
the password form for which the Autofill server classifies one of
nit: "Stores the" -> "Stores each"

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
components/autofill/core/browser/autofill_manager.cc:748: // Autofill Manager
Delegate) to detect account creation forms.
nit: IMO drop the parenthetical phrase.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
File components/autofill/core/browser/form_structure_unittest.cc (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
components/autofill/core/browser/form_structure_unittest.cc:2429:
field.form_control_type = "password";
nit: Please update the field type to be a checkable type (and update the label
and name as well to match).

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
File components/autofill/core/common/autofill_messages.h (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
components/autofill/core/common/autofill_messages.h:192: // Sent when autofill
manager gets the query response from the autofill server
nit: "autofill" -> "Autofill"

[zysxqn, 2013-09-04 17:26:19]

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/password_m...
File chrome/browser/password_manager/password_generation_manager_unittest.cc
(right):

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/password_m...
chrome/browser/password_manager/password_generation_manager_unittest.cc:255:
const char * const kServerResponse =
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: "char *" -> "char*"

Done.

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:267: }
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: No need for curly braces.

Done.

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://codereview.chromium.org/23432002/diff/13001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:13: 
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: Spurious newline.

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
File components/autofill/content/renderer/password_generation_manager.cc
(right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:76: // TODO:
use more signals than just origin to compare.
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: TODO(zysxqn):

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:86: }
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: No need for curly braces.

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:235:
account_creation_forms_.begin(), forms.begin(), forms.end());
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: Shouldn't be a big deal in practice, but it's more efficient to append to
> the end of a vector than to insert at the front.

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
File components/autofill/content/renderer/password_generation_manager.h (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.h:82: // Stores
the password form for which the Autofill server classifies one of
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: "Stores the" -> "Stores each"

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
components/autofill/core/browser/autofill_manager.cc:748: // Autofill Manager
Delegate) to detect account creation forms.
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: IMO drop the parenthetical phrase.

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
File components/autofill/core/browser/form_structure_unittest.cc (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
components/autofill/core/browser/form_structure_unittest.cc:2429:
field.form_control_type = "password";
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: Please update the field type to be a checkable type (and update the label
> and name as well to match).

Done.

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
File components/autofill/core/common/autofill_messages.h (right):

https://codereview.chromium.org/23432002/diff/13001/components/autofill/core/...
components/autofill/core/common/autofill_messages.h:192: // Sent when autofill
manager gets the query response from the autofill server
On 2013/09/03 23:58:01, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill"

Done.

[Garrett Casto, 2013-09-04 22:03:29]

LGTM

https://codereview.chromium.org/23432002/diff/26001/components/autofill/conte...
File components/autofill/content/renderer/password_generation_manager.cc
(right):

https://codereview.chromium.org/23432002/diff/26001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:109: if
(!frame->parent()) {
One thing that would be really nice would be some stats to track cases where the
local heuristics fire but we don't show the icon. You don't need to add them in
this CL, but a TODO would be nice.

[zysxqn, 2013-09-04 22:17:49]

https://codereview.chromium.org/23432002/diff/26001/components/autofill/conte...
File components/autofill/content/renderer/password_generation_manager.cc
(right):

https://codereview.chromium.org/23432002/diff/26001/components/autofill/conte...
components/autofill/content/renderer/password_generation_manager.cc:109: if
(!frame->parent()) {
On 2013/09/04 22:03:30, Garrett Casto wrote:
> One thing that would be really nice would be some stats to track cases where
the
> local heuristics fire but we don't show the icon. You don't need to add them
in
> this CL, but a TODO would be nice.

Done.

[commit-bot: I haz the power, 2013-09-04 22:18:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/23432002/47001

[commit-bot: I haz the power, 2013-09-04 22:18:30]

Failed to apply patch for
components/autofill/content/renderer/password_generation_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
components/autofill/content/renderer/password_generation_manager.cc
  Hunk #1 FAILED at 5.
  1 out of 6 hunks FAILED -- saving rejects to file
components/autofill/content/renderer/password_generation_manager.cc.rej

Patch:       components/autofill/content/renderer/password_generation_manager.cc
Index: components/autofill/content/renderer/password_generation_manager.cc
diff --git a/components/autofill/content/renderer/password_generation_manager.cc
b/components/autofill/content/renderer/password_generation_manager.cc
index
aa50cdc0b4dd69e531aa5e5a360c12e730482687..91cb6034f9fab504081e86b6de227885aad3e01f
100644
--- a/components/autofill/content/renderer/password_generation_manager.cc
+++ b/components/autofill/content/renderer/password_generation_manager.cc
@@ -5,7 +5,9 @@
 #include "components/autofill/content/renderer/password_generation_manager.h"
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "components/autofill/core/common/autofill_messages.h"
+#include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/password_generation_util.h"
 #include "content/public/renderer/password_form_conversion_utils.h"
 #include "content/public/renderer/render_view.h"
@@ -64,6 +66,27 @@ bool GetAccountCreationPasswordFields(
   return false;
 }
 
+bool ContainsURL(const std::vector<GURL>& urls, const GURL& url) {
+  return std::find(urls.begin(), urls.end(), url) != urls.end();
+}
+
+// Returns true if the |form1| is essentially equal to |form2|.
+bool FormEquals(const autofill::FormData& form1,
+                const content::PasswordForm& form2) {
+  // TODO(zysxqn): use more signals than just origin to compare.
+  return form1.origin == form2.origin;
+}
+
+bool ContainsForm(const std::vector<autofill::FormData>& forms,
+                  const content::PasswordForm& form) {
+  for (std::vector<autofill::FormData>::const_iterator it =
+           forms.begin(); it != forms.end(); ++it) {
+    if (FormEquals(*it, form))
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 PasswordGenerationManager::PasswordGenerationManager(
@@ -83,10 +106,13 @@ void
PasswordGenerationManager::DidFinishDocumentLoad(WebKit::WebFrame* frame) {
   // as we don't want subframe loads to clear state that we have recieved from
   // the main frame. Note that we assume there is only one account creation
   // form, but there could be multiple password forms in each frame.
+  //
+  // TODO(zysxqn): Add stat when local heuristic fires but we don't show the
+  // password generation icon.
   if (!frame->parent()) {
     not_blacklisted_password_form_origins_.clear();
-    // Initialize to an empty and invalid GURL.
-    account_creation_form_origin_ = GURL();
+    account_creation_forms_.clear();
+    possible_account_creation_form_.reset(new content::PasswordForm());
     passwords_.clear();
   }
 }
@@ -127,7 +153,7 @@ void
PasswordGenerationManager::DidFinishLoad(WebKit::WebFrame* frame) {
       password_generation::LogPasswordGenerationEvent(
           password_generation::SIGN_UP_DETECTED);
       passwords_ = passwords;
-      account_creation_form_origin_ = password_form->origin;
+      possible_account_creation_form_.swap(password_form);
       MaybeShowIcon();
       // We assume that there is only one account creation field per URL.
       return;
@@ -176,6 +202,8 @@ bool PasswordGenerationManager::OnMessageReceived(const
IPC::Message& message) {
                         OnPasswordAccepted)
     IPC_MESSAGE_HANDLER(AutofillMsg_PasswordGenerationEnabled,
                         OnPasswordGenerationEnabled)
+    IPC_MESSAGE_HANDLER(AutofillMsg_AccountCreationFormsDetected,
+                        OnAccountCreationFormsDetected)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -203,27 +231,39 @@ void
PasswordGenerationManager::OnPasswordGenerationEnabled(bool enabled) {
   enabled_ = enabled;
 }
 
+void PasswordGenerationManager::OnAccountCreationFormsDetected(
+    const std::vector<autofill::FormData>& forms) {
+  account_creation_forms_.insert(
+      account_creation_forms_.end(), forms.begin(), forms.end());
+  MaybeShowIcon();
+}
+
 void PasswordGenerationManager::MaybeShowIcon() {
   // We should show the password generation icon only when we have detected
-  // account creation form and we have confirmed from browser that this form
-  // is not blacklisted by the users.
-  if (!account_creation_form_origin_.is_valid() ||
+  // account creation form, we have confirmed from browser that this form
+  // is not blacklisted by the users, and the Autofill server has marked one
+  // of its field as ACCOUNT_CREATION_PASSWORD.
+  if (!possible_account_creation_form_.get() ||
       passwords_.empty() ||
-      not_blacklisted_password_form_origins_.empty()) {
+      not_blacklisted_password_form_origins_.empty() ||
+      account_creation_forms_.empty()) {
     return;
   }
 
-  for (std::vector<GURL>::iterator it =
-           not_blacklisted_password_form_origins_.begin();
-       it != not_blacklisted_password_form_origins_.end(); ++it) {
-    if (*it == account_creation_form_origin_) {
-      passwords_[0].passwordGeneratorButtonElement().setAttribute("style",
-                                                            "display:block");
-      password_generation::LogPasswordGenerationEvent(
-          password_generation::ICON_SHOWN);
-      return;
-    }
+  if (!ContainsURL(not_blacklisted_password_form_origins_,
+                   possible_account_creation_form_->origin)) {
+    return;
   }
+
+  if (!ContainsForm(account_creation_forms_,
+                    *possible_account_creation_form_)) {
+    return;
+  }
+
+  passwords_[0].passwordGeneratorButtonElement().setAttribute("style",
+                                                              "display:block");
+  password_generation::LogPasswordGenerationEvent(
+      password_generation::ICON_SHOWN);
 }
 
 }  // namespace autofill

[commit-bot: I haz the power, 2013-09-04 23:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/23432002/33001

[commit-bot: I haz the power, 2013-09-05 00:02:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[zysxqn, 2013-09-05 00:07:24]

+ cdn@ for changes in autofill_messages.h

[Cris Neckar, 2013-09-05 20:46:49]

On 2013/09/05 00:07:24, zysxqn wrote:
> + cdn@ for changes in autofill_messages.h

lgtm for IPC changes

[commit-bot: I haz the power, 2013-09-05 20:49:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/23432002/33001

[commit-bot: I haz the power, 2013-09-05 21:23:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-05 22:49:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/23432002/122001

[commit-bot: I haz the power, 2013-09-05 23:26:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[zysxqn, 2013-09-05 23:48:38]

+joth@ and benm@ for changes under android_webview

[joth, 2013-09-06 00:24:25]

lgtm

[commit-bot: I haz the power, 2013-09-06 01:33:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/23432002/143001

[commit-bot: I haz the power, 2013-09-06 04:26:25]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-06 17:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zysxqn@google.com/23432002/162001

[commit-bot: I haz the power, 2013-09-06 20:36:54]

Change committed as 221773