predictors: Prefetch resources from manifest.

predictors: Prefetch resources from manifest.

ResourcePrefetchPredictor tries to prefetch resources from a precache manifest
if there is no available prediction based on local history.
This CL also adds the check for the manifest age before save it to the database.

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

[alexilin, 2017-04-18 07:05:03]

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

[alexilin, 2017-04-18 07:05:03]

Hi lizeb, please take a look!

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

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:104: const
base::TimeDelta kMaxManifestAge = base::TimeDelta::FromDays(5);
I don't feel strongly about this. 
Probably, it would be better to add the timestamp of the last fetch instead of
using manifest id.

[Benoit L, 2017-04-18 16:28:39]

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

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:921: if
(base::StartsWith(manifest_host, "www.", base::CompareCase::SENSITIVE))
is www systematically stripped from the manifest keys?

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:969: const float
kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;
Have you looked at the practical meaning of this threshold?


Also, tiny nit: since the name is really local, no need to make it that long :-)

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:984:
(!unused_bitset.has_value() || unused_bitset.value()[i]) &&
what is the meaning of the bits?

if unused, versioned and no_store are all set, is the resource prefetchable?


Update: after offline discussion, the logic is correct, but the naming weird.
Can you add a comment?

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:989:
urls->push_back(GURL(resource.url()));
useless_nit: emplace_back?

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:373: // prefetched.
|urls| pointer may be equal nullptr to get return value only.
nit: |urls| may be nullptr to get the return value only.

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:375: std::vector<GURL>*
urls) const;
nit: PopulateFromManifest()?

[alexilin, 2017-04-19 07:59:25]

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

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:921: if
(base::StartsWith(manifest_host, "www.", base::CompareCase::SENSITIVE))
On 2017/04/18 16:28:39, Benoit L wrote:
> is www systematically stripped from the manifest keys?

Yes. (See history::HostForTopHosts and how it's used for manifests)

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:969: const float
kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;
On 2017/04/18 16:28:39, Benoit L wrote:
> Have you looked at the practical meaning of this threshold?

Roughly speaking, it's percentage of host pages where the resource is used. In
addition, pages are also weighted.
 
> Also, tiny nit: since the name is really local, no need to make it that long
:-)

Done.

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:984:
(!unused_bitset.has_value() || unused_bitset.value()[i]) &&
On 2017/04/18 16:28:39, Benoit L wrote:
> what is the meaning of the bits?
> 
> if unused, versioned and no_store are all set, is the resource prefetchable?
> 
> 
> Update: after offline discussion, the logic is correct, but the naming weird.
> Can you add a comment?

Renamed and commented.
Done.

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:989:
urls->push_back(GURL(resource.url()));
On 2017/04/18 16:28:39, Benoit L wrote:
> useless_nit: emplace_back?

Done.

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:373: // prefetched.
|urls| pointer may be equal nullptr to get return value only.
On 2017/04/18 16:28:39, Benoit L wrote:
> nit: |urls| may be nullptr to get the return value only.

Done.

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:375: std::vector<GURL>*
urls) const;
On 2017/04/18 16:28:39, Benoit L wrote:
> nit: PopulateFromManifest()?

Done.

[Benoit L, 2017-04-19 14:48:54]

lgtm, thanks.

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

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:969: const float
kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;
On 2017/04/19 07:59:25, alexilin wrote:
> On 2017/04/18 16:28:39, Benoit L wrote:
> > Have you looked at the practical meaning of this threshold?
> 
> Roughly speaking, it's percentage of host pages where the resource is used. In
> addition, pages are also weighted.
>  
> > Also, tiny nit: since the name is really local, no need to make it that long
> :-)
> 
> Done.

Ok, so can you add a comment saying that this is roughly in line with the
threshold we use in the rest of the code?

https://codereview.chromium.org/2825693002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2825693002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1922:
"www.google.fr", 10, 0, 0);
🇫🇷 🐓

[Benoit L, 2017-04-19 14:48:54]

lgtm, thanks.

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

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:969: const float
kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;
On 2017/04/19 07:59:25, alexilin wrote:
> On 2017/04/18 16:28:39, Benoit L wrote:
> > Have you looked at the practical meaning of this threshold?
> 
> Roughly speaking, it's percentage of host pages where the resource is used. In
> addition, pages are also weighted.
>  
> > Also, tiny nit: since the name is really local, no need to make it that long
> :-)
> 
> Done.

Ok, so can you add a comment saying that this is roughly in line with the
threshold we use in the rest of the code?

https://codereview.chromium.org/2825693002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2825693002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1922:
"www.google.fr", 10, 0, 0);
🇫🇷 🐓

[alexilin, 2017-04-19 15:35:12]

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

https://codereview.chromium.org/2825693002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:969: const float
kMinPrecacheResourceWeightToTriggerPrefetch = 0.7f;
On 2017/04/19 14:48:54, Benoit L wrote:
> On 2017/04/19 07:59:25, alexilin wrote:
> > On 2017/04/18 16:28:39, Benoit L wrote:
> > > Have you looked at the practical meaning of this threshold?
> > 
> > Roughly speaking, it's percentage of host pages where the resource is used.
In
> > addition, pages are also weighted.
> >  
> > > Also, tiny nit: since the name is really local, no need to make it that
long
> > :-)
> > 
> > Done.
> 
> Ok, so can you add a comment saying that this is roughly in line with the
> threshold we use in the rest of the code?

Done.

https://codereview.chromium.org/2825693002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2825693002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1922:
"www.google.fr", 10, 0, 0);
On 2017/04/19 14:48:54, Benoit L wrote:
> 🇫🇷 🐓

Vive la France!
Actually, I've changed this because for a manifest there is no difference
between "google.com" and "www.google.com".

[alexilin, 2017-04-19 15:35:54]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-04-19 15:35:54]

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/2825693002/#ps40001
(title: "Add comment.")

[commit-bot: I haz the power, 2017-04-19 15:36:23]

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-04-19 17:02:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492616154092170,
"parent_rev": "acba6ce3ba4f318e84b772ce094cc8990bdf2022", "commit_rev":
"ff7d03b40a6e5eaa323dd18fbc873ff868392689"}

[commit-bot: I haz the power, 2017-04-19 17:03:22]

Description was changed from

==========
predictors: Prefetch resources from manifest.

ResourcePrefetchPredictor tries to prefetch resources from a precache manifest
if there is no available prediction based on local history.
This CL also adds the check for the manifest age before save it to the database.

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

to

==========
predictors: Prefetch resources from manifest.

ResourcePrefetchPredictor tries to prefetch resources from a precache manifest
if there is no available prediction based on local history.
This CL also adds the check for the manifest age before save it to the database.

BUG=699115

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

[commit-bot: I haz the power, 2017-04-19 17:03:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ff7d03b40a6e5eaa323dd18fbc87...