[webkit-reviews] review denied: [Bug 24853] Provide a way for WebKit clients to specify a more granular policy for cross-origin XHR access : [Attachment 35151] New patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Aaron Boodman <aa at chromium.org>'s
request for review:
Bug 24853: Provide a way for WebKit clients to specify a more granular policy
for cross-origin XHR access
https://bugs.webkit.org/show_bug.cgi?id=24853

Attachment 35151: New patch
https://bugs.webkit.org/attachment.cgi?id=35151&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +OriginAccessEntry::OriginAccessEntry(const String& protocol, const String&
host, SubdomainSetting subdomainSetting) : m_protocol(protocol), m_host(host),
m_allowSubdomains(subdomainSetting) {

We normally use a vertical format that puts each of these onto a separate line.


> +    ASSERT(protocol == "http" || protocol == "https");

A while back I had a patch that added a protocolIsInHTTPFamily() function for
strings to KURL.h. Looking, though, it seems I did not land it.

> +    UChar last = m_host[m_host.length() - 1];

Can host be an empty string? Does this work properly in that case? I guess it
does.

> +    m_hostIsIPAddress = last >= '0' && last <= '9';

CType.h's isASCIIDigit is the normal way we'd do this.

> +    if (m_protocol != origin.protocol())

Are these made canonically lowercase?

> +    if (m_host == origin.host())

Same question again.

> +    // FIXME: This hasn't actually been tested as I can't figure out a way
to test it with the layout test system.

This is good to note, but maybe not with a comment in the source code.

> +#include "CString.h"

You should be including PlatformString.h here in CrossOriginAccess.h, not
CString.h.

> +    OriginAccessEntry(const String& protocol, const String& host,
SubdomainSetting subdomainSetting);

Normally we would omit the subdomainSetting argument name.

> +    bool matchesOrigin(const SecurityOrigin& origin);

Normally we would omit the origin argument name. And make this a const member
function.

> +// static
> +void SecurityOrigin::whiteListAccessFromOrigin(const SecurityOrigin&
sourceOrigin, const String& destinationProtocol, const String&
destinationDomains, bool allowDestinationSubdomains)

We don't normally include that "// static" comment, and while it's nice to
know, it's not that useful to have it for this one function and not for the
many other static member functions.

> +    String sourceString = sourceOrigin.toString();
> +    OriginAccessWhiteList* list = originAccessMap().get(sourceString);

Applies to other call sites too. If the sourceString is empty or null then this
will crash. So we either need to know it's not empty or have a check for empty
string. Inelegant, I know.

> +	   list = new OriginAccessWhiteList();

No need for the parentheses here.

> +    for (OriginAccessMap::iterator iter = map.begin(); iter != map.end();
++iter) {
> +	   OriginAccessWhiteList* list = iter->second;
> +	   delete list;
> +    }

There's a function template that does this. It's called deleteAllValues.

> ++ (void)_whiteListAccessFromOrigin:(NSString*)sourceOrigin
destinationProtocol:(NSString*)destinationProtocol
destinationHost:(NSString*)destinationHost
allowDestinationSubdomains:(BOOL)allowDestinationSubdomains;

The semicolon at the end of this line is incorrect, albeit tolerated by the
compiler.

The confusing WebKit style rule is that for Objective-C types such as NSString,
we put the * after a space. I know, it's strange, but it's how we do it.

> +2009-08-19  Aaron Boodman  <aa at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=24853: Provide a way for
WebKit clients to
> +	   specify a more granular policy for cross-origin XHR access.
> +
> +	   * DumpRenderTree/LayoutTestController.cpp: Expose
whiteListAccessFromOrigin() to layout tests.
> +	   (whiteListAccessFromOriginCallback): Ditto.
> +	   (LayoutTestController::staticFunctions): Ditto.
> +	   * DumpRenderTree/LayoutTestController.h: Ditto.
> +	   * DumpRenderTree/gtk/LayoutTestControllerGtk.cpp: Ditto.
> +	   (LayoutTestController::whiteListAccessToOrigin): Ditto.
> +	   * DumpRenderTree/mac/LayoutTestControllerMac.mm: Ditto.
> +	   (LayoutTestController::whiteListAccessFromOrigin): Ditto.
> +	   * DumpRenderTree/qt/jsobjects.cpp: Ditto.
> +	   (LayoutTestController::whiteListAccessFromOrigin): Ditto.
> +	   * DumpRenderTree/win/LayoutTestControllerWin.cpp: Stub out
whiteListAccessFromOrigin().
> +	   (LayoutTestController::whiteListAccessFromOrigin): Ditto.
> +	   * DumpRenderTree/gtk/DumpRenderTree.cpp: Reset origin access lists
before each test.
> +	   (resetWebViewToConsistentStateBeforeTesting): Ditto.
> +	   * DumpRenderTree/mac/DumpRenderTree.mm: Ditto.
> +	   (resetWebViewToConsistentStateBeforeTesting): Ditto.
> +	   * DumpRenderTree/qt/DumpRenderTree.cpp: Ditto.
> +	   (WebCore::DumpRenderTree::resetToConsistentStateBeforeTesting):
Ditto.
> +
> +2009-08-18  Aaron Boodman  <aa at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=24853: Provide a way for
WebKit clients to
> +	   specify a more granular policy for cross-origin XHR access.
> +
> +	   * DumpRenderTree/LayoutTestController.cpp: Add plumbing to be able
to manipulate
> +	   origin access whitelists from layout tests.
> +	   (whiteListAccessToOriginCallback): Ditto.
> +	   (resetOriginAccessWhiteListsCallback): Ditto.
> +	   (LayoutTestController::staticFunctions): Ditto.
> +	   * DumpRenderTree/LayoutTestController.h: Ditto.
> +	   * DumpRenderTree/mac/LayoutTestControllerMac.mm: Ditto.
> +	   (LayoutTestController::whiteListAccessToOrigin): Ditto.
> +	   (LayoutTestController::resetOriginAccessWhiteLists): Ditto.
> +

Double change log.

Looks good.

review- because of the comments above