[webkit-reviews] review denied: [Bug 53925] AssociatedURLLoader does not support Cross Origin Requests : [Attachment 84328] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Bill Budge <bbudge at gmail.com>'s
request for review:
Bug 53925: AssociatedURLLoader does not support Cross Origin Requests
https://bugs.webkit.org/show_bug.cgi?id=53925

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84328&action=review

r- due to not returning PassRefPtr from constructor (and the question about
overflow).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:58
> +    static ClientAdapter* create(WebURLLoader*, WebURLLoaderClient*, bool
/*downloadToFile*/);

This should return a PassRefPtr<ClientAdapter>

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:80
> +    return new ClientAdapter(loader, client, downloadToFile);

Use the adoptPtr here (to convert to PassRefPtr).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:114
> +    m_downloadLength += lengthReceived;

I still wonder about overflow. (Not likely but overflow is where security
issues creep in.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:161
> +COMPILE_ASSERT_MATCHING_ENUM(UseAccessControl,  UseAccessControl);

two spaces after comma.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:162
> +COMPILE_ASSERT_MATCHING_ENUM(AllowCrossOriginRequests, 
AllowCrossOriginRequests);

Ditto.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:186
> +    m_clientAdapter = adoptPtr(ClientAdapter::create(this, m_client,
request.downloadToFile()));

adoptPtr shouldn't be here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:200
> +	  m_loader->setDefersLoading(defersLoading);

Should be indented one more space.