[Autofill] Remove longest common prefix from field names when running heuristics.

[Autofill] Remove longest common prefix from field names when running heuristics.

Some frameworks put a prefix on all fields within a form (see bug), which confuses the heuristics.

BUG=497850
TEST=DataDrivenHeuristics,FormStructure

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

[Mathieu, 2016-01-24 01:54:29]

Patchset #2 (id:20001) has been deleted

[Mathieu, 2016-01-24 02:09:13]

Patchset #2 (id:40001) has been deleted

[Mathieu, 2016-01-24 02:09:51]

Description was changed from

==========
[Autofill] Remove longest common prefix from field names when processing
regexes.

Some frameworks put a prefix on all fields within a form (see bug).

BUG=497850
TEST=DataDrivenHeuristics,FormStructure
==========

to

==========
[Autofill] Remove longest common prefix from field names when running
heuristics.

Some frameworks put a prefix on all fields within a form (see bug), which
confuses the heuristics.

BUG=497850
TEST=DataDrivenHeuristics,FormStructure
==========

[Mathieu, 2016-01-24 02:09:59]

Patchset #2 (id:60001) has been deleted

[Mathieu, 2016-01-24 02:12:32]

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

[Mathieu, 2016-01-24 02:12:46]

Hi, please have a look!

[vabr (Chromium), 2016-01-25 10:10:26]

lgtm

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.h (right):

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_field.h:56: void
set_parseable_name(const base::string16& parseable_name) {
nit: passing by value and using std::move is potentially avoiding a copy:

case 1 -- if the argument is a temporary, then passing by value + move makes 0
copies, unlike the current code

case 2 -- otherwise, there is 1 copy in both versions of code (only it happens
during calling with pass by value + move, and on line 57 in the current code)

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

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:1262:
std::vector<base::string16> names;
optional: names.reserve(field_count());

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

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:251: static base::string16
FindLongestCommonPrefix(
optional: This could return a StringPiece, saving a copy. It would need
|strings| to be alive for as long as the result is used, but that's currently
the case.

[Mathieu, 2016-01-25 15:52:53]

Hi, 

Can you have another look to make sure this is what you had in mind? Thanks!

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_field.h (right):

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_field.h:56: void
set_parseable_name(const base::string16& parseable_name) {
On 2016/01/25 10:10:25, vabr (Chromium) wrote:
> nit: passing by value and using std::move is potentially avoiding a copy:
> 
> case 1 -- if the argument is a temporary, then passing by value + move makes 0
> copies, unlike the current code
> 
> case 2 -- otherwise, there is 1 copy in both versions of code (only it happens
> during calling with pass by value + move, and on line 57 in the current code)

Done.

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

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:1262:
std::vector<base::string16> names;
On 2016/01/25 10:10:25, vabr (Chromium) wrote:
> optional: names.reserve(field_count());

Done.

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

https://codereview.chromium.org/1622073002/diff/80001/components/autofill/cor...
components/autofill/core/browser/form_structure.h:251: static base::string16
FindLongestCommonPrefix(
On 2016/01/25 10:10:26, vabr (Chromium) wrote:
> optional: This could return a StringPiece, saving a copy. It would need
> |strings| to be alive for as long as the result is used, but that's currently
> the case.

Done.

[vabr (Chromium), 2016-01-25 16:40:27]

One important comment and one optional, otherwise LGTM.

Thanks!
Vaclav

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_field.h (right):

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_field.h:57: parseable_name_ =
parseable_name;
Here you need std::move(parseable_name).

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
components/autofill/core/browser/form_structure.cc:1276:
field->set_parseable_name(std::move(truncated));
optional nit:
If you inline truncated, you will not need std::move here and spare one line.

[Mathieu, 2016-01-25 18:06:28]

Thanks, I'll send it to the CQ!

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_field.h (right):

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_field.h:57: parseable_name_ =
parseable_name;
On 2016/01/25 16:40:27, vabr (Chromium) wrote:
> Here you need std::move(parseable_name).

Done.

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/1622073002/diff/100001/components/autofill/co...
components/autofill/core/browser/form_structure.cc:1276:
field->set_parseable_name(std::move(truncated));
On 2016/01/25 16:40:27, vabr (Chromium) wrote:
> optional nit:
> If you inline truncated, you will not need std::move here and spare one line.
Thanks!

[Mathieu, 2016-01-25 18:06:32]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2016-01-25 18:06:32]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1622073002/#ps120001
(title: "std::move")

[commit-bot: I haz the power, 2016-01-25 18:06:46]

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

[commit-bot: I haz the power, 2016-01-25 19:26:23]

Description was changed from

==========
[Autofill] Remove longest common prefix from field names when running
heuristics.

Some frameworks put a prefix on all fields within a form (see bug), which
confuses the heuristics.

BUG=497850
TEST=DataDrivenHeuristics,FormStructure
==========

to

==========
[Autofill] Remove longest common prefix from field names when running
heuristics.

Some frameworks put a prefix on all fields within a form (see bug), which
confuses the heuristics.

BUG=497850
TEST=DataDrivenHeuristics,FormStructure
==========

[commit-bot: I haz the power, 2016-01-25 19:26:23]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-01-25 19:28:56]

Description was changed from

==========
[Autofill] Remove longest common prefix from field names when running
heuristics.

Some frameworks put a prefix on all fields within a form (see bug), which
confuses the heuristics.

BUG=497850
TEST=DataDrivenHeuristics,FormStructure
==========

to

==========
[Autofill] Remove longest common prefix from field names when running
heuristics.

Some frameworks put a prefix on all fields within a form (see bug), which
confuses the heuristics.

BUG=497850
TEST=DataDrivenHeuristics,FormStructure

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

[commit-bot: I haz the power, 2016-01-25 19:28:57]

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

[vabr (Chromium), 2016-01-26 08:56:44]

Hi Mathieu,

A follow-up to the std::move discussion -- turns out the old way (const ref
without move) is still preferred in Chromium [1]. While value+move is certainly
not disallowed, I will refrain from pushing for it in reviews in the future.
Sorry about that.

Cheers,
Vaclav

[1]
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/string...