Bug 35846 – Kill applicationID() (part 1)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Aaron Boodman]

It turns out we don't need WebDocument::applicationID in Chromium after all, so remove it and supporting code from the notifications system. Since Chromium relies on this, it will be done in a couple parts.

First part (this bug):
- Change the signature of WebCore::NotificationPresenter::checkPermission() and change call sites
- Add the new signature to Chromium's WebKit port, but don't remove the old one yet.
- Remove the guts of WebDocument::applicationID(), but leave the stub.

Second part:
- Update Chromium call sites to the new methods

Third part:
- Remove the old checkPermission() and applicationID() methods from WebCore and WebKit

[Aaron Boodman]

Created attachment 50177 [details]
Patch

[WebKit Review Bot]

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

[Aaron Boodman]

Created attachment 50178 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 50178 [details]
Patch

We ordinarily pass a SecurityOrigin through the WebKit API as a WebSecurityOrigin.  That way if the consumer needs more than just SecurityOrigin::toString(), then they can have it.

[Aaron Boodman]

Ok, it was using WebString for origin originally, but I will fix it.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> Ok, it was using WebString for origin originally, but I will fix it.

Thanks!  The existing code probably pre-dates the addition of WebSecurityOrigin, or maybe someone didn't know about that interface.

[Aaron Boodman]

Created attachment 50345 [details]
Patch

[Aaron Boodman]

Ok, ready for another look Darin.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50345 [details]
Patch

> +++ b/WebKit/chromium/public/WebNotificationPresenter.h
...
> +    virtual Permission checkPermission(const WebSecurityOrigin& origin)
> +    {
> +        return PermissionNotAllowed;
> +    };

nit: no trailing semi-colon


> +++ b/WebKit/chromium/src/NotificationPresenterImpl.cpp
...
> +    int result = m_presenter->checkPermission(WebSecurityOrigin(origin));
> +
> +    // FIXME: Remove this once clients are updated to use the above signature.
> +    if (result == NotificationPresenter::PermissionNotAllowed)

nit: you can drop the NotificationPresenter:: prefix here.


> +++ b/WebKit/win/WebCoreSupport/WebDesktopNotificationsDelegate.cpp
> @@ -174,8 +174,13 @@ void WebDesktopNotificationsDelegate::requestPermission(SecurityOrigin* origin,
>  
>  NotificationPresenter::Permission WebDesktopNotificationsDelegate::checkPermission(const KURL& url, Document*)
>  {
> +    return (NotificationPresenter::Permission) 0;

perhaps it would be better to explicitly return one of the enum values?
0 means PermissionAllowed, right?


r=me otherwise.  cq- since there are some nits to fix.

[Aaron Boodman]

Created attachment 50359 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 50359 [details]
Patch

Clearing flags on attachment: 50359

Committed r55888: <http://trac.webkit.org/changeset/55888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.