[Webkit-unassigned] [Bug 53925] AssociatedURLLoader does not support Cross Origin Requests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53925


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81901|review?                     |review-
               Flag|                            |




--- Comment #4 from David Levin <levin at chromium.org>  2011-02-09 18:06:23 PST ---
(From update of attachment 81901)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.