[webkit-reviews] review granted: [Bug 107807] Make the existing HTMLPreloadScanner threading-aware : [Attachment 187501] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 107807: Make the existing HTMLPreloadScanner threading-aware
https://bugs.webkit.org/show_bug.cgi?id=107807

Attachment 187501: Patch
https://bugs.webkit.org/attachment.cgi?id=187501&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187501&action=review


> Source/WebCore/ChangeLog:37
> +	     The preload scanners are handed a WeakPtr to the
HTMLResourcePreloader and speak to
> +	     it using callOnMainThread.

Is this still true?

> Source/WebCore/html/parser/CSSPreloadScanner.h:67
> +    // Only non-null during scan()

null -> zero

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:90
> +    , m_preloader(adoptPtr(new HTMLResourcePreloader(document)))

It seems like we could hold this as a member instead of via an OwnPtr.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:48
> +    return token.type() == HTMLTokenTypes::StartTag; // Holy verbosity,
batman.

This comment probably isn't necessary.

> Source/WebCore/html/parser/HTMLPreloadScanner.cpp:302
> +    PreloadTask task(tagName, token.attributes());

The PreloadTask seems like a historical appendage at this point.

> Source/WebCore/html/parser/HTMLResourcePreloader.cpp:59
> +CachedResourceRequest PreloadRequest::resourceRequest(Document* document)

resourceRequest -> createResourceRequest?

> Source/WebCore/html/parser/HTMLResourcePreloader.cpp:65
> +    // FIXME: It's possible CORS should work for other request types?

Yes.  It should work for images too.  Perhaps we should fix that in a followup
patch?

> Source/WebCore/html/parser/HTMLResourcePreloader.cpp:71
> +void HTMLResourcePreloader::preload(PassOwnPtr<PreloadRequest> preload)

It seems odd to separate PreloadRequest::resourceRequest from this function. 
Is there some advantage to creating the CachedResourceRequest in a separate
function?

> Source/WebCore/html/parser/HTMLResourcePreloader.h:34
> +class PreloadRequest {

In principle, PreloadRequest should have its own file.

> Source/WebCore/html/parser/HTMLResourcePreloader.h:59
> +public:

It seems odd to have some of the members be private and some be public. 
Perhaps these should be private and have getters/setters as appropriate?

> Source/WebCore/loader/cache/CachedResourceRequest.h:29
> +#include "Element.h"

Rather than adding this include here, why not add the include to the file that
needs it?