[webkit-reviews] review denied: [Bug 37449] SecurityOrigin needs a way to remove individual OriginAccessEntries : [Attachment 53176] Proposed patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 37449: SecurityOrigin needs a way to remove individual OriginAccessEntries
https://bugs.webkit.org/show_bug.cgi?id=37449

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +Testing: http://127.0.0.1:8000 http:localhost 

I find this output a little confusing to parse. Maybe adding "source origin="
and "destination origin=" would make it clearer, e.g., "Testing: source
origin=http://127.0.0.1:8000 destination origin=http:localhost".

> +Testing: http://127.0.0.1:8000 http:localhost allowing subdoamins

Typo: subdoamins

> +function nextTest()
> +{
> +    if (currentTest < tests.length)
> +	   test.apply(null, tests[currentTest++]);
> +    else
> +	   layoutTestController.notifyDone();
> +}

Seems like a for loop or Array.forEach would work just fine.

> +	   (WebCore::SecurityOrigin::whiteListAccessFromOrigin): Use the add
method to preent a
> +	   second hash lookup.

Typo: preent

> +	   (WebCore::SecurityOrigin::removeWhiteListAccessFromOrigin): Added.
Find a matching
> +	   OriginAccessEntry and remove it.

This name is unfortunately not parallel with "whiteListAccessFromOrigin". That
function uses the verb "white list", while your new function uses the noun
"white list access". We either need a verb that means "un-white list", or we
should rename whiteListAccessFromOrigin. Maybe
addOriginAccessWhitelistEntry/removeOriginAccessWhitelistEntry?

> +inline bool operator==(const OriginAccessEntry& a, const OriginAccessEntry&
b)
> +{
> +    return a.protocol() == b.protocol() && a.host() == b.host() &&
a.subdomainSettings() == b.subdomainSettings();
> +}

Protocols and hosts are supposed to be treated case-insensitively.

r- because of the case-sensitivity issue (maybe you should add a test for
this), but otherwise this looks good to go (though figuring out better names
would be nice).