Clean up PasswordFormConversionUtilsTest and add some missing tests.

Clean up PasswordFormConversionUtilsTest and add some missing tests.

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

[engedy, 2014-07-07 09:30:09]

@Vaclav, please review.
@Garrett, please review for OWNER's approval.

[vabr (Chromium), 2014-07-07 12:12:19]

LGTM with comments.

I checked that the test coverage was not decreased (i.e., tried to map deleted
parts to new ones), and all looks good, except for one place when I'm not sure.
That's highlighted as a comment.

I also like how you made the names better, the new ones make reading and
understanding the tests much easier.

Cheers,
Vaclav

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

https://codereview.chromium.org/372683003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:141:
IdentifyingUsernameFields) {
nit: Seems like this would fit on one line?

https://codereview.chromium.org/372683003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:432:
// Any constellation of password autocomplete attributes should have no
nit: Any constellation ... should have no ... -> No constellation ... should
have effect ...

Optionally, I believe this comment could be simplified and clarified:
"In the absence of username autocomplete attributes, the username should be the
text input field before the first password. No constellation of password
autocomplete attributes should change that."

https://codereview.chromium.org/372683003/diff/20001/components/autofill/cont...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(left):

https://codereview.chromium.org/372683003/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:152:
WebFormWithMultipleUseNameAndPassWordFieldsToPasswordForm) {
If I understand it correctly, this test was replaced by the new {NULL, NULL,
NULL} clause in IdentifyingUsernamFields.
Please confirm, or explain, where did this test go.

[engedy, 2014-07-09 16:58:35]

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

https://codereview.chromium.org/372683003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:141:
IdentifyingUsernameFields) {
On 2014/07/07 12:12:19, vabr (Chromium) wrote:
> nit: Seems like this would fit on one line?

Done.

https://codereview.chromium.org/372683003/diff/1/components/autofill/content/...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:432:
// Any constellation of password autocomplete attributes should have no
On 2014/07/07 12:12:19, vabr (Chromium) wrote:
> nit: Any constellation ... should have no ... -> No constellation ... should
> have effect ...
> 
> Optionally, I believe this comment could be simplified and clarified:
> "In the absence of username autocomplete attributes, the username should be
the
> text input field before the first password. No constellation of password
> autocomplete attributes should change that."

Done.

https://codereview.chromium.org/372683003/diff/20001/components/autofill/cont...
File
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
(left):

https://codereview.chromium.org/372683003/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc:152:
WebFormWithMultipleUseNameAndPassWordFieldsToPasswordForm) {
On 2014/07/07 12:12:19, vabr (Chromium) wrote:
> If I understand it correctly, this test was replaced by the new {NULL, NULL,
> NULL} clause in IdentifyingUsernamFields.
> Please confirm, or explain, where did this test go.

This test was exploded into three parts: the username part went to
IdentifyingUsernamFields; the password part to {"alpha", "beta", "beta"} in
IdentifyingThreePasswordFields; and the basic attributes should already be
tested by BasicFormAttributes.

However, I have observed that even more tests are missing, therefore I have:
 * Added one more password field to IdentifyingUsernameFields, to make sure we
cover all code paths even in the absence of autocomplete attributes (this is not
really needed, but nice).
 * Added extra (irrelevant) password fields to make sure we ignore any number of
them correctly in the presence of autocomplete attributes (plot twist: we
didn't), and, on the other hand, give up on the entire form because of them in
the absence of autocomplete attributes.
 * Repeated all tests for username parsing twice, once with, and once without
the fields having values.
 * Added a test for skipping disabled fields.
 * Added a test for identifying the activated submit button.

[engedy, 2014-07-10 09:25:39]

@Vaclav, I forgot to clarify this: there is a bit of new code, could you please
take another look?

[vabr (Chromium), 2014-07-10 09:29:15]

LGTM, I love the current level of thoroughness and clarity in the tests.

[engedy, 2014-07-10 17:16:03]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2014-07-10 17:19:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/engedy@chromium.org/372683003/60001

[engedy, 2014-07-10 18:06:28]

@Ilya, PTAL for OWNER's approval.

[engedy, 2014-07-10 18:06:32]

The CQ bit was unchecked by engedy@chromium.org

[Garrett Casto, 2014-07-10 18:32:05]

lgtm

Sorry for the delay

[engedy, 2014-07-10 18:32:57]

No problem, thanks!

[engedy, 2014-07-10 18:33:03]

The CQ bit was unchecked by engedy@chromium.org

[engedy, 2014-07-10 18:33:10]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2014-07-10 18:35:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/engedy@chromium.org/372683003/60001

[commit-bot: I haz the power, 2014-07-10 19:37:08]

Change committed as 282400