[Password Generation] Send id attribute to server for HTML based form classifier

[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in AutofillUploadContents). This CL makes sure that we send both attributes. This data will be used by HTML based form classifier. 

The data will be sent only if AutofillFieldMetadata is enabled (i.e. only for dev and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is needed only in one test (FormStructureTest.EncodeUploadRequest_WithCssClassesAndIds), but require |nullptr| in bunch of other cases. The parameter was added in this CL (https://codereview.chromium.org/2073093002/).

Review-Url: https://codereview.chromium.org/2640783002
Cr-Commit-Position: refs/heads/master@{#445042}
Committed: https://chromium.googlesource.com/chromium/src/+/239ed9a242283d9af2e37f609d8e13086ddac1e0

[kolos1, 2017-01-18 13:25:44]

Description was changed from

==========
o



Merge branch 'master' of https://chromium.googlesource.com/chromium/src into
pg_add_id_to_formdata

o

o


o

BUG=
==========

to

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier.
==========

[kolos1, 2017-01-18 13:29:24]

Description was changed from

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier.
==========

to

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only AutofillFieldMetadata is enabled (i.e. only for dev
and beta).
==========

[kolos1, 2017-01-18 13:39:59]

Description was changed from

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only AutofillFieldMetadata is enabled (i.e. only for dev
and beta).
==========

to

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only AutofillFieldMetadata is enabled (i.e. only for dev
and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test (FormStructureTest,
EncodeUploadRequest_WithCssClassesAndIds), but require |nullptr| in bunch of
other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).
==========

[kolos1, 2017-01-18 14:54:25]

Patchset #3 (id:40001) has been deleted

[kolos1, 2017-01-18 14:54:38]

Patchset #2 (id:20001) has been deleted

[kolos1, 2017-01-18 14:56:59]

Description was changed from

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only AutofillFieldMetadata is enabled (i.e. only for dev
and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test (FormStructureTest,
EncodeUploadRequest_WithCssClassesAndIds), but require |nullptr| in bunch of
other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).
==========

to

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only if AutofillFieldMetadata is enabled (i.e. only for
dev and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test (FormStructureTest,
EncodeUploadRequest_WithCssClassesAndIds), but require |nullptr| in bunch of
other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).
==========

[kolos1, 2017-01-18 15:03:22]

kolos@chromium.org changed reviewers:
+ dcheng@chromium.org, mathp@chromium.org, sebsg@chromium.org

[kolos1, 2017-01-18 15:03:23]

sebsg@chromium.org: Please review changes in 
autofill_server_browsertest.cc
form_autofill_util.cc
autofill_test_utils*
form_structure*
server.proto (Yes, I keep in mind a server-side update :) )
form_field_data*

mathp@chromium.org: Please review changes in 
form_autofill_browsertest.cc

dcheng@chromium.org: Please review changes in 
autofill_types.mojom
autofill_types_struct_traits*

[kolos1, 2017-01-18 15:04:19]

Description was changed from

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only if AutofillFieldMetadata is enabled (i.e. only for
dev and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test (FormStructureTest,
EncodeUploadRequest_WithCssClassesAndIds), but require |nullptr| in bunch of
other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).
==========

to

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only if AutofillFieldMetadata is enabled (i.e. only for
dev and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test
(FormStructureTest.EncodeUploadRequest_WithCssClassesAndIds), but require
|nullptr| in bunch of other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).
==========

[kolos1, 2017-01-18 15:10:54]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-01-18 19:22:49]

The files you assigned to me lgtm. And thanks for you change to the tests!!! It
looks much better now.

Going forward, do you think it would make sense to treat the name as only name,
and not fallback to id if it's not present? It would require some changes to the
server, but I feel it could give us better granularity. WDYT? 
(just mentioning this here, not asking you to change this CL)

https://codereview.chromium.org/2640783002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/2640783002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/form_autofill_browsertest.cc:2413:
EXPECT_TRUE(form.fields[2].id.empty());
Can you add a quick comment to explain why the id is not set for this one?

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

https://codereview.chromium.org/2640783002/diff/100001/components/autofill/co...
components/autofill/content/renderer/form_autofill_util.cc:1379: if (id !=
field->name)
Can you also just make a quick note or refer to
HTMLFormControlElement::nameForAutofill like you did in server.proto?
I was a bit confused before I got to that comment :)

