[webkit-reviews] review granted: [Bug 17480] Implement speculative preloading : [Attachment 19694] Darin's comments etc.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 17480: Implement speculative preloading
http://bugs.webkit.org/show_bug.cgi?id=17480

Attachment 19694: Darin's comments etc.
http://bugs.webkit.org/attachment.cgi?id=19694&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+// FIXME: temporary, add PreloadScanner to all build files
+#if PLATFORM(MAC)
+#define PRELOAD_SCANNER_ENABLED 1
+#else
+#define PRELOAD_SCANNER_ENABLED 0
+#endif

I think this should go in the header, not the .cpp file.

+	     // printf("DOCUMENT.WRITE PRELOAD\n");

Please don't check in commented-out printf.

+#include "config.h"
+
+#include "PreloadScanner.h"

Normally we don't leave a blank line there.

+    // FIXME there is a table for more exceptions in the spec

We'd normally use a colon and use a sentence (capital first letter, period) in
a comment like this one.

It would be good to say which specification you're talking about. In general it
seems that a comment saying that this was derived from the HTML 5 specification
and which draft you used would be helpful.

+#define PreloadScanner_h
+
+#include "AtomicString.h"
+#include "PlatformString.h"
+#include "SegmentedString.h"

No need to include PlatformString.h here.

+#include <wtf/ListHashSet.h>
+#include <wtf/Noncopyable.h>
+#include <wtf/OwnPtr.h>

No reason to include ListHashSet.h or OwnPtr.h in this header.

+    unsigned m_preloadCount;
+    PreloadResult m_preloadResult;
+    bool m_requestedFromNetworkingLayer;

Why are these protected rather than private? Please make things private unless
there is a reason not to.

+static const unsigned maxRequestsInFlightForNonHTTPProtocols= 20;

+static const unsigned maxRequestsInFlightForNonHTTPProtocols= 10000;

Missing space before "=".

+#if REQUEST_DEBUG
+    KURL u(resource->url());
+    printf("HOST %s COUNT %d RECEIVED %s\n", u.host().latin1().data(),
m_requestsLoading.size(), resource->url().latin1().data());
+#endif

I think we need to add easier ways to log things. All this x.latin1().data()
and x.utf8().data() gets tiring after a while!

These are all minor editorial comments, so r=me