1222633 - Add support for <link rel=preload>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Ilya Grigorik]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36



Actual results:

Preload is not supported.


Expected results:

Preload provides a declarative fetch primitive that initiates an early fetch and separates fetching from resource execution.

- spec: https://w3c.github.io/preload/
- use cases: https://w3c.github.io/preload/#use-cases

Comment 1

[John Daggett (:jtd)]

This would be extremely helpful for authors with web font loading, which are currently loaded only as needed.

Blog post by one of the Adobe Typekit devs:

http://www.bramstein.com/writing/preload-hints-for-web-fonts.html

Comment 2

[dalimian]

Chrome 50 already has this, already leveraged by many sites to load lot faster.

Comment 3

[Alex Bell]

+1. This is the feature that will kill the Flash of Unstyled Text once and for all. This is a significant user-facing issue, at least experientially, and also a Chrome parity issue. Is anyone working on this?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(It is a bit silly to have separate specs and in general having too many random underdefined pre* stuff, but oh well, I guess we should just implement this. Preload is somewhat well defined actually)

I wonder how much in practice prefetch and preload should differ in our implementation.

There is of course the http/2 part, but if talking only about the DOM part of this all.

Dragana, would you be interested in to take a look at this.

Comment 5

[Dragana Damjanovic [:dragana]]

I will take it.

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24d2ba7c9579

Comment 7

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dd32c8ad367

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633.patch

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633.patch

Here is the first version. I have implemented it similar to prefetch.

In current implementation, when a new load starts we kill all prefetches. I have change that, anyway I think we need a better way to decide what to kill and what not, e.g. if load is almost over why killing it or if load only has small amount of data why killing it we are going to get that data anyway because closing a connection takes a 1/2rtt.

Comment 10

[Ilya Grigorik]

A couple of questions after a quick scan through the code...

The load is initiated via nsPrefetchService. Does this imply some form of prioritization on the request? Note that prefetch and preload have *very* different load semantics: one is a "I might need this on next navigation, fetch it with low priority", and preload is "initiate this fetch right now and treat it as resource type defined by `as` attribute.. same priority, same CSP policy, everything". 

I don't see `as` being used anywhere for security or prioritization. Is this WIP?

It's OK to cancel any and all outstanding preloads when the fetch group is terminated. That's not the case for prefetch, which should be allowed to continue.

Comment 11

[Dragana Damjanovic [:dragana]]

(In reply to Ilya Grigorik from comment #10)
> A couple of questions after a quick scan through the code...
> 
> The load is initiated via nsPrefetchService. Does this imply some form of
> prioritization on the request? Note that prefetch and preload have *very*
> different load semantics: one is a "I might need this on next navigation,
> fetch it with low priority", and preload is "initiate this fetch right now
> and treat it as resource type defined by `as` attribute.. same priority,
> same CSP policy, everything". 
> 

Preload will be started immediately and prefetch will be started only after the main page finishes loading. 
'as' is not currently used (this is work in progress and I am not a dom person so I will need help here from Olli. I am not sure what is already implemented and what not)

> I don't see `as` being used anywhere for security or prioritization. Is this
> WIP?

This is work in progress :)
Prefetch does not have priority build in, but that will be a different bug.

> 
> It's OK to cancel any and all outstanding preloads when the fetch group is
> terminated. That's not the case for prefetch, which should be allowed to
> continue.

Currently I am not canceling preload when the fetch group finishes, but that will be ok to implement.

Comment 12

[Ilya Grigorik]

(In reply to Dragana Damjanovic [:dragana] from comment #11)
> Preload will be started immediately and prefetch will be started only after
> the main page finishes loading. 

Cool. For my own benefit, where and how is this controlled in the code?

> > I don't see `as` being used anywhere for security or prioritization. Is this
> > WIP?

Gotcha. Thanks! Excited to see this coming together :)

Comment 13

[Dragana Damjanovic [:dragana]]

(In reply to Ilya Grigorik from comment #12)
> (In reply to Dragana Damjanovic [:dragana] from comment #11)
> > Preload will be started immediately and prefetch will be started only after
> > the main page finishes loading. 
> 
> Cool. For my own benefit, where and how is this controlled in the code?

Copying this for you, I  figured out that I have a bug :) Thanks :) 

In nsPrefetchService::PrefetchPreload

If we have the same prefetch already queued, move it from mQueue to mCurrentNodes (currently active prefetches)
if (!(aType & nsStyleLinkElement::ePRELOAD)) {
  return added ? NS_OK : NS_ERROR_ABORT;
} else {
  // It is a preload, we will activate fetch.
  LOG(("This is a preload."));
  mQueue.erase(nodeIt);
  rv = node->OpenChannel();
  if (NS_SUCCEEDED(rv)) {
    mCurrentNodes.AppendElement(node);
  } else {
    DispatchEvent(node, false);
  }
  return NS_OK;
}

and if it is a completely a new link, put it directly to the mCurrentNodes and start it:
if (!(aType & nsStyleLinkElement::ePRELOAD)) {
  rv = EnqueueURI(aURI, aReferrerURI, aSource,
                 getter_AddRefs(enqueuedNode));
  NS_ENSURE_SUCCESS(rv, rv);
} else {
  LOG(("This is a preload, so start the prefetch.\n"));
  enqueuedNode = new nsPrefetchNode(this, aURI, aReferrerURI,
                                    aSource);
  rv = enqueuedNode->OpenChannel();
  if (NS_SUCCEEDED(rv)) {
    mCurrentNodes.AppendElement(enqueuedNode);
  } else {
    DispatchEvent(enqueuedNode, false);
  }
}

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633.patch

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Aha, the spec doesn't actually define at all what to do with the preloaded resources.
I guess we'll just use some cache.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8784894 [details] [diff] [review]
bug_1222633.patch

