Add initial link preload support

Add initial link preload support

An initial version of <link rel=preload>: https://w3c.github.io/preload/
Doesn't include:
* Priorities implementation
* Link header support
* Mutation tests  

BUG=471199
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193605

[Yoav Weiss, 2015-04-05 20:15:08]

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

[Yoav Weiss, 2015-04-05 20:15:08]

Hey Mike,

Mind taking a look? :)

[Mike West, 2015-04-07 06:26:28]

Can you please:

* add some tests for `as` behavior?
* link to a spec in the CL description so I have something to skim?

https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:338:
ASSERT(request.resourceRequest().frameType() == WebURLRequest::FrameTypeNone);
Should this have a matching `preload` context as well?

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
File Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:144: return Resource::Raw;
Where is this used? Could we use request contexts instead?

I'm hoping we can get rid of `Resource::Type` at some point. CSP is the only
infrastructure I know of that's actively using it.

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:148: {
ASSERT that the runtime flag is enabled?

[Yoav Weiss, 2015-04-07 07:26:14]

Thanks!

Added spec link. I didn't add tests for `as` since it doesn't really do anything
yet. I'll add an `as` attribute to the tests to make sure it doesn't crash tho

https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:338:
ASSERT(request.resourceRequest().frameType() == WebURLRequest::FrameTypeNone);
On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> Should this have a matching `preload` context as well?

Quite possible. Do you think it should be part of the current CL, or can that be
added later on, after I decouple context from priority?

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
File Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:144: return Resource::Raw;
On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> Where is this used? Could we use request contexts instead?
> 
> I'm hoping we can get rid of `Resource::Type` at some point. CSP is the only
> infrastructure I know of that's actively using it.

It's used by ResourceFetcher to determine context and hence priority.
Currently it does nothing since I'm planning to add that in a followup CL.
Also, I think that I'd need to do some cleanup and decouple request priority
from context, since as currently defined `as` does not determine context, but
does determine the request priority.

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:148: {
On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> ASSERT that the runtime flag is enabled?

There's one a few lines below. I'll bump it up for better visibility.

[Mike West, 2015-04-07 12:23:01]

https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:338:
ASSERT(request.resourceRequest().frameType() == WebURLRequest::FrameTypeNone);
On 2015/04/07 at 07:26:14, Yoav Weiss wrote:
> Quite possible. Do you think it should be part of the current CL, or can that
be added later on, after I decouple context from priority?

If you already know that you need the context, then adding it in this CL should
be pretty straightforward. If the context is debatable (there's a GitHub issue,
right?), then adding it later is fine. In that case, I'd suggest adding a TODO
here.

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
File Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:144: return Resource::Raw;
On 2015/04/07 at 07:26:14, Yoav Weiss wrote:
> On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> > Where is this used? Could we use request contexts instead?
> > 
> > I'm hoping we can get rid of `Resource::Type` at some point. CSP is the only
> > infrastructure I know of that's actively using it.
> 
> It's used by ResourceFetcher to determine context and hence priority.
> Currently it does nothing since I'm planning to add that in a followup CL.
> Also, I think that I'd need to do some cleanup and decouple request priority
from context, since as currently defined `as` does not determine context, but
does determine the request priority.

Hrm. Ok. That seems strange. I wouldn't block you from landing this, but I'd
suggest that thinking about those interactions might be reasonable.

https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:148: {
On 2015/04/07 at 07:26:14, Yoav Weiss wrote:
> On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> > ASSERT that the runtime flag is enabled?
> 
> There's one a few lines below. I'll bump it up for better visibility.

You're right, sorry. My eyes are obviously broken.

[Yoav Weiss, 2015-04-07 12:40:20]

On 2015/04/07 12:23:01, Mike West (OOO until 8th) wrote:
>
https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/1060863003/diff/20001/Source/core/fetch/Resou...
> Source/core/fetch/ResourceFetcher.cpp:338:
> ASSERT(request.resourceRequest().frameType() == WebURLRequest::FrameTypeNone);
> On 2015/04/07 at 07:26:14, Yoav Weiss wrote:
> > Quite possible. Do you think it should be part of the current CL, or can
that
> be added later on, after I decouple context from priority?
> 
> If you already know that you need the context, then adding it in this CL
should
> be pretty straightforward. If the context is debatable (there's a GitHub
issue,
> right?), then adding it later is fine. In that case, I'd suggest adding a TODO
> here.

Yeah, maybe I'll wait for the GH dust to settle.

> 
>
https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
> File Source/core/loader/LinkLoader.cpp (right):
> 
>
https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
> Source/core/loader/LinkLoader.cpp:144: return Resource::Raw;
> On 2015/04/07 at 07:26:14, Yoav Weiss wrote:
> > On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> > > Where is this used? Could we use request contexts instead?
> > > 
> > > I'm hoping we can get rid of `Resource::Type` at some point. CSP is the
only
> > > infrastructure I know of that's actively using it.
> > 
> > It's used by ResourceFetcher to determine context and hence priority.
> > Currently it does nothing since I'm planning to add that in a followup CL.
> > Also, I think that I'd need to do some cleanup and decouple request priority
> from context, since as currently defined `as` does not determine context, but
> does determine the request priority.
> 
> Hrm. Ok. That seems strange. I wouldn't block you from landing this, but I'd
> suggest that thinking about those interactions might be reasonable.

I'll start exploring such separation, and see where it gets me.

> 
>
https://codereview.chromium.org/1060863003/diff/20001/Source/core/loader/Link...
> Source/core/loader/LinkLoader.cpp:148: {
> On 2015/04/07 at 07:26:14, Yoav Weiss wrote:
> > On 2015/04/07 06:26:28, Mike West (OOO until 8th) wrote:
> > > ASSERT that the runtime flag is enabled?
> > 
> > There's one a few lines below. I'll bump it up for better visibility.
> 
> You're right, sorry. My eyes are obviously broken.

[Mike West, 2015-04-07 19:56:18]

LGTM % 'as' and a nit.

https://codereview.chromium.org/1060863003/diff/40001/Source/core/loader/Link...
File Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1060863003/diff/40001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:143: // FIXME: Add actual types here and make
sure priorities work accordingly.
Nit: We spell this `TODO(yoav):` now...

https://codereview.chromium.org/1060863003/diff/40001/Source/core/loader/Link...
Source/core/loader/LinkLoader.cpp:149: if (relAttribute.isLinkPreload() &&
href.isValid()) {
https://w3c.github.io/preload/#fetch-settings says that `as` is mandatory; that
doesn't seem to be part of this patch. Is it intentionally not part of this
patch?

If you're planning to add it later, would you mind updating the CL description?

[Yoav Weiss, 2015-04-09 07:00:31]

On 2015/04/07 19:56:18, Mike West wrote:
> LGTM % 'as' and a nit.
> 
>
https://codereview.chromium.org/1060863003/diff/40001/Source/core/loader/Link...
> File Source/core/loader/LinkLoader.cpp (right):
> 
>
https://codereview.chromium.org/1060863003/diff/40001/Source/core/loader/Link...
> Source/core/loader/LinkLoader.cpp:143: // FIXME: Add actual types here and
make
> sure priorities work accordingly.
> Nit: We spell this `TODO(yoav):` now...

Right

> 
>
https://codereview.chromium.org/1060863003/diff/40001/Source/core/loader/Link...
> Source/core/loader/LinkLoader.cpp:149: if (relAttribute.isLinkPreload() &&
> href.isValid()) {
> https://w3c.github.io/preload/#fetch-settings says that `as` is mandatory;
that
> doesn't seem to be part of this patch. Is it intentionally not part of this
> patch?
> 
> If you're planning to add it later, would you mind updating the CL
description?

Good catch! It was changed in the spec after I started work on that CL and I
forgot to update it.
Updated it now, as well as a check and a warning when the `href` is invalid or
empty.

[Mike West, 2015-04-09 07:17:49]

LGTM, thanks for taking another pass.

[Yoav Weiss, 2015-04-09 09:23:15]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-04-09 09:23:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060863003/60001

[commit-bot: I haz the power, 2015-04-09 22:04:33]

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

[commit-bot: I haz the power, 2015-04-09 22:04:34]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/58507)

[Yoav Weiss, 2015-04-10 05:12:57]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-04-10 05:13:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060863003/60001

[commit-bot: I haz the power, 2015-04-10 18:17:33]

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

[commit-bot: I haz the power, 2015-04-10 18:17:33]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/58674)

[Yoav Weiss, 2015-04-11 18:49:05]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2015-04-11 18:49:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060863003/60001

[commit-bot: I haz the power, 2015-04-12 04:46:22]

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

[commit-bot: I haz the power, 2015-04-12 04:46:23]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

[Mike West, 2015-04-12 05:14:10]

That windows bot is unhappy. Might be worth filing an infra bug. :(

[sof, 2015-04-12 05:18:37]

On 2015/04/12 04:46:23, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

step hardware_accelerated_feature asserts:

ASSERTION FAILED: RuntimeEnabledFeatures::linkPreloadEnabled()
../../third_party/WebKit/Source/core/loader/LinkLoader.cpp(155) : void
blink::LinkLoader::preloadIfNeeded(const blink::LinkRelAttribute &, const
blink::KURL &, blink::Document &, const WTF::String &)

[Mike West, 2015-04-12 05:21:47]

Oh. *ahem* I suppose I could have clicked through to read the stdout before
blaming the bot.

Thanks, Sigbjorn! :)

[Yoav Weiss, 2015-04-12 05:21:49]

On 2015/04/12 05:18:37, sof wrote:
> On 2015/04/12 04:46:23, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)
> 
> step hardware_accelerated_feature asserts:
> 
> ASSERTION FAILED: RuntimeEnabledFeatures::linkPreloadEnabled()
> ../../third_party/WebKit/Source/core/loader/LinkLoader.cpp(155) : void
> blink::LinkLoader::preloadIfNeeded(const blink::LinkRelAttribute &, const
> blink::KURL &, blink::Document &, const WTF::String &)

Hmm, missed that part. I'll look into that then

[Yoav Weiss, 2015-04-12 06:13:00]

On 2015/04/12 05:21:49, Yoav Weiss wrote:
> On 2015/04/12 05:18:37, sof wrote:
> > On 2015/04/12 04:46:23, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   linux_blink_rel on tryserver.blink (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)
> > 
> > step hardware_accelerated_feature asserts:
> > 
> > ASSERTION FAILED: RuntimeEnabledFeatures::linkPreloadEnabled()
> > ../../third_party/WebKit/Source/core/loader/LinkLoader.cpp(155) : void
> > blink::LinkLoader::preloadIfNeeded(const blink::LinkRelAttribute &, const
> > blink::KURL &, blink::Document &, const WTF::String &)
> 
> Hmm, missed that part. I'll look into that then

Uninitialized member var :/

[Yoav Weiss, 2015-04-12 19:03:55]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-04-12 19:03:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1060863003/#ps80001
(title: "Fix uninit member :/")

[commit-bot: I haz the power, 2015-04-12 19:04:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060863003/80001

[commit-bot: I haz the power, 2015-04-12 22:27:39]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193605

[sof, 2015-04-13 18:08:00]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-04-13 18:08:00]

yoav wrote:
>
..
> Uninitialized member var :/

Hmm, -Wuninitialized doesn't catch this statically for me (w/ clang and g++,
release + debug.) Would have been a time saver here.

Do we have any static checkers that are able to catch failures to initialize
(value) fields, or is that left to ASAN (or valgrind) at run-time?

+Cc: thakis, thestig.

[Lei Zhang, 2015-04-13 19:43:42]

On 2015/04/13 18:08:00, sof wrote:
> yoav wrote:
> >
> ..
> > Uninitialized member var :/
> 
> Hmm, -Wuninitialized doesn't catch this statically for me (w/ clang and g++,
> release + debug.) Would have been a time saver here.
> 
> Do we have any static checkers that are able to catch failures to initialize
> (value) fields, or is that left to ASAN (or valgrind) at run-time?
> 
> +Cc: thakis, thestig.

g++'s -Weffc++ can catch this. There's probably some VS options that can catch
this as well.

[Nico, 2015-04-13 20:00:54]

grr rietveld. I just wrote a reply, hit "send" and rietveld sent an empty mail
:-(

This is surprisingly hard to do. I gave it a fairly serious try a bit over a
month ago (http://marc.info/?l=cfe-dev&m=142578376706818&w=2 – somewhat
mojibaked, but comprehensible – lists the heuristics I tried. Let me know if you
can think of better ones.)

[sof, 2015-04-14 14:26:48]

On 2015/04/13 20:00:54, Nico (away until Wed Apr 15) wrote:
> grr rietveld. I just wrote a reply, hit "send" and rietveld sent an empty mail
> :-(
> 
> This is surprisingly hard to do. I gave it a fairly serious try a bit over a
> month ago (http://marc.info/?l=cfe-dev&m=142578376706818&w=2 – somewhat
> mojibaked, but comprehensible – lists the heuristics I tried. Let me know if
you
> can think of better ones.)

Oh cool, I had no idea that you had a go at improving matters recently; thanks
for the pointer!

For Blink, I would have (idly) thought that something implementing the first
four restrictions would get you close. Will have to experiment.. :)