[Payments] Add billing_address_id and has_converted to autofill_table

[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

Both fields were added to the metadata tables, because those are
intended to be read-write and used to sync this modifiable info across
Chrome instances. 

BUG=680178
Review-Url: https://codereview.chromium.org/2626843004
Cr-Commit-Position: refs/heads/master@{#445949}
Committed: https://chromium.googlesource.com/chromium/src/+/3e77d95a76768a00bfb8cffc4799b52e640c1ae7

[sebsg, 2017-01-11 18:14:26]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-01-11 18:17:39]

Description was changed from

==========
[Payments] Add billing_address_id and has_converted to autofill_table

BUG=680178
==========

to

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table and add the
has_converted column to the server_address_metadata table in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping stone
towards converting Wallet addresses to Autofill profiles and preserving the
billing address relation ship.

BUG=680178
==========

[sebsg, 2017-01-11 18:17:49]

Hi Rouslan, PTAL?

[sebsg, 2017-01-11 18:26:05]

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

[commit-bot: I haz the power, 2017-01-11 18:26:42]

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

[please use gerrit instead, 2017-01-11 19:05:09]

Description was changed from

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table and add the
has_converted column to the server_address_metadata table in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping stone
towards converting Wallet addresses to Autofill profiles and preserving the
billing address relation ship.

BUG=680178
==========

to

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

BUG=680178
==========

[please use gerrit instead, 2017-01-11 19:23:02]

rouslan@chromium.org changed reviewers:
+ rouslan@chromium.org

[please use gerrit instead, 2017-01-11 19:23:03]

I'm not super clear on how these new columns will be used. Two different columns
for billing_address_id for the same server card concerns me. Please elaborate on
these topics in the CL description.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:653: if
(is_off_the_record_ || !database_.get())
I don't see why off the record should prevent updating billing address.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1227:
"masked.billing_address_id "    // 11
Why store billing address identifier in two different places?

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1388: "use_date,
billing_address_id, id)"
You appear to have moved the billing_address_id column from the
masked_credit_cards table to the server_card_metadata table. What advantage does
this provide?

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1401: //
TODO(crbug.com/680182): Record the address conversion status.
This comment is not clear. Please explain what you intend to happen.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:249: //   value       
      The serialized ModelTypeState record.
You must add the new fields to this comment block.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_webdata.h (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_webdata.h:112: // Updates the
use count and last use date for a server card (masked or not).
Update the comment, please.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_webdata.h:115: // Updates the
use count and last use date for a server address.
Update the comment please.

https://codereview.chromium.org/2626843004/diff/20001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/2626843004/diff/20001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:1204:
connection.DoesColumnExist("server_address_metadata", "has_converted"));
Check values as well, please.

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

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

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

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-01-12 15:36:20]

Description was changed from

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

BUG=680178
==========

to

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

Both fields were added to the metadata tables, because those are intended to be
read-write and used to sync this modifiable info across Chrome instances. 

BUG=680178
==========

[sebsg, 2017-01-12 15:36:31]

Thanks! Another look?

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:653: if
(is_off_the_record_ || !database_.get())
On 2017/01/11 19:23:03, rouslan wrote:
> I don't see why off the record should prevent updating billing address.

off the record is currently used to avoid modifying anything relating to
autofill data models (use stats, field values, etc) so I think it makes sense to
make it not update billing address too

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1227:
"masked.billing_address_id "    // 11
On 2017/01/11 19:23:03, rouslan wrote:
> Why store billing address identifier in two different places?

Right, I didn't realize I could just migrate the data. Fixed!

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1388: "use_date,
billing_address_id, id)"
On 2017/01/11 19:23:03, rouslan wrote:
> You appear to have moved the billing_address_id column from the
> masked_credit_cards table to the server_card_metadata table. What advantage
does
> this provide?

I think it makes more sense to have it be in the server_card_metadata table,
because this table is used as read-write, compared to maked_credit_cards which
is supposed to be read only (only refreshed by the messages from sync).

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1401: //
TODO(crbug.com/680182): Record the address conversion status.
On 2017/01/11 19:23:03, rouslan wrote:
> This comment is not clear. Please explain what you intend to happen.

Tried to make the comment more clear. It's just to explain that this value
(has_converted) will not always be set to false. It will get assigned and read
from in a future CL, the one where the wallet addresses get converted to local
Autofill profiles.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:249: //   value       
      The serialized ModelTypeState record.
