Bug 37578 – [Chromium] Consider implementing addOriginAccessWhitelistEntry method

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[anton muhin]

http://trac.webkit.org/changeset/57535/ brought this into Safari.

For now those layout tests:

http/tests/xmlhttprequest/origin-whitelisting-all.html
http/tests/xmlhttprequest/origin-whitelisting-exact-match.html
http/tests/xmlhttprequest/origin-whitelisting-https.html
http/tests/xmlhttprequest/origin-whitelisting-ip-addresses-with-
subdomains.html
http/tests/xmlhttprequest/origin-whitelisting-ip-addresses.html
http/tests/xmlhttprequest/origin-whitelisting-subdomains.html
http/tests/security/local-image-from-remote-whitelisted.html
http/tests/xmlhttprequest/origin-whitelisting-removal.html

are skipped for Chromium.

[anton muhin]

https://bugs.webkit.org/show_bug.cgi?id=37577 modified Chromium's test_expectations to skip the tests.

[David Levin]

This simply looks like a work item for Chromium's DRT/test_shell.

The work involved would be to expose this through the Chromium WebKit API and then use it in test_shell. Similar to what was done for Windows and OSX in https://bugs.webkit.org/attachment.cgi?id=53259&action=prettypatch

Anton, perhaps you could tackle this as a clean up task?

[anton muhin]

