Raze old WebDatabases.

Raze old WebDatabases.

BUG=382562
Committed: https://crrev.com/4299f239e7cced41cfb6f3ac2f8464957197822d
Cr-Commit-Position: refs/heads/master@{#318713}

[Evan Stade, 2015-02-26 22:06:05]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org, shess@chromium.org

[Peter Kasting, 2015-02-26 22:33:19]

LGTM

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:93:
DCHECK_LT(kDeprecatedVersionNumber, kCurrentVersionNumber);
I don't know that this DCHECK is really needed.

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:159: //
version_nn.sql file.
So why not do that?  There aren't so many of these .sql files anymore.

[Evan Stade, 2015-02-26 22:37:19]

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:93:
DCHECK_LT(kDeprecatedVersionNumber, kCurrentVersionNumber);
On 2015/02/26 22:33:19, Peter Kasting wrote:
> I don't know that this DCHECK is really needed.

This is copied from TopSitesDatabase. I am ambivalent about its necessity
overall but I would like to be consistent.

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:159: //
version_nn.sql file.
On 2015/02/26 22:33:19, Peter Kasting wrote:
> So why not do that?  There aren't so many of these .sql files anymore.

That's somewhat orthogonal. This wasn't being tested before either
(kFirstVersion == 53, kDeprecatedVersion + 1 == 52). This comment just explains
why.

[Peter Kasting, 2015-02-26 22:40:08]

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:159: //
version_nn.sql file.
On 2015/02/26 22:37:19, Evan Stade wrote:
> On 2015/02/26 22:33:19, Peter Kasting wrote:
> > So why not do that?  There aren't so many of these .sql files anymore.
> 
> That's somewhat orthogonal. This wasn't being tested before either
> (kFirstVersion == 53, kDeprecatedVersion + 1 == 52). This comment just
explains
> why.

OK... seems like something that would be nice to fix separately, though.

[Scott Hess - ex-Googler, 2015-02-27 06:49:38]

Would there be any disadvantage to keeping the old golden files and just
verifying that they all end up empty?  Mostly asking because my theory on such
things is that if they represent potentially-valid schema in the fleet, than
it's reasonable to test that they end up in a usable state.

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
components/search_engines/keyword_table.cc:281: "VARCHAR");
Is there a strong argument for changing this?  It means that users who traversed
this point before will have DEFAULT '' while users traversing it in the future
will have no DEFAULT.  It seems somewhat likely that the majority of users will
have the old schema style, so I'd just stick with that.

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:93:
DCHECK_LT(kDeprecatedVersionNumber, kCurrentVersionNumber);
On 2015/02/26 22:37:19, Evan Stade wrote:
> On 2015/02/26 22:33:19, Peter Kasting wrote:
> > I don't know that this DCHECK is really needed.
> 
> This is copied from TopSitesDatabase. I am ambivalent about its necessity
> overall but I would like to be consistent.

The rationale was that if someone accidentally got it wrong, all of the
databases would be reset with no way to recover.  I don't think there was any
reason not to use a compile-time assertion, though.

Of course given appropriate test coverage this is completely redundant.  I wish
there were a way to assert "You can only use this if you're testing this edge".

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:145: if (current_version <
kDeprecatedVersionNumber) {
This case should not be possible anymore, right?

[Peter Kasting, 2015-02-27 09:15:25]

On 2015/02/27 06:49:38, Scott Hess wrote:
> Would there be any disadvantage to keeping the old golden files and just
> verifying that they all end up empty?  Mostly asking because my theory on such
> things is that if they represent potentially-valid schema in the fleet, than
> it's reasonable to test that they end up in a usable state.

Maybe it's worth keeping one?  Keeping them all seems unnecessary...

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
components/search_engines/keyword_table.cc:281: "VARCHAR");
On 2015/02/27 06:49:38, Scott Hess wrote:
> Is there a strong argument for changing this?  It means that users who
traversed
> this point before will have DEFAULT '' while users traversing it in the future
> will have no DEFAULT.  It seems somewhat likely that the majority of users
will
> have the old schema style, so I'd just stick with that.

I had assumed that the two were equivalent.  If they're not, we should leave the
DEFAULT.

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:93:
DCHECK_LT(kDeprecatedVersionNumber, kCurrentVersionNumber);
On 2015/02/27 06:49:38, Scott Hess wrote:
> On 2015/02/26 22:37:19, Evan Stade wrote:
> > On 2015/02/26 22:33:19, Peter Kasting wrote:
> > > I don't know that this DCHECK is really needed.
> > 
> > This is copied from TopSitesDatabase. I am ambivalent about its necessity
> > overall but I would like to be consistent.
> 
> The rationale was that if someone accidentally got it wrong, all of the
> databases would be reset with no way to recover.  I don't think there was any
> reason not to use a compile-time assertion, though.

