predictors: Take the priority into account for scoring resources.

predictors: Take the priority into account for scoring resources.

This changes the way the resource score is computed in the
resource_prefetch_predictor. It takes into account the request priority
as the most important feature, then resource type, then average position.

The rationale is that resource priority surfaces the render-blocking
resources, that are the most critical to prefetch.

BUG=631966
Committed: https://crrev.com/3eb631ab216ff9ea0302a9e89efe33003b11d935
Cr-Commit-Position: refs/heads/master@{#422753}

[Benoit L, 2016-10-03 14:00:44]

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

[pasko, 2016-10-03 16:48:20]

lovely, just a few small suggestions and nits

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

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:483: static
constexpr int kMaxResourcesPerType = 100;
is static constexpr better in some way than just constexpr? curious

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:506: switch
(data.resource_type()) {
nit: Seems better to move declaration of kMaxResourcesPerType and
type_multiplier immediately above this line, closer to their use

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

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:578:
auto compute_score = [](net::RequestPriority priority,
Why would lamda be a necessary complication here?

compute_score(params) is more surprising to read than ComputeScore(params).
Let's make it a regular function with naming in function style.

[Benoit L, 2016-10-04 09:49:54]

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

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:483: static
constexpr int kMaxResourcesPerType = 100;
On 2016/10/03 16:48:20, pasko wrote:
> is static constexpr better in some way than just constexpr? curious

Done.

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:506: switch
(data.resource_type()) {
On 2016/10/03 16:48:20, pasko wrote:
> nit: Seems better to move declaration of kMaxResourcesPerType and
> type_multiplier immediately above this line, closer to their use

Done.

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

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:578:
auto compute_score = [](net::RequestPriority priority,
On 2016/10/03 16:48:20, pasko wrote:
> Why would lamda be a necessary complication here?
> 
> compute_score(params) is more surprising to read than ComputeScore(params).
> Let's make it a regular function with naming in function style.

It was to make the function extra-duper local.
Also, ResourcePrefetchPredictoTables::ComputeResourceScore() is private, so this
is actually less verbose. Finally, it puts the definition right next to its use.

wdyt?

[Benoit L, 2016-10-04 09:54:44]

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

[commit-bot: I haz the power, 2016-10-04 09:54: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-10-04 10:34:39]

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

[commit-bot: I haz the power, 2016-10-04 10:34:39]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-10-04 11:10:36]

lgtm

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

https://codereview.chromium.org/2385173002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:578:
auto compute_score = [](net::RequestPriority priority,
On 2016/10/04 09:49:54, Benoit L wrote:
> On 2016/10/03 16:48:20, pasko wrote:
> > Why would lamda be a necessary complication here?
> > 
> > compute_score(params) is more surprising to read than ComputeScore(params).
> > Let's make it a regular function with naming in function style.
> 
> It was to make the function extra-duper local.
> Also, ResourcePrefetchPredictoTables::ComputeResourceScore() is private, so
this
> is actually less verbose. Finally, it puts the definition right next to its
use.
> 
> wdyt?

Thank you for explanation.

For my personal taste: lambda syntax is annoying, rarely increases readability
when it is not below maybe-5-liner.

For consistency with other code: an extra ForTesting method is usually used in
this case.

Style guide: allows this type of use.

Current use in Chromium:

$ git grep 'auto .* = \[.*\]' | wc -l
111

Agreed that such lambda is better than alternatives in this case, thanks again!

[Benoit L, 2016-10-04 11:16:21]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-10-04 11:16:36]

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-10-04 11:21:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-04 11:25:14]

Description was changed from

==========
predictors: Take the priority into account for scoring resources.

This changes the way the resource score is computed in the
resource_prefetch_predictor. It takes into account the request priority
as the most important feature, then resource type, then average position.

The rationale is that resource priority surfaces the render-blocking
resources, that are the most critical to prefetch.

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

to

==========
predictors: Take the priority into account for scoring resources.

This changes the way the resource score is computed in the
resource_prefetch_predictor. It takes into account the request priority
as the most important feature, then resource type, then average position.

The rationale is that resource priority surfaces the render-blocking
resources, that are the most critical to prefetch.

BUG=631966

Committed: https://crrev.com/3eb631ab216ff9ea0302a9e89efe33003b11d935
Cr-Commit-Position: refs/heads/master@{#422753}
==========

[commit-bot: I haz the power, 2016-10-04 11:25:16]

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