[sync] Add autofill sync metadata to the web db

[sync] Add autofill sync metadata to the web db

Two types of sync metadata, once stored in the sync directory, are now being
migrated to the web database. These two types are the per-entry EntityMetadata
and the ModelTypeState, which is globalstate for the autofill model type. This
change adds a migration to add the tables and functions on AutofillTable to
read/write/delete it, with associated tests.

BUG=671832
R=shess@chromium.org,mathp@chromium.org,pavely@chromium.org

Committed: https://crrev.com/285a1dabd3a38350900c07648b91a74aeacb2089
Cr-Commit-Position: refs/heads/master@{#437722}

[Patrick Noland, 2016-12-05 22:02:16]

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

[commit-bot: I haz the power, 2016-12-05 22:03:05]

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, 2016-12-05 22:06:41]

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

[commit-bot: I haz the power, 2016-12-05 22:06:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Patrick Noland, 2016-12-06 00:50:22]

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

[commit-bot: I haz the power, 2016-12-06 00:50:56]

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, 2016-12-06 01:09:57]

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

[commit-bot: I haz the power, 2016-12-06 01:09:58]

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

[Patrick Noland, 2016-12-06 19:54:42]

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

[commit-bot: I haz the power, 2016-12-06 19:55:08]

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, 2016-12-06 22:10:17]

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

[commit-bot: I haz the power, 2016-12-06 22:10:18]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-12-06 23:37:31]

Description was changed from

==========
[sync] Migrate web database to store sync metadata for autofill

TODO fix description
==========

to

==========
[sync] Add autofill sync metadata to the web db

Two types of sync metadata, once stored in the sync directory, are now being
migrated to the web database. These two types are the per-entry EntityMetadata
and the ModelTypeState, which is globalstate for the autofill model type. This
change adds a migration to add the tables and functions on AutofillTable to
read/write/delete it, with associated tests.

BUG=671832

R=shess@chromium.org,mathp@chromium.org,pavely@chromium.org
==========

[Patrick Noland, 2016-12-06 23:37:31]

pnoland@chromium.org changed reviewers:
+ mathp@chromium.org, pavely@chromium.org, shess@chromium.org

[Patrick Noland, 2016-12-06 23:38:02]

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

[Patrick Noland, 2016-12-06 23:38:13]

Patchset #1 (id:1) has been deleted

[Patrick Noland, 2016-12-06 23:38:20]

Patchset #1 (id:20001) has been deleted

[Patrick Noland, 2016-12-06 23:38:25]

Patchset #1 (id:40001) has been deleted

[Patrick Noland, 2016-12-06 23:38:32]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-12-06 23:39:37]

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, 2016-12-06 23:50:03]

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

[commit-bot: I haz the power, 2016-12-06 23:50:04]

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...)

[Mathieu, 2016-12-07 16:17:42]

lgtm, thanks

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

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2461: 
nit: remove extra line to be consistent

[Patrick Noland, 2016-12-07 22:12:36]