> nsPrefetchService::StopPrefetching()
> {
>     mStopCount++;
> 
>     LOG(("StopPrefetching [stopcount=%d]\n", mStopCount));
>-
>-    // only kill the prefetch queue if we are actively prefetching right now
>-    if (mCurrentNodes.IsEmpty()) {
>-        return;
>-    }
>-
>-    for (uint32_t i = 0; i < mCurrentNodes.Length(); ++i) {
>-        mCurrentNodes[i]->CancelChannel(NS_BINDING_ABORTED);
>-    }
>-    mCurrentNodes.Clear();
>-    EmptyQueue();
>+    // We will not kill already started prefetch.
Why not?



So to support 'as' I think one would need to change nsPrefetchNode::OpenChannel() so that right contentpolicy type could be passed.

But yes, makes sense to reuse the existing pre* stuff.

(sorry about delay. I tend to process feedback queue after review queue)

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8784894 [details] [diff] [review]
> bug_1222633.patch
> 
> > nsPrefetchService::StopPrefetching()
> > {
> >     mStopCount++;
> > 
> >     LOG(("StopPrefetching [stopcount=%d]\n", mStopCount));
> >-
> >-    // only kill the prefetch queue if we are actively prefetching right now
> >-    if (mCurrentNodes.IsEmpty()) {
> >-        return;
> >-    }
> >-
> >-    for (uint32_t i = 0; i < mCurrentNodes.Length(); ++i) {
> >-        mCurrentNodes[i]->CancelChannel(NS_BINDING_ABORTED);
> >-    }
> >-    mCurrentNodes.Clear();
> >-    EmptyQueue();
> >+    // We will not kill already started prefetch.
> Why not?

We should not stop preloads and for prefetchs if we already started transfer, depends on how much was already transferred, how big is the file, and there is still sometime until we actually cancel connection (until close connection is received by the server), it is sometime no use in canceling it. We should add some logic to decide on that, e.g. do we already have response headers and how much data is already transmitted. I would add that in a separate bug.

> 
> 
> 
> So to support 'as' I think one would need to change
> nsPrefetchNode::OpenChannel() so that right contentpolicy type could be
> passed.
> 

We probably have a function that parses request destination similar to 'as'. Can you point me to it? Thanks.


> But yes, makes sense to reuse the existing pre* stuff.
> 
> (sorry about delay. I tend to process feedback queue after review queue)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dragana Damjanovic [:dragana] from comment #17)
> We probably have a function that parses request destination similar to 'as'.
> Can you point me to it? Thanks.

Apparently we don't have quite the right list of things. Our service worker and related stuff seem to use older version of the spec.
bkelly might know if we have patches to update our impl to support the "destination"s which Fetch spec is talking about.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Apparently we don't have quite the right list of things. Our service worker
> and related stuff seem to use older version of the spec.
> bkelly might know if we have patches to update our impl to support the
> "destination"s which Fetch spec is talking about.

AFAIK no one has started work on updating the code to this part of the spec.

Comment 20

[Yoav Weiss]

Any recent progress on preload support?

Comment 21

[:Harald Kirschner :digitarald]

Marking [qf] for triage, not sure if this appropriate as this isn't just a fix.

To add reason, this was brought up by partners that heavily use preload, especially for prioritizing their core scripts that handle loading and rendering of page components. Examples are Facebook (uses Big Pipe for generating page from pagelets) and Zalando (tailor is their "streaming layout service").

Comment 22

[Dragana Damjanovic [:dragana]]

I will try to find time to work on this, targeting ff55

Comment 23

[Naveed Ihsanullah [:naveed]]

If Facebook will use this than it is a qf:p1. Kannan will ask Facebook for details. Otherwise we may wish to move it to a p3.

Comment 24

[Kannan Vijayan [:djvj]]

Facebook is already using rel=preload for CSS and JS.

Comment 25

[Andrey]

We use preload at search results page of Yandex for loading core sctipts from page header that flushes to user at the very beginning of request handling.

You can see it in this url: https://yandex.com/search/?text=cats

And we will be happy if this feature appears in Firefox :)

Comment 26

[Simon Cropp]

This is probably already known, but preload can also be used in a http header. ie not just in the html response

Comment 27

[Darkspirit]

(In reply to Simon Cropp from comment #26)
> This is probably already known, but preload can also be used in a http
> header. ie not just in the html response

Oh, I thought this bug was (only) about the http response header. http://caniuse.com/#search=preload
But the HTTP Response Header "Link: <https://example.com/other/styles.css>; rel=preload; as=style" isn't supported right now, is it? https://w3c.github.io/preload/
The current rel=prefetch (http://caniuse.com/#feat=link-rel-prefetch) is more like a "should" and get's loaded at the end of all requests, so it's inadequate to preload a css plus fonts/images with a reponse header of index.html. The css url could get read fast from the html, but not the images/fonts of the css (we have to load first). Preloading would be good here.

Comment 28

[Dragana Damjanovic [:dragana]]

(In reply to Darkspirit from comment #27)
> (In reply to Simon Cropp from comment #26)
> > This is probably already known, but preload can also be used in a http
> > header. ie not just in the html response
> 
> Oh, I thought this bug was (only) about the http response header.
> http://caniuse.com/#search=preload
> But the HTTP Response Header "Link: <https://example.com/other/styles.css>;
> rel=preload; as=style" isn't supported right now, is it?
> https://w3c.github.io/preload/
> The current rel=prefetch (http://caniuse.com/#feat=link-rel-prefetch) is
> more like a "should" and get's loaded at the end of all requests, so it's
> inadequate to preload a css plus fonts/images with a reponse header of
> index.html. The css url could get read fast from the html, but not the
> images/fonts of the css (we have to load first). Preloading would be good
> here.

Preload is not implemented at all. Prefetch is implemented but that is a different thing.

Comment 29

[David Dias]

That will be really great to have preload working on Firefox... Hope to see that soon!

Comment 30

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633.patch

This is a new version.

I have some questions:
I have not implemented "media" part. Do we have implementation of its parser somewhere?

I have implemented checking for "type" attribute. I am not sure I have all the types listed. For some types, e.g. images, I found helper functions for some other I have not found any. If there are some helper functions I have not found, please let me know.

"as" attribute is a bit confusing. The draft says that if a "as" is empty string, the preload should be treated as XHR. If "as" attribute is not a correct destination, preload should dispatch an error. On the other hand there are tests that expect that if you set a incorrect destination "as" attribute should be set to empty string, which is putting this 2 cases into one. I decided to implement one version for now and we can change this later.

nsPrefetchService will stop preloads as soon as there as no active loads any more. This will cause some of the web-platform tests to fail, because the page dispatching reload finish loading. Some test will run fine if run as a single test, but fail if I run a whole directory(https://treeherder.mozilla.org/#/jobs?repo=try&revision=937d9572f092501d592e3134cf57c3a72e23aa90). 


For now (for firefox55) we will not implement nocache version. If a page wants to preload resources it must send a response that is cachable. The nocache version is a lot of work that we do not have cycles for at this moment.

Some web-platform tests fail because the tests uses PerformanceResourceTiming to check whether load(not preload) uses the preloaded resources. Since we store data in the cache and loads from the cache have performance entry. The tests expect that loads using preloaded resources do not have such a entry. Even if we implement "nocache" version of preload this test will failed in the same way (if I remember correctly our discussion on how to implement "nocahe" version).

For missing "as" attribute I set contentPolicy to TYPE_OTHER. Is this correct?  testing/web-platform/tests/preload/preload-csp.sub.html
will fail for that case. I am not sure if the test is not correct or as="" should have a different contectType.

Comment 31

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webtest.patch

This are some changes to the web-platfom tests. Mainly we need resources to be cachable.

Comment 32

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #30)
> Created attachment 8869499 [details] [diff] [review]
> bug_1222633.patch
>
> nsPrefetchService will stop preloads as soon as there as no active loads any
> more. This will cause some of the web-platform tests to fail, because the
> page dispatching reload finish loading. Some test will run fine if run as a
> single test, but fail if I run a whole
> directory(https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=937d9572f092501d592e3134cf57c3a72e23aa90). 
> 
> 

I figured out why this is happening. I have added cache headers so the request that was suppose to prolong the load of a page are served from cache and than loads finishes and cancel preloads. So this is not a problem, the tests are wrong.

Comment 33

[Yoav Weiss]

(In reply to Dragana Damjanovic [:dragana] from comment #30)
> Created attachment 8869499 [details] [diff] [review]
> bug_1222633.patch
> 
> This is a new version.
> 
> I have some questions:
> I have not implemented "media" part. Do we have implementation of its parser
> somewhere?
> 
> I have implemented checking for "type" attribute. I am not sure I have all
> the types listed. For some types, e.g. images, I found helper functions for
> some other I have not found any. If there are some helper functions I have
> not found, please let me know.
> 
> "as" attribute is a bit confusing. The draft says that if a "as" is empty
> string, the preload should be treated as XHR. If "as" attribute is not a
> correct destination, preload should dispatch an error. On the other hand
> there are tests that expect that if you set a incorrect destination "as"
> attribute should be set to empty string, which is putting this 2 cases into
> one. I decided to implement one version for now and we can change this later.

Note that the `as` attribute handling is about to change with regard to empty values and firing of onerror events. See https://groups.google.com/a/chromium.org/d/msg/blink-dev/mKkVXwMhOLc/1X2m8u7HBQAJ

> 
> nsPrefetchService will stop preloads as soon as there as no active loads any
> more. This will cause some of the web-platform tests to fail, because the
> page dispatching reload finish loading. Some test will run fine if run as a
> single test, but fail if I run a whole
> directory(https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=937d9572f092501d592e3134cf57c3a72e23aa90). 
> 
> 
> For now (for firefox55) we will not implement nocache version. If a page
> wants to preload resources it must send a response that is cachable. The
> nocache version is a lot of work that we do not have cycles for at this
> moment.
> 
> Some web-platform tests fail because the tests uses
> PerformanceResourceTiming to check whether load(not preload) uses the
> preloaded resources. Since we store data in the cache and loads from the
> cache have performance entry. The tests expect that loads using preloaded
> resources do not have such a entry. Even if we implement "nocache" version
> of preload this test will failed in the same way (if I remember correctly
> our discussion on how to implement "nocahe" version).
> 
> For missing "as" attribute I set contentPolicy to TYPE_OTHER. Is this
> correct?  testing/web-platform/tests/preload/preload-csp.sub.html
> will fail for that case. I am not sure if the test is not correct or as=""
> should have a different contectType.

Comment 34

[Dragana Damjanovic [:dragana]]

(In reply to Yoav Weiss from comment #33)
> (In reply to Dragana Damjanovic [:dragana] from comment #30)
> > Created attachment 8869499 [details] [diff] [review]
> > bug_1222633.patch
> > 
> > This is a new version.
> > 
> > I have some questions:
> > I have not implemented "media" part. Do we have implementation of its parser
> > somewhere?
> > 
> > I have implemented checking for "type" attribute. I am not sure I have all
> > the types listed. For some types, e.g. images, I found helper functions for
> > some other I have not found any. If there are some helper functions I have
> > not found, please let me know.
> > 
> > "as" attribute is a bit confusing. The draft says that if a "as" is empty
> > string, the preload should be treated as XHR. If "as" attribute is not a
> > correct destination, preload should dispatch an error. On the other hand
> > there are tests that expect that if you set a incorrect destination "as"
> > attribute should be set to empty string, which is putting this 2 cases into
> > one. I decided to implement one version for now and we can change this later.
> 
> Note that the `as` attribute handling is about to change with regard to
> empty values and firing of onerror events. See
> https://groups.google.com/a/chromium.org/d/msg/blink-dev/mKkVXwMhOLc/
> 1X2m8u7HBQAJ
> 


Thanks.

I have follow github issues regarding this.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dragana Damjanovic [:dragana] from comment #30)

Sorry about delay, was on a work week.
 
> I have some questions:
> I have not implemented "media" part. Do we have implementation of its parser
> somewhere?
Do you mean something like
http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/layout/style/MediaList.cpp#75

> Some web-platform tests fail because the tests uses
> PerformanceResourceTiming to check whether load(not preload) uses the
> preloaded resources. Since we store data in the cache and loads from the
> cache have performance entry. The tests expect that loads using preloaded
> resources do not have such a entry. Even if we implement "nocache" version
> of preload this test will failed in the same way (if I remember correctly
> our discussion on how to implement "nocahe" version).
Sounds like invalid tests. Not very rare thing to have ;)

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dragana Damjanovic [:dragana] from comment #30)

> I have implemented checking for "type" attribute. I am not sure I have all
> the types listed. For some types, e.g. images, I found helper functions for
> some other I have not found any. If there are some helper functions I have
> not found, please let me know.

Hmm, StyleLinkElementAudioMimeTypes and StyleLinkElementVideoMimeTypes in nsStyleLinkElement look weird.
I would assume media code to be able to provide something like that. Ask media folks?

Same with StyleLinkElementFontMimeTypes

Comment 37

[Dragana Damjanovic [:dragana]]

Thanks Olli.

Comment 38

[Yoav Weiss]

(In reply to Olli Pettay [:smaug] from comment #35)
> (In reply to Dragana Damjanovic [:dragana] from comment #30)
> 
> > Some web-platform tests fail because the tests uses
> > PerformanceResourceTiming to check whether load(not preload) uses the
> > preloaded resources. Since we store data in the cache and loads from the
> > cache have performance entry. The tests expect that loads using preloaded
> > resources do not have such a entry. Even if we implement "nocache" version
> > of preload this test will failed in the same way (if I remember correctly
> > our discussion on how to implement "nocahe" version).
> Sounds like invalid tests. Not very rare thing to have ;)

The tests in question are there to verify that resources are not double downloading when preloaded.
I understand that Firefox's architecture of caching is different, and that part of the platform is not specified (and it should be, as AnneVK keeps reminding me), and as a result, Firefox is seeing these resources multiple times in ResourceTiming despite the fact that they are eventually only loaded once.

Until the MemoryCache is better specified, we could extend the tests to make sure that the `transferSize` property of the preloaded resources is populated only once. That way they would cover all implementations.

Comment 39

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633.patch

Comment 40

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webplatformtest.patch

Comment 41

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #36)
> (In reply to Dragana Damjanovic [:dragana] from comment #30)
> 
> > I have implemented checking for "type" attribute. I am not sure I have all
> > the types listed. For some types, e.g. images, I found helper functions for
> > some other I have not found any. If there are some helper functions I have
> > not found, please let me know.
> 
> Hmm, StyleLinkElementAudioMimeTypes and StyleLinkElementVideoMimeTypes in
> nsStyleLinkElement look weird.
> I would assume media code to be able to provide something like that. Ask
> media folks?
> 
> Same with StyleLinkElementFontMimeTypes

I have chatted with jya and we can use DecoderTraits::CanHandleContainerType (we do not have something that distinguishes between audio and video, but I think this is just fine).

Fonts do not have such a function, because font loader never relies on the mime type. (I have talked to jfkthame)
I suggested and implemented a list that is currently registered at:
https://www.iana.org/assignments/media-types/media-types.xhtml#font
With this we will not support older (deprecated) types like "application/font-woff". I think this is fine and we should document this.

Comment 42

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webplatformtest.patch

Comment 43

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e95969a75fb46c0fdb1342030718dd2bb53a3d9

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8873539 [details] [diff] [review]
bug_1222633_webplatformtest.patch

I don't understand the changes to delaying-onload-link-preload-after-discovery.html.ini, dynamic-adding-preload.html.ini, fetch-destination.https.html.ini and single-download-late-used-preload.html.ini
Why something times out?

Please explain and add perhaps some comment to .ini files?
Do we need to fix the tests?
I think I need to see explanation before I can r+.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment 30 explained the testing issues, but would be good to have .ini files updated, or perhaps tests fixed. We don't want wpt to have tests for Chrome's implementation details.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8873405 [details] [diff] [review]
bug_1222633.patch

Sorry about delay

>+  if ((aAsTypeMediaUpdate || aCrossOriginUpdate) &&
>+      !(linkTypes & nsStyleLinkElement::ePRELOAD)) {
>+    return;
>+  }
Shouldn't this be more like an assertion


>+  nsString mCrossOriginSet;
Why we need mCrossOriginSet? The element already knows that value in it attributes.
You can check whether the value changes by overriding BeforeSetAttr

>+  nsString type = PromiseFlatString(aType);
Why PromiseFlatString? Why not just assign value to type?
Could you possible split test changes to another patch.

>+nsresult
>+nsPrefetchService::CheckURIs(nsIURI *aURI, nsIURI *aReferrerURI)
Nit, nsIURI*

>+{
>+    //
>+    // XXX we should really be asking the protocol handler if it supports
>+    // caching, so we can determine if there is any value to prefetching.
>+    // for now, we'll only prefetch http links since we know that's the 
>+    // most common case.
We do? Very surprising. I'd guess largest web entities have moved to https already.
But this code just moved. Is the comment even valid anymore?
CheckURIs is super vague name. What does it check? Could you rename


>     nsTArray<nsCOMPtr<nsIWeakReference>>  mSources;
>+    nsContentPolicyType                   mPolicyType;
>+    bool                                  mPreload;
Could you please document that these variables mean.


This is so massive patch that I will need to re-read it several times and do small reviews while reading. Splitting tests to a separate patch would help a bit.

Comment 47

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8873405 [details] [diff] [review]
> bug_1222633.patch
> 
> Sorry about delay
> 
> >+  if ((aAsTypeMediaUpdate || aCrossOriginUpdate) &&
> >+      !(linkTypes & nsStyleLinkElement::ePRELOAD)) {
> >+    return;
> >+  }
> Shouldn't this be more like an assertion
> 

It cannot be because type and media attribute can be associated with other link types like stylesheet.

> 
> >+  nsString mCrossOriginSet;
> Why we need mCrossOriginSet? The element already knows that value in it
> attributes.
> You can check whether the value changes by overriding BeforeSetAttr
> 

Thank, I found a better why to do this. This parameters are removed.

> >+  nsString type = PromiseFlatString(aType);
> Why PromiseFlatString? Why not just assign value to type?

Sorry I used it for debugging and forgot to remove it.

> Could you possible split test changes to another patch.

I have split this into 3 patches, tests, nsPrefetchService and the third one are changes in dom/.
> 
> >+nsresult
> >+nsPrefetchService::CheckURIs(nsIURI *aURI, nsIURI *aReferrerURI)
> Nit, nsIURI*
> 

networking code uses nsIURI *aURI. And this file as well. (uriloader/prefetch/* somehow belongs to necko :)
I will leave it since the whole file uses it.

> >+{
> >+    //
> >+    // XXX we should really be asking the protocol handler if it supports
> >+    // caching, so we can determine if there is any value to prefetching.
> >+    // for now, we'll only prefetch http links since we know that's the 
> >+    // most common case.
> We do? Very surprising. I'd guess largest web entities have moved to https
> already.
> But this code just moved. Is the comment even valid anymore?
> CheckURIs is super vague name. What does it check? Could you rename
> 

The code checks for http and https. I think they use http in comment as a general name for the protocol not scheme.

I will update the comment and the name.

Comment 48

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #47)
> (In reply to Olli Pettay [:smaug] from comment #46)
> > >+{
> > >+    //
> > >+    // XXX we should really be asking the protocol handler if it supports
> > >+    // caching, so we can determine if there is any value to prefetching.
> > >+    // for now, we'll only prefetch http links since we know that's the 
> > >+    // most common case.
> > We do? Very surprising. I'd guess largest web entities have moved to https
> > already.


sorry, I just read the whole come, yes the comment is wrong.

Comment 49

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38dfa753fbd65fbc6b6784f3c0ff6d601d9cfa15

Comment 50

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aa389727b05ec0006b19a50943791035b336d8a

Comment 51

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #44)
> Comment on attachment 8873539 [details] [diff] [review]
> bug_1222633_webplatformtest.patch
> 
> I don't understand the changes to
> delaying-onload-link-preload-after-discovery.html.ini,
> dynamic-adding-preload.html.ini, fetch-destination.https.html.ini and
> single-download-late-used-preload.html.ini
> Why something times out?
> 
> Please explain and add perhaps some comment to .ini files?
> Do we need to fix the tests?
> I think I need to see explanation before I can r+.

I have updates the test. The reason why they are failing is that we preload only resources that are cacheable.

testing/web-platform/tests/preload/fetch-destination.https.html still fails because page onload is fired before the preload resource is fetched. (We stop preloading resources if a event STATE_IS_DOCUMENT and STATE_STOP is fired, this is the same event that triggers onload event). fetch-destination.https.html will pass if it is run separately because at the beginning, when the browse starts, some time we do not get events like STATE_IS_DOCUMENT (this has been like this for a long time, therefore mHaveProcessed variable was introduced long time ago).

testing/web-platform/tests/preload/single-download-late-used-preload.html fails for the same reason. The test preload an image and in the onload event of the preload link it adds the image to the page. We will fire the onload event for the page before onload event of the link and the preload is canceled.

testing/web-platform/tests/content-security-policy/font-src/font-self-allowed.html fails for the same reason.

testing/web-platform/tests/service-workers/service-worker/resources/opaque-response-preloaded-iframe.html fails for the same reason as single-download-late-used-preload.html

Comment 52

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part0_v1.patch

This patch contains changes to nsPrefetchService only.

Comment 53

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part1_v1.patch

This are changes to dom/.

Currently we allow preload of any type of resource, but we can limit that if we change kAsAttributeTable (chrome does not allow everything). The table corresponds to the definition of "as" attribute in the draft.

Comment 54

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part2.patch

This are non web-platform tests.

Comment 55

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webtest.patch

The explanation why some test fail is given in comment 51.

The main changes to the web-platform test is to make resources cacheable. I have added *.headres files. I have also change some uri by adding the name of the test at the end so that caches resources do not interfere between tests. I do not know how to clear the cache and relying on the cache expiration time is too unreliable.

I have also updated web-platform tests according to update https://codereview.chromium.org/2903653005/ which adds the lates modification to "as" attribute (https://github.com/whatwg/html/pull/2588 and ).

Comment 56

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7196115550ca490fa38f76e7ee9f8e67dd06c99c

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876621 [details] [diff] [review]
bug_1222633_part0_v1.patch

>         mHaveProcessed = true;
>-        while (!mQueue.empty() && mCurrentNodes.Length() < static_cast<uint32_t>(mMaxParallelism)) {
>-            ProcessNextURI(nullptr);
>+        // mAggressive mode is used only for testing. If we are in the
>+        // aggressive mode, we will not stop preloads even id page load
even if

>+        // stops.
>+        if (!mAggressive) {
Can we please rename mAggressive to something more descriptive. But no need to do that in this bug.

> nsPrefetchService::StopPrefetching()
> {
>     mStopCount++;
> 
>     LOG(("StopPrefetching [stopcount=%d]\n", mStopCount));
> 
>-    // only kill the prefetch queue if we are actively prefetching right now
>-    if (mCurrentNodes.IsEmpty()) {
>-        return;
>+    if (mStopCount == 1) {
This needs a comment, why call StopAll() only when mStopCount is 1

>+void
>+nsPrefetchService::StopCurrentPrefetchsPreloads(bool aPreload)
>+{
>+    for (int32_t i = mCurrentNodes.Length() - 1; i >= 0; --i) {
>+        if (mCurrentNodes[i]->mPreload == aPreload) {
>+            mCurrentNodes[i]->CancelChannel(NS_BINDING_ABORTED);
>+            mCurrentNodes.RemoveElementAt(i);
>+        }
>     }
> 
>+    if (!aPreload) {
>+        EmptyQueue();
>+    }
This is unclear, because EmptyQueue should be renamed to EmptyPrefetchQueue or such.

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876622 [details] [diff] [review]
bug_1222633_part1_v1.patch

>+Link::UpdatePreload(nsIAtom* aName, const nsAttrValue* aValue,
>+                    const nsAttrValue* aOldValue)
>+  nsAutoString type;
>+  mElement->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type);
here type gets the current type value

>+  } else if (aName == nsGkAtoms::type) {
>+    if (aOldValue) {
>+      aOldValue->ToString(type);
>+    } else {
>+      type = EmptyString();
>+    }
here it is used for the old one. That is rather confusing.
Could we please have some explicit oldType
>+Link::AsValueToContentPolicy(const nsAttrValue& aValue)
>+{
>+  switch(aValue.GetEnumValue()) {
>+  case DESTINATION_INVALID:
>+    return nsIContentPolicy::TYPE_INVALID;
>+  case DESTINATION_AUDIO:
>+  case DESTINATION_TRACK:
>+  case DESTINATION_VIDEO:
>+    return nsIContentPolicy::TYPE_MEDIA;
>+  case DESTINATION_DOCUMENT:
>+    return nsIContentPolicy::TYPE_SUBDOCUMENT;
Why TYPE_SUBDOCUMENT... hmm, perhaps that is ok.

>+IsFontMimeType(const nsAString& aType)
>+{
>+  if (aType.IsVoid() || aType.IsEmpty()) {
>+    return true;
>+  }
void string should be also empty

>+  if ((policyType == nsIContentPolicy::TYPE_SUBDOCUMENT) ||
>+      (policyType == nsIContentPolicy::TYPE_OBJECT) ||
>+      (policyType == nsIContentPolicy::TYPE_WEB_MANIFEST) ||
>+      (policyType == nsIContentPolicy::TYPE_XSLT) ||
>+      (policyType == nsIContentPolicy::TYPE_OTHER)) {
>+    // This 5 should not have a type!
This 5 ? These 5 perhaps?

But where is this behavior defined?


>+void
>+HTMLLinkElement::GetAs(nsAString& aResult)
>+{
>+  GetEnumAttr(nsGkAtoms::as, EmptyCString().get(), aResult);
>+  if (aResult.EqualsASCII("fetch")) {
>+    aResult = EmptyString();
>+  }
I don't see anything requiring this behavior in specs.
Why "fetch" is converted to ""?
Chrome doesn't seem to do that.


>+  void GetAs(nsAString& aResult);
>+  void SetAs(const nsAString& aAs, ErrorResult& aRv)
>+  {
>+    SetOrRemoveNullableStringAttr(nsGkAtoms::as ,aAs, aRv);
>+  }
This looks wrong, since .as isn't nullable.


I think I could take another look.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876623 [details] [diff] [review]
bug_1222633_part2.patch

>+    link.addEventListener("load", () => {
>+      ok(preloaded, "this will heppen only on a preload");
happen

>+// Turn on network.prefetch-next.aggressive, this make writig this tests easier
writing

>+// because it is going to perform preloads even if thereis no corresponding
there is

>+reflectLimitedEnumerated({
>+  element: document.createElement("link"),
>+  attribute: "as",
>+  // "fetch" is a valid value per spec, but gets mapped to the a empty string,
No. Fix this.
'A potential destination is "fetch" or a destination which is not the empty string. '

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dragana Damjanovic [:dragana] from comment #55)

> I have also updated web-platform tests according to update
> https://codereview.chromium.org/2903653005/ which adds the lates
> modification to "as" attribute (https://github.com/whatwg/html/pull/2588 and
> ).
Oh, hmm, hopefully this doesn't make merging w3c wpt to gecko wpt harder.
Usually we just wait the merge to happen to get the tests.

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

And it is hard to review your changes and the changes from https://codereview.chromium.org/2903653005/.
I shouldn't need to review anything coming from https://codereview.chromium.org/2903653005/

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8876627 [details] [diff] [review]
bug_1222633_webtest.patch

>+++ b/testing/web-platform/meta/preload/fetch-destination.https.html.ini
>@@ -1,5 +1,6 @@
> [fetch-destination.https.html]
>   type: testharness
>+  expected: TIMEOUT
>   [Fetch destination preload]
>-    expected: FAIL
>+    expected: TIMEOUT
Could you explain this one?

>+++ b/testing/web-platform/meta/preload/single-download-late-used-preload.html.ini
>@@ -1,5 +1,6 @@
> [single-download-late-used-preload.html]
>   type: testharness
>+  expected: TIMEOUT
>   [Ensure preloaded resources are not downloaded again when used]
>-    expected: FAIL
>+    expected: NOTRUN
and this one

Could I get a patch with just the changes coming to w3c wpt (from google) and then perhaps another patch with your changes.
Otherwise reviewing is really hard.

Comment 63

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part0_v1.patch

addressed comments

Comment 64

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #58)
> Comment on attachment 8876622 [details] [diff] [review]
> bug_1222633_part1_v1.patch

> >+Link::AsValueToContentPolicy(const nsAttrValue& aValue)
> >+{
> >+  switch(aValue.GetEnumValue()) {
> >+  case DESTINATION_INVALID:
> >+    return nsIContentPolicy::TYPE_INVALID;
> >+  case DESTINATION_AUDIO:
> >+  case DESTINATION_TRACK:
> >+  case DESTINATION_VIDEO:
> >+    return nsIContentPolicy::TYPE_MEDIA;
> >+  case DESTINATION_DOCUMENT:
> >+    return nsIContentPolicy::TYPE_SUBDOCUMENT;
> Why TYPE_SUBDOCUMENT... hmm, perhaps that is ok.
> 

I was not sure what to use for destination "document", do you have a better suggestion?

> >+  if ((policyType == nsIContentPolicy::TYPE_SUBDOCUMENT) ||
> >+      (policyType == nsIContentPolicy::TYPE_OBJECT) ||
> >+      (policyType == nsIContentPolicy::TYPE_WEB_MANIFEST) ||
> >+      (policyType == nsIContentPolicy::TYPE_XSLT) ||
> >+      (policyType == nsIContentPolicy::TYPE_OTHER)) {
> >+    // This 5 should not have a type!
> This 5 ? These 5 perhaps?
> 
> But where is this behavior defined?
> 

I do know how correctly to check mime type for this destination. Most of the times we do not have functions for this.
I have added text/xsl for xslt. If you have suggestion how to check them, please let me know, I do not know where to look for this utility functions.

> 
> >+void
> >+HTMLLinkElement::GetAs(nsAString& aResult)
> >+{
> >+  GetEnumAttr(nsGkAtoms::as, EmptyCString().get(), aResult);
> >+  if (aResult.EqualsASCII("fetch")) {
> >+    aResult = EmptyString();
> >+  }
> I don't see anything requiring this behavior in specs.
> Why "fetch" is converted to ""?
> Chrome doesn't seem to do that.
> 

https://fetch.spec.whatwg.org/#miscellaneous
As I understand "fetch" maps to a empty string.

> 
> >+  void GetAs(nsAString& aResult);
> >+  void SetAs(const nsAString& aAs, ErrorResult& aRv)
> >+  {
> >+    SetOrRemoveNullableStringAttr(nsGkAtoms::as ,aAs, aRv);
> >+  }
> This looks wrong, since .as isn't nullable.
> 
> 
> I think I could take another look.

I will change it.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

"The as attribute specifies the potential destination for a preload request for the resource given by the href attribute. It is an enumerated attribute. Each potential destination is a keyword for this attribute, mapping to a state of the same name."

and 
https://fetch.spec.whatwg.org/#concept-potential-destination
"A potential destination
is "fetch" or a destination which is not the empty string. "

Comment 66

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #65)
> "The as attribute specifies the potential destination for a preload request
> for the resource given by the href attribute. It is an enumerated attribute.
> Each potential destination is a keyword for this attribute, mapping to a
> state of the same name."
> 
> and 
> https://fetch.spec.whatwg.org/#concept-potential-destination
> "A potential destination
> is "fetch" or a destination which is not the empty string. "

I was looking at the sentence:
"1. If potentialDestination is "fetch", then return the empty string. "

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

But HTML spec refers to https://fetch.spec.whatwg.org/#concept-potential-destination
And yes, this is confusing.

Comment 68

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part2.patch

Comment 69

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part1_v2.patch

> >+  if ((policyType == nsIContentPolicy::TYPE_SUBDOCUMENT) ||
> >+      (policyType == nsIContentPolicy::TYPE_OBJECT) ||
> >+      (policyType == nsIContentPolicy::TYPE_WEB_MANIFEST) ||
> >+      (policyType == nsIContentPolicy::TYPE_XSLT) ||
> >+      (policyType == nsIContentPolicy::TYPE_OTHER)) {
> >+    // This 5 should not have a type!
> This 5 ? These 5 perhaps?
> 
> But where is this behavior defined?
> 

I do know how correctly to check mime type for this destination. Most of the times we do not have functions for this.
I have added text/xsl for xslt. If you have suggestion how to check them, I would appreciate help, I do not know where to look for this utility functions.

Comment 70

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webtest_part0.patch

web-platform test changes from https://codereview.chromium.org/2903653005/

this only adds fetch in some cases and loads without "as" attribute will fail without an error.

Comment 71

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webtest_part1.patch

Changes to the preload tests
1) make resources cachable
2) append some text to the uris to remove the influence of cached resources from previous tests.

Comment 72

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #62)
> Comment on attachment 8876627 [details] [diff] [review]
> bug_1222633_webtest.patch
> 
> >+++ b/testing/web-platform/meta/preload/fetch-destination.https.html.ini
> >@@ -1,5 +1,6 @@
> > [fetch-destination.https.html]
> >   type: testharness
> >+  expected: TIMEOUT
> >   [Fetch destination preload]
> >-    expected: FAIL
> >+    expected: TIMEOUT
> Could you explain this one?
> 

The load of a page finishes before preload is actually requested. So nsPrefetchService rejects preload because there is no page loading. Preload resource must be for current load.
    if ((!mStopCount && mHaveProcessed) && !mAggressive) {
      LOG(("rejected: there is not load active!"));
      return NS_ERROR_ABORT;
    }


> >+++ b/testing/web-platform/meta/preload/single-download-late-used-preload.html.ini
> >@@ -1,5 +1,6 @@
> > [single-download-late-used-preload.html]
> >   type: testharness
> >+  expected: TIMEOUT
> >   [Ensure preloaded resources are not downloaded again when used]
> >-    expected: FAIL
> >+    expected: NOTRUN
> and this one
> 

It is a similar case:

the test requests a preload of a resource and in the onload event of this preload it adds the same resource to the page, but load of the page ends before preload finishes and the preload gets canceled.

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8881001 [details] [diff] [review]
bug_1222633_webtest_part1.patch

I don't understand what these changes are. I mean, why all the new files and such?

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8881000 [details] [diff] [review]
bug_1222633_webtest_part0.patch

>+<!DOCTYPE html>
>+<script src="/resources/testharness.js"></script>
>+<script src="/resources/testharnessreport.js"></script>
>+<script>
>+test(function() {
>+  var link = document.createElement("link");
>+  var values = {
>+    "Image": "image",
>+    "images": "",
>+    "scripT": "script",
>+    "style": "style",
>+    "": "",
>+    "foNt": "font",
>+    "foobar": "",
>+    "video": "video",
>+    "audio": "audio",
>+    "track": "track",
>+  };
>+  var keys = Object.keys(values);
>+  for (var i = 0; i < keys.length; ++i) {
>+    link.as = keys[i];
>+    assert_true(link.as == values[keys[i]]);
>+  }
>+}, "Make sure that the `as` value reflects only known values");
>+</script>
Please add a check for 'fetch' too

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8881001 [details] [diff] [review]
bug_1222633_webtest_part1.patch

Please explain the changes and ask review again. Why all the new files etc.
Especially when I don't see them being loaded anywhere. What uses A4.mp4?

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8880999 [details] [diff] [review]
bug_1222633_part1_v2.patch


>+  nsCOMPtr<nsIPrefetchService> prefetchService(do_GetService(NS_PREFETCHSERVICE_CONTRACTID));
>+  if (!prefetchService) {
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIURI> uri(GetURI());
>+  if (!uri) {
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIDOMNode> domNode = GetAsDOMNode(mElement);
>+
>+  nsAutoString as;
>+  mElement->GetAttr(kNameSpaceID_None, nsGkAtoms::as, as);
>+  nsAttrValue asAttr;
>+  Link::ParseAsValue(as, asAttr);
>+  nsContentPolicyType asPolicyType = AsValueToContentPolicy(asAttr);
>+
>+  if (asPolicyType == nsIContentPolicy::TYPE_INVALID) {
>+    // Ignore preload with a wrong or empty as attribute, but be sure to cancel
>+    // the old one.
>+    prefetchService->CancelPrefetchPreloadURI(uri, domNode);
>+    return;
>+  }
>+
>+  nsAutoString type;
>+  mElement->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type);
>+  nsAutoString mimeType;
>+  nsAutoString notUsed;
>+  nsContentUtils::SplitMimeType(type, mimeType, notUsed);
>+
>+  nsAutoString media;
>+  mElement->GetAttr(kNameSpaceID_None, nsGkAtoms::media, media);
So there is very similar code both in TryDNSPrefetchOrPreconnectOrPrefetchOrPreloadOrPrerender and UpdatePreload.
would it be possible to refactor that to a helper method?

Comment 77

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #74)
> Comment on attachment 8881000 [details] [diff] [review]
> bug_1222633_webtest_part0.patch
> 
> >+<!DOCTYPE html>
> >+<script src="/resources/testharness.js"></script>
> >+<script src="/resources/testharnessreport.js"></script>
> >+<script>
> >+test(function() {
> >+  var link = document.createElement("link");
> >+  var values = {
> >+    "Image": "image",
> >+    "images": "",
> >+    "scripT": "script",
> >+    "style": "style",
> >+    "": "",
> >+    "foNt": "font",
> >+    "foobar": "",
> >+    "video": "video",
> >+    "audio": "audio",
> >+    "track": "track",
> >+  };
> >+  var keys = Object.keys(values);
> >+  for (var i = 0; i < keys.length; ++i) {
> >+    link.as = keys[i];
> >+    assert_true(link.as == values[keys[i]]);
> >+  }
> >+}, "Make sure that the `as` value reflects only known values");
> >+</script>
> Please add a check for 'fetch' too

I will add this in bug_1222633_webtest_part1.patch, the patch that contains my changes to web-platform tests.

Comment 78

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #69)
 Created attachment 8880999 [details] [diff] [review]
 bug_1222633_part1_v2.patch
 
 > >+  if ((policyType == nsIContentPolicy::TYPE_SUBDOCUMENT) ||
 > >+      (policyType == nsIContentPolicy::TYPE_OBJECT) ||
 > >+      (policyType == nsIContentPolicy::TYPE_WEB_MANIFEST) ||
 > >+      (policyType == nsIContentPolicy::TYPE_XSLT) ||
 > >+      (policyType == nsIContentPolicy::TYPE_OTHER)) {
 > >+    // This 5 should not have a type!
 > This 5 ? These 5 perhaps?
 > 
 > But where is this behavior defined?
 > 
 
 I do know how correctly to check mime type for this destination. Most of the
 times we do not have functions for this.
 I have added text/xsl for xslt. If you have suggestion how to check them, I
 would appreciate help, I do not know where to look for this utility
 functions.

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dragana Damjanovic [:dragana] from comment #77)
> I will add this in bug_1222633_webtest_part1.patch, the patch that contains
> my changes to web-platform tests.
FWIW, it isn't clear to me what part where are coming from chromium or w3c wpt, and what are your changes.

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Dragana Damjanovic [:dragana] from comment #78)
> (In reply to Dragana Damjanovic [:dragana] from comment #69)
>  Created attachment 8880999 [details] [diff] [review]
>  bug_1222633_part1_v2.patch
>  
>  > >+  if ((policyType == nsIContentPolicy::TYPE_SUBDOCUMENT) ||
>  > >+      (policyType == nsIContentPolicy::TYPE_OBJECT) ||
>  > >+      (policyType == nsIContentPolicy::TYPE_WEB_MANIFEST) ||
>  > >+      (policyType == nsIContentPolicy::TYPE_XSLT) ||
>  > >+      (policyType == nsIContentPolicy::TYPE_OTHER)) {
>  > >+    // This 5 should not have a type!
>  > This 5 ? These 5 perhaps?
>  > 
>  > But where is this behavior defined?
>  > 
>  
>  I do know how correctly to check mime type for this destination. Most of the
>  times we do not have functions for this.
>  I have added text/xsl for xslt. If you have suggestion how to check them, I
>  would appreciate help, I do not know where to look for this utility
>  functions.

Well, check do we allow xslt files to be loaded using other mime types too. I assume we do allow any generic xml mimetype too.

Comment 81

[Olli Pettay [:smaug][[e-mail]]]

Maybe we even allow any mimetype for xslt? Peterv might recall.

Comment 82

[Dragana Damjanovic [:dragana]]

Which mime types to use for:
nsIContentPolicy::TYPE_SUBDOCUMENT, nsIContentPolicy::TYPE_OBJECT, nsIContentPolicy::TYPE_WEB_MANIFEST and nsIContentPolicy::TYPE_OTHER (I guess non for TYPE-OTHER)?

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

not sure. Should we accept all? What do other browsers do?

Comment 84

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #83)
> not sure. Should we accept all? What do other browsers do?

Chrome limits to:
"script","style","image","video", "audio", "track", "font", "fetch"

Comment 85

[Olli Pettay [:smaug][[e-mail]]]

I wondered about the mimetypes

Comment 86

[Dragana Damjanovic [:dragana]]

(In reply to Olli Pettay [:smaug] from comment #85)
> I wondered about the mimetypes

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/LinkLoader.cpp?gsn=LoadLink&l=270

For some types it is complicated.

track is only "text/vtt"
font is: font/"woff" || "woff2" || "otf" || "ttf" || "sfnt"
style: "text/css"
script:     "application/ecmascript",
    "application/javascript",
    "application/x-ecmascript",
    "application/x-javascript",
    "text/ecmascript",
    "text/javascript",
    "text/javascript1.0",
    "text/javascript1.1",
    "text/javascript1.2",
    "text/javascript1.3",
    "text/javascript1.4",
    "text/javascript1.5",
    "text/jscript",
    "text/livescript",
    "text/x-ecmascript",
    "text/x-javascript"

fetch: non

media(audio/video): https://cs.chromium.org/chromium/src/media/base/mime_util_internal.cc?gsn=IsSupportedMediaMIMEType&l=481

image: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/network/mime/MIMETypeRegistry.cpp?l=103&gsn=IsSupportedImagePrefixedMIMEType

Comment 87

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ced75e2ac1bcbfb9cf39009fecce7018ac1eeead

Comment 88

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e389e39c99802a901902fa5167b3c76851e15200

Comment 89

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94e4c86e05bda1349a4eecae012b9f58c1b86cc4

Comment 90

[Dragana Damjanovic [:dragana]]

I will upload patches that only accept "script","style","image","video", "audio", "track", "font" and "fetch".

I would suggest that we land that version in this patch and if we need to extend it we can do it in a separate patch (that will not be a lot of work)

Comment 91

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part0a_v1.patch

I read the draft again and there is no requirement to cancel prepload when the load is finished. This patch fixes this in bug_1222633_part0_v1.patch

Comment 92

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part2.patch

Only removes "aggresive" it is not needed any more since bug_1222633_part0a_v1.patch

Comment 93

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part1_v3.patch

only accept as= "script","style","image","video", "audio", "track", "font" and "fetch"

Comment 94

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webtest_part1_v2.patch

Our version of preload only accept resources that are cacheable.
Changes:
1) I needed to add headers for all preloaded resources.
2) Some tests use resources from ./testing/web-platform/tests/media/*. Since I needed to add cache headers for these resources as well and I did not want to interfere with other tests that use the resources from ./testing/web-platform/tests/media/*, I copied these resources to ./testing/web-platform/tests/preload/resources/*. And added headers for these resources as well.
3) Since the resources are cacheable, a cached resource from one test could interfere with following tests. Therefore I added "?<name-of-a-test>" so that the resources' uri differ. (In some tests only onload event of a preload is check and for such tests, the resource can be loaded from cache as well, therefore a "?<name-of-a-test>" is not needed. For other tests the amount of loaded bytes is check and for these tests a "?<name-of-a-test>" is needed)

Comment 95

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8882760 [details] [diff] [review]
bug_1222633_webtest_part1_v2.patch

I assume this didn't use hg cp, but you added new files?
Could you use hg cp.

mostly rs+

Comment 96

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8882752 [details] [diff] [review]
bug_1222633_part1_v3.patch




>+  } else if (policyType == nsIContentPolicy::TYPE_SCRIPT) {
>+    if (aAs.GetEnumValue() != DESTINATION_SCRIPT) {
>+      // serverworker, sharedworker and worker do not have a type.
serviceworker

Comment 97

[Olli Pettay [:smaug][[e-mail]]]

(I'm rather surprised if some followups won't be needed, but better to land sooner and get feedback.)

Comment 98

[Ruvim Pinka]

(In reply to Dragana Damjanovic [:dragana] from comment #93)
> 
> only accept as= "script","style","image","video", "audio", "track", "font"
> and "fetch"

By the way, why do you constrain content-type for "style" by "text/css" only? (in nsStyleLinkElement::CheckPreloadAttrs function)
It seems that xml-stylesheet also falls into "style" category (which actually XSLT file with with several possible content types).

Also, could you please clarify why do you restrict content-type at all?

Citation from https://w3c.github.io/preload/
"The preload link provides a low-level and content-type agnostic primitive"

Comment 99

[Dragana Damjanovic [:dragana]]

(In reply to Ruvim Pinka from comment #98)
> (In reply to Dragana Damjanovic [:dragana] from comment #93)
> > 
> > only accept as= "script","style","image","video", "audio", "track", "font"
> > and "fetch"
> 
> By the way, why do you constrain content-type for "style" by "text/css"
> only? (in nsStyleLinkElement::CheckPreloadAttrs function)
> It seems that xml-stylesheet also falls into "style" category (which
> actually XSLT file with with several possible content types).
> 

We support only "style" for now. xslt is not supported. Please file a sepparate bug if it is needed. Chrome does not support it either.

> Also, could you please clarify why do you restrict content-type at all?
> 
> Citation from https://w3c.github.io/preload/
> "The preload link provides a low-level and content-type agnostic primitive"

look at 2.1 bullet point starting with "When the type attribute of..."

Comment 100

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_part1_v3.patch

Comment 101

[Dragana Damjanovic [:dragana]]

Attachment: bug_1222633_webtest_part1_v2.patch

Comment 102

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e394b0cba4
Add rel=preload - nsPrefetchService part. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2f2fb3235e
Web tests changes that incorporate new semantics of "as" attribute. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed771a73112
nsPrefetchService should not cancel rel=preload if the load ends. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd70042c05c8
Add rel=preload - tests. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c324e7db4c
Add rel=preload - dom part. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6451b2ea64
Web tests for rel=preload: 1) make resources cachable, 2) append some text to the uris to remove the influence of cached resources from previous tests. r=smaug

Comment 103

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0e394b0cba4
https://hg.mozilla.org/mozilla-central/rev/8c2f2fb3235e
https://hg.mozilla.org/mozilla-central/rev/aed771a73112
https://hg.mozilla.org/mozilla-central/rev/bd70042c05c8
https://hg.mozilla.org/mozilla-central/rev/09c324e7db4c
https://hg.mozilla.org/mozilla-central/rev/ce6451b2ea64

Comment 104

[Boris Zbarsky [:bzbarsky]]

So does this implementation just depend on the HTTP cache?

I was under the impression that the point of the "as" attribute was to enable dispatch to the right loader, so the actual data received was retained on a per-document basis and used if the real load request came through....

Comment 105

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (vacation until 7/7) from comment #104)
> So does this implementation just depend on the HTTP cache?
> 

Yes.

> I was under the impression that the point of the "as" attribute was to
> enable dispatch to the right loader, so the actual data received was
> retained on a per-document basis and used if the real load request came
> through....

We do not support "per-document basis" for now. This is a lot of work that we cannot promise before 57. Therefore we decided to use cache and to preload only cacheable requests for now. "as" attribute is used to set proper flags and to enable proper security checks. The responses will be stored in cache and a right loaded will be involved only when the real loads come through. (for example facebook preloads cacheable resources, so this will work for facebook).

Comment 106

[Boris Zbarsky [:bzbarsky]]

OK.  Is there a followup bug or bugs for doing the rest of the work?

Comment 107

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (vacation until 7/7) from comment #106)
> OK.  Is there a followup bug or bugs for doing the rest of the work?

Currently there is no followup bug, but I insure you that this is in out back-log list of future work( after 57 or before if we have have some engineering cycles free, but not promising). It will not be forgotten.

Comment 108

[Dragana Damjanovic [:dragana]]

Just a short summary for documentation:

we support as= "script","style","image","video", "audio", "track", "font" and "fetch"

for fonts we only support mime types from list
https://www.iana.org/assignments/media-types/media-types.xhtml#font except for font/collection which firefox does not support.

Comment 109

[Chris Mills [:cmills]]

I've documented this feature.

First of all, I've updated the following ref pages:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link
https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement

I've also written a guide to how it is used:

https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content

finally, I've added a note to the Fx56 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/56

I've love a tech review of these...thanks!

Comment 110

[Anne (:annevk)]

Dragana, would you mind sending a belated "Intent to Ship" to dev-platform for this feature as per https://wiki.mozilla.org/WebAPI/ExposureGuidelines? Thanks!

Comment 111

[Dragana Damjanovic [:dragana]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #109)
> I've documented this feature.
> 
> First of all, I've updated the following ref pages:
> 
> https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link
> https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement
> 
> I've also written a guide to how it is used:
> 
> https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content
> 
> finally, I've added a note to the Fx56 rel notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/56
> 
> I've love a tech review of these...thanks!

Can you add that Firefox support only preloading of resources that are cacheable. (chrome supports no-cache as well)

Comment 112

[itkachev]

Currently when trying to preload non-cacheable scripts (e.g., a JSONP API request) the request will be sent twice.

This seems like a serious bug to me.

Comment 113

[Dragana Damjanovic [:dragana]]

(In reply to itkachev from comment #112)
> Currently when trying to preload non-cacheable scripts (e.g., a JSONP API
> request) the request will be sent twice.
> 
> This seems like a serious bug to me.

Currently we only support cacheable resource and to check if a resource is cacheable we need to send a request. So preload sends a request and find out that we will not store this resource so it cancel it and when the page want to get the resource for real(not a preload) it will send the request again.

This is the current implementation non-cacheable resource should not be requested with rel=preload.

Comment 114

[Boris Zbarsky [:bzbarsky]]

> This is the current implementation non-cacheable resource should not be requested with rel=preload.

I should note that this is not a reasonable thing to say, because other browsers do support non-cacheable resources here and it makes sense to preload them there.

Comment 115

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #114)
> > This is the current implementation non-cacheable resource should not be requested with rel=preload.
> 
> I should note that this is not a reasonable thing to say, because other
> browsers do support non-cacheable resources here and it makes sense to
> preload them there.

Sorry for not being precise enough, but i did talk about "we" in the previous sentence.

Firefox currently only supports preloading of cacheable resources.

Comment 116

[itkachev]

> This is the current implementation non-cacheable resource should not be requested with rel=preload.

At the very least, there should be a big, bold flashing notice about this in the documentation.

A valid and common use-case for preloading is for making various Web API's faster, and currently under Firefox this will double server load and generate suspicious-looking traffic. 

(Non-cacheable scripts are usually dynamically generated on the server using complex backends.)

Comment 117

[Dragana Damjanovic [:dragana]]

(In reply to Dragana Damjanovic [:dragana] from comment #111)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #109)
> > I've documented this feature.
> > 
> > First of all, I've updated the following ref pages:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link
> > https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types
> > https://developer.mozilla.org/en-US/docs/Web/API/HTMLLinkElement
> > 
> > I've also written a guide to how it is used:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content
> > 
> > finally, I've added a note to the Fx56 rel notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/56
> > 
> > I've love a tech review of these...thanks!
> 
> Can you add that Firefox support only preloading of resources that are
> cacheable. (chrome supports no-cache as well)

Can you please update the docs. Thanks a lot.

Comment 118

[Anthony Ricaud (:rik)]

There's a lot of already shipping code out there that might rely on preload working for non-cacheable resources. Have you done some measurements if it’s improving or degrading performance of real websites?

Could shipping this be delayed until non-cacheable resource are supported? Even with documentation, this might be very confusing.

Comment 119

[Ruvim Pinka]

Does this bug-report cover the case of preload in HTTP Link header? Or should we file a separate bug for the case of HTTP header?

Comment 120

[Chris Mills [:cmills]]

> (In reply to Dragana Damjanovic [:dragana] from comment #111)
> 
> Can you please update the docs. Thanks a lot.

Oops, sorry for missing this! I've added this detail in all appropriate places.

Comment 121

[Raphaël Droz]

Is as=fetch the right attribute to preload JSON data file?

Comment 122

[Boris Zbarsky [:bzbarsky]]

Is there a reason this all got placed in nsStyleLinkElement instead of HTMLLinkElement?  I'd think only the latter really does preloads...

Comment 123

[Olli Pettay [:smaug][[e-mail]]]

I guess no particular reason. oversight from my side?

Comment 124

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #122)
> Is there a reason this all got placed in nsStyleLinkElement instead of
> HTMLLinkElement?  I'd think only the latter really does preloads...

No particular reason, probably just me not knowing dom code well enough.

Comment 125

[Boris Zbarsky [:bzbarsky]]

OK, I filed bug 1441246 to move this code to the right place.

Comment 126

[709922234]

Dynamically added link element does not work



```
link = document.createElement('link');
link.href = './test1.js';
link.as = 'script';
link.rel = "preload"
document.head.append(link);
```

Comment 127

[709922234]

(In reply to 709922234 from comment #126)
> Dynamically added link element does not work
> 
> 
> 
> ```
> link = document.createElement('link');
> link.href = './test1.js';
> link.as = 'script';
> link.rel = "preload"
> document.head.append(link);
> ```

https://www.w3.org/TR/preload/#developer-server-and-proxy-initiated-fetching

Comment 128

[Boris Zbarsky [:bzbarsky]]

I filed bug 1447394.  In general, commenting in long-closed bugs about followup issues is not very useful, because chances are people won't even see it.

Comment 129

[Dragana Damjanovic [:dragana]]

(In reply to 709922234 from comment #126)
> Dynamically added link element does not work
> 
> 
> 
> ```
> link = document.createElement('link');
> link.href = './test1.js';
> link.as = 'script';
> link.rel = "preload"
> document.head.append(link);
> ```

rel=preload is disabled on firefox. Have you enabled it?

there is a web-platfom test for this:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/preload/dynamic-adding-preload.html

Comment 130

[709922234]

(In reply to Dragana Damjanovic [:dragana] from comment #129)
> (In reply to 709922234 from comment #126)
> > Dynamically added link element does not work
> > 
> > 
> > 
> > ```
> > link = document.createElement('link');
> > link.href = './test1.js';
> > link.as = 'script';
> > link.rel = "preload"
> > document.head.append(link);
> > ```
> 
> rel=preload is disabled on firefox. Have you enabled it?
> 
> there is a web-platfom test for this:
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/
> preload/dynamic-adding-preload.html

yes. i not enabled it.

Retest, meet expectations

Comment 131

[Peter Van der Beken [:peterv]]

(In reply to Olli Pettay [:smaug] from comment #81)

Maybe we even allow any mimetype for xslt? Peterv might recall.

IIIRC we force a mimetype, ignoring what comes from the network.