I didn't even think of that.  Yes, please use a static_assert if keeping this,
that's safer.

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:145: if (current_version <
kDeprecatedVersionNumber) {
On 2015/02/27 06:49:38, Scott Hess wrote:
> This case should not be possible anymore, right?

I think not; even if the user manually copies old DB files in after we raze
them, we'll just raze them again.  So this could probably become a DCHECK and
omit the rest of the block here.

[Scott Hess - ex-Googler, 2015-02-27 17:00:21]

On 2015/02/27 09:15:25, Peter Kasting wrote:
> On 2015/02/27 06:49:38, Scott Hess wrote:
> > Would there be any disadvantage to keeping the old golden files and just
> > verifying that they all end up empty?  Mostly asking because my theory on
such
> > things is that if they represent potentially-valid schema in the fleet, than
> > it's reasonable to test that they end up in a usable state.
> 
> Maybe it's worth keeping one?  Keeping them all seems unnecessary...

Just to be clear, my thinking here is that you can either model the fleet and
try to pick the specific case which reliably covers the possibilities the fleet
will see.  Or you can just punt and keep all the cases.  The latter is certainly
an easier option, and it's not particularly expensive (these tests should run in
a few milliseconds).

It would certainly make sense to shift them into a deprecated/ subdir, though.

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
components/search_engines/keyword_table.cc:281: "VARCHAR");
On 2015/02/27 09:15:25, Peter Kasting wrote:
> On 2015/02/27 06:49:38, Scott Hess wrote:
> > Is there a strong argument for changing this?  It means that users who
> traversed
> > this point before will have DEFAULT '' while users traversing it in the
future
> > will have no DEFAULT.  It seems somewhat likely that the majority of users
> will
> > have the old schema style, so I'd just stick with that.
> 
> I had assumed that the two were equivalent.  If they're not, we should leave
the
> DEFAULT.

They are probably functionally equivalent, but they end up baked into the
schema, so once they're in there, they stay there in the fleet.  In the long
term developers really can only make future decisions based on what they see in
the current code, so it's generally preferrable to just leave mistakes in place
and fix the worst with a schema upgrade.

[Evan Stade, 2015-02-27 19:31:17]

> Just to be clear, my thinking here is that you can either model the fleet and
> try to pick the specific case which reliably covers the possibilities the
fleet
> will see.  Or you can just punt and keep all the cases.  The latter is
certainly
> an easier option, and it's not particularly expensive (these tests should run
in
> a few milliseconds).

I chose to keep just one (version 50). I don't see much benefit to keeping
everything in that it doesn't increase the code coverage of the tests.

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
components/search_engines/keyword_table.cc:281: "VARCHAR");
On 2015/02/27 17:00:21, Scott Hess wrote:
> On 2015/02/27 09:15:25, Peter Kasting wrote:
> > On 2015/02/27 06:49:38, Scott Hess wrote:
> > > Is there a strong argument for changing this?  It means that users who
> > traversed
> > > this point before will have DEFAULT '' while users traversing it in the
> future
> > > will have no DEFAULT.  It seems somewhat likely that the majority of users
> > will
> > > have the old schema style, so I'd just stick with that.
> > 
> > I had assumed that the two were equivalent.  If they're not, we should leave
> the
> > DEFAULT.
> 
> They are probably functionally equivalent, but they end up baked into the
> schema, so once they're in there, they stay there in the fleet.  In the long
> term developers really can only make future decisions based on what they see
in
> the current code, so it's generally preferrable to just leave mistakes in
place
> and fix the worst with a schema upgrade.

oops, I meant to leave this alone. This was part of my attempt to fix
crbug.com/462394 which I ultimately punted to a later date.

