predictors: Support fonts in resource_prefetch_predictors.

predictors: Support fonts in resource_prefetch_predictors.

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

[Benoit L, 2016-07-28 11:50:04]

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

[pasko, 2016-07-28 14:04:05]

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

https://codereview.chromium.org/2184323003/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:306: else if
(net::MatchesMimeType("application/font-woff", mime_type))
let's add more mime types for fonts. I don't know whether all below is properly
recognized, but here is how I found popular font mime types:

cat httparchive_Jul_15_2016_requests.csv | cut -d ',' -f23 | grep font >
/tmp/font-mime-types

$ sort /tmp/font-mime-types | uniq -c | sort -n | grep -v css | tail -30
$ sort /hdd/tmp/font-mime-types | uniq -c | sort -n | grep -v css | tail -30
     11 "font-woff"
     14 "application/font-otf"
     17 "font/font-woff2"
     22 "application/x-font"
     22 "woff/x-font"
     23 "application/font"
     30 "font/x-font-woff"
     41 "x-font/otf"
     42 "font/font-woff"
    190 "x-font/ttf"
    211 "application/x-font-truetype"
    316 "application/x-font-opentype"
    474 "font/x-woff2"
    539 "application/vnd.ms-fontobject"
    567 "font/otf"
    790 "application/x-font-woff2"
    996 "font/truetype"
   1218 "font/opentype"
   1222 "application/font-ttf"
   2092 "application/font-sfnt"
   2172 "x-font/woff"
   2242 "application/x-font-otf"
   7445 "font/ttf"
  12905 "font/woff"
  14095 "application/x-font-ttf"
  14541 "font/x-woff"
  31035 "application/font-woff2"
  86496 "application/font-woff"
 105720 "application/x-font-woff"
 657314 "font/woff2"

.. grepping out 'css' because there are some bugs in my usage of 'cut' and
perhaps also in httparchive.

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

https://codereview.chromium.org/2184323003/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:450:
EXPECT_NEAR(99, image_resource.score, 1e-4);
any reason not to define it as double, but do it on the previous line?

https://codereview.chromium.org/2184323003/diff/1/tools/resource_prefetch_pre...
File tools/resource_prefetch_predictor/prefetch_predictor_tool.py (right):

https://codereview.chromium.org/2184323003/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/prefetch_predictor_tool.py:22: FONT_RESOURCE =
5
Let's drop a comment saying that this should mirror content::ResourceType

[Benoit L, 2016-07-28 16:11:54]

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

https://codereview.chromium.org/2184323003/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.cc:306: else if
(net::MatchesMimeType("application/font-woff", mime_type))
On 2016/07/28 14:04:04, pasko wrote:
> let's add more mime types for fonts. I don't know whether all below is
properly
> recognized, but here is how I found popular font mime types:
> 
> cat httparchive_Jul_15_2016_requests.csv | cut -d ',' -f23 | grep font >
> /tmp/font-mime-types
> 
> $ sort /tmp/font-mime-types | uniq -c | sort -n | grep -v css | tail -30
> $ sort /hdd/tmp/font-mime-types | uniq -c | sort -n | grep -v css | tail -30
>      11 "font-woff"
>      14 "application/font-otf"
>      17 "font/font-woff2"
>      22 "application/x-font"
>      22 "woff/x-font"
>      23 "application/font"
>      30 "font/x-font-woff"
>      41 "x-font/otf"
>      42 "font/font-woff"
>     190 "x-font/ttf"
>     211 "application/x-font-truetype"
>     316 "application/x-font-opentype"
>     474 "font/x-woff2"
>     539 "application/vnd.ms-fontobject"
>     567 "font/otf"
>     790 "application/x-font-woff2"
>     996 "font/truetype"
>    1218 "font/opentype"
>    1222 "application/font-ttf"
>    2092 "application/font-sfnt"
>    2172 "x-font/woff"
>    2242 "application/x-font-otf"
>    7445 "font/ttf"
>   12905 "font/woff"
>   14095 "application/x-font-ttf"
>   14541 "font/x-woff"
>   31035 "application/font-woff2"
>   86496 "application/font-woff"
>  105720 "application/x-font-woff"
>  657314 "font/woff2"
> 
> .. grepping out 'css' because there are some bugs in my usage of 'cut' and
> perhaps also in httparchive.

