80485 – Migrate permission functions to Notification from NotificationCenter

RESOLVED FIXED 80485

Migrate permission functions to Notification from NotificationCenter

https://bugs.webkit.org/show_bug.cgi?id=80485

Summary Migrate permission functions to Notification from NotificationCenter

Jon Lee

Reported 2012-03-06 21:41:37 PST

1. Move NotificationCenter.checkPermission() to Notification.permissionLevel(). Update to return string instead of enum. 2. Move NotificationCenter.requestPermission() to Notification.requestPermission. <rdar://problem/10965458>

Attachments
Patch (47.06 KB, patch) 2012-03-09 12:18 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (47.09 KB, patch) 2012-03-09 13:00 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (50.29 KB, patch) 2012-03-09 14:22 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (56.90 KB, patch) 2012-03-09 22:50 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (56.83 KB, patch) 2012-03-09 23:01 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (57.42 KB, patch) 2012-03-09 23:27 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (59.50 KB, patch) 2012-03-10 00:36 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (60.17 KB, patch) 2012-03-10 01:01 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (60.18 KB, patch) 2012-03-10 12:44 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (60.89 KB, patch) 2012-03-10 13:11 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (60.91 KB, patch) 2012-03-10 13:51 PST, Jon Lee	no flags	Details Formatted Diff Diff
Patch (61.66 KB, patch) 2012-03-16 00:58 PDT, Jon Lee	no flags	Details Formatted Diff Diff
Patch (63.17 KB, patch) 2012-04-27 02:14 PDT, Jon Lee	no flags	Details Formatted Diff Diff
Patch (69.55 KB, patch) 2012-04-27 23:40 PDT, Jon Lee	no flags	Details Formatted Diff Diff
Patch (69.67 KB, patch) 2012-04-28 13:00 PDT, Jon Lee	no flags	Details Formatted Diff Diff
Patch (69.69 KB, patch) 2012-04-28 13:41 PDT, Jon Lee	jianli: review+	Details Formatted Diff Diff
Show Obsolete (15) View All Add attachment proposed patch, testcase, etc.

Jon Lee

Comment 1 2012-03-09 12:18:24 PST

Created attachment 131072 [details] Patch

Early Warning System Bot

Comment 2 2012-03-09 12:53:36 PST

Comment on attachment 131072 [details] Patch Attachment 131072 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11904703

Early Warning System Bot

Comment 3 2012-03-09 12:58:52 PST

Comment on attachment 131072 [details] Patch Attachment 131072 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11904705

Jon Lee

Comment 4 2012-03-09 13:00:47 PST

Created attachment 131084 [details] Patch

Early Warning System Bot

Comment 5 2012-03-09 14:05:10 PST

Comment on attachment 131084 [details] Patch Attachment 131084 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11906749

Early Warning System Bot

Comment 6 2012-03-09 14:14:11 PST

Comment on attachment 131084 [details] Patch Attachment 131084 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11898733

Jon Lee

Comment 7 2012-03-09 14:22:45 PST

Created attachment 131109 [details] Patch

Early Warning System Bot

Comment 8 2012-03-09 15:30:18 PST

Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11903824

Early Warning System Bot

Comment 9 2012-03-09 16:25:56 PST

Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11927003

WebKit Review Bot

Comment 10 2012-03-09 16:48:23 PST

Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11906808

Gyuyoung Kim

Comment 11 2012-03-09 17:39:45 PST

Comment on attachment 131109 [details] Patch Attachment 131109 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11894891

Jon Lee

Comment 12 2012-03-09 22:50:33 PST

Created attachment 131154 [details] Patch

Jon Lee

Comment 13 2012-03-09 23:01:25 PST

Created attachment 131156 [details] Patch

Early Warning System Bot

Comment 14 2012-03-09 23:20:09 PST

Comment on attachment 131156 [details] Patch Attachment 131156 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11915945

WebKit Review Bot

Comment 15 2012-03-09 23:22:16 PST

Comment on attachment 131156 [details] Patch Attachment 131156 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11915944

Early Warning System Bot

Comment 16 2012-03-09 23:22:43 PST

Comment on attachment 131156 [details] Patch Attachment 131156 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11906987

Jon Lee

Comment 17 2012-03-09 23:27:35 PST

