Convert invalid WebPasswordFormData to null (rather than empty) PasswordForm.

Convert invalid WebPasswordFormData to null (rather than empty) PasswordForm.

https://chromiumcodereview.appspot.com/11000016/ inadvertantly changed the
WebPasswordFormData->PasswordForm conversion to convert an invalid
WebPasswordFormData to a new PasswordForm() rather than NULL. This change
breaks the assumptions of autofill code on the behavior of this conversion
function (e.g., PasswordGenerationManager::DidFinishLoad()), and can result in
buggy behavior such as the save password infobar not being shown.

BUG=174199
TEST=Log in to forum.linode.com: you should see the save password infobar.


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

[blundell, 2013-03-12 13:08:07]

isherman: for autofill expertise.
jamesr: content/renderer OWNER.

I couldn't trace down exactly how the bug that this CL fixes was resulting in
the behavior described in 174199, which I couldn't reproduce locally.
Nonetheless, the bug is clear, and the report from the user is that the patch
fixed his problems.

I looked at adding password_form_conversion_utils unittests, but it would
involve either instantiating or mocking a WebFormElement sufficiently that the
WebPasswordFormData constructor would be happy. This seemed fragile to me.
Suggestions welcome.

[Ilya Sherman, 2013-03-12 21:59:24]

LGTM, thanks!

You can take a look at the tests under //chrome/renderer/autofill for some ideas
on how to test this code.  All of those are browsertests though; I'm not sure
whether there's a more light-weight way to test code that depends on WebKit
objects.

[blundell, 2013-03-14 12:21:22]

jamesr: Friendly ping :). I'd like to get this into 26 if possible.

On 2013/03/12 21:59:24, Ilya Sherman wrote:
> LGTM, thanks!
> 
> You can take a look at the tests under //chrome/renderer/autofill for some
ideas
> on how to test this code.  All of those are browsertests though; I'm not sure
> whether there's a more light-weight way to test code that depends on WebKit
> objects.

[jamesr, 2013-03-14 16:21:00]

lgtm, but you'll write tests before landing right?

[blundell, 2013-03-14 18:07:42]

On 2013/03/14 16:21:00, jamesr wrote:
> lgtm, but you'll write tests before landing right?

Yes, I'll look into writing a browser test with Ilya's pointers. Thanks.

[blundell, 2013-03-15 16:59:17]

+jam for browsertest addition in content_tests.gypi.

I added a browsertest for converting a valid and invalid WebPasswordForm to a
PasswordForm. There were a few minor issues I couldn't resolve, which I put in a
comment here in case anyone had suggestions. PTAL!

https://chromiumcodereview.appspot.com/12665003/diff/9001/content/renderer/pa...
File content/renderer/password_form_conversion_utils_browsertest.cc (right):

https://chromiumcodereview.appspot.com/12665003/diff/9001/content/renderer/pa...
content/renderer/password_form_conversion_utils_browsertest.cc:56:
ASSERT_EQ("data:", password_form->signon_realm);
A few issues I couldn't resolve:
1. I don't have enough expertise here to know if "data:" is a proper signon
realm...I just went off what the WebPasswordForm had.
2. I couldn't get password_form->submit_element to be filled in in spit of
including the submit element in the HTML.
3. password_form->origin ends up just being the HTML itself.

[blundell, 2013-03-19 15:36:39]

Friendly ping to jam and whoever of isherman/jamesr might be interested in
looking at the new test. Thanks!

On 2013/03/15 16:59:17, blundell wrote:
> +jam for browsertest addition in content_tests.gypi.
> 
> I added a browsertest for converting a valid and invalid WebPasswordForm to a
> PasswordForm. There were a few minor issues I couldn't resolve, which I put in
a
> comment here in case anyone had suggestions. PTAL!
> 
>
https://chromiumcodereview.appspot.com/12665003/diff/9001/content/renderer/pa...
> File content/renderer/password_form_conversion_utils_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12665003/diff/9001/content/renderer/pa...
> content/renderer/password_form_conversion_utils_browsertest.cc:56:
> ASSERT_EQ("data:", password_form->signon_realm);
> A few issues I couldn't resolve:
> 1. I don't have enough expertise here to know if "data:" is a proper signon
> realm...I just went off what the WebPasswordForm had.
> 2. I couldn't get password_form->submit_element to be filled in in spit of
> including the submit element in the HTML.
> 3. password_form->origin ends up just being the HTML itself.

