Add support for <link rel=preload>

RESOLVED FIXED in Firefox 56
(Needinfo from 2 people)

Status

()

Core
DOM: Core & HTML
P2
enhancement
RESOLVED FIXED
2 years ago
7 days ago

People

(Reporter: Ilya Grigorik, Assigned: dragana, NeedInfo)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla56
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1][necko-active], URL)

Attachments

(6 attachments, 17 obsolete attachments)

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
(Reporter)

Description

2 years ago
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

Updated

2 years ago
Component: Untriaged → Networking
Product: Firefox → Core

Updated

2 years ago
Severity: normal → enhancement
OS: Unspecified → All
Hardware: Unspecified → All
Component: Networking → DOM: Core & HTML

Comment 1

2 years ago
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

Updated

2 years ago
Priority: -- → P2

Updated

2 years ago
Blocks: 1229634

Comment 2

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

Comment 3

a year ago
+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)
(Assignee)

Comment 5

a year ago
I will take it.
Flags: needinfo?(dd.mozilla)
(Assignee)

Updated

a year ago
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24d2ba7c9579
(Assignee)

Comment 7

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

Updated

11 months ago
Keywords: dev-doc-needed
(Assignee)

Comment 8

11 months ago
Created attachment 8784806 [details] [diff] [review]
bug_1222633.patch
(Assignee)

Comment 9

11 months ago
Created attachment 8784807 [details] [diff] [review]
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.
Attachment #8784806 - Attachment is obsolete: true
Attachment #8784807 - Flags: feedback?(bugs)
(Reporter)

Comment 10

11 months ago
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.
(Assignee)

Comment 11

11 months ago
(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.
(Reporter)

Comment 12

11 months ago
(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 :)
(Assignee)

Comment 13

11 months ago
(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);
  }
}
(Assignee)

Comment 14

11 months ago
Created attachment 8784894 [details] [diff] [review]
bug_1222633.patch
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+
(Assignee)

Comment 17

11 months ago
(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)

Comment 19

11 months ago
(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)

Comment 20

5 months ago
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)
(Assignee)

Comment 22

4 months ago
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)

Comment 25

3 months ago
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]

Comment 26

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

Comment 27

