[Autofill] Check for full state name and abbreviation when determining types to upload.

Added code to try getting the full text value and abbreviation of a possible state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest

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

[sebsg, 2016-03-17 17:26:10]

Patchset #2 (id:20001) has been deleted

[sebsg, 2016-03-17 17:34:13]

Patchset #2 (id:40001) has been deleted

[sebsg, 2016-03-17 17:34:20]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-03-17 17:57:35]

Patchset #1 (id:60001) has been deleted

[sebsg, 2016-03-17 17:59:20]

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

[sebsg, 2016-03-17 17:59:20]

Could you please take a look when you get the chance? Thanks!

[sebsg, 2016-03-17 18:00:01]

Description was changed from

==========
[Autofill] Check for full state name and abbreviation when determining types to
upload.

Added code to try getting the full text value and abbeviation of a possible
state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest
==========

to

==========
Added code to try getting the full text value and abbreviation of a possible
state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest
==========

[sebsg, 2016-03-17 18:43:12]

Patchset #1 (id:80001) has been deleted

[vabr (Chromium), 2016-03-18 12:03:50]

LGTM with an optional nit.

Cheers,
Vaclav

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

https://codereview.chromium.org/1814883002/diff/100001/components/autofill/co...
components/autofill/core/browser/address.cc:208: base::string16 state_name,
state_abbreviation;
optional: One line per variable, please.
I believe this was part of the style guide at some point, but could not find it
anymore (hence this is optional). Most of the code still honours this rule,
however, so it might confuse the reader to have two variables defined on one
line.

[sebsg, 2016-03-18 13:03:55]

Thanks, I also changed the other calls to this method that used a one line
instantiation.

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

https://codereview.chromium.org/1814883002/diff/100001/components/autofill/co...
components/autofill/core/browser/address.cc:208: base::string16 state_name,
state_abbreviation;
On 2016/03/18 12:03:50, vabr (Chromium) wrote:
> optional: One line per variable, please.
> I believe this was part of the style guide at some point, but could not find
it
> anymore (hence this is optional). Most of the code still honours this rule,
> however, so it might confuse the reader to have two variables defined on one
> line.

Thanks, I also changed the other calls to this method that used a one line
instantiation.

[vabr (Chromium), 2016-03-18 13:12:58]

LGTM, thank you!
Vaclav

[Mathieu, 2016-03-18 13:56:04]

The code looks good, I just have a question.

https://codereview.chromium.org/1814883002/diff/120001/components/autofill/co...
File components/autofill/core/browser/address.cc (right):

https://codereview.chromium.org/1814883002/diff/120001/components/autofill/co...
components/autofill/core/browser/address.cc:212: if (!state_name.empty() ||
!state_abbreviation.empty()) {
At this point, we're pretty certain the field is a state field, right? We've
looked it up in our state database and it's a match (Note that this may become
an expensive lookup when we add tons more countries and their regions). I wonder
why/if we need to check the person's profile to make sure it's their own state.
WDYT, is it good to be cautious? Or does GetMatchingTypes get used for *other*
things other than uploading votes?

[sebsg, 2016-03-18 14:25:54]

Good question Mathieu, thanks for bringing it up!

https://codereview.chromium.org/1814883002/diff/120001/components/autofill/co...
File components/autofill/core/browser/address.cc (right):

https://codereview.chromium.org/1814883002/diff/120001/components/autofill/co...
components/autofill/core/browser/address.cc:212: if (!state_name.empty() ||
!state_abbreviation.empty()) {
On 2016/03/18 13:56:04, Mathieu Perreault wrote:
> At this point, we're pretty certain the field is a state field, right? We've
> looked it up in our state database and it's a match (Note that this may become
> an expensive lookup when we add tons more countries and their regions). I
wonder
> why/if we need to check the person's profile to make sure it's their own
state.
> WDYT, is it good to be cautious? Or does GetMatchingTypes get used for *other*
> things other than uploading votes?

It was to be cautious and also consistent with all the other cases where we
check if it matches an information from a profile. The only case I can think of
is New York city (which would be detected as a state since it's in the New York
state). However, this problem seems inevitable and happens regardless of
checking the person's state or not. It should also not happen often enough to be
a problem (compared with all the uploads). 

Since the only place GetMatchingTypes is called from is
DeterminePossibleFieldTypesForUpload, I think we could probably remove the
matching with the person's state.

So I'll go ahead and remove it unless there are objections. Thanks!

[Mathieu, 2016-03-18 14:35:20]

lgtm keep as is

https://codereview.chromium.org/1814883002/diff/120001/components/autofill/co...
File components/autofill/core/browser/address.cc (right):

https://codereview.chromium.org/1814883002/diff/120001/components/autofill/co...
components/autofill/core/browser/address.cc:212: if (!state_name.empty() ||
!state_abbreviation.empty()) {
On 2016/03/18 at 14:25:54, sebsg wrote:
> On 2016/03/18 13:56:04, Mathieu Perreault wrote:
> > At this point, we're pretty certain the field is a state field, right? We've
> > looked it up in our state database and it's a match (Note that this may
become
> > an expensive lookup when we add tons more countries and their regions). I
wonder
> > why/if we need to check the person's profile to make sure it's their own
state.
> > WDYT, is it good to be cautious? Or does GetMatchingTypes get used for
*other*
> > things other than uploading votes?
> 
> It was to be cautious and also consistent with all the other cases where we
check if it matches an information from a profile. The only case I can think of
is New York city (which would be detected as a state since it's in the New York
state). However, this problem seems inevitable and happens regardless of
checking the person's state or not. It should also not happen often enough to be
a problem (compared with all the uploads). 
> 
> Since the only place GetMatchingTypes is called from is
DeterminePossibleFieldTypesForUpload, I think we could probably remove the
matching with the person's state.
> 
> So I'll go ahead and remove it unless there are objections. Thanks!

Another case of the New York, New York case is Quebec, Quebec. But that one is
irrelevant like you said. What interests me is if you live in a city that is
another country's state name. For example, if there is a city in the world named
California. In that case we DON'T want to vote for STATE, because it's a city
field. I think we should keep your code as is.

[sebsg, 2016-03-18 14:39:26]

Patchset #3 (id:140001) has been deleted

[sebsg, 2016-03-18 14:40:08]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-03-18 14:40:24]

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

[sebsg, 2016-03-18 14:42:47]

A quick search revealed that there are in fact a couple of cities names
California in the US. In Maine, Kentucky, etc... So keeping the check is a good
idea!

[commit-bot: I haz the power, 2016-03-18 15:26:43]

Description was changed from

==========
Added code to try getting the full text value and abbreviation of a possible
state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest
==========

to

==========
Added code to try getting the full text value and abbreviation of a possible
state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest
==========

[commit-bot: I haz the power, 2016-03-18 15:26:44]

Committed patchset #2 (id:120001)

[commit-bot: I haz the power, 2016-03-18 15:28:00]

Description was changed from

==========
Added code to try getting the full text value and abbreviation of a possible
state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest
==========

to

==========
Added code to try getting the full text value and abbreviation of a possible
state and compare these values to the saved profile's state value.

BUG=595734
TEST=AutofillManagerTest

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

[commit-bot: I haz the power, 2016-03-18 15:28:02]

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