[Autofill] Disambiguate similar types before uploading.

Added the uploads for the new credit card types.

Added the DisambiguateUploadTypes method that uses the context from the other fields to disambiguate a field that has multiple upload types.

BUG=589535, 589536
TEST=AutofillManagerTest

Committed: https://crrev.com/2c0656f3a65e7acf418534c3f1d014e3e023859e
Cr-Commit-Position: refs/heads/master@{#382852}

[sebsg, 2016-03-21 21:37:38]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-03-21 21:39:22]

Description was changed from

==========
[Autofill] Disambiguate similar types before uploading.

BUG=589535,589536
==========

to

==========
Added the uploads for the new credit card types.

Added the DisambiguateUploadTypes method that uses the context from the other
fields to disambiguate a field that has multiple upload types.

BUG=589535, 589536
TEST=AutofillManagerTest
==========

[sebsg, 2016-03-22 02:16:39]

Patchset #1 (id:20001) has been deleted

[sebsg, 2016-03-22 02:28:17]

Patchset #1 (id:40001) has been deleted

[sebsg, 2016-03-22 02:36:41]

Patchset #1 (id:60001) has been deleted

[sebsg, 2016-03-22 02:38:48]

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

[sebsg, 2016-03-22 02:38:48]

Hi Václav,

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

Thanks!

[vabr (Chromium), 2016-03-22 17:08:10]

Hello Sebastien,

LGTM, just nits.

Cheers,
Vaclav

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

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1924: bool continue_search
= true;
This variable looks unused (only assigned, never read).

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1960:
SelectRightNameType(upload_types, &matching_types,
Because we already know that is_previous_credit_card == is_next_credit_card,
lines 1960-1 and 1963 are equivalent, aren't they? If so, then we can omit the
if-branching and just make it a single line.

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

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3544: {
This is an initializer list, IIUC, and those are currently banned in
http://chromium-cpp.appspot.com/.

Could you make this a TestFieldData test_cases[][] or TestFieldData * const
test_cases[]?

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3649: for
(std::vector<TestFieldData> test_fields : test_cases) {
nit: Please do not copy into test_fields, use a const reference instead. Feel
free to name the type const auto&.

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3657: for (size_t
i = 0; i < test_fields.size(); ++i) {
Let's use a range-based loop here, it's less clutter.
(I see why that's not possible for the other two loops below.)

[sebsg, 2016-03-22 18:30:27]

Could you please take another look? Thanks!

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

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1924: bool continue_search
= true;
On 2016/03/22 17:08:10, vabr (Chromium) wrote:
> This variable looks unused (only assigned, never read).

Done.

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1960:
SelectRightNameType(upload_types, &matching_types,
On 2016/03/22 17:08:10, vabr (Chromium) wrote:
> Because we already know that is_previous_credit_card == is_next_credit_card,
> lines 1960-1 and 1963 are equivalent, aren't they? If so, then we can omit the
> if-branching and just make it a single line.

Not necessarily. The previous check (line 1949) only returns if both a previous
and next type are found and are not from the same type group. You can still
reach line 1955 with is_previous_credit_card != is_next_credit_card.

For example a case where the next field is credit card related but there is no
previous field. is_next_credit_card would be true and is_previous_credit_card
would be false (but have no meaning since has_found_previous_type is false).
Therefore that "if" (line 1957) is necessary to send the is_next_credit_card for
which the value is meaningful in this case and not is_previous_credit_card. 

I updated the comment to be clearer thanks!

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

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3544: {
On 2016/03/22 17:08:10, vabr (Chromium) wrote:
> This is an initializer list, IIUC, and those are currently banned in
> http://chromium-cpp.appspot.com/.
> 
> Could you make this a TestFieldData test_cases[][] or TestFieldData * const
> test_cases[]?

I initialized the vector manually. I think it was the simplest solution. The
others were not as straightforward as they looked unfortunately. I really hope
they allow initializer lists soon.

Thanks for spotting this.

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3649: for
(std::vector<TestFieldData> test_fields : test_cases) {
On 2016/03/22 17:08:10, vabr (Chromium) wrote:
> nit: Please do not copy into test_fields, use a const reference instead. Feel
> free to name the type const auto&.

Done.

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3657: for (size_t
i = 0; i < test_fields.size(); ++i) {
On 2016/03/22 17:08:10, vabr (Chromium) wrote:
> Let's use a range-based loop here, it's less clutter.
> (I see why that's not possible for the other two loops below.)

Done.

[vabr (Chromium), 2016-03-23 08:41:22]

LGTM, thanks!
Vaclav

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

https://codereview.chromium.org/1821053002/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:1960:
SelectRightNameType(upload_types, &matching_types,
On 2016/03/22 18:30:26, sebsg wrote:
> On 2016/03/22 17:08:10, vabr (Chromium) wrote:
> > Because we already know that is_previous_credit_card == is_next_credit_card,
> > lines 1960-1 and 1963 are equivalent, aren't they? If so, then we can omit
the
> > if-branching and just make it a single line.
> 
> Not necessarily. The previous check (line 1949) only returns if both a
previous
> and next type are found and are not from the same type group. You can still
> reach line 1955 with is_previous_credit_card != is_next_credit_card.
> 
> For example a case where the next field is credit card related but there is no
> previous field. is_next_credit_card would be true and is_previous_credit_card
> would be false (but have no meaning since has_found_previous_type is false).
> Therefore that "if" (line 1957) is necessary to send the is_next_credit_card
for
> which the value is meaningful in this case and not is_previous_credit_card. 
> 
> I updated the comment to be clearer thanks!

Thanks for the explanation, you are right.

[sebsg, 2016-03-23 15:40:58]

The CQ bit was checked by sebsg@chromium.org

[commit-bot: I haz the power, 2016-03-23 15:41:05]

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

[commit-bot: I haz the power, 2016-03-23 15:48:52]

Description was changed from

==========
Added the uploads for the new credit card types.

Added the DisambiguateUploadTypes method that uses the context from the other
fields to disambiguate a field that has multiple upload types.

BUG=589535, 589536
TEST=AutofillManagerTest
==========

to

==========
Added the uploads for the new credit card types.

Added the DisambiguateUploadTypes method that uses the context from the other
fields to disambiguate a field that has multiple upload types.

BUG=589535, 589536
TEST=AutofillManagerTest
==========

[commit-bot: I haz the power, 2016-03-23 15:48:53]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2016-03-23 15:49:59]

Description was changed from

==========
Added the uploads for the new credit card types.

Added the DisambiguateUploadTypes method that uses the context from the other
fields to disambiguate a field that has multiple upload types.

BUG=589535, 589536
TEST=AutofillManagerTest
==========

to

==========
Added the uploads for the new credit card types.

Added the DisambiguateUploadTypes method that uses the context from the other
fields to disambiguate a field that has multiple upload types.

BUG=589535, 589536
TEST=AutofillManagerTest

Committed: https://crrev.com/2c0656f3a65e7acf418534c3f1d014e3e023859e
Cr-Commit-Position: refs/heads/master@{#382852}
==========

[commit-bot: I haz the power, 2016-03-23 15:50:00]

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