3 months ago
(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.
(Assignee)

Comment 28

3 months ago
(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

3 months ago
That will be really great to have preload working on Firefox... Hope to see that soon!
(Assignee)

Comment 30

2 months ago
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.

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)
(Assignee)

Comment 31

2 months ago
Created attachment 8869500 [details] [diff] [review]
bug_1222633_webtest.patch

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

Updated

2 months ago
Attachment #8784894 - Attachment is obsolete: true
(Assignee)

Comment 32

2 months ago
(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

2 months ago
(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.
(Assignee)

Comment 34

2 months ago
(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)
(Assignee)

Comment 37

2 months ago
Thanks Olli.

Comment 38

2 months ago
(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.
(Assignee)

Comment 39

2 months ago
Created attachment 8873405 [details] [diff] [review]
bug_1222633.patch
Attachment #8869499 - Attachment is obsolete: true
Attachment #8873405 - Flags: review?(bugs)
(Assignee)

Comment 40

2 months ago
Created attachment 8873406 [details] [diff] [review]
bug_1222633_webplatformtest.patch
Attachment #8869500 - Attachment is obsolete: true
Attachment #8873406 - Flags: review?(bugs)
(Assignee)

Comment 41

2 months ago
(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.
(Assignee)

Comment 42

2 months ago
Created attachment 8873539 [details] [diff] [review]
bug_1222633_webplatformtest.patch
Attachment #8873406 - Attachment is obsolete: true
Attachment #8873406 - Flags: review?(bugs)
Attachment #8873539 - Flags: review?(bugs)
(Assignee)

Comment 43

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e95969a75fb46c0fdb1342030718dd2bb53a3d9
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-
(Assignee)

Comment 47

2 months ago
(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.
(Assignee)

Comment 48

2 months ago
(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.
(Assignee)

Comment 49

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38dfa753fbd65fbc6b6784f3c0ff6d601d9cfa15
(Assignee)

Comment 50

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aa389727b05ec0006b19a50943791035b336d8a
(Assignee)

Comment 51

2 months ago
(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
(Assignee)

Comment 52

2 months ago
Created attachment 8876621 [details] [diff] [review]
bug_1222633_part0_v1.patch

This patch contains changes to nsPrefetchService only.
Attachment #8873405 - Attachment is obsolete: true
Attachment #8876621 - Flags: review?(bugs)
(Assignee)

Comment 53

2 months ago
Created attachment 8876622 [details] [diff] [review]
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.
Attachment #8876622 - Flags: review?(bugs)
(Assignee)

Comment 54

2 months ago
Created attachment 8876623 [details] [diff] [review]
bug_1222633_part2.patch

This are non web-platform tests.
Attachment #8876623 - Flags: review?(bugs)
(Assignee)

Comment 55

2 months ago
Created attachment 8876627 [details] [diff] [review]
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 ).
Attachment #8873539 - Attachment is obsolete: true
Attachment #8876627 - Flags: review?(bugs)
(Assignee)

Comment 56

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7196115550ca490fa38f76e7ee9f8e67dd06c99c
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-
(Assignee)

Comment 63

a month ago
Created attachment 8879055 [details] [diff] [review]
bug_1222633_part0_v1.patch

addressed comments
Attachment #8876621 - Attachment is obsolete: true
Attachment #8879055 - Flags: review+
(Assignee)

Comment 64

a month ago
(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)
(Assignee)

Comment 66

a month ago
(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. "
But HTML spec refers to https://fetch.spec.whatwg.org/#concept-potential-destination
And yes, this is confusing.
(Assignee)

Comment 68

a month ago
Created attachment 8880998 [details] [diff] [review]
bug_1222633_part2.patch
Attachment #8876623 - Attachment is obsolete: true
Attachment #8880998 - Flags: review+
(Assignee)

Comment 69

a month ago
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.
Attachment #8876622 - Attachment is obsolete: true
Attachment #8880999 - Flags: feedback?(bugs)
(Assignee)

Comment 70

a month ago
Created attachment 8881000 [details] [diff] [review]
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.
Attachment #8876627 - Attachment is obsolete: true
Attachment #8881000 - Flags: review?(bugs)
(Assignee)

Comment 71

a month ago
Created attachment 8881001 [details] [diff] [review]
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.
Attachment #8881001 - Flags: review?(bugs)
(Assignee)

Comment 72

a month ago
(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+
(Assignee)

Comment 77

a month ago
(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.
(Assignee)

Comment 78

a month ago
(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)
(Assignee)

Comment 82

a month ago
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?
(Assignee)

Comment 84

28 days ago
(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
(Assignee)

Comment 86

28 days ago
(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
(Assignee)

Comment 87

27 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ced75e2ac1bcbfb9cf39009fecce7018ac1eeead
(Assignee)

Comment 88

27 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e389e39c99802a901902fa5167b3c76851e15200
(Assignee)

Comment 89

27 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=94e4c86e05bda1349a4eecae012b9f58c1b86cc4
(Assignee)

Comment 90

27 days ago
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)
(Assignee)

Comment 91

27 days ago
Created attachment 8882748 [details] [diff] [review]
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
Attachment #8882748 - Flags: review?(bugs)
(Assignee)

Comment 92

27 days ago
Created attachment 8882750 [details] [diff] [review]
bug_1222633_part2.patch

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+
(Assignee)

Comment 93

27 days ago
Created attachment 8882752 [details] [diff] [review]
bug_1222633_part1_v3.patch

only accept as= "script","style","image","video", "audio", "track", "font" and "fetch"
Attachment #8880999 - Attachment is obsolete: true
Attachment #8882752 - Flags: review?(bugs)
(Assignee)

Comment 94

27 days ago
Created attachment 8882760 [details] [diff] [review]
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)
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.)

Comment 98

18 days ago
(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"
(Assignee)

Comment 99

17 days ago
(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..."
(Assignee)

Comment 100

17 days ago
Created attachment 8884813 [details] [diff] [review]
bug_1222633_part1_v3.patch
Attachment #8882752 - Attachment is obsolete: true
Attachment #8884813 - Flags: review+
(Assignee)

Comment 101

17 days ago
Created attachment 8884814 [details] [diff] [review]
bug_1222633_webtest_part1_v2.patch
Attachment #8882760 - Attachment is obsolete: true
Attachment #8884814 - Flags: review+

Comment 102

17 days ago
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

Comment 103

17 days ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 17 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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)
(Assignee)

Comment 105

17 days ago
(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?
(Assignee)

Comment 107

17 days ago
(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.
(Assignee)

Comment 108

9 days ago
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)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.