Bug 50187 - implement onload events for <link rel=prefetch>
Summary: implement onload events for <link rel=prefetch>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-29 14:44 PST by Gavin Peters
Modified: 2010-12-04 13:04 PST (History)
7 users (show)

See Also:


Attachments
Patch (12.82 KB, patch)
2010-11-29 17:28 PST, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (16.59 KB, patch)
2010-12-03 21:35 PST, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (16.39 KB, patch)
2010-12-04 07:51 PST, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (16.52 KB, patch)
2010-12-04 09:09 PST, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2010-11-29 14:44:29 PST
Right now link rel=prefetch doesn't dispatch onload events.  However, it probably would be really good if it did.  Tests would be less flaky, for one.

I emailed hixie to see what he thought, and here's the pasted correspondence:

Ian Hickson to Gavin, Adam
show details 5 Nov
- Hide quoted text -
On Fri, 5 Nov 2010, Gavin Peters (�~S~K�~V~G彼德�~V�) wrote:
>
> WebKit currently supports link rel=prefetch, but doesn't have onload or
> onerror events for this.  There was some discussion when this feature
> was first added about if this was appropriate:
> https://bugs.webkit.org/show_bug.cgi?id=3652
>
> We've since landed it, and I've had web developers ask me if we can
> generate these events: especially since WebKit supports mutating the DOM
> to include link rel=prefetch (mozilla doesn't), developers who are
> adding these programatically want a way to know how many are in flight.
> This mechanism would do it for them.
>
> What do you think?  Is this within spec and reasonable, or do you have
> any other comments about it?

Sounds good to me. Technically the HTML spec requires load and error
events to be fired on all <link> elements whenever they load anything:

  http://www.whatwg.org/specs/web-apps/current-work/complete.html#concept-link-obtain

...so it's definitely per-spec.

--
Ian Hickson               U+1047E                )\._.,--....,'``.    fL
http://ln.hixie.ch/       U+263A                /,   _.. \   _\  ;`._ ,.
Things that are impossible just take longer.   `._.-(,_..'--(,_..'`-.;.'
Comment 1 Gavin Peters 2010-11-29 17:28:19 PST
Created attachment 75089 [details]
Patch
Comment 2 Gavin Peters 2010-11-29 17:29:35 PST
Comment on attachment 75089 [details]
Patch

This is my first pass.  I've reworked a lot of tests, and I'm very interested to hear back what people think.
Comment 3 Early Warning System Bot 2010-11-29 17:45:28 PST
Attachment 75089 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6354099
Comment 4 Build Bot 2010-11-29 18:07:11 PST
Attachment 75089 [details] did not build on win:
Build output: http://queues.webkit.org/results/6304125
Comment 5 WebKit Review Bot 2010-11-30 00:07:56 PST
Attachment 75089 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6344091
Comment 6 Gavin Peters 2010-11-30 17:36:56 PST
Comment on attachment 75089 [details]
Patch

Don't commit my patch: besides the bad builds, I have realised it takes totally the wrong approach to delaying onload events.  I have a new idea I'll submit in a forthcoming patch.
Comment 7 Gavin Peters 2010-12-03 21:35:17 PST
Created attachment 75598 [details]
Patch
Comment 8 Gavin Peters 2010-12-03 21:39:02 PST
I left my approach to onload events in.  It somewhat closely mirrors what's done in the ImageLoader; the use of a SetTimer(0) forces us to wait until more of the document exists in the case of a load from cache.

Adam, I'm interested to see what you think of what I have here.  I'd like to clean this up before I start adding the onerror element.  Let me know if you think it's best to land this with onerror or in two CLs.
Comment 9 Adam Barth 2010-12-04 01:05:30 PST
Comment on attachment 75598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75598&action=review

This is looks to be correct and a nice approach.  I have a couple questions before, mostly for my education.

> LayoutTests/ChangeLog:8
> +        Implement onload events for WebKit
> +
> +        implement onload events for <link rel=prefetch>
> +        https://bugs.webkit.org/show_bug.cgi?id=50187

One of these lines seems redundant.  :)

> LayoutTests/fast/dom/HTMLLinkElement/link-and-subresource-test.html:16
> +    ++nick_load_count

missing ;

> LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload.html:15
> +                if (window.layoutTestController) {
> +                    layoutTestController.notifyDone();
> +                }

No braces needed.

> LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload.html:22
> +	</script>

bad indent

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

Yay!

> LayoutTests/http/tests/misc/prefetch-purpose.html:-17
> -<body onload="setTimeout('finishUp()', 50);">

yay!

> WebCore/html/HTMLLinkElement.cpp:376
> +#if ENABLE(LINK_PREFETCH)
> +void HTMLLinkElement::onloadTimerFired(Timer<HTMLLinkElement>*)
> +{

We usually have an ASSERT_UNUSED that we're being called back with the right timer.

> WebCore/html/HTMLLinkElement.cpp:377
> +    m_loading = false;

This line surprises me.  I'm not quite sure I understand it.

> WebCore/html/HTMLLinkElement.cpp:382
> +void HTMLLinkElement::notifyFinished(CachedResource*)
> +{

Again, an ASSERT here would be nice.

> WebCore/html/HTMLLinkElement.cpp:383
> +    m_onloadTimer.startOneShot(0);

Should we remove ourselves as a client of the prefetch at this point?

> WebCore/loader/cache/CachedResource.cpp:118
> +    setLoading(false);

I'm not sure I quite understand this line either.  Would you be willing to explain what it does?
Comment 10 Gavin Peters 2010-12-04 07:50:48 PST
Comment on attachment 75598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75598&action=review

>> LayoutTests/ChangeLog:8
>> +        https://bugs.webkit.org/show_bug.cgi?id=50187
> 
> One of these lines seems redundant.  :)

Done.

>> LayoutTests/fast/dom/HTMLLinkElement/link-and-subresource-test.html:16
>> +    ++nick_load_count
> 
> missing ;

Now it's not.

>> LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload.html:15
>> +                }
> 
> No braces needed.

