Delete unmasked credit cards when clearing data.

Delete unmasked credit cards when clearing data.

This hooks up unmasked server credit cards to the Chrome clear private data feature. When all data is cleared, all cards will be re-masked.

For time ranges, only cards unmasked in that time range will be re-masked. The use-case for re-masking cards unmasked in the past hour is very weak, but we just don't want to re-mask cards that you unmasked a long time ago, and this is how everything else works.

This adds a "phone number" column to the server address table. This will be hooked up in a future patch. Doing it here saves another database version bump.

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

[brettw, 2015-03-04 23:03:22]

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

[Evan Stade, 2015-03-04 23:12:25]

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

https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1198: void
AutofillTable::SetServerCreditCards(
Why does this function support an input vector of both masked and unmasked
cards? Isn't it only used with masked cards? Unmasking should always happen via
UnmaskServerCreditCard, right?

https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1233: sql::Statement
masked_insert(db_->GetUniqueStatement(
there are multiple inserts in this function --- wrap in a transaction?

[brettw, 2015-03-04 23:21:46]

Now with phone number

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

https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1198: void
AutofillTable::SetServerCreditCards(
It was convenient for testing.

https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1233: sql::Statement
masked_insert(db_->GetUniqueStatement(
On 2015/03/04 23:12:25, Evan Stade wrote:
> there are multiple inserts in this function --- wrap in a transaction?

Sure. I added one. I think this will actually be in a bigger outer transaction
anyway. The WebDB transaction model is pretty messed up so it's "hard to say"
and I have a post-it on my desk to fix it.

[Evan Stade, 2015-03-05 01:54:24]

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

https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1198: void
AutofillTable::SetServerCreditCards(
On 2015/03/04 23:21:45, brettw wrote:
> It was convenient for testing.

I don't think it makes tests that much easier to write, but if you find it does
then I'd rather there be a test util function that calls SetServerCreditCards
then UnmaskServerCreditCard as appropriate, rather than putting test-only code
in this file. Then we don't have to maintain as much code and we get better test
coverage of real code.

[brettw, 2015-03-05 20:52:19]

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

https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1198: void
AutofillTable::SetServerCreditCards(
I spent a while changing this but it's not worthwhile. Almost all of the server
card tests, including the ones you wrote, depend on this behavior. It also means
that you can't make a vector of cards, set them to the DB, and compare results
you get out the other end since you have to maintain both a masked card for
adding, and an unmasked card for doing the comparison out the other end. This
definitely makes the tests harder.

[Evan Stade, 2015-03-05 22:36:23]

On 2015/03/05 20:52:19, brettw wrote:
>
https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
> File components/autofill/core/browser/webdata/autofill_table.cc (right):
> 
>
https://codereview.chromium.org/967453005/diff/20001/components/autofill/core...
> components/autofill/core/browser/webdata/autofill_table.cc:1198: void
> AutofillTable::SetServerCreditCards(
> I spent a while changing this but it's not worthwhile. Almost all of the
server
> card tests, including the ones you wrote, depend on this behavior. It also
means
> that you can't make a vector of cards, set them to the DB, and compare results
> you get out the other end since you have to maintain both a masked card for
> adding, and an unmasked card for doing the comparison out the other end. This
> definitely makes the tests harder.

what do you think of this: https://codereview.chromium.org/982203002

[Evan Stade, 2015-03-06 00:20:51]

lg aside from SetServerCreditCards

https://codereview.chromium.org/967453005/diff/40001/components/autofill/core...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/967453005/diff/40001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1287: return;
nit: unnecessary return

https://codereview.chromium.org/967453005/diff/40001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:1445: if
(!s_unmasked_cards.Run())
nit: return s_unmasked_cards.Run()

https://codereview.chromium.org/967453005/diff/40001/components/autofill/core...
components/autofill/core/browser/webdata/autofill_table.cc:2031: bool
AutofillTable::MigrateToVersion64AddUnmaskDate() {
transaction

[brettw, 2015-03-06 18:48:28]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2015-03-06 18:49:43]

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

[commit-bot: I haz the power, 2015-03-06 18:49:44]

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

[commit-bot: I haz the power, 2015-03-06 18:49:45]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[brettw, 2015-03-06 19:08:15]

PTAL

[Evan Stade, 2015-03-06 20:23:10]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-03-06 20:23:11]

lgtm

[commit-bot: I haz the power, 2015-03-06 20:31:53]

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

[brettw, 2015-03-06 21:25:04]

The CQ bit was unchecked by brettw@chromium.org

[brettw, 2015-03-06 21:25:44]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2015-03-06 21:26:57]

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

[commit-bot: I haz the power, 2015-03-07 00:09:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-07 00:10:05]

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