Summary: | Kill applicationID() (part 1) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Boodman <aa> | ||||||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, fishd, johnnyg, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Aaron Boodman
2010-03-07 14:18:16 PST
Created attachment 50177 [details]
Patch
Attachment 50177 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/344325 Created attachment 50178 [details]
Patch
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.
Ok, it was using WebString for origin originally, but I will fix it. (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. Created attachment 50345 [details]
Patch
Ok, ready for another look Darin. 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. Created attachment 50359 [details]
Patch
Comment on attachment 50359 [details] Patch Clearing flags on attachment: 50359 Committed r55888: <http://trac.webkit.org/changeset/55888> All reviewed patches have been landed. Closing bug. |