[chromium] Use the security origin instead of the URL when checking notification permissions

[Daniel Cheng]

[chromium] Use the security origin instead of the URL when checking notification permissions

[Daniel Cheng]

Created attachment 113718 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 113718 [details]
Patch

Attachment 113718 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10332103

[Adam Barth]

Comment on attachment 113718 [details]
Patch

Technically fishd should review this patch.

If you've updated Chromium already, you might need to update the chromium_rev in Source/WebKit/chromium/DEPS

[Daniel Cheng]

Yep, I'm waiting for fishd and verifying that rolling the chromium DEPS in this patch will fix it.

[Daniel Cheng]

Created attachment 113723 [details]
Patch

[Daniel Cheng]

One weird thing I noticed--it looks like WebString has a conversion operator to WTF::String (http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/public/WebString.h&exact_package=chromium&q=WebString&type=cs&l=104). However... it doesn't seem to work and I need to manually construct a WTF::String. Am I doing something wrong?

[Adam Barth]

Comment on attachment 113723 [details]
Patch

LGTM, but please wait for a thumbs up from fishd before landing.

[WebKit Review Bot]

Comment on attachment 113723 [details]
Patch

Attachment 113723 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10332122

New failing tests:
fast/notifications/notifications-display-close-events.html
fast/notifications/notifications-click-event.html
fast/notifications/notifications-check-permission.html
fast/notifications/notifications-rtl.html
fast/notifications/notifications-no-icon.html
fast/notifications/notifications-replace.html
fast/notifications/notifications-with-permission.html
fast/notifications/notifications-double-show.html

[Daniel Cheng]

Comment on attachment 113723 [details]
Patch

R- until I figure out how to fix the tests. Oddly enough the corresponding Chrome extension tests still seem to work.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113723 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113723&action=review

> Source/WebKit/chromium/public/WebNotificationPresenter.h:63
> +    virtual Permission checkPermission(const WebSecurityOrigin&) = 0;

WebKit API changes LGTM

[Daniel Cheng]

Committed r99368: <http://trac.webkit.org/changeset/99368>

[Yuta Kitamura]

This change caused compile errors on Chromium canary builders:
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/15854

Could you fix them or roll out your change at your earliest convenience? (I'm on  limited network connection right now and can't fix this for you.)

[Daniel Cheng]

(In reply to comment #13)
> This change caused compile errors on Chromium canary builders:
> http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/15854
> 
> Could you fix them or roll out your change at your earliest convenience? (I'm on  limited network connection right now and can't fix this for you.)

Sorry for not catching this locally. I have a Chromium-side fix ready to go, I'm waiting for the Chromium tree to re-open.

[Daniel Cheng]

Chromium r108814 should fix this. I'll clobber the Windows builder to be sure.

[Yuta Kitamura]

I confirmed the fix. Thanks!