[webkit-reviews] review denied: [Bug 53925] AssociatedURLLoader does not support Cross Origin Requests : [Attachment 81901] Change AssociatedURLLoader to support CORS, using DocumentThreadableLoader

[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 81901: Change AssociatedURLLoader to support CORS, using
DocumentThreadableLoader
https://bugs.webkit.org/attachment.cgi?id=81901&action=review

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

I tend to do two passes. In my first pass, I catch surface level things. In my
second pass I try to understand the whole patch and make sense of it.

These are the comments from my first pass but there are so many things that it
makes it harder for me to go more in depth right now, so I'm r- because I
believe that they can be quickly addressed which will get rid of a lot of
comment clutter in this review.

Also it appears that this patch is doing several things at once. It may speed
the process of landing parts and improve the quality of review to try to keep
these items separate if possible (and make my "second pass" easier). For
example, it looks like m_downloadFilePath is pushed downward and that could be
a patch by itself. (At this point I haven't yet figured out why that is needed
for this change because I haven't really started my "second pass".)

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

There either need to be tests or an explanation of why tests aren't needed (or
possible), but regardless this line should disappear.

> Source/WebCore/ChangeLog:11
> +	   (WebCore::DocumentThreadableLoader::DocumentThreadableLoader):

This is where an explanation of why a change was done should go.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:-301
> -    actualRequest->setHTTPOrigin(m_document->securityOrigin()->toString());

Why is this line being deleted?

> Source/WebCore/loader/DocumentThreadableLoader.h:58
> +	   virtual void setDefersLoading(bool value);

value adds no information so it shouldn't be here.

> Source/WebCore/platform/network/chromium/ResourceRequest.h:124
> +	   bool m_downloadToFile;

Good call adding it here, but this structure is used to copy
ResourceRequestData and I don't see that code being modified in this change.

> Source/WebCore/platform/network/chromium/ResourceResponse.h:-100
> -	   void setSocketAddress(const String& value) { m_socketAddress =
value; }

Why is this being deleted?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:37
> +

No blank line needed here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:80
> +// SubresourceLoaderClient, which is close to them WebURLLoaderClient.

typo? "close to them WebURLLoaderClient"

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:85
> +    Loader(AssociatedURLLoader* owner, Document* document, BlockingBehavior
blockingBehavior, const ResourceRequest& request, const
ThreadableLoaderOptions& options);

This constructor should be private (so that the create method is the only way
to construct this).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:87
> +    virtual void
AssociatedURLLoader::Loader::willSendRequest(SubresourceLoader* loader,
ResourceRequest& request, const ResourceResponse& redirectResponse);

s/AssociatedURLLoader::Loader::willSendRequest/willSendRequest/

The same for other functions declared here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:96
> +    class DummyClient : public ThreadableLoaderClient

Why not just use ThreadableLoaderClient?

>> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:121
>> +	: DocumentThreadableLoader(document, &m_dummyClient, blockingBehavior,
request, options), m_owner(owner) {
> 
> Place brace on its own line for function definitions.  [whitespace/braces]
[4]

The initializers should be on separate lines as well.

> Source/WebKit/chromium/src/WebURLResponse.cpp:383
> + 
m_private->m_resourceResponse->setDownloadFilePath(downloadFilePath.data());

4 space indent.