Bug 3652 - add support for link prefetching
Summary: add support for link prefetching
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Enhancement
Assignee: Leon Clarke
URL: http://gemal.dk/browserspy/prefetch.php
Keywords: InRadar
: 42165 (view as bug list)
Depends on: 38900
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-22 14:23 PDT by Darin Adler
Modified: 2010-08-11 09:55 PDT (History)
24 users (show)

See Also:


Attachments
Proposed patch (21.78 KB, patch)
2010-05-24 06:51 PDT, Leon Clarke
koivisto: review-
koivisto: commit-queue-
Details | Formatted Diff | Diff
Adding missing CachedLinkPrefetch.h (24.39 KB, patch)
2010-05-27 04:21 PDT, Leon Clarke
no flags Details | Formatted Diff | Diff
As before, but updated against newer webkit (24.81 KB, patch)
2010-06-01 07:07 PDT, Leon Clarke
no flags Details | Formatted Diff | Diff
Fix silly merge error in previous patch (24.78 KB, patch)
2010-06-01 07:43 PDT, Leon Clarke
abarth: review-
Details | Formatted Diff | Diff
patch for TargetIsPrefetch (1.59 KB, application/octet-stream)
2010-06-09 07:12 PDT, Gavin Peters
no flags Details
Following abarth's review comments (28.54 KB, patch)
2010-06-14 08:29 PDT, Leon Clarke
no flags Details | Formatted Diff | Diff
Improve test slightly (29.02 KB, patch)
2010-06-14 10:07 PDT, Leon Clarke
abarth: review-
Details | Formatted Diff | Diff
Remove the header (28.77 KB, patch)
2010-06-18 07:40 PDT, Leon Clarke
no flags Details | Formatted Diff | Diff
Remove tabs from changelog (28.79 KB, patch)
2010-06-18 07:46 PDT, Leon Clarke
abarth: review-
Details | Formatted Diff | Diff
alternate layout test for prefetch (398 bytes, text/html)
2010-07-06 18:03 PDT, Gavin Peters
no flags Details
Patch that removes onload, simplifies cache object, and adds a new layouttest (37.86 KB, patch)
2010-07-07 14:39 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Remove tabs from changelog from last patch (37.87 KB, patch)
2010-07-07 14:52 PDT, Gavin Peters
abarth: review-
Details | Formatted Diff | Diff
remediated to abarth's comments (37.14 KB, patch)
2010-07-07 18:48 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
fixed chromium fat finger (37.14 KB, patch)
2010-07-08 05:33 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
really fix chromium fat finger (37.11 KB, patch)
2010-07-08 07:02 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
fat finger begone (37.11 KB, patch)
2010-07-08 08:04 PDT, Gavin Peters
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Actually skip the layout tests (37.04 KB, patch)
2010-07-09 06:30 PDT, Leon Clarke
no flags Details | Formatted Diff | Diff
Fix the issue Nate pointed out with accessing deleted objects. (1.76 KB, patch)
2010-07-13 03:04 PDT, Leon Clarke
no flags Details | Formatted Diff | Diff
And again without tabs in the changelog (1.79 KB, patch)
2010-07-13 03:10 PDT, Leon Clarke
pfeldman: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2005-06-22 14:23:07 PDT
<rdar://problem/4158270> ER: support link prefetching