On 2017/01/11 19:23:03, rouslan wrote:
> You must add the new fields to this comment block.

Seems somehow I ctr-Z them.... thanks for spotting this :)

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_webdata.h (right):

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_webdata.h:112: // Updates the
use count and last use date for a server card (masked or not).
On 2017/01/11 19:23:03, rouslan wrote:
> Update the comment, please.

Done.

https://codereview.chromium.org/2626843004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_webdata.h:115: // Updates the
use count and last use date for a server address.
On 2017/01/11 19:23:03, rouslan wrote:
> Update the comment please.

Done.

https://codereview.chromium.org/2626843004/diff/20001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/2626843004/diff/20001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:1204:
connection.DoesColumnExist("server_address_metadata", "has_converted"));
On 2017/01/11 19:23:03, rouslan wrote:
> Check values as well, please.

Done.

[sebsg, 2017-01-12 18:34:07]

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

[commit-bot: I haz the power, 2017-01-12 18:34:47]

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-12 18:59:00]

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

[commit-bot: I haz the power, 2017-01-12 18:59:01]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[please use gerrit instead, 2017-01-12 19:04:17]

Description was changed from

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

Both fields were added to the metadata tables, because those are intended to be
read-write and used to sync this modifiable info across Chrome instances. 

BUG=680178
==========

to

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

Both fields were added to the metadata tables, because those are
intended to be read-write and used to sync this modifiable info across
Chrome instances. 

BUG=680178
==========

[please use gerrit instead, 2017-01-12 19:10:59]

lgtm % comments

https://codereview.chromium.org/2626843004/diff/60001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2626843004/diff/60001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:195: //  
billing_address_id The guid string that identifies the local profile which
1) Remove the word "guid" from here, please, because guids identify only local
profiles.

2) Note that the billing_address_id can identify server profiles, too.

https://codereview.chromium.org/2626843004/diff/60001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/2626843004/diff/60001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:1214:
EXPECT_EQ("card_1", s_cards_metadata.ColumnString(0));
Add a comment that this is defined in version_70.sql.

[sebsg, 2017-01-12 19:21:11]

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

[sebsg, 2017-01-12 19:21:13]

