Bug 37449 - SecurityOrigin needs a way to remove individual OriginAccessEntries
Summary: SecurityOrigin needs a way to remove individual OriginAccessEntries
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Timothy Hatcher
Depends on:
Reported: 2010-04-12 09:48 PDT by Timothy Hatcher
Modified: 2010-04-13 15:33 PDT (History)
1 user (show)

See Also:

Proposed patch (32.27 KB, patch)
2010-04-12 10:27 PDT, Timothy Hatcher
aroben: review-
timothy: commit-queue-
Details | Formatted Diff | Diff
Rename part (45.01 KB, patch)
2010-04-13 00:51 PDT, Timothy Hatcher
hyatt: review+
timothy: commit-queue-
Details | Formatted Diff | Diff
Final part (33.37 KB, patch)
2010-04-13 10:21 PDT, Timothy Hatcher
hyatt: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2010-04-12 09:48:08 PDT
See summary.
Comment 1 Timothy Hatcher 2010-04-12 10:27:04 PDT
Created attachment 53176 [details]
Proposed patch
Comment 2 Adam Roben (:aroben) 2010-04-12 16:34:50 PDT
Comment on attachment 53176 [details]
Proposed patch

> +Testing: 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= destination origin=http:localhost".

> +Testing: 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).
Comment 3 Timothy Hatcher 2010-04-13 00:51:31 PDT
Created attachment 53230 [details]
Rename part
Comment 4 Timothy Hatcher 2010-04-13 10:21:58 PDT
Created attachment 53259 [details]
Final part
Comment 5 Dave Hyatt 2010-04-13 14:55:13 PDT
Comment on attachment 53259 [details]
Final part

Comment 6 Dave Hyatt 2010-04-13 14:55:34 PDT
Comment on attachment 53230 [details]
Rename part

Comment 7 Timothy Hatcher 2010-04-13 15:33:26 PDT
Fixed in r57535 and 57537.