Remove LinkPreload resource type

Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type,
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute
to regular raw resource types (e.g. XHR).

BUG=593267
Committed: https://crrev.com/a5a314d3249ecf1c291b417fbe067e8c2a65fad2
Cr-Commit-Position: refs/heads/master@{#413374}

[Yoav Weiss, 2016-08-18 20:33:00]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 20:34:08]

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

[Yoav Weiss, 2016-08-18 20:45:56]

yoav@yoav.ws changed reviewers:
+ csharrison@chromium.org

[Yoav Weiss, 2016-08-18 20:45:56]

Hey Charlie :)

Took a stab at removing LinkPreload as we discussed. Can you take a look?

[Charlie Harrison, 2016-08-18 21:31:34]

Thanks for the patch!

I thought we'd have to thread a flag isLinkPreload() through these requests?

https://codereview.chromium.org/2257943002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/2257943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:85:
DEFINE_SINGLE_RESOURCE_HISTOGRAM(prefix, LinkPreload)    \
Update histograms.xml with this change. Not sure what the best approach is but
you may have to <obsolete> the histogram and make a new one?

https://codereview.chromium.org/2257943002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:123: case
Resource::LinkPreload:
If we move LinkPreload -> Raw, will this accidentally bump priorities from Low
to High? If this will be overridden in loadPriority let's add a comment (I think
that's what will happen...).

[Yoav Weiss, 2016-08-18 22:07:36]

On 2016/08/18 21:31:34, Charlie Harrison wrote:
> Thanks for the patch!
> 
> I thought we'd have to thread a flag isLinkPreload() through these requests?

I was thinking of adding that in a follow up patch.

> 
>
https://codereview.chromium.org/2257943002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):
> 
>
https://codereview.chromium.org/2257943002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:85:
> DEFINE_SINGLE_RESOURCE_HISTOGRAM(prefix, LinkPreload)    \
> Update histograms.xml with this change. Not sure what the best approach is but
> you may have to <obsolete> the histogram and make a new one?
> 
>
https://codereview.chromium.org/2257943002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:123: case
> Resource::LinkPreload:
> If we move LinkPreload -> Raw, will this accidentally bump priorities from Low
> to High? If this will be overridden in loadPriority let's add a comment (I
think
> that's what will happen...).

For LinkLoader, the request is set to be deferred, but I think you're right and
that's not the case for HTMLPreloadScanner loaded resources.
I thought those priorities are covered by HTMLPreloadScannerTests, but looks
like I was wrong.

With that said, I think we can revise the decision to separate priorities for
LinkPreload from Raw's priorities.
As it is destined to preload raw resources (XHR, etc), I think we can safely
match their priorities.

[Charlie Harrison, 2016-08-18 22:10:35]

Matching priorities sounds ok to me, but do you think this will have a
performance impact in the wild?

[Charlie Harrison, 2016-08-18 22:25:43]

Also, could you wrap the description to 80 cols and add a BUG=?

[Yoav Weiss, 2016-08-18 22:28:01]

On 2016/08/18 22:10:35, Charlie Harrison wrote:
> Matching priorities sounds ok to me, but do you think this will have a
> performance impact in the wild?

Overall <link rel=preload> usage is around 0.1% -
https://www.chromestatus.com/metrics/feature/timeline/popularity/901

I'm guessing most of those have an `as` value, so a priority change shouldn't
impact them.

And for the cases where `as` is missing, arguably a priority change should
improve performance.

[Yoav Weiss, 2016-08-18 22:31:46]

Description was changed from

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, as it is not
really necessary.
No functional change in this CL, so no new tests.

BUG=
==========

to

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
No functional change in this CL, so no new tests.

BUG=593267
==========

[Charlie Harrison, 2016-08-18 22:34:45]

Great, lgtm in that case.

[igrigorik1, 2016-08-18 22:36:40]

Oh, interesting.. If we currently don't match XHR then that's a bug. Per spec,
omitted value is initialized to empty destination, which maps to:
https://fetch.spec.whatwg.org/#concept-request-destination

Empty as should have same priority as XHR/fetch() initiated by the page.

[commit-bot: I haz the power, 2016-08-18 22:56:32]

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

[commit-bot: I haz the power, 2016-08-18 22:56:33]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-08-20 20:49:44]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 20:49:48]

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

[Yoav Weiss, 2016-08-20 20:50:54]

Description was changed from

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
No functional change in this CL, so no new tests.

BUG=593267
==========

to

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute to
regular raw resource types (e.g. XHR).

BUG=593267
==========

[Yoav Weiss, 2016-08-20 20:57:06]

On 2016/08/18 22:34:45, Charlie Harrison wrote:
> Great, lgtm in that case.

Removed the defer for these requests that go through LinkLoader and changed
description. Let me know if the LGTM still stands.

[Yoav Weiss, 2016-08-20 21:14:23]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 21:14:34]

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-20 22:10:06]

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

[commit-bot: I haz the power, 2016-08-20 22:10:06]

Dry run: 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_...)

[Charlie Harrison, 2016-08-20 22:13:56]

Change looks good, I like getting rid of that defer logic.

Couple quick points:
 - CL desc still needs to be wrapped.
 - histograms.xml needs to be updated with the change in ResourceFetcher.

[Yoav Weiss, 2016-08-20 22:27:28]

Description was changed from

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute to
regular raw resource types (e.g. XHR).

BUG=593267
==========

to

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute
to regular raw resource types (e.g. XHR).

BUG=593267
==========

[Yoav Weiss, 2016-08-20 22:28:17]

On 2016/08/20 22:13:56, Charlie Harrison wrote:
> Change looks good, I like getting rid of that defer logic.
> 
> Couple quick points:
>  - CL desc still needs to be wrapped.
wrapped

>  - histograms.xml needs to be updated with the change in ResourceFetcher.

Ran the update script and got no change. Can't find a LinkPreload type in
https://chromium.googlesource.com/chromium/src.git/+/master/tools/metrics/his...

[Charlie Harrison, 2016-08-20 22:38:45]

Ha, you're right! Sorry about that guess it was just a bug in the original code.

still lgtm!

[Yoav Weiss, 2016-08-20 22:46:09]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 22:46:14]

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-20 23:55:14]

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

[commit-bot: I haz the power, 2016-08-20 23:55:15]

Dry run: 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_...)

[Yoav Weiss, 2016-08-21 08:08:58]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 08:09:10]

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-21 09:33:56]

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

[commit-bot: I haz the power, 2016-08-21 09:33:57]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-08-21 10:20:40]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2016-08-21 10:20:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2257943002/#ps80001
(title: "Really fixed test")

[commit-bot: I haz the power, 2016-08-21 10:20:52]

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-21 10:24:43]

Description was changed from

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute
to regular raw resource types (e.g. XHR).

BUG=593267
==========

to

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute
to regular raw resource types (e.g. XHR).

BUG=593267
==========

[commit-bot: I haz the power, 2016-08-21 10:24:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-21 10:27:34]

Description was changed from

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type, 
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute
to regular raw resource types (e.g. XHR).

BUG=593267
==========

to

==========
Remove LinkPreload resource type

This is a refactoring CL, removing the LinkPreload resource type,
as it is not really necessary.
It also matches the priority of <link rel=preload> with no `as` attribute
to regular raw resource types (e.g. XHR).

BUG=593267

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

[commit-bot: I haz the power, 2016-08-21 10:27:35]

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