Hi pkasting@, could you please take a look at the files in components/webdata/*
?

Thanks!

https://codereview.chromium.org/2626843004/diff/60001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/2626843004/diff/60001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:195: //  
billing_address_id The guid string that identifies the local profile which
On 2017/01/12 19:10:59, rouslan wrote:
> 1) Remove the word "guid" from here, please, because guids identify only local
> profiles.
> 
> 2) Note that the billing_address_id can identify server profiles, too.

Right, my initial plan was for this one to contain only guids to local
addresses, but since I merged the columns that's not true anymore. Thanks!

https://codereview.chromium.org/2626843004/diff/60001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/2626843004/diff/60001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:1214:
EXPECT_EQ("card_1", s_cards_metadata.ColumnString(0));
On 2017/01/12 19:10:59, rouslan wrote:
> Add a comment that this is defined in version_70.sql.

Done.

[Peter Kasting, 2017-01-12 23:37:48]

I'm unclear on the compatible version handling here.  In one place, you seem to
be updating the compatible version number, bu in another you set the variable
about "update compatible version" to false.  It's not clear to me whether the
compatible version needs updating, because I can't tell whether you're moving or
copying data; "migrating" data from one table to another sounds like "moving" to
me, but the code looks more as if it's copying this data.  Maybe I'm misreading
it.

It seems like you want to move (not copy) the data, in which case you'd need to
update the compatible version number everywhere as old versions of Chrome would
not be able to read the new tables.

https://codereview.chromium.org/2626843004/diff/80001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2626843004/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2471: bool
AutofillTable::MigrateToVersion71AddHasConvertedAndBillingAddressIdMetadata() {
Nit: 80 columns (git cl format will find this sort of thing)

[sebsg, 2017-01-13 15:23:10]

Hi Peter, thanks for the comments. From what I understand now, I think copying
might be better? From what I see it's either

1- Copy the data, old compatible version still work.
2- Migrate the data, old version won't work and will need updating.

Feels like 1 is less troublesome? Also it seems like it's been done in the past,
like unmasked_server_card which still has the use_count and use_data columns
that are deprecated. But you know a lot more about this stuff so I'll defer to
you.

Thanks again.

https://codereview.chromium.org/2626843004/diff/80001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/2626843004/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2471: bool
AutofillTable::MigrateToVersion71AddHasConvertedAndBillingAddressIdMetadata() {
On 2017/01/12 23:37:48, Peter Kasting wrote:
> Nit: 80 columns (git cl format will find this sort of thing)

Done.

[Peter Kasting, 2017-01-17 20:54:22]

On 2017/01/13 15:23:10, sebsg wrote:
> Hi Peter, thanks for the comments. From what I understand now, I think copying
> might be better? From what I see it's either
> 
> 1- Copy the data, old compatible version still work.
> 2- Migrate the data, old version won't work and will need updating.
> 
> Feels like 1 is less troublesome? Also it seems like it's been done in the
past,
> like unmasked_server_card which still has the use_count and use_data columns
> that are deprecated. But you know a lot more about this stuff so I'll defer to
> you.

The problem with copying data is that now you have two copies of the data, which
requires more disk space; and you need to keep both in sync, which requires more
code and more runtime cost, and increases the chance of bugs; or you leave one
unmaintained, which is basically a guaranteed way of getting bugs :)

The compatible version thing is basically a developer nicety -- we don't
guarantee Chrome version X will read a profile from Chrome version X+1, we just
don't break it unless we have to.  But don't sacrifice the other principles for
it.

So my suggestion is to migrate, not copy (and to also remove the deprecated
other columns, probably in a separate CL; deprecating columns is basically an
antipattern for the same reasons as above).

[sebsg, 2017-01-18 18:31:04]

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

[sebsg, 2017-01-18 18:31:17]

Patchset #6 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-01-18 18:31:26]

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

[sebsg, 2017-01-18 18:32:54]

I see, thanks for the explanation! Could you take another? Thanks!

[commit-bot: I haz the power, 2017-01-18 18:35:04]

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

[commit-bot: I haz the power, 2017-01-18 18:35:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sebsg, 2017-01-18 19:02:20]

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

[sebsg, 2017-01-18 19:02:26]

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

[commit-bot: I haz the power, 2017-01-18 19:03:11]

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-18 19:31:26]

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

[commit-bot: I haz the power, 2017-01-18 19:31:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

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

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

[commit-bot: I haz the power, 2017-01-18 19:44:57]

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-18 20:53:13]

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

[commit-bot: I haz the power, 2017-01-18 20:53:14]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-01-24 21:13:31]

components/webdata LGTM, but I did not review the autofill code (meat of the
change), which you should get a re-review of.

[sebsg, 2017-01-24 21:24:09]

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

[sebsg, 2017-01-24 21:24:10]

Thanks Peter! +mathp for the Autofill code.

[Mathieu, 2017-01-25 03:04:30]

lgtm, thanks

[sebsg, 2017-01-25 03:28:31]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-01-25 03:28:32]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2626843004/#ps180001
(title: "Fixed android compile failure in test")

[commit-bot: I haz the power, 2017-01-25 03:28:59]

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-25 04:51:28]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485314911458920,
"parent_rev": "c5daf520b6c9deab9b911e19827b4216f1818ed4", "commit_rev":
"3e77d95a76768a00bfb8cffc4799b52e640c1ae7"}

[commit-bot: I haz the power, 2017-01-25 04:52:13]

Description was changed from

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

Both fields were added to the metadata tables, because those are
intended to be read-write and used to sync this modifiable info across
Chrome instances. 

BUG=680178
==========

to

==========
[Payments] Add billing_address_id and has_converted to autofill_table

Adds the billing_address_id column to the server_card_metadata table
and add the has_converted column to the server_address_metadata table
in autofill_table.

This CL does not modify any user noticeable behavior but is a stepping
stone towards converting Wallet addresses to Autofill profiles and
preserving the billing address relation ship.

Both fields were added to the metadata tables, because those are
intended to be read-write and used to sync this modifiable info across
Chrome instances. 

BUG=680178

Review-Url: https://codereview.chromium.org/2626843004
Cr-Commit-Position: refs/heads/master@{#445949}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e77d95a76768a00bfb8cffc4799...
==========

[commit-bot: I haz the power, 2017-01-25 04:52:15]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3e77d95a76768a00bfb8cffc4799...