Bug 35846 - Kill applicationID() (part 1)
Summary: Kill applicationID() (part 1)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-07 14:18 PST by Aaron Boodman
Modified: 2010-03-11 22:12 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.75 KB, patch)
2010-03-07 14:24 PST, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (12.31 KB, patch)
2010-03-07 15:49 PST, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (13.78 KB, patch)
2010-03-09 14:00 PST, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (13.78 KB, patch)
2010-03-09 16:00 PST, Aaron Boodman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2010-03-07 14:18:16 PST
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
Comment 1 Aaron Boodman 2010-03-07 14:24:53 PST
Created attachment 50177 [details]
Patch
Comment 2 WebKit Review Bot 2010-03-07 14:31:15 PST
Attachment 50177 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/344325
Comment 3 Aaron Boodman 2010-03-07 15:49:46 PST
Created attachment 50178 [details]
Patch
Comment 4 Darin Fisher (:fishd, Google) 2010-03-08 09:45:28 PST
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.
Comment 5 Aaron Boodman 2010-03-08 09:49:35 PST
Ok, it was using WebString for origin originally, but I will fix it.
Comment 6 Darin Fisher (:fishd, Google) 2010-03-08 09:52:15 PST
(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.
Comment 7 Aaron Boodman 2010-03-09 14:00:55 PST
Created attachment 50345 [details]
Patch
Comment 8 Aaron Boodman 2010-03-09 14:02:54 PST
Ok, ready for another look Darin.
Comment 9 Darin Fisher (:fishd, Google) 2010-03-09 15:53:00 PST
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.
Comment 10 Aaron Boodman 2010-03-09 16:00:45 PST
Created attachment 50359 [details]
Patch
Comment 11 WebKit Commit Bot 2010-03-11 22:12:04 PST
Comment on attachment 50359 [details]
Patch

Clearing flags on attachment: 50359

Committed r55888: <http://trac.webkit.org/changeset/55888>
Comment 12 WebKit Commit Bot 2010-03-11 22:12:09 PST
All reviewed patches have been landed.  Closing bug.