[jam, 2013-03-19 18:02:44]

On 2013/03/15 16:59:17, blundell wrote:
> +jam for browsertest addition in content_tests.gypi.

lgtm

> 
> I added a browsertest for converting a valid and invalid WebPasswordForm to a
> PasswordForm. There were a few minor issues I couldn't resolve, which I put in
a
> comment here in case anyone had suggestions. PTAL!

I'll defer to Ilya for this.
> 
>
https://chromiumcodereview.appspot.com/12665003/diff/9001/content/renderer/pa...
> File content/renderer/password_form_conversion_utils_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12665003/diff/9001/content/renderer/pa...
> content/renderer/password_form_conversion_utils_browsertest.cc:56:
> ASSERT_EQ("data:", password_form->signon_realm);
> A few issues I couldn't resolve:
> 1. I don't have enough expertise here to know if "data:" is a proper signon
> realm...I just went off what the WebPasswordForm had.
> 2. I couldn't get password_form->submit_element to be filled in in spit of
> including the submit element in the HTML.
> 3. password_form->origin ends up just being the HTML itself.

[Ilya Sherman, 2013-03-19 19:09:15]

LGTM

https://codereview.chromium.org/12665003/diff/9001/content/renderer/password_...
File content/renderer/password_form_conversion_utils_browsertest.cc (right):

https://codereview.chromium.org/12665003/diff/9001/content/renderer/password_...
content/renderer/password_form_conversion_utils_browsertest.cc:56:
ASSERT_EQ("data:", password_form->signon_realm);
On 2013/03/15 16:59:18, blundell wrote:
> A few issues I couldn't resolve:
> 1. I don't have enough expertise here to know if "data:" is a proper signon
> realm...I just went off what the WebPasswordForm had.

I also don't know; but if it seems to work, then that should be fine.

> 2. I couldn't get password_form->submit_element to be filled in in spit of
> including the submit element in the HTML.

This might be because the submit element comes before the password, which is
rather unusual.  Anyway, it's probably not a particularly big deal for this
test.

> 3. password_form->origin ends up just being the HTML itself.

Again, this doesn't seem like a big deal for this test.

[blundell, 2013-03-20 10:13:28]

Thanks! Tried moving the submit element to the end, but it didn't change
anything.

On 2013/03/19 19:09:15, Ilya Sherman wrote:
> LGTM
> 
>
https://codereview.chromium.org/12665003/diff/9001/content/renderer/password_...
> File content/renderer/password_form_conversion_utils_browsertest.cc (right):
> 
>
https://codereview.chromium.org/12665003/diff/9001/content/renderer/password_...
> content/renderer/password_form_conversion_utils_browsertest.cc:56:
> ASSERT_EQ("data:", password_form->signon_realm);
> On 2013/03/15 16:59:18, blundell wrote:
> > A few issues I couldn't resolve:
> > 1. I don't have enough expertise here to know if "data:" is a proper signon
> > realm...I just went off what the WebPasswordForm had.
> 
> I also don't know; but if it seems to work, then that should be fine.
> 
> > 2. I couldn't get password_form->submit_element to be filled in in spit of
> > including the submit element in the HTML.
> 
> This might be because the submit element comes before the password, which is
> rather unusual.  Anyway, it's probably not a particularly big deal for this
> test.
> 
> > 3. password_form->origin ends up just being the HTML itself.
> 
> Again, this doesn't seem like a big deal for this test.

[commit-bot: I haz the power, 2013-03-20 10:14:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/12665003/9001

[commit-bot: I haz the power, 2013-03-20 10:54:24]

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.

[commit-bot: I haz the power, 2013-03-20 13:38:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/12665003/25003

[commit-bot: I haz the power, 2013-03-20 14:01:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/12665003/25003

[commit-bot: I haz the power, 2013-03-20 19:41:25]

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

[commit-bot: I haz the power, 2013-03-20 19:57:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/12665003/25003

[commit-bot: I haz the power, 2013-03-20 23:47:47]

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

[commit-bot: I haz the power, 2013-03-21 10:51:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/12665003/25003

[commit-bot: I haz the power, 2013-03-21 14:44:07]

Change committed as 189594