Enable password saving for dynamic password forms

Enable password saving for dynamic password forms

For a list of major sites with dynamic password forms and whether they are fixed with this CL, please refer to the 'JavaScript Forms' column in https://docs.google.com/a/google.com/spreadsheet/ccc?key=0AkbIXgaqCC1tdGQ4NHlwd0phbWhBOEs1eGFTa1BsZFE#gid=0.

Each remaining issue is tracked in separate bugs, crbug/26186,  crbug/43219, crbug/234292, crbug/282487, crbug/282488, crbug/282522


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

[guohui, 2013-08-26 16:35:40]

Hello Ilya,

Could you please review the CL?

Thanks,

Hui

[Ilya Sherman, 2013-08-26 20:33:15]

Wow, nice that most of the plumbing is already handled for us!  Could you please
add some test coverage for this CL?  Otherwise looks pretty good :)

https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:51: // Called
when new form controls are inserted.
nit: Please leave a blank line before this one.

[Garrett Casto, 2013-08-27 01:02:09]

On 2013/08/26 20:33:15, Ilya Sherman (Away Aug. 9-25) wrote:
> Wow, nice that most of the plumbing is already handled for us!  Could you
please
> add some test coverage for this CL?  Otherwise looks pretty good :)
> 
>
https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
> File components/autofill/content/renderer/password_autofill_agent.h (right):
> 
>
https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
> components/autofill/content/renderer/password_autofill_agent.h:51: // Called
> when new form controls are inserted.
> nit: Please leave a blank line before this one.

+1 to tests. I'm excited that this is finally going to be fixed.

I wouldn't link that spreadsheet by the way, it's something that is constantly
changing and isn't really organized in a way that makes it easy to digest. Could
you instead list some of the known problems either in the description and/or on
issue 123955 and create new bugs for them so that we can track them? Some of the
problems (like no navigation after submit) I'm not sure if we are every really
going to solve, but we should at least have a record of them.

[guohui, 2013-09-06 19:30:46]

Thanks and sorry for the delay, unit tests added.

the checkout code for associating dynamic forms has been mostly removed in the
trunk, had to add a few lines back to support our use case.

https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
File components/autofill/content/renderer/password_autofill_agent.h (right):

https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
components/autofill/content/renderer/password_autofill_agent.h:51: // Called
when new form controls are inserted.
On 2013/08/26 20:33:16, Ilya Sherman wrote:
> nit: Please leave a blank line before this one.
done

[guohui, 2013-09-06 19:37:18]

On 2013/09/06 19:30:46, guohui wrote:
> Thanks and sorry for the delay, unit tests added.
> 
> the checkout code for associating dynamic forms has been mostly removed in the
> trunk, had to add a few lines back to support our use case.
> 
>
https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
> File components/autofill/content/renderer/password_autofill_agent.h (right):
> 
>
https://codereview.chromium.org/22926033/diff/17001/components/autofill/conte...
> components/autofill/content/renderer/password_autofill_agent.h:51: // Called
> when new form controls are inserted.
> On 2013/08/26 20:33:16, Ilya Sherman wrote:
> > nit: Please leave a blank line before this one.
> done

@garrett, i noticed all tests that expects prompt to be shown are currently
disabled, so i guess my test should be disabled as well, though it never fails
on my local machine?

[Ilya Sherman, 2013-09-10 01:27:58]

> @garrett, i noticed all tests that expects prompt to be shown are currently
> disabled, so i guess my test should be disabled as well, though it never fails
> on my local machine?

Let's not add new disabled tests if we can avoid it.  That doesn't really
provide additional test coverage, which rather defeats the purpose of adding
tests in the CL.

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
File chrome/test/data/password/dynamic_password_form.html (right):

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
chrome/test/data/password/dynamic_password_form.html:8:
dynamicForm.setAttribute('action','done.html');
nit: Please leave a space after the comma (lines 7 and 8).

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
chrome/test/data/password/dynamic_password_form.html:13:
inputUsername.setAttribute('name','username');
Ditto here and below.

