Refactor detection of account creation forms and fix two minor issues.

Refactor detection of account creation forms and fix two minor issues.

- We only take into account visible password fields now.
- We won't show an icon for forms that don't have a valid PasswordFormData.
  These forms would not be saved by the PasswordManager anyway.

BUG=130833, 131317
TEST=Ran PasswordGenerationManagerTest


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141289

[Ilya Sherman, 2012-06-08 05:22:18]

LGTM

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:101:
passwords->push_back(*input_element);
nit: We usually do include curly braces for if-stmts whose conditions span
multiple lines.

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.h (right):

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.h:56:
std::vector<WebKit::WebInputElement>* passwords);
nit: The method name does not give any indication that the |passwords| vector
will be modified by this method.  Perhaps "GetAccountCreationPasswordFields"?

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.h:56:
std::vector<WebKit::WebInputElement>* passwords);
nit: Can this be tucked into an anonymous namespace in the implementation file?

[zysxqn, 2012-06-08 17:10:39]

lgtm

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.h (right):

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.h:43: virtual void
DidFinishLoad(WebKit::WebFrame* frame) OVERRIDE;
is it because only until then will we know whether the password field is visible
or not?

[Garrett Casto, 2012-06-08 20:15:09]

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:101:
passwords->push_back(*input_element);
On 2012/06/08 05:22:18, Ilya Sherman wrote:
> nit: We usually do include curly braces for if-stmts whose conditions span
> multiple lines.

Done.

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.h (right):

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.h:43: virtual void
DidFinishLoad(WebKit::WebFrame* frame) OVERRIDE;
On 2012/06/08 17:10:39, zysxqn wrote:
> is it because only until then will we know whether the password field is
visible
> or not?

Yeah, layout hasn't happened by DidFinishDocumentLoad time.

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.h:56:
std::vector<WebKit::WebInputElement>* passwords);
On 2012/06/08 05:22:18, Ilya Sherman wrote:
> nit: The method name does not give any indication that the |passwords| vector
> will be modified by this method.  Perhaps "GetAccountCreationPasswordFields"?

Done.

http://codereview.chromium.org/10540069/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.h:56:
std::vector<WebKit::WebInputElement>* passwords);
On 2012/06/08 05:22:18, Ilya Sherman wrote:
> nit: Can this be tucked into an anonymous namespace in the implementation
file?

Done.

[commit-bot: I haz the power, 2012-06-08 20:19:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/10540069/5001

[commit-bot: I haz the power, 2012-06-08 21:30:35]

Change committed as 141289