Skip to content

Conversation

@yoavweiss
Copy link
Contributor

This adds preload as a body-ok, link-enabled link type. I mostly followed @domenic's pattern from the addition of the various resource hints link types, but let me know if I'm doing it wrong, or if you think this is a bad idea.

/cc @igrigorik

<td><code data-x="rel-preload">preload</code></td>
<td><span data-x="external resource link">External Resource</span></td>
<td><em>not allowed</em></td>
<td class="yes"> Yes </td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify what "for current navigation" and "according to specified destination and priority" mean?

It's notable that these are the only phrases in the spec that seem to distinguish preload from prefetch, so it'd be worthwhile to be clear about them.

@domenic
Copy link
Member

domenic commented Jun 13, 2016

What is the status of implementer interest in this rel? Specifically, is it implemented or are there active plans for implementation in 2+ engines?

@domenic domenic added the addition/proposal New features or enhancements label Jun 13, 2016
@yoavweiss
Copy link
Contributor Author

Thanks for the comments. I'll fix things up and clarify the differences with prefetch.
Regarding implementer interest, this is already implemented in Blink and I'm currently working on a WebKit implementation. (@sideshowbarker helpfully added a meta-issue for the feature)

Firefox have an open bug marked as Priority 2 enhancement, and MS Edge marked the feature as likely to be implemented in the future.

@yoavweiss
Copy link
Contributor Author

Review comments (hopefully) addressed. PTAL

@igrigorik
Copy link
Member

I'll defer editorial knits to @domenic, but otherwise lgtm!

@zcorpan
Copy link
Member

zcorpan commented Jun 14, 2016

Shouldn't this also add the as attribute?

@sideshowbarker
Copy link
Member

Shouldn't this also add the as attribute?

Yeah I think it should. Either that or we should have an accompanying PR for adding it. If so I’d be happy to volunteer to write a separate one up for that if needed—but equally happy to just help review it if it gets added here or in another PR

@sideshowbarker
Copy link
Member

LGTM for purposes of what I need as far as adding support for rel=preload to the HTML checker (as well as the refinements to rel=prefetch also included in the patch). (And I have actually already gone ahead and added rel=preload support to the HTML checker.)

<p>The <code data-x="rel-prefetch">prefetch</code> keyword indicates that preemptively fetching and
caching the specified resource is likely to be beneficial, as it is highly likely that the user
will require this resource. <span w-nodev>User agents must implement the processing model of the
will require this resource for future navigations. <span w-nodev>User agents must implement the processing model of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to rewrap to 100 chars

@domenic
Copy link
Member

domenic commented Jun 14, 2016

I'll fix the line wrapping and merge this myself. A follow-up PR adding in the as attribute would definitely be a good idea.

@domenic
Copy link
Member

domenic commented Jun 14, 2016

I was about to land this, but I had a question: I never got clarification on what "according to the specified destination and priority" means. I can guess that maybe "specified destination" is related to the missing as attribute, but I don't see the word "priority" used much in the preload spec at all.

@yoavweiss
Copy link
Contributor Author

I added links to the definition of destination and priority in fetch. The specified destination is indeed related to as and priority derives from that.

If you wish I could add as to this PR, and make sure that everything is well defined. Otherwise, I can add it in a followup PR and tie the "specified destination" phrase to that definition.

@domenic
Copy link
Member

domenic commented Jun 14, 2016

Ah OK. I can merge this without adding as; I'll just change the phrase to "specified destination (and associated priority)"

@yoavweiss
Copy link
Contributor Author

OK, cool
On Jun 14, 2016 7:02 PM, "Domenic Denicola" notifications@github.com
wrote:

Ah OK. I can merge this without adding as; I'll just change the phrase to
"specified destination (and associated priority)"


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1418 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAv_C1nVKFlpC5zumK6LF4dm6AO9YpHHks5qLt6LgaJpZM4I0gvQ
.

domenic pushed a commit that referenced this pull request Jun 14, 2016
@domenic
Copy link
Member

domenic commented Jun 14, 2016

Merged as d013cd1; woohoo! Thanks so much for adding this. It's really great for both authors and implementers to have these definitions centralized.

@domenic domenic closed this Jun 14, 2016
@sideshowbarker
Copy link
Member

I went ahead and created #1449 for the as attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addition/proposal New features or enhancements topic: link

Development

Successfully merging this pull request may close these issues.

5 participants