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

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

> Source/WebKit/chromium/ChangeLog:11
> +	   (WTF::ConvertOptions):

Please fix this. (It isn't WTF::ConvertOptions. The script that generates this
got confused.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Typically in WebKit land, additional years just get tacked on to the end as
opposed to replacing the other year. (I know this is different from how
chromium does things.)

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

No need for a blank line here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:61
> +    switch (options.crossOriginRequestPolicy) {

You could add compile asserts to verify that these enums have the same values.
Then replace the switch with a static cast.

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

Two things look wrong here:
1. adoptPtr should be used instead of PassOwnPtr.
2. Really, the typical pattern is to make the constructor private and and
expose a static create method in ClientAdapter which returns
PassOwnPtr<ClientAdapter> and does
return adoptPtr(new ClientAdapter(...));
internally.

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

This code looks the same as loadSynchronously. Can they share more code?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:199
> +	  m_loader->cancel();

Indentation appears not to be 4 spaces.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Same thing about the copyright years.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:42
> +class ThreadableLoaderClient;

ThreadableLoaderClient isn't used in this header (so remove the fwd decl.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:44
> +} // namespace WebCore

No need for the ending comment in WebKit (and in this case I would omit it
since the namespace is so small).

> Source/WebKit/chromium/src/AssociatedURLLoader.h:71
> +class AssociatedURLLoader : public WebURLLoader {

I would add a WTF_MAKE_NONCOPYABLE(AssociatedURLLoader); here (from
wtf/Noncopyable.h).

> Source/WebKit/chromium/src/AssociatedURLLoader.h:87
> +    class ClientAdapter;

This feels out of place. I would put it after private.