[webkit-reviews] review denied: [Bug 3652] add support for link prefetching : [Attachment 57543] Fix silly merge error in previous patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 10 10:33:28 PDT 2010


Adam Barth <abarth at webkit.org> has denied Leon Clarke <leonclarke at google.com>'s
request for review:
Bug 3652: add support for link prefetching
https://bugs.webkit.org/show_bug.cgi?id=3652

Attachment 57543: Fix silly merge error in previous patch
https://bugs.webkit.org/attachment.cgi?id=57543&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?


More information about the webkit-reviews mailing list