Gone.

>> LayoutTests/fast/dom/HTMLLinkElement/prefetch-onload.html:22
>> +	</script>
> 
> bad indent

Fixed.

>> WebCore/html/HTMLLinkElement.cpp:376
>> +{
> 
> We usually have an ASSERT_UNUSED that we're being called back with the right timer.

Good plan.  Done.

>> WebCore/html/HTMLLinkElement.cpp:377
>> +    m_loading = false;
> 
> This line surprises me.  I'm not quite sure I understand it.

Neither did I.  It's gone.

The original patch in 3652 manipulated m_loading quite a bit; this was the last vestige of that.  In the case of HTMLLinkElement through, isLoading() is really about CSS, and not any other kind of resource, so I'm not touching it any more.

>> WebCore/html/HTMLLinkElement.cpp:382
>> +{
> 
> Again, an ASSERT here would be nice.

Done.

>> WebCore/html/HTMLLinkElement.cpp:383
>> +    m_onloadTimer.startOneShot(0);
> 
> Should we remove ourselves as a client of the prefetch at this point?

I think it's a good idea.  When the HTMLLinkElement is deleted, it'll remove itself.  And of course the CachedResource has a life at least as long as this element, since it should survive in the MemoryCache for the length of the page.

Done.

>> WebCore/loader/cache/CachedResource.cpp:118
>> +    setLoading(false);
> 
> I'm not sure I quite understand this line either.  Would you be willing to explain what it does?

As it happens, CachedResource::data is used only in the LinkPrefetch case.  This method is overriden specifically in all the other subclasses.  The omission of this line was a bug, but invisible since nothing cared about completion of prefetch loads.  In other CachedResources, loading is normally set to false after completion, or after discovering it will fail.  Grep through ImageLoader.cpp say looking for setLoading(false) and you'll see a very similar call in ::data() (around like 299).

The most important use probably comes in CachedResource::didAddClient(), if isLoading() returns false, we immediately send our new client a notifyFinished.  It's actually that immediate event-pass-through that required I put the timer in HTMLLinkElement; when the memory cache contained the target of the prefetch, onload on prefetch events was launching before we even had a fully baked DOM.
Comment 11 Gavin Peters 2010-12-04 07:51:25 PST
Created attachment 75610 [details]
Patch
Comment 12 Gavin Peters 2010-12-04 07:56:00 PST
Adam,

Thanks for your review.  I've remediated to your concerns, and answered your questions.  I will go ahead and get working on onerror events, so we can either wait for that patch, or just land this and have the onerror events add to it.
Comment 13 Gavin Peters 2010-12-04 08:10:45 PST
Comment on attachment 75610 [details]
Patch

The removeClient is causing trouble on mac.  Wait for more.
Comment 14 Gavin Peters 2010-12-04 09:09:55 PST
Created attachment 75611 [details]
Patch
Comment 15 Gavin Peters 2010-12-04 09:11:08 PST
Comment on attachment 75611 [details]
Patch

This patch solves the crash I was seeing on Mac (and I'm surprised I didn't see in gtk).  

However, I'd like to add one more test; so I'm putting it here more to have stashed it than anything else.
Comment 16 Gavin Peters 2010-12-04 10:06:03 PST
Ixnay adding that test; I wanted to confirm if this patch was introducing onload to rel="stylesheet" elements, and it is not.  Perhaps that's a good followup CL to this.
Comment 17 Adam Barth 2010-12-04 11:17:13 PST
Comment on attachment 75611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75611&action=review

Thanks!

> WebCore/html/HTMLLinkElement.cpp:384
> +    if (m_cachedLinkPrefetch.get() == resource) {

Can this be false?  Or are we just planning ahead?
Comment 18 Gavin Peters 2010-12-04 11:34:21 PST
Comment on attachment 75611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75611&action=review

>> WebCore/html/HTMLLinkElement.cpp:384
>> +    if (m_cachedLinkPrefetch.get() == resource) {
> 
> Can this be false?  Or are we just planning ahead?

I don't believe it can be false, since I don't believe CachedCSSStyleSheet sends notifyFinished() events.  But, yeah, planning ahead since one day it might, and I adding onload for link rel=stylesheet should be a careful conscious choice in the future.
Comment 19 Adam Barth 2010-12-04 12:34:46 PST
Maybe we should use an ASSERT with a comment to document that fact?
Comment 20 WebKit Commit Bot 2010-12-04 12:40:26 PST
Comment on attachment 75611 [details]
Patch

Clearing flags on attachment: 75611

Committed r73335: <http://trac.webkit.org/changeset/73335>
Comment 21 WebKit Commit Bot 2010-12-04 12:40:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Commit Bot 2010-12-04 13:04:43 PST
The commit-queue encountered the following flaky tests while processing attachment 75611 [details]:

animations/combo-transform-translate+scale.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by abarth@webkit.org, cmarrin@apple.com, darin@apple.com, ojan@chromium.org, and pol@apple.com.  The commit-queue is continuing to process your patch.