Created attachment 131157 [details] Patch

WebKit Review Bot

Comment 18 2012-03-09 23:43:50 PST

Comment on attachment 131157 [details] Patch Attachment 131157 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11906994

Gyuyoung Kim

Comment 19 2012-03-09 23:49:51 PST

Comment on attachment 131157 [details] Patch Attachment 131157 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11906999

Jon Lee

Comment 20 2012-03-10 00:36:11 PST

Created attachment 131163 [details] Patch

WebKit Review Bot

Comment 21 2012-03-10 00:52:50 PST

Comment on attachment 131163 [details] Patch Attachment 131163 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11937011

Jon Lee

Comment 22 2012-03-10 01:01:01 PST

Created attachment 131164 [details] Patch

WebKit Review Bot

Comment 23 2012-03-10 01:15:58 PST

Comment on attachment 131164 [details] Patch Attachment 131164 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11931019

Gyuyoung Kim

Comment 24 2012-03-10 01:28:31 PST

Comment on attachment 131164 [details] Patch Attachment 131164 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11935047

Jon Lee

Comment 25 2012-03-10 12:44:05 PST

Created attachment 131184 [details] Patch

Jon Lee

Comment 26 2012-03-10 13:11:32 PST

Created attachment 131185 [details] Patch

WebKit Review Bot

Comment 27 2012-03-10 13:33:45 PST

Comment on attachment 131185 [details] Patch Attachment 131185 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11934231

Jon Lee

Comment 28 2012-03-10 13:51:23 PST

Created attachment 131189 [details] Patch

Jon Lee

Comment 29 2012-03-10 15:44:42 PST

Comment on attachment 131189 [details] Patch This is pending discussion in the WG. Removing r? for now.

Jon Lee

Comment 30 2012-03-16 00:58:41 PDT

Created attachment 132223 [details] Patch

WebKit Review Bot

Comment 31 2012-03-16 01:23:37 PDT

Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11960274

Build Bot

Comment 32 2012-03-16 02:21:36 PDT

Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11967045

WebKit Review Bot

Comment 33 2012-03-16 02:33:16 PDT

Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11964100

WebKit Review Bot

Comment 34 2012-03-16 03:45:54 PDT

Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11965113

WebKit Review Bot

Comment 35 2012-03-16 04:57:18 PDT

Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11962226

WebKit Review Bot

Comment 36 2012-03-16 05:52:55 PDT

Comment on attachment 132223 [details] Patch Attachment 132223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11968036

Jon Lee

Comment 37 2012-04-27 02:14:49 PDT

Created attachment 139152 [details] Patch

Early Warning System Bot

Comment 38 2012-04-27 02:33:29 PDT

Comment on attachment 139152 [details] Patch Attachment 139152 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12549145

Early Warning System Bot

Comment 39 2012-04-27 02:41:49 PDT

Comment on attachment 139152 [details] Patch Attachment 139152 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12557138

Jon Lee

Comment 40 2012-04-27 23:40:16 PDT

Created attachment 139343 [details] Patch

Jon Lee

Comment 41 2012-04-28 13:00:29 PDT

Created attachment 139370 [details] Patch

WebKit Review Bot

Comment 42 2012-04-28 13:28:35 PDT

Comment on attachment 139370 [details] Patch Attachment 139370 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12555707

Jon Lee

Comment 43 2012-04-28 13:41:51 PDT

Created attachment 139372 [details] Patch

Jian Li

Comment 44 2012-04-30 13:03:26 PDT

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 "*".

Jon Lee

Comment 45 2012-04-30 13:41:14 PDT

(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.

Jon Lee

Comment 46 2012-05-03 00:05:53 PDT

Committed r115943: <http://trac.webkit.org/changeset/115943>

Andrey Kosyakov

Comment 47 2012-05-03 04:05:26 PDT

(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

Andrey Kosyakov

Comment 48 2012-05-03 05:43:41 PDT

(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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Mac

OS OS X 10.7

Product WebKit

Component DOM

Assignee

Jon Lee

Reported

2012-03-06 21:41 PST

Modified

2012-05-03 05:43 PDT History

CC List

10 users Show

URL

Keywords InRadar

Depends on

80573

Blocks

80472

Dependencies

tree graph