Rewrite functions from WebPasswordFormData and WebPasswordFormUtils in

Rewrite functions from WebPasswordFormData and WebPasswordFormUtils in
password_form_conversion_utils.

There are Some old Blink code implemented in Webkit that predates the
Chromium/WebKit API, specifically WebPasswordFormData and WebPasswordFormUtils. At
this point the split just makes this code harder to update, as you need to wait
for a Blink roll when adding a new element to autofill::PasswordForm. This patch is
re-writing this code in components/autofill/content/renderer/password_form_conversion_util
s to make it cleaner.

BUG=None

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

[ziran.sun, 2014-04-15 12:41:25]

Rewrite functions from WebPasswordFormData and WebPasswordFormUtils in Chromium.
Please review. Thanks!

[Ilya Sherman, 2014-04-16 20:37:33]

Garrett, I'll defer to your review unless you ask me to do otherwise.  Thanks!

[Garrett Casto, 2014-04-16 22:56:54]

Can you also make a test for this? Unfortunately one doesn't exist for the
current implementation to compare against, but we should still have something.
You can take a look at PasswordAutofillAgentTest for comparison. You basically
just want something where you can load some simple HTML with a form and then
parse it and verify that the autofill::PasswordForm is correct. It doesn't need
to be super involved, but checking things like the action and signon_realm,
username_element in the presence of multiple fields, password_element with
multiple fields, etc. would be good.

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

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:7:
#include "base/logging.h"
I think that this is unnecessary.

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:51: {
In Chromium, starting braces don't get their own line. This is done a few times
in this file.

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:111: if
(!input_element->isEnabled())
Nit: Can you merge this with the previous if statement?

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:159:
password_form->action = full_action.ReplaceComponents(rep);
You don't want to do that here. Checking to see if action.is_valid() is how we
determine if we successfully parsed the form, but LocateSpecificPasswords() can
still fail after this point.

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:165:
password_form->other_possible_usernames.insert(
Inserting in this way was only necessary because the types were different
between Blink and Chrome. Now you can just .swap() them.

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_form_conversion_utils.h
(right):

https://codereview.chromium.org/225823006/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.h:19: //
PasswordForm struct.
I don't think that we want to make this public now. If we have a need for it in
the future we can make it public, but I'd rather there just be one method if we
don't. Plus this doesn't actually fill out form_data right now.

[ziran.sun, 2014-04-17 15:29:17]

Updated code as per review comments. All review comments have been addressed.
Please review. Thanks!

[Garrett Casto, 2014-04-17 18:20:22]

LGTM with some nits

After this is submitted, would you mind deleting the old code in Blink? Thanks!

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

https://codereview.chromium.org/225823006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:194: } 
// namespace autofill
Nit: Remove trailing whitespace.

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

https://codereview.chromium.org/225823006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:91:
" <INPUT type=\"password\" name=\"password\" id=\"password\" "
Would you mind changing the name and id's of these fields to be distinct? It's
nice to verify that password_element and username_element are being set
correctly as well.

[ziran.sun, 2014-04-22 15:20:00]

Update code as per 2nd set comments. Please review. Thanks!

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

https://codereview.chromium.org/225823006/diff/40001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils.cc:194: } 
// namespace autofill
On 2014/04/17 18:20:22, Garrett Casto wrote:
> Nit: Remove trailing whitespace.

Sorry, but I couldn't find whitespace here...

[Garrett Casto, 2014-04-22 23:09:35]

On 2014/04/22 15:20:00, ziran.sun wrote:
> Update code as per 2nd set comments. Please review. Thanks!
> 
>
https://codereview.chromium.org/225823006/diff/40001/components/autofill/cont...
> File components/autofill/content/renderer/password_form_conversion_utils.cc
> (right):
> 
>
https://codereview.chromium.org/225823006/diff/40001/components/autofill/cont...
> components/autofill/content/renderer/password_form_conversion_utils.cc:194: } 
> // namespace autofill
> On 2014/04/17 18:20:22, Garrett Casto wrote:
> > Nit: Remove trailing whitespace.
> 
> Sorry, but I couldn't find whitespace here...

Yeah, it isn't showing up anymore.

[Garrett Casto, 2014-04-22 23:09:40]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-04-22 23:10:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/225823006/60001

[commit-bot: I haz the power, 2014-04-23 02:04:30]

Change committed as 265496