[webkit-reviews] review denied: [Bug 53925] AssociatedURLLoader does not support Cross Origin Requests : [Attachment 84301] 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 84301: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=84301&action=review

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

A few comments.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:71
> +    int m_downloadLength;

Can this overflow?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:79
> +    , m_downloadingToFile(downloadingToFile) { }

put the {} on different lines.

Would be nice to assert(!client); to make it clear that it should have a value.


> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:160
> +COMPILE_ASSERT((int)WebKit::DenyCrossOriginRequests ==
(int)WebCore::DenyCrossOriginRequests, enum_mismatch);

If these casts remain (in another version), use C++ style casts (static_cast).

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

Have a static create method do this and make the constructor private.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:35
>  #include "WebURLLoaderClient.h"

This include isn't needed anymore (only a fwd declaration).