166647 - allow link prefetching from a META tag

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Darin Fisher]

allow link prefetching from a META tag:

e.g., 

 <meta HTTP-EQUIV="link" CONTENT="rel=next href=http://foo/bar">

Comment 1

[Bill Mason]

FWIW: This might be better done from a LINK tag:
<link href="foo.html" rel="next" title="next page" />

Such functionality is already in the HTML 4.01 spec:
"Next: Refers to the next document in a linear sequence of documents. User
agents may choose to preload the "next" document, to reduce the perceived load
time."
<http://www.w3.org/TR/html4/types.html#type-links>

AOL TV already does this, and I think WebTV/MSN TV does it as well.

Comment 2

[Darin Fisher]

so does mozilla... see bug 12274 :-)

mozilla also looks for rel=next in HTTP Link: headers, but mozilla fails to pick
up HTTP Link headers specified in HTML meta tags.  that's what this bug is about.

Comment 3

[Hixie (not reading bugmail)]

Note the syntax given in the initial description is incorrect. It should be the
same syntax as for the header, namely

   <meta http-equiv="Link"
         content='&lt;foo.html&gt;; rel=next, &lt;foo.css&gt;; rel=stylesheet; 
                  type="text/css"'>

...for a link equivalent to:

   Link: <foo.html>; rel=next, <foo.css>; rel=stylesheet; type="text/css"

...which is equivalent to:

   <link href="foo.html" rel=next>
   <link href="foo.css" rel=stylesheet type="text/css">

I am assuming you did use the right syntax to parse the Link headers... (i.e.
you used the same code as we use for stylesheet linking... right?)


This bug would be a great opportunity to unify the link handling in Mozilla and
use a single interface to get all the links out of a document (and to notify
observers when links are added or removed). Links are currently implemented
independently, each with a different set of bugs, for stylesheet importing, the
link toolbar, site icons, and prefetching. It's a bit sad really.

Comment 4

[Darin Fisher]

yup, in fact it looks like i really biffed the syntax for Link headers :(

Comment 5

[Hixie (not reading bugmail)]

doh. Another reason to unify the handling! :-)

I suggest finding the stylesheet import HTTP handling code (note: it's in two
places, since to implement it on XML documents the code was just copy/pasted
from HTML documents... sigh...) and abstracting and using that.

Comment 6

[Darin Fisher]

yeah, i briefly looked at that code.  i definitely agree with you that a unified
link handler sounds like the right solution for this.  i'll take a look to see
how much effort might be involved.

Comment 7

[Darin Fisher]

Attachment: v1 patch

ok, this patch really smacks the prefetching implementation in a big way.  i'm
actually going back to my original design of directly adding calls to
nsIPrefetchService inside nsHTMLContentSink.  in the end this makes a lot more
sense.	it's a very easy way to reuse the Link header parsing already done by
the content sink.  it also fixes the bug in which Link headers wouldn't trigger
prefetching when browsing using back/forward.  i've also added the fix for bug
166774.

Comment 8

[Hixie (not reading bugmail)]

+    // FIXME: is this the right check?
+    if (rel.EqualsIgnoreCase("next")) {

No, it's not. The rel attribute is a space separated list of tokens.

   rel="foo next bar"

...is still a "next" link.

Comment 9

[Hixie (not reading bugmail)]

+        if (relVal.Equals(NS_LITERAL_STRING("next"))) {

Same thing as above.


Note that this implies that the processing model shouldn't be "if next, else if
stylesheet" but "if next. if stylesheet.".

HTH.

Comment 10

[Darin Fisher]

thx ian... i was afraid it was something like that.  patch coming up.

Comment 11

[Darin Fisher]

Attachment: v1.1 patch

ian: please let me know if you see any problems with this solution.  thx!

Comment 12

[Hixie (not reading bugmail)]

Would that patch (incorrectly) notice this kind of link?:

   <link rel="alternate unextended" href="...">

...or does |Find| look only for space separated words?

(The word "unextended" contains the string "next".)

Comment 13

[Darin Fisher]

good catch... that is a problem.

Comment 14

[Darin Fisher]

Attachment: v1.2 patch

OK, this should do it.	the HTMLContentSink is a beast!  and the current code
for parsing the rel type could definitely be improved... it is far from
performant :(

Comment 15

[Darin Fisher]

Attachment: v1.2 patch (whitespace changes removed)

Comment 16

[Hixie (not reading bugmail)]

That seems much better. :-)

Two minor nits, first is that "ProcessLink" and "ProcessLink2" seem like poor
names, how about "ProcessLinkHeader" and "ProcessLink" respectively? Is there
any reason ProcessLinkTag doesn't want to use what is currently called ProcessLink2?

The second nit is that the XML Content Sink also has this code and this patch
doesn't update it. It would be good if we could find some way to share the code.

But other than those minor nits, it looks good! Thank you for caring enough
about standards compliance in edge cases like this to actually take the time to
do it right, I appreciate it. :-)

Comment 17

[Darin Fisher]

yeah, i hate the names too... i also thought about trying to share more of the
code with ProcessLINKTag, but to tell you the truth, i haven't studied the way
it handles rel=stylesheet enough... it seems surprisingly different.  i think
after this patch goes in, i'll go back and take a look at unifying more of the
code.  same goes for the duplicate code in the XML content sink.  in fact, i'm
finding a lot of stuff i'd like to cleanup in the content sink code :)

ProcessLinkHeader and ProcessLink sound great to me... thx for the suggestion!

Comment 18

[Darin Fisher]

Attachment: v1.3 patch

revised method names.

Comment 19

[Darin Fisher]

Attachment: v1.3 patch (whitespace changes removed)

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 98465 [details] [diff] [review]
v1.3 patch (whitespace changes removed)

r=dougt

Comment 21

[rpotts (gone)]

in HTMLContentSync::ProcessStyleLink(...) it appears that 'isAlternate' is never
changed from it's initial value of PR_FALSE.

looks good other than that it looks good...

Comment 22

[Darin Fisher]

Comment on attachment 98465 [details] [diff] [review]
v1.3 patch (whitespace changes removed)

>Index: content/html/document/src/nsHTMLContentSink.cpp

>+HTMLContentSink::ProcessStyleLink(nsIHTMLContent* aElement,
>+{
>+  PRBool isAlternate = PR_FALSE;

>+  if (-1 != aLinkTypes.IndexOf(NS_LITERAL_STRING("alternate"))) { // if alternate, does it have title?
>       if (0 == aTitle.Length()) { // alternates must have title
>         return NS_OK; //return without error, for now
>       } else {
	  isAlternate = PR_TRUE;
	}

rick, if you see the patch in context, you'll notice that isAlternate
is set to TRUE as i've indicated above.

Comment 23

[rpotts (gone)]

oops... the diff threw me -- because it looked like isAlternate was a new local
variable ...

thanks!
-- rick

Comment 24

[rpotts (gone)]

Comment on attachment 98465 [details] [diff] [review]
v1.3 patch (whitespace changes removed)

sr=[e-mail]

Comment 25

[Darin Fisher]

fixed-on-trunk

Comment 26

[Darin Fisher]

mac project changes didn't actually go in until today, so if you are looking for
these changes on the mac be sure to pull a build on or after 9/13.

Comment 27

[benc]

tom loves META tags...

Comment 28

[Tom Everingham]

verified  - 09/16/02 trunk builds - winNT4, linux rh6, mac osX