https://codereview.chromium.org/22926033/diff/32001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/22926033/diff/32001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:619: if
(node.document().frame() == topmost_frame_) {
Do we really need to cache |topmost_frame_|, or can we just check whether
node.document().frame().parent() is NULL here?

[gcasto (DO NOT USE), 2013-09-10 17:03:42]

I'll have a CL out to fix the flakiness issue today, and Hui should be able
to submit this with the test enabled.


On Mon, Sep 9, 2013 at 6:27 PM, <isherman@chromium.org> wrote:

> @garrett, i noticed all tests that expects prompt to be shown are currently
>> disabled, so i guess my test should be disabled as well, though it never
>> fails
>> on my local machine?
>>
>
> Let's not add new disabled tests if we can avoid it.  That doesn't really
> provide additional test coverage, which rather defeats the purpose of
> adding
> tests in the CL.
>
>
> https://codereview.chromium.**org/22926033/diff/32001/**
>
chrome/test/data/password/**dynamic_password_form.html<https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password/dynamic_password_form.html>
> File chrome/test/data/password/**dynamic_password_form.html (right):
>
> https://codereview.chromium.**org/22926033/diff/32001/**
>
chrome/test/data/password/**dynamic_password_form.html#**newcode8<https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password/dynamic_password_form.html#newcode8>
> chrome/test/data/password/**dynamic_password_form.html:8:
> dynamicForm.setAttribute('**action','done.html');
> nit: Please leave a space after the comma (lines 7 and 8).
>
> https://codereview.chromium.**org/22926033/diff/32001/**
>
chrome/test/data/password/**dynamic_password_form.html#**newcode13<https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password/dynamic_password_form.html#newcode13>
> chrome/test/data/password/**dynamic_password_form.html:13:
> inputUsername.setAttribute('**name','username');
> Ditto here and below.
>
> https://codereview.chromium.**org/22926033/diff/32001/**
>
components/autofill/content/**renderer/autofill_agent.cc<https://codereview.chromium.org/22926033/diff/32001/components/autofill/content/renderer/autofill_agent.cc>
> File components/autofill/content/**renderer/autofill_agent.cc (right):
>
> https://codereview.chromium.**org/22926033/diff/32001/**
>
components/autofill/content/**renderer/autofill_agent.cc#**newcode619<https://codereview.chromium.org/22926033/diff/32001/components/autofill/content/renderer/autofill_agent.cc#newcode619>
> components/autofill/content/**renderer/autofill_agent.cc:**619: if
> (node.document().frame() == topmost_frame_) {
> Do we really need to cache |topmost_frame_|, or can we just check
> whether node.document().frame().**parent() is NULL here?
>
>
https://codereview.chromium.**org/22926033/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Garrett Casto, 2013-09-10 17:13:45]

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
File chrome/test/data/password/dynamic_password_form.html (right):

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
chrome/test/data/password/dynamic_password_form.html:31: <body
onload='createDynamicForm()'></body>
We discussed this offline, but just as a reminder:

Could you check and make sure that this is actually testing the new code path? I
think that DidFinishLoad may be called after onload(), which means we would have
caught this anyway. You may need to move the 'createDynamicForm()' call to the
test itself.

https://codereview.chromium.org/22926033/diff/32001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/22926033/diff/32001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:619: if
(node.document().frame() == topmost_frame_) {
Could you put a comment here as to why we are only looking at the topmost frame?
I'm assuming that it's just because we assume that dynamically created content
will be in this frame, but If we want to change this in the future it would be
nice to know for certain what we were thinking.

[guohui, 2013-09-13 15:09:39]

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
File chrome/test/data/password/dynamic_password_form.html (right):

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
chrome/test/data/password/dynamic_password_form.html:8:
dynamicForm.setAttribute('action','done.html');
On 2013/09/10 01:27:58, Ilya Sherman wrote:
> nit: Please leave a space after the comma (lines 7 and 8).

Done.

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
chrome/test/data/password/dynamic_password_form.html:13:
inputUsername.setAttribute('name','username');
On 2013/09/10 01:27:58, Ilya Sherman wrote:
> Ditto here and below.

Done.

https://codereview.chromium.org/22926033/diff/32001/chrome/test/data/password...
chrome/test/data/password/dynamic_password_form.html:31: <body
onload='createDynamicForm()'></body>
On 2013/09/10 17:13:45, Garrett Casto wrote:
> We discussed this offline, but just as a reminder:
> 
> Could you check and make sure that this is actually testing the new code path?
I
> think that DidFinishLoad may be called after onload(), which means we would
have
> caught this anyway. You may need to move the 'createDynamicForm()' call to the
> test itself.
As discussed offline, this does test the new code path. Though DidFinishLoad is
called after onLoad, it does not add new forms to PasswordManager, its sole
purpose is to show password save info bar if a managed form is just submitted,
so without this CL, the test would have failed.

However, I agree this test could be confusing, and it is not the best example
for JS generated form, so i updated the test to trigger form creation upon
simulated button click.

https://codereview.chromium.org/22926033/diff/32001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/22926033/diff/32001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:619: if
(node.document().frame() == topmost_frame_) {
On 2013/09/10 01:27:58, Ilya Sherman wrote:
> Do we really need to cache |topmost_frame_|, or can we just check whether
> node.document().frame().parent() is NULL here?

Done.

[Garrett Casto, 2013-09-13 18:30:05]

https://codereview.chromium.org/22926033/diff/40001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/22926033/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_browsertest.cc:184:
ui_test_utils::NavigateToURL(browser(), url);
Use NavigateToFile() for this or else the test is going to be flaky.

https://codereview.chromium.org/22926033/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_browsertest.cc:190:
"window.setTimeout(function() {"
Out of curiosity, why do you need to use a setTimeout function? Is it because
document.dynamic_form isn't visible right after you call click()?

[guohui, 2013-09-13 20:07:56]

https://codereview.chromium.org/22926033/diff/40001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/22926033/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_browsertest.cc:184:
ui_test_utils::NavigateToURL(browser(), url);
On 2013/09/13 18:30:05, Garrett Casto wrote:
> Use NavigateToFile() for this or else the test is going to be flaky.

Done.

https://codereview.chromium.org/22926033/diff/40001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_browsertest.cc:190:
"window.setTimeout(function() {"
On 2013/09/13 18:30:05, Garrett Casto wrote:
> Out of curiosity, why do you need to use a setTimeout function? Is it because
> document.dynamic_form isn't visible right after you call click()?
yes, because dynamic_form is inserted in click handler, and a click events is
triggered asynchronously.

[Garrett Casto, 2013-09-13 21:25:24]

lgtm

[Ilya Sherman, 2013-09-14 02:18:07]

LGTM, thanks.

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (left):

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:610: // TODO(isherman):
Decide if we want to support autofill with AJAX.
Please keep this TODO, though feel free to tweak it to make it clear that this
is referring to non-password Autofill.

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:614: // Only monitors
dynamic forms created in the top frame.
nit: Would be nice to document *why* that's important/desirable.

[commit-bot: I haz the power, 2013-09-16 18:03:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/22926033/41006

[guohui, 2013-09-16 19:51:41]

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (left):

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:610: // TODO(isherman):
Decide if we want to support autofill with AJAX.
On 2013/09/14 02:18:07, Ilya Sherman wrote:
> Please keep this TODO, though feel free to tweak it to make it clear that this
> is referring to non-password Autofill.

Done.

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/22926033/diff/59001/components/autofill/conte...
components/autofill/content/renderer/autofill_agent.cc:614: // Only monitors
dynamic forms created in the top frame.
On 2013/09/14 02:18:07, Ilya Sherman wrote:
> nit: Would be nice to document *why* that's important/desirable.

Done.

[commit-bot: I haz the power, 2013-09-16 21:19:15]

Change committed as 223413