[Evan Stade, 2015-02-28 00:13:41]

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
File components/search_engines/keyword_table.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/search_engines/ke...
components/search_engines/keyword_table.cc:281: "VARCHAR");
On 2015/02/27 19:31:17, Evan Stade wrote:
> On 2015/02/27 17:00:21, Scott Hess wrote:
> > On 2015/02/27 09:15:25, Peter Kasting wrote:
> > > On 2015/02/27 06:49:38, Scott Hess wrote:
> > > > Is there a strong argument for changing this?  It means that users who
> > > traversed
> > > > this point before will have DEFAULT '' while users traversing it in the
> > future
> > > > will have no DEFAULT.  It seems somewhat likely that the majority of
users
> > > will
> > > > have the old schema style, so I'd just stick with that.
> > > 
> > > I had assumed that the two were equivalent.  If they're not, we should
leave
> > the
> > > DEFAULT.
> > 
> > They are probably functionally equivalent, but they end up baked into the
> > schema, so once they're in there, they stay there in the fleet.  In the long
> > term developers really can only make future decisions based on what they see
> in
> > the current code, so it's generally preferrable to just leave mistakes in
> place
> > and fix the worst with a schema upgrade.
> 
> oops, I meant to leave this alone. This was part of my attempt to fix
> crbug.com/462394 which I ultimately punted to a later date.

reverted

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:93:
DCHECK_LT(kDeprecatedVersionNumber, kCurrentVersionNumber);
On 2015/02/27 09:15:25, Peter Kasting wrote:
> On 2015/02/27 06:49:38, Scott Hess wrote:
> > On 2015/02/26 22:37:19, Evan Stade wrote:
> > > On 2015/02/26 22:33:19, Peter Kasting wrote:
> > > > I don't know that this DCHECK is really needed.
> > > 
> > > This is copied from TopSitesDatabase. I am ambivalent about its necessity
> > > overall but I would like to be consistent.
> > 
> > The rationale was that if someone accidentally got it wrong, all of the
> > databases would be reset with no way to recover.  I don't think there was
any
> > reason not to use a compile-time assertion, though.
> 
> I didn't even think of that.  Yes, please use a static_assert if keeping this,
> that's safer.

Done.

