Autofill: Modify various utility function to deal with unowned form fields.

Autofill: Modify various utility function to deal with unowned form fields.

BUG=428919
Committed: https://crrev.com/5912ae5c18cfd0d485784d35e0b5bf1e2a4aa036
Cr-Commit-Position: refs/heads/master@{#308224}

[Lei Zhang, 2014-12-12 09:58:22]

thestig@chromium.org changed reviewers:
+ estade@chromium.org

[Lei Zhang, 2014-12-12 09:58:22]

See diffs between patch sets to better understand what's going on.

[Evan Stade, 2014-12-12 19:59:35]

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

https://codereview.chromium.org/804443002/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:692: if
(!FindFormAndFieldForFormControlElement(element, &form, &field,
hmmm, why doesn't the unowned form just go in the cache at initial parse time?

https://codereview.chromium.org/804443002/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:694: // No associated
form element, try the form for unowned form elements.
nit: s/form for/synthetic form for/

https://codereview.chromium.org/804443002/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:700: const ExtractMask
extract_mask =
nit: dislike this variable because it makes it seem like it's going to be used
more than once

[Lei Zhang, 2014-12-12 22:06:23]

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

https://codereview.chromium.org/804443002/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:692: if
(!FindFormAndFieldForFormControlElement(element, &form, &field,
On 2014/12/12 19:59:35, Evan Stade wrote:
> hmmm, why doesn't the unowned form just go in the cache at initial parse time?

FindFormAndFieldForFormControlElement() works on the live WebDocument to get the
current form.fields, not the cached copies, so we need to do the same for the
synthetic form. I first tried the cached copy approach and many tests fail, e.g.
FormAutofillTest.FindFormForInputElementForUnownedForm.

https://codereview.chromium.org/804443002/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:694: // No associated
form element, try the form for unowned form elements.
On 2014/12/12 19:59:35, Evan Stade wrote:
> nit: s/form for/synthetic form for/

Done.

https://codereview.chromium.org/804443002/diff/60001/components/autofill/cont...
components/autofill/content/renderer/autofill_agent.cc:700: const ExtractMask
extract_mask =
On 2014/12/12 19:59:35, Evan Stade wrote:
> nit: dislike this variable because it makes it seem like it's going to be used
> more than once

Done.

[Lei Zhang, 2014-12-12 22:43:53]

I undid the autofill_agent.cc changes and put that into
FindFormAndFieldForFormControlElement().

[Evan Stade, 2014-12-12 23:05:53]

lgtm

https://codereview.chromium.org/804443002/diff/100001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/804443002/diff/100001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:526: //
ForEachMatchingUnownedFormField().
this documentation isn't necessary imo

[Lei Zhang, 2014-12-12 23:11:17]

https://codereview.chromium.org/804443002/diff/100001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/804443002/diff/100001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:526: //
ForEachMatchingUnownedFormField().
On 2014/12/12 23:05:53, Evan Stade wrote:
> this documentation isn't necessary imo

Done.

[Lei Zhang, 2014-12-12 23:32:46]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-12-12 23:33:07]

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

[commit-bot: I haz the power, 2014-12-13 02:12:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-13 02:12:42]

Patchset 7 (id:??) landed as
https://crrev.com/5912ae5c18cfd0d485784d35e0b5bf1e2a4aa036
Cr-Commit-Position: refs/heads/master@{#308224}