Bug 73960 – [WK2] Add permissions support to web notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jon Lee]

Implement current permissions API for WK2.

<rdar://problem/10357008>

[Jon Lee]

Created attachment 118147 [details]
Patch

[Jon Lee]

Created attachment 118150 [details]
Patch

Added new files to Windows vcproj

[Gustavo Noronha (kov)]

Comment on attachment 118150 [details]
Patch

Attachment 118150 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10756001

[Jon Lee]

Created attachment 118226 [details]
Patch with GNU build fixes

[Gustavo Noronha (kov)]

Comment on attachment 118226 [details]
Patch with GNU build fixes

Attachment 118226 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10752207

[Jon Lee]

Created attachment 118234 [details]
Fix gtk again

[Collabora GTK+ EWS bot]

Comment on attachment 118234 [details]
Fix gtk again

Attachment 118234 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10748211

[Jon Lee]

Created attachment 118247 [details]
Fix gtk again and adjust some xcodeproj settings and paths

[Gustavo Noronha (kov)]

Comment on attachment 118247 [details]
Fix gtk again and adjust some xcodeproj settings and paths

Attachment 118247 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10767181

[Jon Lee]

Created attachment 118257 [details]
Trying again with gtk

[Darin Adler]

Comment on attachment 118257 [details]
Trying again with gtk

View in context: https://bugs.webkit.org/attachment.cgi?id=118257&action=review

> Source/WebKit2/UIProcess/Notifications/WebNotification.cpp:38
> +WebNotification::WebNotification()
> +{
> +}

Shouldn’t we set m_notificationID to 0 in this constructor? I suppose maybe not if we always just decode afterwards, but it seems slightly strange that the strings will both be initialized and the notification will not.

> Source/WebKit2/UIProcess/Notifications/WebNotification.cpp:49
> +WebNotification::~WebNotification()
> +{
> +}

I suggest not defining or declaring this and just letting the compiler generate it.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:53
> +WebNotificationManagerProxy::~WebNotificationManagerProxy()
> +{
> +}

Seems we should just let the compiler generate this and need not define or declare it.

> Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:62
> +NotificationPermissionRequestManager::~NotificationPermissionRequestManager()
> +{
> +}

Seems we should just let the compiler generate this.

[Jon Lee]

(In reply to comment #11)
> (From update of attachment 118257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118257&action=review
> 
> > Source/WebKit2/UIProcess/Notifications/WebNotification.cpp:38
> > +WebNotification::WebNotification()
> > +{
> > +}
> 
> Shouldn’t we set m_notificationID to 0 in this constructor? I suppose maybe not if we always just decode afterwards, but it seems slightly strange that the strings will both be initialized and the notification will not.
> 
> > Source/WebKit2/UIProcess/Notifications/WebNotification.cpp:49
> > +WebNotification::~WebNotification()
> > +{
> > +}
> 
> I suggest not defining or declaring this and just letting the compiler generate it.
> 
> > Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:53
> > +WebNotificationManagerProxy::~WebNotificationManagerProxy()
> > +{
> > +}
> 
> Seems we should just let the compiler generate this and need not define or declare it.
> 
> > Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:62
> > +NotificationPermissionRequestManager::~NotificationPermissionRequestManager()
> > +{
> > +}
> 
> Seems we should just let the compiler generate this.

Will do. Thank you!

[Jon Lee]

Committed r102352: <http://trac.webkit.org/changeset/102352>