pavel, PTAL
shess, PTAL at components/webdata/common/*

[Scott Hess - ex-Googler, 2016-12-07 23:15:20]

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

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2001: "value BLOB)"))
{
I'd drop a NOT NULL in there.  PRIMARY KEY does not imply NOT NULL in SQLite,
see https://www.sqlite.org/lang_createtable.html#constraints .

Also, PRIMARY KEY to be consistent with your other capitalization of SQL code.

Obviously same deal in the migrate code.

Rather than testing for existence then creating, instead consider CREATE TABLE
IF NOT EXISTS.  It succeeds if a table of that name already exists.  [Not
suggesting that for the migrate case, since the expected db state is different
there.]  [[Hmm, and I guess there's the argument from consistency with prior
code, so I don't consider that a blocker.]]

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2462: return
db_->Execute("CREATE TABLE autofill_model_type_state (value BLOB)");
This probably wants a transaction to keep these atomic.  Though I guess the
caller _must_ have a transaction so that bumping the version happens atomically
with the migration?  Well, in that case the transaction is cheap.

[Patrick Noland, 2016-12-08 20:27:03]

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

[commit-bot: I haz the power, 2016-12-08 20:27:40]

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

[maxbogue, 2016-12-08 21:14:05]

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

https://codereview.chromium.org/2550293002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.h:446:
sync_pb::ModelTypeState& model_type_state);
const

[commit-bot: I haz the power, 2016-12-08 22:12:06]

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

[commit-bot: I haz the power, 2016-12-08 22:12:08]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2016-12-08 22:51:14]

PTAL

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

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2001: "value BLOB)"))
{
On 2016/12/07 23:15:20, Scott Hess wrote:
> I'd drop a NOT NULL in there.  PRIMARY KEY does not imply NOT NULL in SQLite,
> see https://www.sqlite.org/lang_createtable.html#constraints .
> 
> Also, PRIMARY KEY to be consistent with your other capitalization of SQL code.
> 
> Obviously same deal in the migrate code.
> 
> Rather than testing for existence then creating, instead consider CREATE TABLE
> IF NOT EXISTS.  It succeeds if a table of that name already exists.  [Not
> suggesting that for the migrate case, since the expected db state is different
> there.]  [[Hmm, and I guess there's the argument from consistency with prior
> code, so I don't consider that a blocker.]]

I've added the NOT NULL constraint, but I'm leaving the if-else structure as is
to be consistent with other code.

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2461: 
On 2016/12/07 16:17:42, Mathieu Perreault wrote:
> nit: remove extra line to be consistent

Done.

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2462: return
db_->Execute("CREATE TABLE autofill_model_type_state (value BLOB)");
On 2016/12/07 23:15:20, Scott Hess wrote:
> This probably wants a transaction to keep these atomic.  Though I guess the
> caller _must_ have a transaction so that bumping the version happens
atomically
> with the migration?  Well, in that case the transaction is cheap.

Migration functions are called in the context of a transaction already:
https://cs.chromium.org/chromium/src/components/webdata/common/web_database.c...

[Scott Hess - ex-Googler, 2016-12-08 23:24:01]

OK, LGTM.

Hmm.  WRT the rowid usage, just so you know, unbound rowid is a SQLite-internal
concept.  For instance, rowid may not be stable across VACUUM, and there are no
guarantees.  In practice, it is probably safe.  Doing this:
   CREATE TABLE autofill_model_type_state (id INTEGER PRIMARY KEY, value BLOB)
would make id an alias for rowid, with no storage overhead, and if SQLite's
implementation of rowid changes, your id will remain stable.

Sorry I didn't think to mention that first time through.  If you make that
CREATE change (and the obvious s/rowid/id/ changes elsewhere), my LGTM holds. 
And if you decide to just go for it, I guess that's fine, too, though in that
case if anyone ever asks me about some problem they're having, I'll send them to
you even if you've moved on to a new project or company :-).

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

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2001: "value BLOB)"))
{
On 2016/12/08 22:51:13, Patrick Noland wrote:
> On 2016/12/07 23:15:20, Scott Hess wrote:
> > I'd drop a NOT NULL in there.  PRIMARY KEY does not imply NOT NULL in
SQLite,
> > see https://www.sqlite.org/lang_createtable.html#constraints .
> > 
> > Also, PRIMARY KEY to be consistent with your other capitalization of SQL
code.
> > 
> > Obviously same deal in the migrate code.
> > 
> > Rather than testing for existence then creating, instead consider CREATE
TABLE
> > IF NOT EXISTS.  It succeeds if a table of that name already exists.  [Not
> > suggesting that for the migrate case, since the expected db state is
different
> > there.]  [[Hmm, and I guess there's the argument from consistency with prior
> > code, so I don't consider that a blocker.]]
> 
> I've added the NOT NULL constraint, but I'm leaving the if-else structure as
is
> to be consistent with other code.

Acknowledged.

https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2462: return
db_->Execute("CREATE TABLE autofill_model_type_state (value BLOB)");
On 2016/12/08 22:51:13, Patrick Noland wrote:
> On 2016/12/07 23:15:20, Scott Hess wrote:
> > This probably wants a transaction to keep these atomic.  Though I guess the
> > caller _must_ have a transaction so that bumping the version happens
> atomically
> > with the migration?  Well, in that case the transaction is cheap.
> 
> Migration functions are called in the context of a transaction already:
>
https://cs.chromium.org/chromium/src/components/webdata/common/web_database.c...

Acknowledged.

[pavely, 2016-12-08 23:34:46]

lgtm % Scott's comments.

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

https://codereview.chromium.org/2550293002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.h:428:
syncer::EntityMetadataMap* metadata_records);
Since you are accumulating metadata records in EntityMetadataMap we'll need
additional method in MetadataBatch to take prepopulated EntityMetadataMap in
addition to setting metadata record by record. This can be done in separate CL.

[maxbogue, 2016-12-09 00:18:13]

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

https://codereview.chromium.org/2550293002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1691: bool
AutofillTable::GetAllSyncMetadata(
As we discussed offline, please combine this and GetModelTypeState into one
function that fills a MetadataBatch*.

[Patrick Noland, 2016-12-09 21:51:02]

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

[commit-bot: I haz the power, 2016-12-09 21:51:56]

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

[Patrick Noland, 2016-12-09 22:04:00]

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

[commit-bot: I haz the power, 2016-12-09 22:04:22]

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

[Patrick Noland, 2016-12-09 22:04:56]

Patchset #3 (id:120001) has been deleted

[Patrick Noland, 2016-12-09 22:05:00]

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

[commit-bot: I haz the power, 2016-12-09 22:05:55]

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

[Patrick Noland, 2016-12-10 00:37:59]

The CQ bit was unchecked by pnoland@chromium.org

[Patrick Noland, 2016-12-10 00:39:18]

On 2016/12/08 23:24:01, Scott Hess wrote:
> OK, LGTM.
> 
> Hmm.  WRT the rowid usage, just so you know, unbound rowid is a
SQLite-internal
> concept.  For instance, rowid may not be stable across VACUUM, and there are
no
> guarantees.  In practice, it is probably safe.  Doing this:
>    CREATE TABLE autofill_model_type_state (id INTEGER PRIMARY KEY, value BLOB)
> would make id an alias for rowid, with no storage overhead, and if SQLite's
> implementation of rowid changes, your id will remain stable.
> 
> Sorry I didn't think to mention that first time through.  If you make that
> CREATE change (and the obvious s/rowid/id/ changes elsewhere), my LGTM holds. 
> And if you decide to just go for it, I guess that's fine, too, though in that
> case if anyone ever asks me about some problem they're having, I'll send them
to
> you even if you've moved on to a new project or company :-).
> 
>
https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
> File components/autofill/core/browser/webdata/autofill_table.cc (right):
> 
>
https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
> components/autofill/core/browser/webdata/autofill_table.cc:2001: "value
BLOB)"))
> {
> On 2016/12/08 22:51:13, Patrick Noland wrote:
> > On 2016/12/07 23:15:20, Scott Hess wrote:
> > > I'd drop a NOT NULL in there.  PRIMARY KEY does not imply NOT NULL in
> SQLite,
> > > see https://www.sqlite.org/lang_createtable.html#constraints .
> > > 
> > > Also, PRIMARY KEY to be consistent with your other capitalization of SQL
> code.
> > > 
> > > Obviously same deal in the migrate code.
> > > 
> > > Rather than testing for existence then creating, instead consider CREATE
> TABLE
> > > IF NOT EXISTS.  It succeeds if a table of that name already exists.  [Not
> > > suggesting that for the migrate case, since the expected db state is
> different
> > > there.]  [[Hmm, and I guess there's the argument from consistency with
prior
> > > code, so I don't consider that a blocker.]]
> > 
> > I've added the NOT NULL constraint, but I'm leaving the if-else structure as
> is
> > to be consistent with other code.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2550293002/diff/80001/components/autofill/cor...
> components/autofill/core/browser/webdata/autofill_table.cc:2462: return
> db_->Execute("CREATE TABLE autofill_model_type_state (value BLOB)");
> On 2016/12/08 22:51:13, Patrick Noland wrote:
> > On 2016/12/07 23:15:20, Scott Hess wrote:
> > > This probably wants a transaction to keep these atomic.  Though I guess
the
> > > caller _must_ have a transaction so that bumping the version happens
> > atomically
> > > with the migration?  Well, in that case the transaction is cheap.
> > 
> > Migration functions are called in the context of a transaction already:
> >
>
https://cs.chromium.org/chromium/src/components/webdata/common/web_database.c...
> 
> Acknowledged.

I've switched to using an explicit id column instead of rowid.

[Patrick Noland, 2016-12-10 00:39:35]

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

https://codereview.chromium.org/2550293002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.cc:1691: bool
AutofillTable::GetAllSyncMetadata(
On 2016/12/09 00:18:13, maxbogue wrote:
> As we discussed offline, please combine this and GetModelTypeState into one
> function that fills a MetadataBatch*.

Done.

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

https://codereview.chromium.org/2550293002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.h:428:
syncer::EntityMetadataMap* metadata_records);
On 2016/12/08 23:34:45, pavely wrote:
> Since you are accumulating metadata records in EntityMetadataMap we'll need
> additional method in MetadataBatch to take prepopulated EntityMetadataMap in
> addition to setting metadata record by record. This can be done in separate
CL.

I've added a todo for this.

https://codereview.chromium.org/2550293002/diff/100001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table.h:446:
sync_pb::ModelTypeState& model_type_state);
On 2016/12/08 21:14:05, maxbogue wrote:
> const

Done.

[Patrick Noland, 2016-12-10 00:39:42]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2016-12-10 00:39:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
pavely@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/2550293002/#ps140001
(title: "Return a metadata batch; use id instead of rowid")

[commit-bot: I haz the power, 2016-12-10 00:40:28]

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, 2016-12-10 02:05:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481330382234050,
"parent_rev": "eac91ecda483b6f07b3bb2a46e824466efabace9", "commit_rev":
"48ba79ddd54a34835c2151d578beb58b9c053275"}

[commit-bot: I haz the power, 2016-12-10 02:05:44]

Description was changed from

==========
[sync] Add autofill sync metadata to the web db

Two types of sync metadata, once stored in the sync directory, are now being
migrated to the web database. These two types are the per-entry EntityMetadata
and the ModelTypeState, which is globalstate for the autofill model type. This
change adds a migration to add the tables and functions on AutofillTable to
read/write/delete it, with associated tests.

BUG=671832

R=shess@chromium.org,mathp@chromium.org,pavely@chromium.org
==========

to

==========
[sync] Add autofill sync metadata to the web db

Two types of sync metadata, once stored in the sync directory, are now being
migrated to the web database. These two types are the per-entry EntityMetadata
and the ModelTypeState, which is globalstate for the autofill model type. This
change adds a migration to add the tables and functions on AutofillTable to
read/write/delete it, with associated tests.

BUG=671832

R=shess@chromium.org,mathp@chromium.org,pavely@chromium.org

Review-Url: https://codereview.chromium.org/2550293002
==========

[commit-bot: I haz the power, 2016-12-10 02:05:45]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-12-12 15:05:18]

Description was changed from

==========
[sync] Add autofill sync metadata to the web db

Two types of sync metadata, once stored in the sync directory, are now being
migrated to the web database. These two types are the per-entry EntityMetadata
and the ModelTypeState, which is globalstate for the autofill model type. This
change adds a migration to add the tables and functions on AutofillTable to
read/write/delete it, with associated tests.

BUG=671832

R=shess@chromium.org,mathp@chromium.org,pavely@chromium.org

Review-Url: https://codereview.chromium.org/2550293002
==========

to

==========
[sync] Add autofill sync metadata to the web db

Two types of sync metadata, once stored in the sync directory, are now being
migrated to the web database. These two types are the per-entry EntityMetadata
and the ModelTypeState, which is globalstate for the autofill model type. This
change adds a migration to add the tables and functions on AutofillTable to
read/write/delete it, with associated tests.

BUG=671832

R=shess@chromium.org,mathp@chromium.org,pavely@chromium.org

Committed: https://crrev.com/285a1dabd3a38350900c07648b91a74aeacb2089
Cr-Commit-Position: refs/heads/master@{#437722}
==========

[commit-bot: I haz the power, 2016-12-12 15:05:19]

Patchset 3 (id:??) landed as
https://crrev.com/285a1dabd3a38350900c07648b91a74aeacb2089
Cr-Commit-Position: refs/heads/master@{#437722}