[Autofill] Fill fields that have an autocomplete attributes even if not in a form.

If the web site designer took the time to put the autocomplete attributes, we should use those.

Since these fields have an autocomplete attribute defined, the heuristics and server predctions won't be run. Therefore, more fields would be form with a minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest

Committed: https://crrev.com/d2293ca3fd96dd69a6d0f815d8b25d19ab5847cf
Cr-Commit-Position: refs/heads/master@{#374243}

[sebsg, 2016-02-05 16:04:14]

Description was changed from

==========
[Autofill] Fill fields that have an autocomplete attributes even if not in a
form.

If the web site designer took the time to put the autocomplete attributes, we
should use those.

Since these fields have an autocomplete attribute defined, the heuristics and
server predctions won't be run. Therefore, more fields would be form with a
minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest
==========

to

==========
If the web site designer took the time to put the autocomplete attributes, we
should use those.

Since these fields have an autocomplete attribute defined, the heuristics and
server predctions won't be run. Therefore, more fields would be form with a
minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest
==========

[sebsg, 2016-02-05 16:04:14]

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

[sebsg, 2016-02-05 16:42:58]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-02-05 16:43:36]

PTAL, this should fix a couple of sites including play.google.com and
store.google.com :)

[Mathieu, 2016-02-05 18:39:06]

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

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1530: // Since it's
not a checkout flow, only add fields that have an autocomplete
*a non-"off" autocomplete attribute

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1535: if
(autocomplete.utf8() != "" && autocomplete.utf8() != "off")
Other methods seem to define statics like so:

CR_DEFINE_STATIC_LOCAL(WebString, kRole, ("role"));

Perhaps you can use those for comparison?

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1535: if
(autocomplete.utf8() != "" && autocomplete.utf8() != "off")
if (autocomplete.length() && ...

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1542:
form->is_syntehtic_non_checkout_form = true;
Could we do only with a |non_checkout| attribute in combination with the
|is_form_tag|?

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:348:
!is_syntehtic_non_checkout_form) {
I would change this to a combination !(is_form_tag && is_non_checkout_form) ?

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:305: // fields should be
filled using only the autocomplete attributes.
don't mention behavior, just what the attribute is for.

[sebsg, 2016-02-05 20:08:00]

Thanks!

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

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1530: // Since it's
not a checkout flow, only add fields that have an autocomplete
On 2016/02/05 18:39:06, Mathieu Perreault wrote:
> *a non-"off" autocomplete attribute

Done.

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1535: if
(autocomplete.utf8() != "" && autocomplete.utf8() != "off")
On 2016/02/05 18:39:06, Mathieu Perreault wrote:
> if (autocomplete.length() && ...

Done.

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1535: if
(autocomplete.utf8() != "" && autocomplete.utf8() != "off")
On 2016/02/05 18:39:06, Mathieu Perreault wrote:
> Other methods seem to define statics like so:
> 
> CR_DEFINE_STATIC_LOCAL(WebString, kRole, ("role"));
> 
> Perhaps you can use those for comparison?

Done.

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1542:
form->is_syntehtic_non_checkout_form = true;
On 2016/02/05 18:39:06, Mathieu Perreault wrote:
> Could we do only with a |non_checkout| attribute in combination with the
> |is_form_tag|?

Done.

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:348:
!is_syntehtic_non_checkout_form) {
On 2016/02/05 18:39:06, Mathieu Perreault wrote:
> I would change this to a combination !(is_form_tag && is_non_checkout_form) ?

Done.

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.h (right):

https://codereview.chromium.org/1671753004/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:305: // fields should be
filled using only the autocomplete attributes.
On 2016/02/05 18:39:06, Mathieu Perreault wrote:
> don't mention behavior, just what the attribute is for.

Done.

[Mathieu, 2016-02-05 22:36:46]

lgtm with nits

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:345: // Then, if there are
enough active fields and if the form is not a synthetic
// Then if there are enough active fields, and if we are dealing with either a
proper <form> or a <form>-less checkout, run the...

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_structure_unittest.cc (right):

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure_unittest.cc:454: // Verify that
the heuristics are not run for non checkout synthetic forms.
I would change synthetic -> formless everywhere.

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
components/autofill/core/common/form_data.h:38: // fields should be filled using
only the autocomplete attributes.
nit: don't mention behavior.

[sebsg, 2016-02-07 19:05:17]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-02-07 19:05:18]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/1671753004/#ps60001
(title: "Nits")

[commit-bot: I haz the power, 2016-02-07 19:05:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671753004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671753004/60001

[commit-bot: I haz the power, 2016-02-07 19:35:35]

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

[commit-bot: I haz the power, 2016-02-07 19:35:36]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sebsg, 2016-02-08 14:43:04]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-02-08 14:43:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671753004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671753004/60001

[commit-bot: I haz the power, 2016-02-08 15:21:23]

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

[commit-bot: I haz the power, 2016-02-08 15:21:25]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sebsg, 2016-02-08 22:57:23]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-02-08 22:57:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/1671753004/#ps80001
(title: "Added IPC Trait for new attribute")

[commit-bot: I haz the power, 2016-02-08 23:04:29]

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

[commit-bot: I haz the power, 2016-02-09 00:48:46]

Description was changed from

==========
If the web site designer took the time to put the autocomplete attributes, we
should use those.

Since these fields have an autocomplete attribute defined, the heuristics and
server predctions won't be run. Therefore, more fields would be form with a
minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest
==========

to

==========
If the web site designer took the time to put the autocomplete attributes, we
should use those.

Since these fields have an autocomplete attribute defined, the heuristics and
server predctions won't be run. Therefore, more fields would be form with a
minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest
==========

[commit-bot: I haz the power, 2016-02-09 00:48:47]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-02-09 00:50:28]

Description was changed from

==========
If the web site designer took the time to put the autocomplete attributes, we
should use those.

Since these fields have an autocomplete attribute defined, the heuristics and
server predctions won't be run. Therefore, more fields would be form with a
minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest
==========

to

==========
If the web site designer took the time to put the autocomplete attributes, we
should use those.

Since these fields have an autocomplete attribute defined, the heuristics and
server predctions won't be run. Therefore, more fields would be form with a
minimal additionnal cost without the risk of making a bad prediction.

BUG=579651
TEST=FormAutofillBrowserTest

Committed: https://crrev.com/d2293ca3fd96dd69a6d0f815d8b25d19ab5847cf
Cr-Commit-Position: refs/heads/master@{#374243}
==========

[commit-bot: I haz the power, 2016-02-09 00:50:30]

Patchset 4 (id:??) landed as
https://crrev.com/d2293ca3fd96dd69a6d0f815d8b25d19ab5847cf
Cr-Commit-Position: refs/heads/master@{#374243}

[sebsg, 2016-02-09 01:13:57]

Thanks

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:345: // Then, if there are
enough active fields and if the form is not a synthetic
On 2016/02/05 22:36:45, Mathieu Perreault wrote:
> // Then if there are enough active fields, and if we are dealing with either a
> proper <form> or a <form>-less checkout, run the...

Done.

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_structure_unittest.cc (right):

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_structure_unittest.cc:454: // Verify that
the heuristics are not run for non checkout synthetic forms.
On 2016/02/05 22:36:45, Mathieu Perreault wrote:
> I would change synthetic -> formless everywhere.

Done.

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/1671753004/diff/40001/components/autofill/cor...
components/autofill/core/common/form_data.h:38: // fields should be filled using
only the autocomplete attributes.
On 2016/02/05 22:36:45, Mathieu Perreault wrote:
> nit: don't mention behavior.

Done.