Autofill: Improve heuristics for city/state/zip fields.

Autofill: Improve heuristics for city/state/zip fields.

These fields are often grouped together, but in different orders.
In several cases, fields have labels that are ambiguous but names that
are not. When there is ambiguity, parse fields with the name and label
separately and see which field type they best match.

BUG=442552
Committed: https://crrev.com/cc4cecdfadf9ac3c52e596d36241555668e461be
Cr-Commit-Position: refs/heads/master@{#322315}

[Lei Zhang, 2015-03-23 21:01:18]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-03-23 21:19:29]

Patchset #1 (id:20001) has been deleted

[Lei Zhang, 2015-03-23 21:23:02]

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

[Lei Zhang, 2015-03-23 21:23:02]

I went with this different approach and dropped the pattern attribute approach
that we talked about last week. The pattern attribute didn't help with the Ikea
test case. I may bring it back in the future if I find a use for it.

[Evan Stade, 2015-03-24 00:04:32]

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

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:35: // static
don't think the // static notation is useful for constants

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:293: // Check for name + label
matches.
nit: "Check for matches to both name and label."

comes across slightly clearer (it's not obvious that the plus is a logical and)

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:305: bool maybe_city =
(city_result != RESULT_MATCH_NONE);
nit: remove excess parens

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:371: // Parse a city name. 
Some UK pages (e.g. The China Shop2.html) use
I dunno where this comment belongs, but not here

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:376: // Select fields are
allowed here.  This occurs on top-100 site rediff.com.
repeated comment

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

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:109:
FormField::ParseNameLabelResult FormField::ParseNameAndLabelSeparately(
do you anticipate using this for any other form field type

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:247: return match_type &
~MATCH_LABEL;
just inline these two functions imo

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_field.h (right):

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_field.h:58: enum ParseNameLabelResult {
docs

[Lei Zhang, 2015-03-25 00:42:29]

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

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:35: // static
On 2015/03/24 00:04:32, Evan Stade wrote:
> don't think the // static notation is useful for constants

Done.

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:293: // Check for name + label
matches.
On 2015/03/24 00:04:32, Evan Stade wrote:
> nit: "Check for matches to both name and label."
> 
> comes across slightly clearer (it's not obvious that the plus is a logical
and)

Done.

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:305: bool maybe_city =
(city_result != RESULT_MATCH_NONE);
On 2015/03/24 00:04:32, Evan Stade wrote:
> nit: remove excess parens

Done.

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:371: // Parse a city name. 
Some UK pages (e.g. The China Shop2.html) use
On 2015/03/24 00:04:32, Evan Stade wrote:
> I dunno where this comment belongs, but not here

That's because it started it out as a copy + paste of AddressField::ParseCity().
I've removed both comments.

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/address_field.cc:376: // Select fields are
allowed here.  This occurs on top-100 site rediff.com.
On 2015/03/24 00:04:32, Evan Stade wrote:
> repeated comment

deleted

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

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:109:
FormField::ParseNameLabelResult FormField::ParseNameAndLabelSeparately(
On 2015/03/24 00:04:32, Evan Stade wrote:
> do you anticipate using this for any other form field type

Not 100% sure, but I feel this may come in handy later.

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_field.cc:247: return match_type &
~MATCH_LABEL;
On 2015/03/24 00:04:32, Evan Stade wrote:
> just inline these two functions imo

Done.

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
File components/autofill/core/browser/form_field.h (right):

https://codereview.chromium.org/1028633004/diff/40001/components/autofill/cor...
components/autofill/core/browser/form_field.h:58: enum ParseNameLabelResult {
On 2015/03/24 00:04:32, Evan Stade wrote:
> docs

Done.

[Evan Stade, 2015-03-26 01:56:47]

lgtm

[Lei Zhang, 2015-03-26 02:09:36]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-03-26 02:10:03]

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

[commit-bot: I haz the power, 2015-03-26 04:07:28]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-03-26 04:08:02]

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