Closed Bug 1222633 Opened 9 years ago Closed 7 years ago

Add support for <link rel=preload>

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Performance Impact high
Tracking Status
firefox56 --- fixed

People

(Reporter: igrigorik, Assigned: dragana)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-active])

Attachments

(6 files, 17 obsolete files)

35.73 KB, patch
dragana
: review+
Details | Diff | Splinter Review
9.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.05 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.16 KB, patch
dragana
: review+
Details | Diff | Splinter Review
40.77 KB, patch
dragana
: review+
Details | Diff | Splinter Review
63.66 KB, patch
dragana
: review+
Details | Diff | Splinter Review
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
Component: Untriaged → Networking
Product: Firefox → Core
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
Component: Networking → DOM: Core & HTML
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
Priority: -- → P2
Blocks: 1229634
Chrome 50 already has this, already leveraged by many sites to load lot faster.
+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?
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)
I will take it.
Flags: needinfo?(dd.mozilla)
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
Attached patch bug_1222633.patch (obsolete) — Splinter Review
Attached patch bug_1222633.patch (obsolete) — Splinter Review
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.
Attachment #8784806 - Attachment is obsolete: true
Attachment #8784807 - Flags: feedback?(bugs)
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.
(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.
(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 :)
(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);
  }
}
Attached patch bug_1222633.patch (obsolete) — Splinter Review
Attachment #8784807 - Attachment is obsolete: true
Attachment #8784807 - Flags: feedback?(bugs)
Attachment #8784894 - Flags: feedback?(bugs)
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 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)
Attachment #8784894 - Flags: feedback?(bugs) → feedback+
(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)
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs) → needinfo?(bkelly)
(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.
Flags: needinfo?(bkelly)
Any recent progress on preload support?
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").
Whiteboard: [qf]
Flags: needinfo?(jduell.mcbugs)
I will try to find time to work on this, targeting ff55
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.
Flags: needinfo?(kvijayan)
Whiteboard: [qf] → [qf:p1]
Facebook is already using rel=preload for CSS and JS.
Flags: needinfo?(kvijayan)
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 :)
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [qf:p1] → [qf:p1][necko-active]
This is probably already known, but preload can also be used in a http header. ie not just in the html response
(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.
(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.
That will be really great to have preload working on Firefox... Hope to see that soon!
Attached patch bug_1222633.patch (obsolete) — Splinter Review
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.
Attachment #8869499 - Flags: feedback?(bugs)
Attached patch bug_1222633_webtest.patch (obsolete) — Splinter Review
This are some changes to the web-platfom tests. Mainly we need resources to be cachable.
Attachment #8784894 - Attachment is obsolete: true
(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.
(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.
(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.
(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 ;)
(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
Attachment #8869499 - Flags: feedback?(bugs)
Thanks Olli.
(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.
Attached patch bug_1222633.patch (obsolete) — Splinter Review
Attachment #8869499 - Attachment is obsolete: true
Attachment #8873405 - Flags: review?(bugs)
Attachment #8869500 - Attachment is obsolete: true
Attachment #8873406 - Flags: review?(bugs)
(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.
Attachment #8873406 - Attachment is obsolete: true
Attachment #8873406 - Flags: review?(bugs)
Attachment #8873539 - Flags: review?(bugs)
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+.
Attachment #8873539 - Flags: review?(bugs) → review-
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 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.
Attachment #8873405 - Flags: review?(bugs) → review-
(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.
(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.
(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
Attached patch bug_1222633_part0_v1.patch (obsolete) — Splinter Review
This patch contains changes to nsPrefetchService only.
Attachment #8873405 - Attachment is obsolete: true
Attachment #8876621 - Flags: review?(bugs)
Attached patch bug_1222633_part1_v1.patch (obsolete) — Splinter Review
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.
Attachment #8876622 - Flags: review?(bugs)
Attached patch bug_1222633_part2.patch (obsolete) — Splinter Review
This are non web-platform tests.
Attachment #8876623 - Flags: review?(bugs)
Attached patch bug_1222633_webtest.patch (obsolete) — Splinter Review
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 ).
Attachment #8873539 - Attachment is obsolete: true
Attachment #8876627 - Flags: review?(bugs)
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.
Attachment #8876621 - Flags: review?(bugs) → review+
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.
Attachment #8876622 - Flags: review?(bugs) → review-
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. '
Attachment #8876623 - Flags: review?(bugs) → review+
(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.
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 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.
Attachment #8876627 - Flags: review?(bugs) → review-
addressed comments
Attachment #8876621 - Attachment is obsolete: true
Attachment #8879055 - Flags: review+
(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.
Flags: needinfo?(bugs)
"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. "
Flags: needinfo?(bugs)
(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. "
Attached patch bug_1222633_part2.patch (obsolete) — Splinter Review
Attachment #8876623 - Attachment is obsolete: true
Attachment #8880998 - Flags: review+
Attached patch bug_1222633_part1_v2.patch (obsolete) — Splinter Review
> >+  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.
Attachment #8876622 - Attachment is obsolete: true
Attachment #8880999 - Flags: feedback?(bugs)
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.
Attachment #8876627 - Attachment is obsolete: true
Attachment #8881000 - Flags: review?(bugs)
Attached patch bug_1222633_webtest_part1.patch (obsolete) — Splinter Review
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.
Attachment #8881001 - Flags: review?(bugs)
(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 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 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
Attachment #8881000 - Flags: review?(bugs) → review+
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?
Attachment #8881001 - Flags: review?(bugs)
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?
Attachment #8880999 - Flags: feedback?(bugs) → feedback+
(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.
(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.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
(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.
Maybe we even allow any mimetype for xslt? Peterv might recall.
Flags: needinfo?(peterv)
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)?
not sure. Should we accept all? What do other browsers do?
(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"
I wondered about the mimetypes
(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
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)
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
Attachment #8882748 - Flags: review?(bugs)
Only removes "aggresive" it is not needed any more since bug_1222633_part0a_v1.patch
Attachment #8880998 - Attachment is obsolete: true
Attachment #8882750 - Flags: review+
Attached patch bug_1222633_part1_v3.patch (obsolete) — Splinter Review
only accept as= "script","style","image","video", "audio", "track", "font" and "fetch"
Attachment #8880999 - Attachment is obsolete: true
Attachment #8882752 - Flags: review?(bugs)
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)
Attachment #8881001 - Attachment is obsolete: true
Attachment #8882760 - Flags: review?(bugs)
Attachment #8882748 - Flags: review?(bugs) → review+
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+
Attachment #8882760 - Flags: review?(bugs) → review+
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
Attachment #8882752 - Flags: review?(bugs) → review+
(I'm rather surprised if some followups won't be needed, but better to land sooner and get feedback.)
(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"
(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..."
Attachment #8882752 - Attachment is obsolete: true
Attachment #8884813 - Flags: review+
Attachment #8882760 - Attachment is obsolete: true
Attachment #8884814 - Flags: review+
Pushed by dd.mozilla@gmail.com:
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
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....
Flags: needinfo?(dd.mozilla)
(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).
Flags: needinfo?(dd.mozilla)
OK.  Is there a followup bug or bugs for doing the rest of the work?
(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.
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.
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!
Flags: needinfo?(dd.mozilla)
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!
(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)
Flags: needinfo?(dd.mozilla)
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.
(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.
> 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.
(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.
> 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.)
(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.
Flags: needinfo?(cmills)
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.
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?
> (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.
Flags: needinfo?(cmills)
Depends on: 1405761
Is as=fetch the right attribute to preload JSON data file?
Depends on: 1419848
Is there a reason this all got placed in nsStyleLinkElement instead of HTMLLinkElement?  I'd think only the latter really does preloads...
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(bugs)
I guess no particular reason. oversight from my side?
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(dd.mozilla)
Depends on: 1441246
OK, I filed bug 1441246 to move this code to the right place.
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);
```
(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
Depends on: 1447394
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.
(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
Flags: needinfo?(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
Flags: needinfo?(709922234)
Depends on: 1394778
See Also: → 1515760

(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.

Flags: needinfo?(peterv)
Blocks: 1419848
No longer depends on: 1419848
Performance Impact: --- → P1
Whiteboard: [qf:p1][necko-active] → [necko-active]
Depends on: 1765884
You need to log in before you can comment on or make changes to this bug.