(In reply to comment #2)
> This simply looks like a work item for Chromium's DRT/test_shell.
> 
> The work involved would be to expose this through the Chromium WebKit API and
> then use it in test_shell. Similar to what was done for Windows and OSX in
> https://bugs.webkit.org/attachment.cgi?id=53259&action=prettypatch
> 
> Anton, perhaps you could tackle this as a clean up task?

David, I'll try to implement it, but probably somewhat later.  Or you think this is urgent?

[David Levin]

(In reply to comment #3)
> David, I'll try to implement it, but probably somewhat later.  Or you think
> this is urgent?

Not urgent.

[anton muhin]

Some more updates.

There were two relevant changes:

http://trac.webkit.org/changeset/57535 renamed two methods and
http://trac.webkit.org/changeset/57537 which added another one.

My current plan would be to add new method into 
WebKit/WebKit/chromium and then submit a change to Chromium's test_shell which would both add new method and rename two renamed ones.

After that we probably need two-sided patch to rename methods in Chromium's bridge API.

Does it sound sane?

[anton muhin]

Created attachment 54566 [details]
Patch

[anton muhin]

(In reply to comment #6)
> Created an attachment (id=54566) [details]
> Patch

Name---unwhitelist....---is rather poor, but ideally we should move to add/remove as in SecurityPolicy.

[David Levin]

Adding Darin since this is a public api change.

[Darin Fisher (:fishd, Google)]

Comment on attachment 54566 [details]
Patch

WebKit/chromium/src/WebSecurityPolicy.cpp:69
 +  void WebSecurityPolicy::unwhiteListAccessFromOrigin(const WebURL& sourceOrigin,
Why not use the same name as the method on SecurityOrigin?  I have the same question about whiteListAccessFromOrigin.

Oh, I see... in r57535, SecurityOrigin::whiteListAccessFromOrigin was renamed addOriginAccessWhitelistEntry :-(

How about writing a patch that switches us over to the same method names as SecurityOrigin so that we can avoid the awkward sounding "unwhiteListFoo" method name?

[Darin Fisher (:fishd, Google)]

I recommend doing two WebKit patches here: first patch adds the new method names, and the second patch removes the old method name.  The chromium patch that you need to do to call the newly added "remove" function can also include a change to call the new form of the "add" function.  Do that step before you submit the second WebKit patch.

[anton muhin]

Created attachment 54707 [details]
Patch

[anton muhin]

(In reply to comment #10)
> I recommend doing two WebKit patches here: first patch adds the new method
> names, and the second patch removes the old method name.  The chromium patch
> that you need to do to call the newly added "remove" function can also include
> a change to call the new form of the "add" function.  Do that step before you
> submit the second WebKit patch.

Sure, Darin, and thanks a lot for comments.

Please notice that there is almost invisible change in resetOriginAccessWhiteLists name: White<L>ists -> White<l>ists.

May you have another look?

[Darin Fisher (:fishd, Google)]

Comment on attachment 54707 [details]
Patch

> +// To be removed when Chromium's test_shell has proper references.
> +void WebSecurityPolicy::whiteListAccessFromOrigin(

Please add DEPRECATED to comments like the above, so that 
it is easier to grep for these dead bits.

Otherwise R=me

[anton muhin]

Created attachment 54712 [details]
Adding DEPRECATED tags

[anton muhin]

(In reply to comment #13)
> (From update of attachment 54707 [details])
> > +// To be removed when Chromium's test_shell has proper references.
> > +void WebSecurityPolicy::whiteListAccessFromOrigin(
> 
> Please add DEPRECATED to comments like the above, so that 
> it is easier to grep for these dead bits.
> 
> Otherwise R=me

Thanks, Darin.  Could you r+ new patch if it's fine---I'd rather to use commit-queue as this patch is not urgent.

[David Levin]

Rather than make Darin re-review it if this is something you would submit given Darin's r+, then you should upload the patch with the reviewed lines fixed as if you were about to check it in and then just mark the patch as cq+ (and no r? or r+, etc. -- just leave that blank).  The commit queue should handle that then.

[David Levin]

Comment on attachment 54712 [details]
Adding DEPRECATED tags

See comment above.

[anton muhin]

Created attachment 54714 [details]
Same as previous, to bypass r+

[anton muhin]

(In reply to comment #17)
> (From update of attachment 54712 [details])
> See comment above.

Thanks a lot, David.  Trying this approach.  Sorry for noise.

[anton muhin]

Created attachment 54715 [details]
Now with correct Reviewed by

[WebKit Commit Bot]

Comment on attachment 54715 [details]
Now with correct Reviewed by

Clearing flags on attachment: 54715

Committed r58530: <http://trac.webkit.org/changeset/58530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

Created attachment 55247 [details]
Remove deprecated methods

[WebKit Review Bot]

Attachment 55247 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2231020

[anton muhin]

Created attachment 55248 [details]
Adding DRT files

[anton muhin]

Thanks a lot, Darin.

[Eric Seidel (no email)]

I think this patch i supposed to be landed.  So reopening the bug so the commit-queue will see it.

[WebKit Commit Bot]

Comment on attachment 55248 [details]
Adding DRT files

Rejecting patch 55248 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--force']" exit_code: 1
Last 500 characters of output:
atching file WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp
Hunk #1 FAILED at 113.
Hunk #2 FAILED at 469.
Hunk #3 succeeded at 1109 with fuzz 1 (offset 56 lines).
2 out of 3 hunks FAILED -- saving rejects to file WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp.rej
patching file WebKitTools/DumpRenderTree/chromium/LayoutTestController.h
Hunk #1 FAILED at 230.
1 out of 1 hunk FAILED -- saving rejects to file WebKitTools/DumpRenderTree/chromium/LayoutTestController.h.rej

Full output: http://webkit-commit-queue.appspot.com/results/2281052

[anton muhin]

Created attachment 56063 [details]
Resolve a conflict

[anton muhin]

Comment on attachment 56063 [details]
Resolve a conflict

as cq got stuck, someone else partially fixed the issue this patch was supposed to fix.  Merge was needed. cq+ it again.

[WebKit Commit Bot]

Comment on attachment 56063 [details]
Resolve a conflict

Clearing flags on attachment: 56063

Committed r59512: <http://trac.webkit.org/changeset/59512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

Created attachment 56698 [details]
Patch

[anton muhin]

And the last patch, hopefully.

Try bot results: http://codereview.chromium.org/2104016

[WebKit Commit Bot]

Comment on attachment 56698 [details]
Patch

Clearing flags on attachment: 56698

Committed r60033: <http://trac.webkit.org/changeset/60033>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

Thanks a lot, Darin.  Now that should be closed for the last time.