185692 - enable prefetching of '?' URLs in the context of rel=prefetch

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Darin Fisher]

we should not inhibit '?' URLs from being prefetched when they appear along with
rel=prefetch.  doing so unnecessarily limits the scope of prefetching applications.

(prefetching rel=next w/ '?' URL is inhibited b/c a lot of sites put '?' URLs in
<link rel=next> tags simply to indicate the next page, but the contents of these
pages are commonly dynamic and therefore not cacheable -- e.g., bugzilla!)

Comment 1

[Jo Hermans]

*** Bug 182602 has been marked as a duplicate of this bug. ***

Comment 2

[Darin Fisher]

-> suresh

probably sufficient to add a flag to nsIPrefetchService::PrefetchURI that states
whether the prefetch is implicit (rel=next) or explicit (rel=prefetch).  that
way the prefetch service doesn't have to have to know about HTML specific tags, etc.

Comment 3

[Suresh]

Attachment: proposed patch

Comment 4

[Darin Fisher]

Comment on attachment 116385 [details] [diff] [review]
proposed patch

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

>+  void PrefetchHref(const nsAString &aHref, PRBool aHasPrefetch);

nit: i would call the flag |aExplicit|, meaning has the server explicitly
requested prefetching or are we just trying to be clever (implicit).


>+  PRBool hasPrefetch = (linkTypes.IndexOf(NS_LITERAL_STRING("prefetch")) != -1) ? PR_TRUE : PR_FALSE;

here, operator?: is redundant.	the result of operator!= is a boolean.


>+        PRBool hasPrefetch = (linkTypes.IndexOf(NS_LITERAL_STRING("prefetch")) != -1) ? PR_TRUE : PR_FALSE;

same nit here.


>Index: prefetch/nsIPrefetchService.idl

>+    void prefetchURI(in nsIURI aURI, in nsIURI aReferrerURI, in boolean aHasPrefetch);

i think aExplicit would be especially better in the context of this interface
;)


>Index: prefetch/nsPrefetchService.cpp

>+    // skip URLs that contain query strings, except for link tags that
>+    // contain explicit "prefetch" link type.  these URLs are likely to result

skip URLs that contain query strings, except URLs for which prefetching
has been explicitly requested.	best not to mention concept of link tags
here, since the prefetch service could conceptually be used with non-HTML.


>+    if (!aHasPrefetch) {
>     nsCOMPtr<nsIURL> url(do_QueryInterface(aURI, &rv));

indentation :)


otherwise, this looks great.  thx suresh!

Comment 5

[Darin Fisher]

>skip URLs that contain query strings, except URLs for which prefetching
>has been explicitly requested.	best not to mention concept of link tags
>here, since the prefetch service could conceptually be used with non-HTML.

i didn't mean to put these two sentences in the same paragraph.  how confusing!
i meant this:

 // skip URLs that contain query strings, except URLs for which prefetching
 // has been explicitly requested.

the last sentence was just a random comment ;-)

Comment 6

[Suresh]

Attachment: updated patch

Thanks darin for the comments!

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 116408 [details] [diff] [review]
updated patch

why did you get rid of the "prefetch" tag?


   // prefetch href if relation is "next" or "prefetch"
-  if (linkTypes.IndexOf(NS_LITERAL_STRING("next")) != -1 ||
-      linkTypes.IndexOf(NS_LITERAL_STRING("prefetch")) != -1) {
-    PrefetchHref(aHref);
+  if (linkTypes.IndexOf(NS_LITERAL_STRING("next")) != -1 || hasPrefetch) {
+    PrefetchHref(aHref, hasPrefetch);

I just think that this support should be a pref.   If you enable it queries
will be precached - if the pref is disabled it works as it does now.

Comment 8

[Suresh]

>>why did you get rid of the "prefetch" tag?

I didn't get rid of this. There is a search for "prefetch" tag just above that line.
+PRBool hasPrefetch = (linkTypes.IndexOf(NS_LITERAL_STRING("prefetch")) != -1);


Right now, the entire prefetching is supported by a pref. However, this
particular bug deals with automatically prefetching for an explicit prefetch
link type (i.e <link rel="prefetch"....>. I don't think so we should control
this by a pref. 

Comment 9

[Darin Fisher]

i agree with suresh.  there is no one using <link rel="prefetch" href="...">
that doesn't mean for the href to be prefetched, so blocking query strings in
this context is bogus.  there's no need for a pref.

Comment 10

[Darin Fisher]

Comment on attachment 116408 [details] [diff] [review]
updated patch

>Index: prefetch/nsIPrefetchService.idl

>      * @param aURI the URI of the document to prefetch
>      * @param aReferrerURI the URI of the referring page

>+    void prefetchURI(in nsIURI aURI, in nsIURI aReferrerURI, in boolean aExplicit);

sorry, one more nit: please document aExplicit with an @param above :)


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

>+        PRBool hasPrefetch = (linkTypes.IndexOf(NS_LITERAL_STRING("prefetch")) != -1);
>+        if (linkTypes.IndexOf(NS_LITERAL_STRING("next")) != -1 || hasPrefetch) {

nit: check hasPrefetch first

sr=darin with those nits fixed.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 116408 [details] [diff] [review]
updated patch

k

Comment 12

[Suresh]

fixed in trunk.