Thank you for spotting this!
Done.

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

https://codereview.chromium.org/2184323003/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:450:
EXPECT_NEAR(99, image_resource.score, 1e-4);
On 2016/07/28 14:04:04, pasko wrote:
> any reason not to define it as double, but do it on the previous line?

Done.

https://codereview.chromium.org/2184323003/diff/1/tools/resource_prefetch_pre...
File tools/resource_prefetch_predictor/prefetch_predictor_tool.py (right):

https://codereview.chromium.org/2184323003/diff/1/tools/resource_prefetch_pre...
tools/resource_prefetch_predictor/prefetch_predictor_tool.py:22: FONT_RESOURCE =
5
On 2016/07/28 14:04:04, pasko wrote:
> Let's drop a comment saying that this should mirror content::ResourceType

Done.

[pasko, 2016-07-28 18:42:57]

lgtm with a few nits

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

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:289: // anything.
This would be an easier to read comment:

// Restrict content::RESOURCE_TYPE_{PREFETCH,SUB_RESOURCE} to a small set of
mime types.

.. just because words like "anything" and "this" need extra pattern matching..

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:290: if (!handled &&
(resource_type == content::RESOURCE_TYPE_PREFETCH ||
one of these values for resource_type implies (!handled), checking for it looks
slightly confusing

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:321: static const
std::array<std::string, 12> font_mime_types = {
nit: it would be less error prone to avoid indicating the array length constant
here (12).

One way is to replace std::array with std::vector.

Another is:

static std::string font_mime_types[] = {
  ...
};

if (std::any_of(std::begin(font_mime_types), std::end(font_mime_types),
    ...

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:346: })) {
is it the way git cl format prefers this?

[Benoit L, 2016-07-29 08:56:27]

Thanks!

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

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:289: // anything.
On 2016/07/28 18:42:57, pasko wrote:
> This would be an easier to read comment:
> 
> // Restrict content::RESOURCE_TYPE_{PREFETCH,SUB_RESOURCE} to a small set of
> mime types.
> 
> .. just because words like "anything" and "this" need extra pattern matching..

Done.

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:290: if (!handled &&
(resource_type == content::RESOURCE_TYPE_PREFETCH ||
On 2016/07/28 18:42:57, pasko wrote:
> one of these values for resource_type implies (!handled), checking for it
looks
> slightly confusing

Done.

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:321: static const
std::array<std::string, 12> font_mime_types = {
On 2016/07/28 18:42:57, pasko wrote:
> nit: it would be less error prone to avoid indicating the array length
constant
> here (12).
> 
> One way is to replace std::array with std::vector.
> 
> Another is:
> 
> static std::string font_mime_types[] = {
>   ...
> };
> 
> if (std::any_of(std::begin(font_mime_types), std::end(font_mime_types),
>     ...

Done.

https://codereview.chromium.org/2184323003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:346: })) {
On 2016/07/28 18:42:57, pasko wrote:
> is it the way git cl format prefers this?

Yes.

[Benoit L, 2016-07-29 08:56:36]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-07-29 08:56:37]

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/2184323003/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-07-29 08:56:50]

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-07-29 09:05:16]

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

[commit-bot: I haz the power, 2016-07-29 09:05:17]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Benoit L, 2016-07-29 09:32:33]

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

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

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-07-29 10:13:37]

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

[commit-bot: I haz the power, 2016-07-29 10:13:38]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-07-29 10:49:03]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-07-29 10:49:04]

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/2184323003/#ps80001
(title: "Remove exit-time destructors.")

[commit-bot: I haz the power, 2016-07-29 10:49:19]

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-07-29 10:52:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-29 10:55:05]

Description was changed from

==========
predictors: Support fonts in resource_prefetch_predictors.

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

to

==========
predictors: Support fonts in resource_prefetch_predictors.

BUG=631966

Committed: https://crrev.com/f513f1db12aa282b38f3d2624b792af7626b1f5b
Cr-Commit-Position: refs/heads/master@{#408613}
==========

[commit-bot: I haz the power, 2016-07-29 10:55:05]

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