Bug 80485 - Migrate permission functions to Notification from NotificationCenter
Summary: Migrate permission functions to Notification from NotificationCenter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 80573
Blocks: 80472
  Show dependency treegraph
 
Reported: 2012-03-06 21:41 PST by Jon Lee
Modified: 2012-05-03 05:43 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 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>
Comment 1 Jon Lee 2012-03-09 12:18:24 PST
Created attachment 131072 [details]
Patch
Comment 2 Early Warning System Bot 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
Comment 3 Early Warning System Bot 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
Comment 4 Jon Lee 2012-03-09 13:00:47 PST
Created attachment 131084 [details]
Patch
Comment 5 Early Warning System Bot 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
Comment 6 Early Warning System Bot 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
Comment 7 Jon Lee 2012-03-09 14:22:45 PST
Created attachment 131109 [details]
Patch
Comment 8 Early Warning System Bot 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
Comment 9 Early Warning System Bot 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
Comment 10 WebKit Review Bot 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
Comment 11 Gyuyoung Kim 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
Comment 12 Jon Lee 2012-03-09 22:50:33 PST
Created attachment 131154 [details]
Patch
Comment 13 Jon Lee 2012-03-09 23:01:25 PST
Created attachment 131156 [details]
Patch
Comment 14 Early Warning System Bot 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
Comment 15 WebKit Review Bot 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
Comment 16 Early Warning System Bot 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
Comment 17 Jon Lee 2012-03-09 23:27:35 PST
Created attachment 131157 [details]
Patch
Comment 18 WebKit Review Bot 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
Comment 19 Gyuyoung Kim 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
Comment 20 Jon Lee 2012-03-10 00:36:11 PST
Created attachment 131163 [details]
Patch
Comment 21 WebKit Review Bot 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
Comment 22 Jon Lee 2012-03-10 01:01:01 PST
Created attachment 131164 [details]
Patch
Comment 23 WebKit Review Bot 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
Comment 24 Gyuyoung Kim 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
Comment 25 Jon Lee 2012-03-10 12:44:05 PST
Created attachment 131184 [details]
Patch
Comment 26 Jon Lee 2012-03-10 13:11:32 PST
Created attachment 131185 [details]
Patch
Comment 27 WebKit Review Bot 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
Comment 28 Jon Lee 2012-03-10 13:51:23 PST
Created attachment 131189 [details]
Patch
Comment 29 Jon Lee 2012-03-10 15:44:42 PST
Comment on attachment 131189 [details]
Patch

This is pending discussion in the WG. Removing r? for now.
Comment 30 Jon Lee 2012-03-16 00:58:41 PDT
Created attachment 132223 [details]
Patch
Comment 31 WebKit Review Bot 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
Comment 32 Build Bot 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
Comment 33 WebKit Review Bot 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
Comment 34 WebKit Review Bot 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
Comment 35 WebKit Review Bot 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
Comment 36 WebKit Review Bot 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
Comment 37 Jon Lee 2012-04-27 02:14:49 PDT
Created attachment 139152 [details]
Patch
Comment 38 Early Warning System Bot 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
Comment 39 Early Warning System Bot 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
Comment 40 Jon Lee 2012-04-27 23:40:16 PDT
Created attachment 139343 [details]
Patch
Comment 41 Jon Lee 2012-04-28 13:00:29 PDT
Created attachment 139370 [details]
Patch
Comment 42 WebKit Review Bot 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
Comment 43 Jon Lee 2012-04-28 13:41:51 PDT
Created attachment 139372 [details]
Patch
Comment 44 Jian Li 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 "*".
Comment 45 Jon Lee 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.
Comment 46 Jon Lee 2012-05-03 00:05:53 PDT
Committed r115943: <http://trac.webkit.org/changeset/115943>
Comment 48 Andrey Kosyakov 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