[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