Only UTF-8 sync tags for Wallet metadata.

Only UTF-8 sync tags and server ID for Wallet metadata.

Wallet address server ID is not valid UTF-8, so it cannot be used as a
sync tag or as metadata server ID. This patch base64 encodes the sync
tags and the server ID.

BUG=481595
Committed: https://crrev.com/117ee8b5ff7a2d37aaf2f2f05ca9338260ec8bb6
Cr-Commit-Position: refs/heads/master@{#338407}

[please use gerrit instead, 2015-07-01 20:43:11]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2015-07-01 20:44:47]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2015-07-02 03:49:10]

Patchset #1 (id:40001) has been deleted

[please use gerrit instead, 2015-07-03 00:45:24]

Patchset #1 (id:60001) has been deleted

[please use gerrit instead, 2015-07-03 00:47:32]

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

[please use gerrit instead, 2015-07-03 00:47:32]

Ilya, PTAL.

Let me know if you can think of a way to make the tests more readable.

[Ilya Sherman, 2015-07-06 23:39:01]

Hmm, what are the Wallet server ids?  Are they just random numbers, basically? 
Is there a guarantee that there aren't random null terminator characters in the
middle of the ids?  If not, does our code properly handle such things?

It's rather a shame to lose human-readable labels for data that would previously
be human-readable.  If the Wallet server ids are just random numbers anyway,
then it's not really a problem; but if they're mostly human readable, then I'd
rather try to find a different solution.

Finally, what's the migration plan for these?  Do we need to migrate or delete
some existing data from the sync server, to account for the id changeover?

https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:164: // Not valid UTF-8.
nit: "Not valid" -> "Not necessarily valid"

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

https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:248:
remote_.UpdateAddressStats(BuildAddress("addr\xEF\xBF\xBE", 1, 2));
Let's define named constants for these strings now that they're not very
human-readable.  The named constants can have comments explaining their lineage.

[please use gerrit instead, 2015-07-07 00:24:55]

On 2015/07/06 23:39:01, Ilya Sherman (Away July 8-13) wrote:
> Hmm, what are the Wallet server ids?  Are they just random numbers, basically?

> Is there a guarantee that there aren't random null terminator characters in
the
> middle of the ids?  If not, does our code properly handle such things?
>
> It's rather a shame to lose human-readable labels for data that would
previously
> be human-readable.  If the Wallet server ids are just random numbers anyway,
> then it's not really a problem; but if they're mostly human readable, then I'd
> rather try to find a different solution.

Wallet server IDs for cards a base64 encoded identifiers. Wallet addresses have
blank server IDs. Therefore, there will be no random null terminator characters
in the Wallet server IDs for cards and addresses as they come from the sync
server.

Chrome generates a 20 byte identifier for Wallet addresses. This identifier is
not valid UTF8. This was OK while the identifiers were stored in sqlite table.
Now that we are starting to sync Wallet metadata, the metadata needs IDs that
are valid UTF8. Therefore, metadata cannot use the ID of the Wallet address
straight up. That's why I am taking address's non-UTF8 identifier and
base64-encoding it to use as a UTF8 identifier for address metadata.

Note that I am base64-encoding IDs for both cards and addresses for simplicity,
although Wallet cards always have UTF8 server ID.


> Finally, what's the migration plan for these?  Do we need to migrate or delete
> some existing data from the sync server, to account for the id changeover?

Any existing metadata on the sync server will be deleted, because its sync tag
will be different from its base64 encoding. This is a property of the
AutofillWalletMetadataSyncableService. This is not expected to affect many
people, because the feature is behind a command line flag, disabled by default.
Also, the sync server supports metadata for ~1 week so far.

>
https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
> File components/autofill/core/browser/autofill_profile.h (right):
> 
>
https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
> components/autofill/core/browser/autofill_profile.h:164: // Not valid UTF-8.
> nit: "Not valid" -> "Not necessarily valid"
> 
>
https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
> File
>
components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1220093003/diff/80001/components/autofill/cor...
>
components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:248:
> remote_.UpdateAddressStats(BuildAddress("addr\xEF\xBF\xBE", 1, 2));
> Let's define named constants for these strings now that they're not very
> human-readable.  The named constants can have comments explaining their
lineage.

Working on addressing your comments now...

[please use gerrit instead, 2015-07-07 21:17:00]

Ilya, PTAL Patch Set 2.

Turns out all tests use at most two address and cards at at time, so I could get
away with four identifiers total. For example, I replaced "addr" with "addr1";
"new-addr" and "unknown-addr" are replaced with "addr2".

https://codereview.chromium.org/1220093003/diff/100001/components/autofill/co...
File
components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc
(left):

https://codereview.chromium.org/1220093003/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_wallet_metadata_syncable_service_unittest.cc:804:
remote_.UpdateCardStats(BuildCard("card4", 7, 8));
These were card3 and card4 by a typo. They should've been card1 and card2
instead.

[Ilya Sherman, 2015-07-07 22:09:24]

LGTM

[please use gerrit instead, 2015-07-07 22:10:20]

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

[please use gerrit instead, 2015-07-07 22:10:21]

Nicolas, PTAL in Patch Set 2:

 sync/protocol/autofill_specifics.proto

[Nicolas Zea, 2015-07-10 23:21:19]

sync LGTM

[please use gerrit instead, 2015-07-10 23:24:17]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2015-07-10 23:25:55]

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

[commit-bot: I haz the power, 2015-07-11 00:14:43]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2015-07-11 00:15:52]

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