Bug 50187

Summary: implement onload events for <link rel=prefetch>
Product: WebKit Reporter: Gavin Peters <gavinp>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, commit-queue, gustavo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Gavin Peters
Reported 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. `._.-(,_..'--(,_..'`-.;.'
Attachments
Patch (12.82 KB, patch)
2010-11-29 17:28 PST, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Patch (16.59 KB, patch)
2010-12-03 21:35 PST, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Patch (16.39 KB, patch)
2010-12-04 07:51 PST, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Patch (16.52 KB, patch)
2010-12-04 09:09 PST, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Gavin Peters
Comment 1 2010-11-29 17:28:19 PST
Created attachment 75089 [details] Patch
Gavin Peters
Comment 2 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.
Early Warning System Bot
Comment 3 2010-11-29 17:45:28 PST
Attachment 75089 [details] did not build on qt: Build output: http://queues.webkit.org/results/6354099
Build Bot
Comment 4 2010-11-29 18:07:11 PST
Attachment 75089 [details] did not build on win: Build output: http://queues.webkit.org/results/6304125
WebKit Review Bot
Comment 5 2010-11-30 00:07:56 PST
Attachment 75089 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6344091
Gavin Peters
Comment 6 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.
Gavin Peters
Comment 7 2010-12-03 21:35:17 PST
Created attachment 75598 [details] Patch
Gavin Peters
Comment 8 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.
Adam Barth
Comment 9 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?
Gavin Peters
Comment 10 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.
Gavin Peters
Comment 11 2010-12-04 07:51:25 PST
Created attachment 75610 [details] Patch
Gavin Peters
Comment 12 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.
Gavin Peters
Comment 13 2010-12-04 08:10:45 PST
Comment on attachment 75610 [details] Patch The removeClient is causing trouble on mac. Wait for more.
Gavin Peters
Comment 14 2010-12-04 09:09:55 PST
Created attachment 75611 [details] Patch
Gavin Peters
Comment 15 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.
Gavin Peters
Comment 16 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.
Adam Barth
Comment 17 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?
Gavin Peters
Comment 18 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.
Adam Barth
Comment 19 2010-12-04 12:34:46 PST
Maybe we should use an ASSERT with a comment to document that fact?
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2010-12-04 12:40:33 PST
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 22 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.
Note You need to log in before you can comment on or make changes to this bug.