1. Move NotificationCenter.checkPermission() to Notification.permissionLevel(). Update to return string instead of enum. 2. Move NotificationCenter.requestPermission() to Notification.requestPermission. <rdar://problem/10965458>
Created attachment 131072 [details] Patch
Comment on attachment 131072 [details] Patch Attachment 131072 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11904703
Comment on attachment 131072 [details] Patch Attachment 131072 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11904705
Created attachment 131084 [details] Patch
Comment on attachment 131084 [details] Patch Attachment 131084 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11906749
Comment on attachment 131084 [details] Patch Attachment 131084 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11898733
Created attachment 131109 [details] Patch
Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11903824
Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11927003
Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11906808
Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11894891
Created attachment 131154 [details] Patch
Created attachment 131156 [details] Patch
Comment on attachment 131156 [details] Patch Attachment 131156 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11915945
Comment on attachment 131156 [details] Patch Attachment 131156 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11915944
Comment on attachment 131156 [details] Patch Attachment 131156 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11906987
Created attachment 131157 [details] Patch
Comment on attachment 131157 [details] Patch Attachment 131157 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11906994
Comment on attachment 131157 [details] Patch Attachment 131157 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11906999
Created attachment 131163 [details] Patch
Comment on attachment 131163 [details] Patch Attachment 131163 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11937011
Created attachment 131164 [details] Patch
Comment on attachment 131164 [details] Patch Attachment 131164 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11931019
Comment on attachment 131164 [details] Patch Attachment 131164 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11935047
Created attachment 131184 [details] Patch
Created attachment 131185 [details] Patch
Comment on attachment 131185 [details] Patch Attachment 131185 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11934231
Created attachment 131189 [details] Patch
Comment on attachment 131189 [details] Patch This is pending discussion in the WG. Removing r? for now.
Created attachment 132223 [details] Patch
Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11960274
Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11967045
Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11964100
Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11965113
Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11962226
Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11968036
Created attachment 139152 [details] Patch
Comment on attachment 139152 [details] Patch Attachment 139152 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12549145
Comment on attachment 139152 [details] Patch Attachment 139152 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12557138
Created attachment 139343 [details] Patch
Created attachment 139370 [details] Patch
Comment on attachment 139370 [details] Patch Attachment 139370 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12555707
Created attachment 139372 [details] Patch
Comment on attachment 139372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139372&action=review > Source/WebCore/bindings/v8/custom/V8NotificationCustom.cpp:42 > + ScriptExecutionContext* context = notification->scriptExecutionContext(); Better to move these 2 lines just before its 1st access. > Source/WebCore/notifications/Notification.cpp:254 > +#if ENABLE(NOTIFICATIONS) Can you wrap all 3 new methods and existing method taskTimerFired in one single #if guard? > Source/WebCore/notifications/Notification.h:142 > + static const String& stringForPermission(NotificationClient::Permission); Please add const modifier. In addition, it might be simpler to call it permissionString. > Source/WebCore/notifications/NotificationCenter.idl:46 > +#if defined(ENABLE_LEGACY_NOTIFICATIONS) && ENABLE_LEGACY_NOTIFICATIONS It seems that all methods exposed in NotificationCenter interface are wrapped inside ENABLE_LEGACY_NOTIFICATIONS. Do we also want to put NotificationCenter under ENABLE_LEGACY_NOTIFICATIONS? > Source/WebKit/chromium/src/NotificationPresenterImpl.h:63 > + virtual void requestPermission(WebCore::ScriptExecutionContext* , WTF::PassRefPtr<WebCore::NotificationPermissionCallback> callback) { } Extra space after "*". > Source/WebKit/mac/WebCoreSupport/WebNotificationClient.mm:170 > +void WebNotificationClient::requestPermission(ScriptExecutionContext* context, WebNotificationPolicyListener *listener) Space should be placed after "*".
(In reply to comment #44) > (From update of attachment 139372 [details]) > > Source/WebCore/notifications/Notification.h:142 > > + static const String& stringForPermission(NotificationClient::Permission); > > Please add const modifier. In addition, it might be simpler to call it permissionString. I don't think you can have const static functions, can you? Otherwise, thanks for the other feedback, I'll incorporate that into the checked-in patch.
Committed r115943: <http://trac.webkit.org/changeset/115943>
(In reply to comment #46) > Committed r115943: <http://trac.webkit.org/changeset/115943> This broke chromium win build: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/24031/steps/compile/logs/stdio
(In reply to comment #47) > This broke chromium win build: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20Builder/builds/24031/steps/compile/logs/stdio Fixed chromium win build with r115960: http://trac.webkit.org/changeset/115960