Create syncable metadata table for Wallet credit cards and addresses.

Create syncable metadata table for Wallet credit cards and addresses.

This is only used for usage statistics (without which we can't apply smart ordering to masked credit card suggestions, or sync usage stats between devices for unmasked cards).

There was already code in place for IDs for addresses, which was essentially unused (ID was always empty). This generates an ID to help identify server addresses for the usage stat tracking.

The tables are not actually synced yet (that's TODO).

BUG=none

Committed: https://crrev.com/378d323b7aa68cddef9429901a7992c4b6346c85
Cr-Commit-Position: refs/heads/master@{#324142}

[Evan Stade, 2015-03-31 00:49:25]

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

[Evan Stade, 2015-03-31 00:49:26]

this WIP is not ready for full review, but I'd like to get your feedback on this
approach. The new table should be synced.

[brettw, 2015-04-01 22:22:35]

Didn't look at the details but the concept of a separate table for this seems OK
to me. Let me know when this is ready for detailed review.

[Evan Stade, 2015-04-06 23:24:07]

ready for real review

[brettw, 2015-04-07 18:04:06]

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

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.cc:833: std::string contents =
I would write this with different lines of contents.append() since this avoids
making a temporary for every + operation.

I also suspect if we did that it would be better to keep this as a string16 and
then call UTF16ToUTF8 once when passing it to SHA1HashString since this will
again only make one temporary instead of 13.

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

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.h:185: // FIXME documentation
finish this.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:987: sql::Statement
metadata_delete(db_->GetUniqueStatement(
Are you sure that you can delete a row that you have a different cursor to, and
then continue from that same cursor? I'm not sure. I poked around the sqlite
docs and I didn't find any info.

I would normally do this in two passes, saving the ones to delete in a temporary
vector. This has the added benefit that you can prepare the metatdata delete
statement only once above the loop and then re-use that each time through.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1288: sql::Statement
metadata_delete(db_->GetUniqueStatement(
Ditto about deleting.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1376: "INSERT INTO
server_card_metadata("
I think you can join this line and the next.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1403: "INSERT INTO
server_address_metadata("
Can this and the next be joined?

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:2231:
profile.SetRawInfo(COMPANY_NAME, s.ColumnString16(index++));
It seems bad to duplicate this.

Can we call GetServerProfiles + SetServerProfiles to re-use all of this logic?

[Evan Stade, 2015-04-07 19:23:32]

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

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.cc:833: std::string contents =
On 2015/04/07 18:04:06, brettw wrote:
> I would write this with different lines of contents.append() since this avoids
> making a temporary for every + operation.
> 
> I also suspect if we did that it would be better to keep this as a string16
and
> then call UTF16ToUTF8 once when passing it to SHA1HashString since this will
> again only make one temporary instead of 13.

Done.

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

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_profile.h:185: // FIXME documentation
On 2015/04/07 18:04:06, brettw wrote:
> finish this.

Done.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1288: sql::Statement
metadata_delete(db_->GetUniqueStatement(
On 2015/04/07 18:04:06, brettw wrote:
> Ditto about deleting.

This code is essentially a copy-paste of the above code (deleting items from
unmasked table). I've changed all 3 places to use a single sql statement.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1376: "INSERT INTO
server_card_metadata("
On 2015/04/07 18:04:06, brettw wrote:
> I think you can join this line and the next.

Done.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1403: "INSERT INTO
server_address_metadata("
On 2015/04/07 18:04:06, brettw wrote:
> Can this and the next be joined?

Done.

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:2231:
profile.SetRawInfo(COMPANY_NAME, s.ColumnString16(index++));
On 2015/04/07 18:04:06, brettw wrote:
> It seems bad to duplicate this.
> 
> Can we call GetServerProfiles + SetServerProfiles to re-use all of this logic?

I explicitly wanted to avoid that. We generally do copy code into the migration
functions rather than call into non-migration code. In the future,
GetServerProfiles or SetServerProfiles may change as implementation details
change. This migration function should never change.

[brettw, 2015-04-07 21:02:31]

lgtm

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:2231:
profile.SetRawInfo(COMPANY_NAME, s.ColumnString16(index++));
Oh I see. I think we should have a comment to this effect so nobody "refactors"
this in the future.

[Evan Stade, 2015-04-07 21:26:24]

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/1042353003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:2231:
profile.SetRawInfo(COMPANY_NAME, s.ColumnString16(index++));
On 2015/04/07 21:02:31, brettw wrote:
> Oh I see. I think we should have a comment to this effect so nobody
"refactors"
> this in the future.

I added a comment in the header.

[Evan Stade, 2015-04-07 21:26:28]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-04-07 21:26:29]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1042353003/#ps160001
(title: "comment about migration code")

[commit-bot: I haz the power, 2015-04-07 21:27:12]

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

[commit-bot: I haz the power, 2015-04-07 22:54:20]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-07 22:55:41]

Patchset 9 (id:??) landed as
https://crrev.com/378d323b7aa68cddef9429901a7992c4b6346c85
Cr-Commit-Position: refs/heads/master@{#324142}