predictors: Version the resource_prefetch_predictor database.

predictors: Version the resource_prefetch_predictor database.

BUG=631966
Committed: https://crrev.com/83c73f2c78bbdec7b5e335b04c8f1aba2a0a07e5
Cr-Commit-Position: refs/heads/master@{#414999}

[Benoit L, 2016-08-26 09:08:36]

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

[commit-bot: I haz the power, 2016-08-26 09:08:53]

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

[Benoit L, 2016-08-26 09:22:48]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[commit-bot: I haz the power, 2016-08-26 09:55:54]

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

[commit-bot: I haz the power, 2016-08-26 09:55:56]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-08-26 14:37:19]

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right):

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:58: // schema
(including the .proto).
is this mandatory? We might be OK with default backfill for optional fields ..
sometimes, right?

For the comment to be useful, it would be better to keep this description closer
to the protobuf definition. How about enum Version { CURRENT_VERSION = 1; } in
the .proto itself?

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:517:
(db->DoesTableExist(kUrlResourceTableName) ||
what if we decided to drop the table because its version is old, but the table
still exists because we did not commit the transaction? I assume this check
would return true and the table is not created.

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:538: }
nit: I think chromium style suggests no braces in this case. I am not a fan of
this style, but prefer consistency

[Benoit L, 2016-08-26 14:52:39]

Thanks!

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right):

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:58: // schema
(including the .proto).
On 2016/08/26 14:37:18, pasko wrote:
> is this mandatory? We might be OK with default backfill for optional fields ..
> sometimes, right?
> 

I prefer to err on the side of caution.

> For the comment to be useful, it would be better to keep this description
closer
> to the protobuf definition. How about enum Version { CURRENT_VERSION = 1; } in
> the .proto itself?

What about adding a comment in the proto?

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:517:
(db->DoesTableExist(kUrlResourceTableName) ||
On 2016/08/26 14:37:18, pasko wrote:
> what if we decided to drop the table because its version is old, but the table
> still exists because we did not commit the transaction? I assume this check
> would return true and the table is not created.

We are within the transaction, so it's fine.

https://codereview.chromium.org/2285673002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:538: }
On 2016/08/26 14:37:18, pasko wrote:
> nit: I think chromium style suggests no braces in this case. I am not a fan of
> this style, but prefer consistency

Done.

[pasko, 2016-08-26 15:06:44]

https://codereview.chromium.org/2285673002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.proto (right):

https://codereview.chromium.org/2285673002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.proto:7: //         
resource_prefetch_predictor.cc.
I like it less because in case the file name changes or the constant moves
somewhere, this will become inconsistent.

I guess the advantage is less bloat in the protobuf? Or was it just hard to
access the proto enum value in resource_prefetch_predictor.cc?

[pasko, 2016-08-26 15:22:42]

Offline agreed that the database version belongs more to the .cc file because it
can be updated without touching the proto.

Please add a test that would:
1. check that after InitializeSampleData() the new table is created and the
value there is what we expect
2. check that mutating the version after InitializeSampleData() causes empty
GetAllData()

[Benoit L, 2016-08-26 16:37:02]

Thanks, all done.

https://codereview.chromium.org/2285673002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.proto (right):

https://codereview.chromium.org/2285673002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.proto:7: //         
resource_prefetch_predictor.cc.
On 2016/08/26 15:06:44, pasko wrote:
> I like it less because in case the file name changes or the constant moves
> somewhere, this will become inconsistent.
> 
> I guess the advantage is less bloat in the protobuf? Or was it just hard to
> access the proto enum value in resource_prefetch_predictor.cc?

Acknowledged.

[Benoit L, 2016-08-26 16:37:07]

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

[commit-bot: I haz the power, 2016-08-26 16:37:55]

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-08-26 17:21:53]

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

[commit-bot: I haz the power, 2016-08-26 17:21:54]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-08-26 17:52:19]

cool, thank you, lgtm

https://codereview.chromium.org/2285673002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc
(right):

https://codereview.chromium.org/2285673002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:43:
typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
nit: using ResourcePrefetchPredictorTables::PrefetchDataMap

[Benoit L, 2016-08-29 08:09:57]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-08-29 08:09:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2285673002/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-08-29 08:10:17]

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

[Benoit L, 2016-08-29 08:57:16]

https://codereview.chromium.org/2285673002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc
(right):

https://codereview.chromium.org/2285673002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:43:
typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
On 2016/08/26 17:52:18, pasko wrote:
> nit: using ResourcePrefetchPredictorTables::PrefetchDataMap

Done.

[commit-bot: I haz the power, 2016-08-29 09:08:31]

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

[commit-bot: I haz the power, 2016-08-29 09:08:32]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Benoit L, 2016-08-29 09:12:49]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-08-29 09:13:11]

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-08-29 10:11:56]

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

[commit-bot: I haz the power, 2016-08-29 10:11:57]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Benoit L, 2016-08-29 11:31:27]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-08-29 11:31:42]

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-08-29 12:28:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-29 12:30:23]

Description was changed from

==========
predictors: Version the resource_prefetch_predictor database.

BUG=631966
==========

to

==========
predictors: Version the resource_prefetch_predictor database.

BUG=631966

Committed: https://crrev.com/83c73f2c78bbdec7b5e335b04c8f1aba2a0a07e5
Cr-Commit-Position: refs/heads/master@{#414999}
==========

[commit-bot: I haz the power, 2016-08-29 12:30:24]

Patchset 4 (id:??) landed as
https://crrev.com/83c73f2c78bbdec7b5e335b04c8f1aba2a0a07e5
Cr-Commit-Position: refs/heads/master@{#414999}