Bug 80485

Summary:

Migrate permission functions to Notification from NotificationCenter

Product:

WebKit

Reporter:

Jon Lee <jonlee>

Component:

DOM

Assignee:

Jon Lee <jonlee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, caseq, dglazkov, haraken, japhet, ojan, rakuco, vestbo, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.7

Bug Depends on:

80573

Bug Blocks:

80472

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	jianli: review+

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.