https://codereview.chromium.org/962583003/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:145: if (current_version <
kDeprecatedVersionNumber) {
On 2015/02/27 09:15:25, Peter Kasting wrote:
> On 2015/02/27 06:49:38, Scott Hess wrote:
> > This case should not be possible anymore, right?
> 
> I think not; even if the user manually copies old DB files in after we raze
> them, we'll just raze them again.  So this could probably become a DCHECK and
> omit the rest of the block here.

Done.

[Scott Hess - ex-Googler, 2015-02-28 00:19:32]

lgtm w/minor changes (which I think will probably show up on trybots anyhow).

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database.cc:146: DCHECK_LT(current_version,
kDeprecatedVersionNumber);
Probably meant DCHECK_GT(), or swap the parameters.

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

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:210:
TEST_F(WebDatabaseMigrationTest, MigrateVersion45CompatibleToCurrent) {
Shouldn't this one be deleted also?

[Peter Kasting, 2015-02-28 00:26:21]

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database.cc:146: DCHECK_LT(current_version,
kDeprecatedVersionNumber);
On 2015/02/28 00:19:32, Scott Hess wrote:
> Probably meant DCHECK_GT(), or swap the parameters.

(Use the GT route rather than the param swap route)

[Evan Stade, 2015-02-28 00:29:08]

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database.cc:146: DCHECK_LT(current_version,
kDeprecatedVersionNumber);
On 2015/02/28 00:19:32, Scott Hess wrote:
> Probably meant DCHECK_GT(), or swap the parameters.

Done.

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

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:210:
TEST_F(WebDatabaseMigrationTest, MigrateVersion45CompatibleToCurrent) {
On 2015/02/28 00:19:32, Scott Hess wrote:
> Shouldn't this one be deleted also?

I kept this one because it seems to test "compatible version" functionality
rather than anything specific about version 40 or 45.

[Scott Hess - ex-Googler, 2015-02-28 00:37:04]

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

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:210:
TEST_F(WebDatabaseMigrationTest, MigrateVersion45CompatibleToCurrent) {
On 2015/02/28 00:29:08, Evan Stade wrote:
> On 2015/02/28 00:19:32, Scott Hess wrote:
> > Shouldn't this one be deleted also?
> 
> I kept this one because it seems to test "compatible version" functionality
> rather than anything specific about version 40 or 45.

AFAICT it will succeed because when DoMigration() calls db.Init(), the entire
database will be blown away and re-created.  This will happen before any of the
compatible-version migration code has a chance to fire.  So it will just do the
same thing as the version-50 migration code.

I may be grossly misunderstanding how things work.

[Evan Stade, 2015-02-28 00:39:24]

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

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:210:
TEST_F(WebDatabaseMigrationTest, MigrateVersion45CompatibleToCurrent) {
On 2015/02/28 00:37:04, Scott Hess wrote:
> On 2015/02/28 00:29:08, Evan Stade wrote:
> > On 2015/02/28 00:19:32, Scott Hess wrote:
> > > Shouldn't this one be deleted also?
> > 
> > I kept this one because it seems to test "compatible version" functionality
> > rather than anything specific about version 40 or 45.
> 
> AFAICT it will succeed because when DoMigration() calls db.Init(), the entire
> database will be blown away and re-created.  This will happen before any of
the
> compatible-version migration code has a chance to fire.  So it will just do
the
> same thing as the version-50 migration code.
> 
> I may be grossly misunderstanding how things work.

It's testing what happens /before/ migration. The key line of the test is:

ASSERT_TRUE(meta_table.Init(&connection, 40, 45));

For other tests it's always along the lines of:

ASSERT_TRUE(meta_table.Init(&connection, 60, 60));

[Scott Hess - ex-Googler, 2015-02-28 00:58:15]

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

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:210:
TEST_F(WebDatabaseMigrationTest, MigrateVersion45CompatibleToCurrent) {
On 2015/02/28 00:39:24, Evan Stade wrote:
> On 2015/02/28 00:37:04, Scott Hess wrote:
> > On 2015/02/28 00:29:08, Evan Stade wrote:
> > > On 2015/02/28 00:19:32, Scott Hess wrote:
> > > > Shouldn't this one be deleted also?
> > > 
> > > I kept this one because it seems to test "compatible version"
functionality
> > > rather than anything specific about version 40 or 45.
> > 
> > AFAICT it will succeed because when DoMigration() calls db.Init(), the
entire
> > database will be blown away and re-created.  This will happen before any of
> the
> > compatible-version migration code has a chance to fire.  So it will just do
> the
> > same thing as the version-50 migration code.
> > 
> > I may be grossly misunderstanding how things work.
> 
> It's testing what happens /before/ migration. The key line of the test is:
> 
> ASSERT_TRUE(meta_table.Init(&connection, 40, 45));
> 
> For other tests it's always along the lines of:
> 
> ASSERT_TRUE(meta_table.Init(&connection, 60, 60));

I still don't follow.  The values passed in to MetaTable::Init() are going to be
ignored (because the golden file already setup the table), you could pass
anything you want to those values.

[Evan Stade, 2015-02-28 01:03:57]

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

https://codereview.chromium.org/962583003/diff/20001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:210:
TEST_F(WebDatabaseMigrationTest, MigrateVersion45CompatibleToCurrent) {
On 2015/02/28 00:58:15, Scott Hess wrote:
> On 2015/02/28 00:39:24, Evan Stade wrote:
> > On 2015/02/28 00:37:04, Scott Hess wrote:
> > > On 2015/02/28 00:29:08, Evan Stade wrote:
> > > > On 2015/02/28 00:19:32, Scott Hess wrote:
> > > > > Shouldn't this one be deleted also?
> > > > 
> > > > I kept this one because it seems to test "compatible version"
> functionality
> > > > rather than anything specific about version 40 or 45.
> > > 
> > > AFAICT it will succeed because when DoMigration() calls db.Init(), the
> entire
> > > database will be blown away and re-created.  This will happen before any
of
> > the
> > > compatible-version migration code has a chance to fire.  So it will just
do
> > the
> > > same thing as the version-50 migration code.
> > > 
> > > I may be grossly misunderstanding how things work.
> > 
> > It's testing what happens /before/ migration. The key line of the test is:
> > 
> > ASSERT_TRUE(meta_table.Init(&connection, 40, 45));
> > 
> > For other tests it's always along the lines of:
> > 
> > ASSERT_TRUE(meta_table.Init(&connection, 60, 60));
> 
> I still don't follow.  The values passed in to MetaTable::Init() are going to
be
> ignored (because the golden file already setup the table), you could pass
> anything you want to those values.

you got me. I only read this test and its comments and not the functions it
calls into. Removed.

[Evan Stade, 2015-03-02 16:03:52]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-03-02 16:03:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/962583003/#ps80001 (title:
"delete test")

[commit-bot: I haz the power, 2015-03-02 16:04:58]

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

[commit-bot: I haz the power, 2015-03-02 17:12:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-02 17:13:42]

Patchset 5 (id:??) landed as
https://crrev.com/4299f239e7cced41cfb6f3ac2f8464957197822d
Cr-Commit-Position: refs/heads/master@{#318713}