6/22/05 1:31 PM Peter Westen:
* SUMMARY
Browsers that support "link prefetching" can reduce perceived load times by proactively fetching pages the 
user is likely to request. Google, for example, will identify the top search result as a prefetch-able link if 
the browser supports it. This is based on the "rel" attribute of the LINK tag.
Comment 1 Nicholas Shanks 2005-08-08 13:17:32 PDT
I don't see any <link> elements or indeed anywhere where the rel attribute is set on any tag, for example 
on http://google.com/search?q=something. What exactly is the rel attribute supposed to contain to 
indicate to a browser it might want to pre-fetch a certain href?
Comment 2 Darin Adler 2005-08-08 13:21:56 PDT
Documentation on how this works in Mozilla is at <http://www.mozilla.org/projects/netlib/
Link_Prefetching_FAQ.html>. Not sure what the status is of the tags from Google.
Comment 3 Joost de Valk (AlthA) 2005-11-22 12:47:11 PST
Updated the URL to a testcase. I would very much like this to work, since it is very, very useful. Best would 
be though to make it configurable. Only preloading from the same domain, or from external domains, or 
not at all...
Comment 4 John Lascurettes 2006-08-30 11:29:35 PDT
(In reply to comment #2)
Here's (hopefully) the complete link:

http://www.mozilla.org/projects/netlib/Link_Prefetching_FAQ.html

This would be a useful feature; and even though it's not part of any _official_ standards, the way Mozilla describes it, it is a "quasi" standard. The Link: HTTP header had been part of an earlier RFC but was excluded in later docs. Plus the <link> with attributes of rel="prefetch" or rel="next" do not violate the W3C recommendations in any way.

Comment 5 Eric Seidel (no email) 2008-03-26 14:16:21 PDT
Supposedly Google.com returns this on some search results.  I've not seen it myself, but it seems like this would be useful for us to support.
Comment 6 Leon Clarke 2010-05-11 04:18:18 PDT
Android has been implementing this. I'll have a patch real soon now. It's a bit big so I'm refactoring it into a couple of bugs.
Comment 7 Leon Clarke 2010-05-24 06:51:35 PDT
Created attachment 56883 [details]
Proposed patch

Note that I've made the feature optional, behind a new ENABLE_LINK_PREFETCH guard. As a result it's turned off by default, and so I've disabled the new layout test.

I've made it disabled for 2 reasons, firstly there might possibly be controversy about whether the feature is a good thing, and secondly in Android we have a tiny change made outside of the webkit code (specifically we have some code in our HTTP stack to disable the feature when roaming), and other platforms might want to consider whether they want similar changes before they take this.

However, the counter-argument would be that it's a fairly small change and only downloads things that the page requests are downloaded so it's not worth adding a new guard for. If people would prefer that it was always on, I'll edit the patch appropriately.
Comment 8 Tony Gentilcore 2010-05-26 13:53:31 PDT
I'm not a WebKit reviewer but had a question about this patch.

Please correct me if I'm mistaken, but it looks to me like the current implementation would block the window's load event if there are outstanding prefetch requests. If that is the case, it would defeat the usefulness of this feature for pages that are concerned with firing onload asap.

I'm interested in seeing a Layout Test that verifies that window.onload will fire before link.onload if the resource being loaded is delayed. There are examples in LayoutTests/http/tests/ of Layout Tests with delayed subresources.
Comment 9 Leon Clarke 2010-05-27 02:07:52 PDT
Thanks for the comments Tony.

Unless you've discovered a very subtle bug, my patch should ensure that the window's load event isn't delayed by prefetch requests. If you look in DocLoader.cpp, incrementRefCount and decrementRefCount have been changed to ignore prefetch requests. This count seems to be only used to keep track of whether there are any outstanding requests, or whether the load event should fire, so this means that the load event should fire before the prefetches finish.

Adding a layout test to confirm this is probably a good idea. I considered writing one, but wasn't convinced I could guarantee the events would always arrive in the right order if both loads actually finished at the same time, however on further reflection I'm not sure this is a problem. I'll write a test and see if it's reliable.
Comment 10 Antti Koivisto 2010-05-27 04:15:15 PDT
Comment on attachment 56883 [details]
Proposed patch

The patch is missing at least CachedLinkPrefetch.h, can't be reviewed.
Comment 11 Leon Clarke 2010-05-27 04:21:13 PDT
Created attachment 57219 [details]
Adding missing CachedLinkPrefetch.h
Comment 12 Leon Clarke 2010-06-01 07:07:33 PDT
Created attachment 57541 [details]
As before, but updated against newer webkit
Comment 13 Early Warning System Bot 2010-06-01 07:22:50 PDT
Attachment 57541 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2752051
Comment 14 Leon Clarke 2010-06-01 07:43:35 PDT
Created attachment 57543 [details]
Fix silly merge error in previous patch
Comment 15 WebKit Review Bot 2010-06-01 08:18:10 PDT
Attachment 57541 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2800063
Comment 16 WebKit Review Bot 2010-06-01 08:24:25 PDT
Attachment 57541 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2800065
Comment 17 Gavin Peters 2010-06-03 10:33:43 PDT
Leon,

Would it make sense to add a TargetType in RequestResourceBase of TargetIsPrefetch ?  It's getting classified as a Subresource now, and it's not quite that.
Comment 18 Leon Clarke 2010-06-09 03:04:27 PDT
(In reply to comment #17)
> Leon,
> 
> Would it make sense to add a TargetType in RequestResourceBase of TargetIsPrefetch ?  It's getting classified as a Subresource now, and it's not quite that.

Yes, that sounds like a good idea. I'll look into it...
Comment 19 Gavin Peters 2010-06-09 07:12:37 PDT
Created attachment 58241 [details]
patch for TargetIsPrefetch

This patch likely won't apply, but it's a subset of my webkit with the TargetIsPrefetch target type added.
Comment 20 Adam Barth 2010-06-10 10:33:29 PDT
Comment on attachment 57543 [details]
Fix silly merge error in previous patch

Some nits and questions.  The biggest question for me is whether we should be using the X-Moz header.

WebCore/html/HTMLLinkElement.h:48
 +          RelAttribute() : m_isStyleSheet(false), m_isIcon(false), m_isAlternate(false), m_isDNSPrefetch(false)
Please put all these initializers on their own line.

WebCore/loader/CachedLinkPrefetch.h:45
 +      virtual void didAddClient(CachedResourceClient* c)
This implementation looks very generic.  Maybe it should go in the base class as a default implementation?

WebCore/loader/CachedLinkPrefetch.h:51
 +      virtual void data(PassRefPtr<SharedBuffer> data, bool allDataReceived)
Same here.

WebCore/loader/CachedLinkPrefetch.h:56
 +          m_data = data;
Why do we hold onto this data?  It seems that's just a waste of memory.

WebCore/loader/loader.cpp:375
 +              resourceRequest.setHTTPHeaderField("X-Moz", "prefetch");
Do we want to be using an X-Moz header?  Maybe it's more polite to use X-WebKit ?

LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:1
 +  This tests that onload events can be attached to link elements with rel=prefetch. Since prefetch links are just there as a performance optimisation, the onload event is their only programatic side-effect.
"optimisation" -> usually we use american english

LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:5
 +  TEST PASSED. The stylesheet link onload handler has fired
Crazy.  Does that work in Firefox?
Comment 21 Leon Clarke 2010-06-11 02:15:58 PDT
On the subject of the X-Moz header, I agree this might be controversial!

There is a need for a header so that sites doing rather crude analytics based on server access logs can ignore prefetch requests if they want to.

Since the best documentation of all this comes from Mozilla, and they mention that they send the X-Moz one, it seems to have become the de-facto standard. Google used this header in its 'Google Web Accelerator', a now-discontinued caching prefetching proxy that you ran on your desktop machine. When I was originally implementing this, I took that as a precedent for Google using the X-Moz header.

I felt that sending the de-facto standard header is a better idea (less confusing for site developers) than inventing another non-standard header like X-Webkit, but if people feel it'd be better as a different header, I don't feel strongly and I'll change it.  In an ideal world there would be a standard header, but there doesn't seem to be; presumably the HTML5 people didn't want to specify HTTP headers.
Comment 22 Leon Clarke 2010-06-14 08:29:31 PDT
Created attachment 58656 [details]
Following abarth's review comments

Also incorporates Gavin Peters' patch to add a new TargetType
Comment 23 Leon Clarke 2010-06-14 08:35:41 PDT
(In reply to comment #20)
> WebCore/loader/CachedLinkPrefetch.h:45
>  +      virtual void didAddClient(CachedResourceClient* c)
> This implementation looks very generic.  Maybe it should go in the base class as a default implementation?
> 
> WebCore/loader/CachedLinkPrefetch.h:51
>  +      virtual void data(PassRefPtr<SharedBuffer> data, bool allDataReceived)
> Same here.

With didAddClient, I agree and in fact there was already an identical implementation in CachedScript that I've also remove. For data, now that I don't pointlessly keep a copy of the data, I'd like to argue that there's nothing very generic about a CachedResource that throws away its data, and it's very unlikely any other subclass of CachedResource will ever want a data function exactly like this.

> LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:5
>  +  TEST PASSED. The stylesheet link onload handler has fired
> Crazy.  Does that work in Firefox?
No. They don't implement the events.
Comment 24 Leon Clarke 2010-06-14 10:07:04 PDT
Created attachment 58667 [details]
Improve test slightly

This differs from the previous one only in that the test has been imnproved.
It now checks that the prefetch onload fires after the main document onload.
Comment 25 Adam Barth 2010-06-17 19:16:35 PDT
> > LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:5
> >  +  TEST PASSED. The stylesheet link onload handler has fired
> > Crazy.  Does that work in Firefox?
> No. They don't implement the events.

Why are we implementing the events then?

I don't think we should be sending an X-Moz header.  Probably the best thing to do is register a real header with IANA:

http://www.iana.org/assignments/message-headers/perm-headers.html

Is there a spec that describes link rel=prefetch?  Should we write one?  Did you test the X-Moz header?  This patch is very fast and loose.
Comment 26 Adam Barth 2010-06-17 19:16:48 PDT
Comment on attachment 58667 [details]
Improve test slightly

See above.
Comment 27 Andrei Popescu 2010-06-18 04:10:05 PDT
(In reply to comment #25)
> > > LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:5
> > >  +  TEST PASSED. The stylesheet link onload handler has fired
> > > Crazy.  Does that work in Firefox?
> > No. They don't implement the events.
> 
> Why are we implementing the events then?
> 

I think the idea is to allow this functionality to be tested (e.g. wait for the onload event). Or perhaps there is a better way to test?

> I don't think we should be sending an X-Moz header.  Probably the best thing to do is register a real header with IANA:
> 
> http://www.iana.org/assignments/message-headers/perm-headers.html
> 

The spec doesn't say anything about sending a special header. I think we could just leave it out.

> Is there a spec that describes link rel=prefetch?  Should we write one?  

http://dev.w3.org/html5/spec/Overview.html#link-type-prefetch

Thanks,
Andrei
Comment 28 Leon Clarke 2010-06-18 05:05:46 PDT
(In reply to comment #27)
> (In reply to comment #25)
> > > > LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:5
> > > >  +  TEST PASSED. The stylesheet link onload handler has fired
> > > > Crazy.  Does that work in Firefox?
> > > No. They don't implement the events.
> > 
> > Why are we implementing the events then?
> > 
> 
> I think the idea is to allow this functionality to be tested (e.g. wait for the onload event). Or perhaps there is a better way to test?

Correct; while it's in the HTML5 spec, I mainly added it because it means the functionality has a programatic side effect in javascript.

> 
> > I don't think we should be sending an X-Moz header.  Probably the best thing to do is register a real header with IANA:
> > 
> > http://www.iana.org/assignments/message-headers/perm-headers.html
> > 
> 
> The spec doesn't say anything about sending a special header. I think we could just leave it out.

The argument for having it is to enable people to distinguish prefetch requests when analyzing server logs. I'll remove it from this patch, but suggest to the HTML5 working group that they might want to standardize a header. Registering a real header with IANA and not trying to get it into some spec somewhere would probably cause confusion.
Comment 29 Leon Clarke 2010-06-18 07:40:28 PDT
Created attachment 59107 [details]
Remove the header
Comment 30 WebKit Review Bot 2010-06-18 07:42:09 PDT
Attachment 59107 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebKit/chromium/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Leon Clarke 2010-06-18 07:46:01 PDT
Created attachment 59108 [details]
Remove tabs from changelog
Comment 32 Gavin Peters 2010-06-18 08:45:30 PDT
Leon,
 
Thanks for all the work on this; the patch looks awesome.

It seems mozilla does prefetching for link rel=next as well.  Should we consider that too?  I don't know if that deserves a different target type.

Reference: https://developer.mozilla.org/en/link_prefetching_faq

- Gavin
Comment 33 Leon Clarke 2010-06-18 09:22:08 PDT
(In reply to comment #32)

> It seems mozilla does prefetching for link rel=next as well.  Should we consider that too?  I don't know if that deserves a different target type.
> 
> Reference: https://developer.mozilla.org/en/link_prefetching_faq

Interesting. We should certainly consider it. I'd like to suggest that we should consider it in another bug, rather than adding it to this patch.

The behavior of mozilla seems to be a bit divergent from the HTML5. I don't see any evidence in HTML5 that rel=next should be prefetched. Also, it seems they only prefetch when you use rel=next with the link element, not the <a> element where rel=next is also valid. I'm not quite sure I follow why they make that distinction.

My immediate thought is that there's no need for a different target type. Can you think of any use cases for needing to distinguish between the 2 cases?
Comment 34 Adam Barth 2010-06-18 09:33:25 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #25)
> > > > > LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload-expected.txt:5
> > > > >  +  TEST PASSED. The stylesheet link onload handler has fired
> > > > > Crazy.  Does that work in Firefox?
> > > > No. They don't implement the events.
> > > 
> > > Why are we implementing the events then?
> > > 
> > 
> > I think the idea is to allow this functionality to be tested (e.g. wait for the onload event). Or perhaps there is a better way to test?
> 
> Correct; while it's in the HTML5 spec, I mainly added it because it means the functionality has a programatic side effect in javascript.

I think you're mis-reading the spec.  Here's what the spec says:

[[
Once the attempts to obtain the resource and its critical subresources are complete, the user agent must, if the loads were successful, queue a task to fire a simple event named load at the link element, or, if the resource or one of its critical subresources failed to completely load for any reason (e.g. DNS error, HTTP 404 response, a connection being prematurely closed, unsupported Content-Type), queue a task to fire a simple event named error at the link element. Non-network errors in processing the resource or its subresources (e.g. CSS parse errors, PNG decoding errors) are not failures for the purposes of this paragraph.
]]

In the prefetch case, we never "obtain" the resource.  Instead, we just fetch it, which is a much less involved process.  In particular, I don't think we're supposed to fire the load event.

You can still test whether the network event was loaded using the dumpResourceResponseMIMEType function of layoutTestController.  That will print the MIME type for all the requested resources.  You need to be careful when using that feature to make a reliable test, but it can be done.  You can see examples of such tests in http://trac.webkit.org/browser/trunk/LayoutTests/fast/preloader
Comment 35 Gavin Peters 2010-06-18 10:14:30 PDT
(in reply to comment #33)

Leon,

No, I cannot think why you'd want to distinguish these two target types, so I do think both should be TargetIsPrefetch.
Comment 36 Gavin Peters 2010-06-19 04:38:09 PDT
One other perspective on onload events on link elements: I think there's another good reason to avoid these events.  The Link: HTTP header elements is supposed to be semantically equivalent to an HTML link element in HTML (c.f. RFC 2068 http://www.ietf.org/rfc/rfc2068.txt (superseded by RFC 2616), and a draft reviving the element http://tools.ietf.org/id/draft-nottingham-http-link-header-10.html ).

So, if we're going to be putting link rel=prefetch and link rel=next elements in headers (and I plan on hacking at this once this patch lands), then I won't implement onload in that case: javascript in an HTTP header is almost certainly a bad idea.  So from this point of view, it's also desirable to not include them in the HTML, so as to increase the semantic equivalence.
Comment 37 Adam Barth 2010-06-19 16:49:13 PDT
Comment on attachment 59108 [details]
Remove tabs from changelog

As discussed above, I believe firing the load event here is incorrect.
Comment 38 Gavin Peters 2010-07-06 18:03:52 PDT
Created attachment 60668 [details]
alternate layout test for prefetch

I had some time on the long weekend, and I hacked up this working test.  The timeout is required of course because webkit prioritizes prefetches low enough that you'll get onload events normally before a prefetch has launched.  Is there a more elegant solution?

Leon, if you'd like I'm happy to remove the onload event from HTMLLinkElement.cpp and put together another patch?
Comment 39 Andrei Popescu 2010-07-07 04:31:02 PDT
(In reply to comment #38)
> Created an attachment (id=60668) [details]
> alternate layout test for prefetch
> 
> I had some time on the long weekend, and I hacked up this working test.  The timeout is required of course because webkit prioritizes prefetches low enough that you'll get onload events normally before a prefetch has launched.  Is there a more elegant solution?
> 

onload was clearly more elegant but I guess there are good reasons not to do that either. I am afraid that having a magic constant (50ms) will make the test pass on Chrome but fail on Android. 

Anyway, since I don't have a better solution, I think we should get this patch in and unblock you. We'll worry about it on Android once we've implemented dumpResourceResponseMIMETypes() in our DRT app.

Andrei
Comment 40 Gavin Peters 2010-07-07 14:39:32 PDT
Created attachment 60779 [details]
Patch that removes onload, simplifies cache object, and adds a new layouttest

This is an expansion of some hacking at Leon's patch I did on the weekend: it's still mostly his work, but I've tried to simplify it without onload events.
Comment 41 WebKit Review Bot 2010-07-07 14:45:08 PDT
Attachment 60779 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Gavin Peters 2010-07-07 14:52:23 PDT
Created attachment 60782 [details]
Remove tabs from changelog from last patch
Comment 43 Adam Barth 2010-07-07 15:20:59 PDT
Comment on attachment 60782 [details]
Remove tabs from changelog from last patch

Thanks, this is looking much better.  Mostly questions and a few nits:

WebCore/html/HTMLLinkElement.h:56
 +              { };
These shoudn't be indented quite as much and { and } should be on separate lines.  Also, the ; is superfluous.

WebCore/loader/DocLoader.cpp:234
 +      case CachedResource::LinkPrefetch:
I don't think a link prefetch can corrupt a frame's pixels.  Maybe we need to add a new category here?

+ ENABLE_DEVICE_ORIENTATION = ;
Device orientation?

+ ENABLE_IMAGE_RESIZER = ;
??

LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:7
 +      setTimeout("layoutTestController.notifyDone()",50);
Boo

LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:6
 +      layoutTestController.dumpResourceResponseMIMETypes();
Why doesn't the show up in the results?  Maybe you need to use something with a known file extension, like HTML?

LayoutTests/platform/mac/Skipped:285
 +  # Link prefetch is disabled by default
What platform will run the test?  Why are we putting this behind a #define?  We should just implement the feature, no?
Comment 44 Gavin Peters 2010-07-07 18:48:59 PDT
Created attachment 60822 [details]
remediated to abarth's comments

A remediation per review for abarth's comments.

> WebCore/html/HTMLLinkElement.h:56
> +              { };
> These shoudn't be indented quite as much and { and } should be on separate lines.  Also, the ; is superfluous.

Done.

> WebCore/loader/DocLoader.cpp:234
>  +      case CachedResource::LinkPrefetch:
> I don't think a link prefetch can corrupt a frame's pixels.  Maybe we need to add a new category here?

Done.

> + ENABLE_DEVICE_ORIENTATION = ;
> Device orientation?
> 
> + ENABLE_IMAGE_RESIZER = ;
> ??

My bad: copied from other build scripts.  Now fixed.

> LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:7
>  +      setTimeout("layoutTestController.notifyDone()",50);
> Boo

Boo hoo!

> LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:6
>  +      layoutTestController.dumpResourceResponseMIMETypes();
> Why doesn't the show up in the results?  Maybe you need to use something with a known file extension, like HTML?

My bad.  Modified the test, now everything is in the results very well.

> LayoutTests/platform/mac/Skipped:285
> +  # Link prefetch is disabled by default
> What platform will run the test?  Why are we putting this behind a #define?  We should just implement the feature, no?

So I've had good luck testing this enabled in mac, and in chromium.  However for chromium there's an extra patch required (see FromTargetType in http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/weburlloader_impl.cc?view=markup ).  To safely build this in chromium, we need to land this patch with define guards in webkit, and then enable the feature in webkit at the same time that we update FromTargetType (among other things).  Unfortunately, DumpRenderTree in chromium won't let this feature be tested.

However, my testing to date shows no reason not to enable this in mac: i'd be excited to do it.
Comment 45 WebKit Review Bot 2010-07-07 22:21:48 PDT
Attachment 60822 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3465011
Comment 46 Leon Clarke 2010-07-08 03:27:35 PDT
(In reply to comment #43)
> (From update of attachment 60782 [details])
> LayoutTests/platform/mac/Skipped:285
>  +  # Link prefetch is disabled by default
> What platform will run the test?  Why are we putting this behind a #define?  We should just implement the feature, no?

I mostly put it behind a #define as I could see arguments either way, and decided it might be most polite to put in the #ifdefs and remove them if people felt they were unnecessary.

The main arguments for the #ifdefs were
a) On Android, there's actually a tiny bit more code in our HTTP stack that ignores prefetch requests if on a roaming network. Other platforms might like to make similar changes before they enable the feature.
b) Antti was suggesting that this implementation would interact with the iPhone's cache code in a way that would make it useless on iPhone.
c) I felt there was a risk someone might object to the concept of prefetching, but that doesn't seem to be an issue.

Having written that, the arguments seem a bit weak. I'd have no objections to removing the #define.
Comment 47 Gavin Peters 2010-07-08 05:33:38 PDT
Created attachment 60868 [details]
fixed chromium fat finger

This oughta build on chromium.
Comment 48 Gavin Peters 2010-07-08 05:35:27 PDT
(In reply to comment #46)
> ...
> 
> The main arguments for the #ifdefs were
> a) On Android, there's actually a tiny bit more code in our HTTP stack that ignores prefetch requests if on a roaming network. Other platforms might like to make similar changes before they enable the feature.

(a) is the case on chromium, too.  We specifically wanted to make sure we placed the new TargetType in the right priorities in SPDY and HTTP.

> ...
Comment 49 WebKit Review Bot 2010-07-08 06:56:59 PDT
Attachment 60868 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3502022
Comment 50 Gavin Peters 2010-07-08 07:02:24 PDT
Created attachment 60874 [details]
really fix chromium fat finger
Comment 51 Gavin Peters 2010-07-08 08:04:26 PDT
Created attachment 60883 [details]
fat finger begone
Comment 52 Adam Barth 2010-07-08 09:42:33 PDT
Comment on attachment 60883 [details]
fat finger begone

WebCore/loader/DocLoader.cpp:383
 +      if (res->isPrefetch())
It's slightly better to use something semantic here instead of RTTI.

LayoutTests/fast/dom/HTMLLinkElement/prefetch.html:7
 +      setTimeout("layoutTestController.notifyDone()",50);
Still sad about this line.  This test is going to be flaky, especially on slower machines and in configurations like valgrind.
Comment 53 WebKit Commit Bot 2010-07-09 05:35:33 PDT
Comment on attachment 60883 [details]
fat finger begone

Rejecting patch 60883 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20633 test cases.
fast/dom/HTMLLinkElement/prefetch.html -> failed

Exiting early after 1 failures. 6684 tests run.
126.68s total testing time

6683 test cases (99%) succeeded
1 test case (<1%) had incorrect layout

Full output: http://webkit-commit-queue.appspot.com/results/3450081
Comment 54 Leon Clarke 2010-07-09 06:30:16 PDT
Created attachment 61041 [details]
Actually skip the layout tests
Comment 55 WebKit Commit Bot 2010-07-09 19:41:22 PDT
Comment on attachment 61041 [details]
Actually skip the layout tests

Clearing flags on attachment: 61041

Committed r63032: <http://trac.webkit.org/changeset/63032>
Comment 56 Nate Chapin 2010-07-12 15:55:02 PDT
(In reply to comment #55)
> (From update of attachment 61041 [details])
> Clearing flags on attachment: 61041
> 
> Committed r63032: <http://trac.webkit.org/changeset/63032>

http://trac.webkit.org/browser/trunk/WebCore/loader/loader.cpp?rev=63032#L593

This function now contains the following code:

delete request;
docLoader->decrementRequestCount(request->cachedResource());

It seems like a Very Bad Thing that we're deleting an object then immediately referencing it. :(
Comment 57 Adam Barth 2010-07-12 18:09:06 PDT
I fail at reading code.
Comment 58 Leon Clarke 2010-07-13 03:04:23 PDT
Created attachment 61349 [details]
Fix the issue Nate pointed out with accessing deleted objects.

Thanks for soptting this Nate. Very sorry about that.
Comment 59 WebKit Review Bot 2010-07-13 03:07:19 PDT
Attachment 61349 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 60 Leon Clarke 2010-07-13 03:10:58 PDT
Created attachment 61350 [details]
And again without tabs in the changelog
Comment 61 Pavel Feldman 2010-07-13 07:30:42 PDT
Comment on attachment 61350 [details]
And again without tabs in the changelog

I'll land.
Comment 62 Pavel Feldman 2010-07-13 07:33:42 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLLinkElement.h
	M	WebCore/loader/loader.cpp
Committed r63204
Comment 63 Nicholas Shanks 2010-07-13 07:34:51 PDT
That ought to have thrown up a compiler warning (or even an error), but on the assumption that it didn't, can we not add some check to one of the style bots to look for things like this?
Comment 64 Pavel Feldman 2010-07-13 07:35:16 PDT
*** Bug 42165 has been marked as a duplicate of this bug. ***
Comment 65 Tony Gentilcore 2010-07-13 08:19:01 PDT
I'm not completely familiar with this code, but doesn't this mean that request count is out of sync now?

Previously it did:
delete request; // Unconditionally delete
decrementRequestCount(); // Unconditionally decrement request count.

Now it does:
decrementRequestCount(cachedResource); // Conditionally decrement request count iff not a prefetch
delete request; // Unconditionally delete

It seems odd that the cachedResource->isPrefetch() check only guards the request count decrement and not the delete. But perhaps I'm just missing something.
Comment 66 Leon Clarke 2010-07-13 08:30:12 PDT
I believe it is balanced, since there is a corresponding change that prefetch requests never increase the request count.

The request count seems to be only used to find out when 'everything' has finished loading, to fire he page's onload event etc.. Since the prefetch requests should load after onload, I exclude them from the request count.

I agree it's a bit unaesthetic that there's a 'request count' that excludes some requests, but it always gives the answer that the caller wants.
Comment 67 Tony Gentilcore 2010-07-13 08:36:18 PDT
(In reply to comment #66)
> I believe it is balanced, since there is a corresponding change that prefetch requests never increase the request count.
> 
> The request count seems to be only used to find out when 'everything' has finished loading, to fire he page's onload event etc.. Since the prefetch requests should load after onload, I exclude them from the request count.
> 
> I agree it's a bit unaesthetic that there's a 'request count' that excludes some requests, but it always gives the answer that the caller wants.

Ah, thanks for the explanation. I can't think of any way to add ASSERTs to guarantee that it's balanced, too bad.
Comment 68 WebKit Commit Bot 2010-08-11 03:48:46 PDT
Comment on attachment 61350 [details]
And again without tabs in the changelog

Rejecting patch 61350 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Pavel Feldman', u'--force']" exit_code: 1
Parsed 3 diffs from patch file(s).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/html/HTMLLinkElement.h
Hunk #1 FAILED at 31.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/HTMLLinkElement.h.rej
patching file WebCore/loader/loader.cpp
Hunk #1 FAILED at 590.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/loader/loader.cpp.rej

Full output: http://queues.webkit.org/results/3760046
Comment 69 Adam Barth 2010-08-11 09:55:58 PDT
Looks like this has been landed.