Modifying Autofill Agent to use synthetic form (i.e., formless elements) when no form is present on…

Modifying Autofill Agent to use synthetic form (i.e., formless elements) when no form is present on page.

BUG=613600
Committed: https://crrev.com/283dda14a5b2f072082d18be905a145a79857f7e
Cr-Commit-Position: refs/heads/master@{#401733}

[tmartino, 2016-06-14 23:05:19]

tmartino@chromium.org changed reviewers:
+ mathp@chromium.org

[Mathieu, 2016-06-15 00:56:48]

On 2016/06/14 at 23:05:19, tmartino wrote:
> 

Hi Tommy, 

Looks great. Could you update form_autocomplete_browsertest.cc with a sensible
test (interacting with a <form>-less form, seeing that messages get sent). Have
a look at InteractedFormNoLongerFocused_FocusNoLongerOnForm for example.

[tmartino, 2016-06-20 19:23:48]

Doing one last end-to-end test of the new logic but it should be good to go.

[Mathieu, 2016-06-21 18:04:07]

Thanks, just a few comments

https://codereview.chromium.org/2065303002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right):

https://codereview.chromium.org/2065303002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:65: ASSERT_LE(2U,
std::get<0>(submitted_forms).fields.size());
wouldn't this pass if there were 0 messages detected, therefore making this test
useless?

https://codereview.chromium.org/2065303002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2065303002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:645:
std::vector<WebElement> fieldsets;
Would be great to extract this in a function in this file, or reuse the logic in
form_cache by extracting it to form_autofill_util.

[tmartino, 2016-06-21 23:38:31]

https://codereview.chromium.org/2065303002/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right):

https://codereview.chromium.org/2065303002/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:65: ASSERT_LE(2U,
std::get<0>(submitted_forms).fields.size());
On 2016/06/21 at 18:04:07, Mathieu Perreault wrote:
> wouldn't this pass if there were 0 messages detected, therefore making this
test useless?

You're reading it backwards...I did too the first time :) It's asserting that 2U
<= size.

https://codereview.chromium.org/2065303002/diff/40001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2065303002/diff/40001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:645:
std::vector<WebElement> fieldsets;
On 2016/06/21 at 18:04:07, Mathieu Perreault wrote:
> Would be great to extract this in a function in this file, or reuse the logic
in form_cache by extracting it to form_autofill_util.

Done. It's not quite doing the same thing as form_cache--there are some failure
conditions there that we don't want to transfer and I think it would be messier
to try and hack around them. But most of the meaty logic is already encapsulated
in the two calls from form_util anyway.

[Mathieu, 2016-06-22 15:20:51]

Thanks, few comments

https://codereview.chromium.org/2065303002/diff/60001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/2065303002/diff/60001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:655: bool
AutofillAgent::CollectFormlessElements(FormData& output) {
Please pass FormData* instead

https://codereview.chromium.org/2065303002/diff/60001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:656: WebDocument document
= render_frame()->GetWebFrame()->document();
This is pretty well unit-testable :)

form_autocomplete_browsertest, despite its name is a unittest (doesn't start the
browser). Please add a few tests that load arbitrary HTML and that way you can
test CollectFormlessElements pretty thoroughly.

Let's make sure it works for input text fields, <select> fields, etc.

[tmartino, 2016-06-23 14:47:33]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 14:47:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2065303002/80001

[tmartino, 2016-06-23 14:52:06]

Added a test; to my eye, it covers every interesting case. LMK if there's
something I neglected.

[commit-bot: I haz the power, 2016-06-23 15:25:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 15:25:19]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-06-23 18:25:26]

lgtm, please comment with repro steps on the bug so that it can be verified!

[tmartino, 2016-06-23 21:37:34]

The CQ bit was checked by tmartino@chromium.org

[commit-bot: I haz the power, 2016-06-23 21:37:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2065303002/80001

[commit-bot: I haz the power, 2016-06-23 21:44:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-23 21:45:25]

Description was changed from

==========
Modifying Autofill Agent to use synthetic form (i.e., formless elements) when no
form is present on page.

BUG=613600
==========

to

==========
Modifying Autofill Agent to use synthetic form (i.e., formless elements) when no
form is present on page.

BUG=613600

Committed: https://crrev.com/283dda14a5b2f072082d18be905a145a79857f7e
Cr-Commit-Position: refs/heads/master@{#401733}
==========

[commit-bot: I haz the power, 2016-06-23 21:45:26]

Patchset 5 (id:??) landed as
https://crrev.com/283dda14a5b2f072082d18be905a145a79857f7e
Cr-Commit-Position: refs/heads/master@{#401733}