[kolos1, 2017-01-19 11:09:08]

Hi Sebastien,

Thanks for fast reviewing. 

I believe that treating the name as only name wouldn't be a trivial change.
|name| is used to match fields. And it makes sense to use the id attribute if
the name attribute is absent. So, we should initialize |name| as it is in
nameForAutofill.

I added ids because the more info the classifier gets the better it will
perform. For example, we looking for "registration" word, names might not
contain it, but id might. So, it is better to send both attributes. 

Added comments you requested. 

Regards,
Maxim

[kolos1, 2017-01-19 11:09:16]

https://codereview.chromium.org/2640783002/diff/100001/chrome/renderer/autofi...
File chrome/renderer/autofill/form_autofill_browsertest.cc (right):

https://codereview.chromium.org/2640783002/diff/100001/chrome/renderer/autofi...
chrome/renderer/autofill/form_autofill_browsertest.cc:2413:
EXPECT_TRUE(form.fields[2].id.empty());
On 2017/01/18 19:22:49, sebsg wrote:
> Can you add a quick comment to explain why the id is not set for this one?

Done.

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

https://codereview.chromium.org/2640783002/diff/100001/components/autofill/co...
components/autofill/content/renderer/form_autofill_util.cc:1379: if (id !=
field->name)
Done.

Also removed the comment about the label. As I understand, the author of this CL
(https://codereview.chromium.org/1622004, see form_manager.cc) forgot to remove
it.

[kolos1, 2017-01-19 11:15:21]

Hi Mathieu and Daniel!

Friendly ping. Looks like the general design will stay the same. Please review
changes in the following files:

mathp@chromium.org: 
form_autofill_browsertest.cc

dcheng@chromium.org:
autofill_types.mojom
autofill_types_struct_traits*

Best regards,
Maxim

[kolos1, 2017-01-19 15:50:37]

Patchset #4 (id:120001) has been deleted

[Mathieu, 2017-01-19 19:51:17]

Hi Maxim, 

lgtm for the test, thanks

[kolos1, 2017-01-20 09:29:52]

kolos@chromium.org changed reviewers:
+ mkwst@chromium.org

[kolos1, 2017-01-20 09:29:52]

Hi Mike,

Could you please review changes in autofill_types*? Thanks.

Regards,
Maxim

[Mike West, 2017-01-20 10:42:31]

Adding the property to mojo LGTM.

[kolos1, 2017-01-20 10:48:07]

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

[commit-bot: I haz the power, 2017-01-20 10:48:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 11:28:06]

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

[commit-bot: I haz the power, 2017-01-20 11:28:07]

Dry run: This issue passed the CQ dry run.

[kolos1, 2017-01-20 11:40:06]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2017-01-20 11:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2640783002/#ps140001
(title: "Changes addressed to reviewer comments")

[commit-bot: I haz the power, 2017-01-20 11:40:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 11:44:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484912406764740,
"parent_rev": "166c2e5079a20a859b1b85586146b2eba3058e4c", "commit_rev":
"239ed9a242283d9af2e37f609d8e13086ddac1e0"}

[commit-bot: I haz the power, 2017-01-20 11:45:44]

Description was changed from

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only if AutofillFieldMetadata is enabled (i.e. only for
dev and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test
(FormStructureTest.EncodeUploadRequest_WithCssClassesAndIds), but require
|nullptr| in bunch of other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).
==========

to

==========
[Password Generation] Send id attribute to server for HTML based form classifier

name or id attribute is already sent to the server (see |name| field in
AutofillUploadContents). This CL makes sure that we send both attributes. This
data will be used by HTML based form classifier. 

The data will be sent only if AutofillFieldMetadata is enabled (i.e. only for
dev and beta).

This CL also removes |css_classes| parameter in FillUploadField, because it is
needed only in one test
(FormStructureTest.EncodeUploadRequest_WithCssClassesAndIds), but require
|nullptr| in bunch of other cases. The parameter was added in this CL
(https://codereview.chromium.org/2073093002/).

Review-Url: https://codereview.chromium.org/2640783002
Cr-Commit-Position: refs/heads/master@{#445042}
Committed:
https://chromium.googlesource.com/chromium/src/+/239ed9a242283d9af2e37f609d8e...
==========

[commit-bot: I haz the power, 2017-01-20 11:45:45]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/239ed9a242283d9af2e37f609d8e...