Bug 3652 – add support for link prefetching

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

<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.

[Nicholas Shanks]

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?

[Darin Adler]

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.

[Joost de Valk (AlthA)]

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...

[John Lascurettes]

(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.

[Eric Seidel (no email)]

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.

[Leon Clarke]

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.

[Leon Clarke]

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.

[Tony Gentilcore]

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.

[Leon Clarke]

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.

[Antti Koivisto]

Comment on attachment 56883 [details]
Proposed patch

The patch is missing at least CachedLinkPrefetch.h, can't be reviewed.

[Leon Clarke]

Created attachment 57219 [details]
Adding missing CachedLinkPrefetch.h

[Leon Clarke]

Created attachment 57541 [details]
As before, but updated against newer webkit

[Early Warning System Bot]

Attachment 57541 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2752051

[Leon Clarke]

Created attachment 57543 [details]
Fix silly merge error in previous patch

[WebKit Review Bot]

Attachment 57541 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2800063

[WebKit Review Bot]

Attachment 57541 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2800065

[Gavin Peters]

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.

[Leon Clarke]

(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...

[Gavin Peters]

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.

[Adam Barth]

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?

[Leon Clarke]

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.

[Leon Clarke]

Created attachment 58656 [details]
Following abarth's review comments

Also incorporates Gavin Peters' patch to add a new TargetType

[Leon Clarke]

(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.

[Leon Clarke]

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.

[Adam Barth]

> > 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.

[Adam Barth]

Comment on attachment 58667 [details]
Improve test slightly

See above.

[Andrei Popescu]

(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

[Leon Clarke]

(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.

[Leon Clarke]

Created attachment 59107 [details]
Remove the header

[WebKit Review Bot]

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.

[Leon Clarke]

Created attachment 59108 [details]
Remove tabs from changelog

[Gavin Peters]

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

[Leon Clarke]

(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?

[Adam Barth]

(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

[Gavin Peters]

(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.

[Gavin Peters]

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.

[Adam Barth]

Comment on attachment 59108 [details]
Remove tabs from changelog

As discussed above, I believe firing the load event here is incorrect.

[Gavin Peters]

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?

[Andrei Popescu]

(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

[Gavin Peters]

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.

[WebKit Review Bot]

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.

[Gavin Peters]

Created attachment 60782 [details]
Remove tabs from changelog from last patch

[Adam Barth]

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?

[Gavin Peters]

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.

[WebKit Review Bot]

Attachment 60822 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3465011

[Leon Clarke]

(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.

[Gavin Peters]

Created attachment 60868 [details]
fixed chromium fat finger

This oughta build on chromium.

[Gavin Peters]

(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.

> ...

[WebKit Review Bot]

Attachment 60868 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3502022

[Gavin Peters]

Created attachment 60874 [details]
really fix chromium fat finger

[Gavin Peters]

Created attachment 60883 [details]
fat finger begone

[Adam Barth]

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.

[WebKit Commit Bot]

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

[Leon Clarke]

Created attachment 61041 [details]
Actually skip the layout tests

[WebKit Commit Bot]

Comment on attachment 61041 [details]
Actually skip the layout tests

Clearing flags on attachment: 61041

Committed r63032: <http://trac.webkit.org/changeset/63032>

[Nate Chapin]

(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. :(

[Adam Barth]

I fail at reading code.

[Leon Clarke]

Created attachment 61349 [details]
Fix the issue Nate pointed out with accessing deleted objects.

Thanks for soptting this Nate. Very sorry about that.

[WebKit Review Bot]

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.

[Leon Clarke]

Created attachment 61350 [details]
And again without tabs in the changelog

[Pavel Feldman]

Comment on attachment 61350 [details]
And again without tabs in the changelog

I'll land.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/html/HTMLLinkElement.h
	M	WebCore/loader/loader.cpp
Committed r63204

[Nicholas Shanks]

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?

[Pavel Feldman]

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

[Tony Gentilcore]

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.

[Leon Clarke]

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.

[Tony Gentilcore]

(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.

[WebKit Commit Bot]

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

[Adam Barth]

Looks like this has been landed.