predictors: Pass manifests from Bork to store in ResourcePrefetchPredictor.

predictors: Pass manifests from Bork to store in ResourcePrefetchPredictor.

This CL makes ResourcePrefetchPredictor accessible from PrecacheManager to
notify about new manifests through PrecacheManifestDelegate interface.
These manifests are saved then in a dedicated table on a
ResourcePrefetchPredictor side.

BUG=699115
Review-Url: https://codereview.chromium.org/2762673002
Cr-Commit-Position: refs/heads/master@{#458735}
Committed: https://chromium.googlesource.com/chromium/src/+/b0b2d6ffa58b392bd30b49465ba29590c1616fe5

[alexilin, 2017-03-20 15:23:49]

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

[alexilin, 2017-03-20 15:23:49]

Hi Benoit,

Sorry for the size of review, there are many small changes in plenty of files.

Basically, I pass manifest though several layers.
PrecacheFetcher (1) -> PrecacheManager (2) -> ResourcePrefetchPredictor
Transition (1) goes through already existed PrecacheFetcher::PrecacheDelegate
interface that is implemented by PrecacheManager.
Transition (2) goes through newly created PrecacheManifestDelegate that is
implemented by ResourcePrefetchPredictor and injected by
precache_manager_factory that has access to chrome/browser/* classes.

I'm still missing some tests for precache_manager and precache_fetcher but I
think this CL is already reviewable.

[alexilin, 2017-03-20 15:26:36]

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

[commit-bot: I haz the power, 2017-03-20 15:26:49]

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, 2017-03-20 15:29:48]

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

[commit-bot: I haz the power, 2017-03-20 15:29:49]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[alexilin, 2017-03-20 15:46:30]

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

[commit-bot: I haz the power, 2017-03-20 15:46:47]

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, 2017-03-20 16:36:55]

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

[commit-bot: I haz the power, 2017-03-20 16:36:56]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-03-20 16:51:54]

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/precache...
File chrome/browser/precache/precache_manager_factory.cc (right):

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/precache...
chrome/browser/precache/precache_manager_factory.cc:66:
predictors::ResourcePrefetchPredictorFactory::GetForProfile(
Does this cause issues if the prefetcher is disabled?

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:108: static
constexpr size_t kMaxManifestByteSize = 16384;  // 16 KB
tiny nit: 16 * (1 << 10)?
Then you can drop the comment.

Or drop the comment altogether.

https://codereview.chromium.org/2762673002/diff/20001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2762673002/diff/20001/components/precache/con...
components/precache/content/precache_manager.h:228: // notify browser about new
manifest available. May be null.
nit: notify the browser about a new available manifest.

https://codereview.chromium.org/2762673002/diff/20001/components/precache/cor...
File components/precache/core/precache_manifest_delegate.h (right):

https://codereview.chromium.org/2762673002/diff/20001/components/precache/cor...
components/precache/core/precache_manifest_delegate.h:15: class
PrecacheManifestDelegate {
Seems to me that this is conceptually more a PrecacheManagerDelegate.

In this case, any reason for not declaring it in precache_manager.h? Just
curious.

[alexilin, 2017-03-21 09:50:35]

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/precache...
File chrome/browser/precache/precache_manager_factory.cc (right):

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/precache...
chrome/browser/precache/precache_manager_factory.cc:66:
predictors::ResourcePrefetchPredictorFactory::GetForProfile(
On 2017/03/20 16:51:53, Benoit L wrote:
> Does this cause issues if the prefetcher is disabled?

Then GetForProfile returns nullptr and this is fine. SyncService and
HistoryService also may be null.

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2762673002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:108: static
constexpr size_t kMaxManifestByteSize = 16384;  // 16 KB
On 2017/03/20 16:51:53, Benoit L wrote:
> tiny nit: 16 * (1 << 10)?
> Then you can drop the comment.
> 
> Or drop the comment altogether.

Done.

https://codereview.chromium.org/2762673002/diff/20001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2762673002/diff/20001/components/precache/con...
components/precache/content/precache_manager.h:228: // notify browser about new
manifest available. May be null.
On 2017/03/20 16:51:53, Benoit L wrote:
> nit: notify the browser about a new available manifest.

Done.

https://codereview.chromium.org/2762673002/diff/20001/components/precache/cor...
File components/precache/core/precache_manifest_delegate.h (right):

https://codereview.chromium.org/2762673002/diff/20001/components/precache/cor...
components/precache/core/precache_manifest_delegate.h:15: class
PrecacheManifestDelegate {
On 2017/03/20 16:51:53, Benoit L wrote:
> Seems to me that this is conceptually more a PrecacheManagerDelegate.
> 
> In this case, any reason for not declaring it in precache_manager.h? Just
> curious.

Done.

[Benoit L, 2017-03-21 13:24:15]

Thanks!

Only small comments and a question: how do we deal with the stale manifests in
the DB?

Say for instance that precache gets disabled for some reason, or doesn't run for
a while. Then we have manifests in the DB that are stale, but I don't see where
we take care of that.

Is it an issue?

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_manager_factory.cc (right):

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_manager_factory.cc:66:
predictors::ResourcePrefetchPredictorFactory::GetForProfile(
here (see below) :-)

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

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1099: return
lhs.second.id().id() < rhs.second.id().id();
This means that there is now a constraint that the ID should always be
increasing. Is this checked anywhere?

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:108: static
constexpr size_t kMaxManifestByteSize = 16 * 1024;
nit: Can this be in the implementation file? Up to you.

https://codereview.chromium.org/2762673002/diff/40001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2762673002/diff/40001/components/precache/con...
components/precache/content/precache_manager.h:235: Delegate* delegate_;
I understand why this is fine here, as the service resolution mechanism enforces
a longer lifetime for the delegate than this service, but that's quite subtle.

Can you put a comment at the location where you provide the delegate, to make it
clear that's safe?

[alexilin, 2017-03-21 15:32:13]

Well, we don't clean other predictor tables, now it works like LRU cache. If we
want to add the cleaning procedure, it'd make sense to do it for all tables.
Meanwhile we could just don't use stale manifests when we're merging.

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/precache...
File chrome/browser/precache/precache_manager_factory.cc (right):

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/precache...
chrome/browser/precache/precache_manager_factory.cc:66:
predictors::ResourcePrefetchPredictorFactory::GetForProfile(
On 2017/03/21 13:24:15, Benoit L wrote:
> here (see below) :-)

Replied (see below) (-:

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

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1099: return
lhs.second.id().id() < rhs.second.id().id();
On 2017/03/21 13:24:15, Benoit L wrote:
> This means that there is now a constraint that the ID should always be
> increasing. Is this checked anywhere?

ID = creation time. So yes, it's always increasing.

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2762673002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:108: static
constexpr size_t kMaxManifestByteSize = 16 * 1024;
On 2017/03/21 13:24:15, Benoit L wrote:
> nit: Can this be in the implementation file? Up to you.

I tried to keep it consistent with kMaxStringLength, but it seems that latter
also could be moved in the implementation file and this will have more sense.
Done.

https://codereview.chromium.org/2762673002/diff/40001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2762673002/diff/40001/components/precache/con...
components/precache/content/precache_manager.h:235: Delegate* delegate_;
On 2017/03/21 13:24:15, Benoit L wrote:
> I understand why this is fine here, as the service resolution mechanism
enforces
> a longer lifetime for the delegate than this service, but that's quite subtle.
> 
> Can you put a comment at the location where you provide the delegate, to make
it
> clear that's safe?

Isn't it clear from DependsOn() list in factory class? 
There are other dependencies injected by a factory in the same way. And it seems
like it's a common practice in Chromium.
On the contrary, I'd suggest to add comment to PrecacheManger class that all
injected in constructor dependencies must outlive the class, WDYT?

[Benoit L, 2017-03-21 15:46:15]

Thanks for the answers.
lgtm

Per offline discussion, handling stale versions will be done in a forthcoming
CL.

https://codereview.chromium.org/2762673002/diff/40001/components/precache/con...
File components/precache/content/precache_manager.h (right):

https://codereview.chromium.org/2762673002/diff/40001/components/precache/con...
components/precache/content/precache_manager.h:235: Delegate* delegate_;
On 2017/03/21 15:32:13, alexilin wrote:
> On 2017/03/21 13:24:15, Benoit L wrote:
> > I understand why this is fine here, as the service resolution mechanism
> enforces
> > a longer lifetime for the delegate than this service, but that's quite
subtle.
> > 
> > Can you put a comment at the location where you provide the delegate, to
make
> it
> > clear that's safe?
> 
> Isn't it clear from DependsOn() list in factory class? 
> There are other dependencies injected by a factory in the same way. And it
seems
> like it's a common practice in Chromium.
> On the contrary, I'd suggest to add comment to PrecacheManger class that all
> injected in constructor dependencies must outlive the class, WDYT?

Acknowledged.

[alexilin, 2017-03-22 12:52:40]

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

[commit-bot: I haz the power, 2017-03-22 12:53:00]

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, 2017-03-22 13:35:55]

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

[commit-bot: I haz the power, 2017-03-22 13:35:56]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-03-22 13:42:04]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-03-22 13:42:05]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/2762673002/#ps80001
(title: "Add delegate tests for PrecacheFetcher.")

[commit-bot: I haz the power, 2017-03-22 13:42:16]

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, 2017-03-22 13:46:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490190124586460,
"parent_rev": "531f367559cc5168afc8ef2ce1219ecbcc9de2a8", "commit_rev":
"b0b2d6ffa58b392bd30b49465ba29590c1616fe5"}

[commit-bot: I haz the power, 2017-03-22 13:47:03]

Description was changed from

==========
predictors: Pass manifests from Bork to store in ResourcePrefetchPredictor.

This CL makes ResourcePrefetchPredictor accessible from PrecacheManager to
notify about new manifests through PrecacheManifestDelegate interface.
These manifests are saved then in a dedicated table on a
ResourcePrefetchPredictor side.

BUG=699115
==========

to

==========
predictors: Pass manifests from Bork to store in ResourcePrefetchPredictor.

This CL makes ResourcePrefetchPredictor accessible from PrecacheManager to
notify about new manifests through PrecacheManifestDelegate interface.
These manifests are saved then in a dedicated table on a
ResourcePrefetchPredictor side.

BUG=699115

Review-Url: https://codereview.chromium.org/2762673002
Cr-Commit-Position: refs/heads/master@{#458735}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0b2d6ffa58b392bd30b49465ba2...
==========

[commit-bot: I haz the power, 2017-03-22 13:47:04]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b0b2d6ffa58b392bd30b49465ba2...