Bug 96171 – [Qt] Implement WebNotification in WebKit2 port

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Qt

[Olivier Crête]

The WebKit2 port is lacking support for WebNotifications.

There are 2 parts of this patch:

1. Inform the NotificationManager of the application/user's decision.
1.1 Verify that the permission was really granted
2. Implement a NotificationProvider

[Olivier Crête]

Created attachment 162931 [details]
Patch to implement web notifications on the WebKit2/qt port

I also exposed the allowFileAccessFromFileURLs preference so that Notifications permissions can be tested from a file:/// URL

[WebKit Review Bot]

Attachment 162931 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:170:  Declaration has space between type name and * in QQuickWebView *webView  [whitespace/declaration] [3]
Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 162931 [details]
Patch to implement web notifications on the WebKit2/qt port

Attachment 162931 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13772981

[Simon Hausmann]

Comment on attachment 162931 [details]
Patch to implement web notifications on the WebKit2/qt port

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

A few comments.

    1) The patch doesn't compile (see EWS) and has coding style issues.
    2) QSystemTrayIcon is part of QtWidgets, which we choose not to use in the WebKit2 API.
    3) Conceptually I'm not sure if QSystemTrayIcon is the right choice in a QML based environment. I'd much rather prefer an API that permits convenient delegation to the application or platform layer, given that the notifications are most likely use-case specific.
    4) For myself I'm trying to focus on getting everything in shape for the Qt 5.0 release, and since we're past the feature freeze I will down-prioritize time for API/new-feature reviews on my TODO list. So you might have to find somebody else who is willing to review the API.

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:56
> +    WKNotificationManagerRef notificationManager;

This should be in a RetainPtr, otherwise it will leak.

[Olivier Crête]

Created attachment 163140 [details]
better patch

Thanks for the quick review, here is a patch that fixes the style/etc issues. As for the issue of using QSystemTrayIcon, it's use was suggested by Thiago. I'm really unconvinced that forcing every app to re-implement notifications is a good idea as it's really a platform thing. I have  a couple possible proposals on how to implement it:

1. Move QSystemTrayIcon out of QtWidgets, it's not a QWidget and only the Unix implementation uses a QWidget I believe. Maybe that could be replaced with using the libnotify dbus API? That would only depend on QtCore. My goal was to implement the platform specific bits in the QPA instead of spreading them into QtWebKit.
2. We could use weak linking for the QSystemTrayIcon to use it if it's linked against QtWidget and do nothing if it's not there.

[José Dapena Paz]

(In reply to comment #4)

>     2) QSystemTrayIcon is part of QtWidgets, which we choose not to use in the WebKit2 API.
>     3) Conceptually I'm not sure if QSystemTrayIcon is the right choice in a QML based environment. I'd much rather prefer an API that permits convenient delegation to the application or platform layer, given that the notifications are most likely use-case specific.

I am working now on a new implementation of this. We want to avoid dependency on QtWidgets, and, as QSystemTrayIcon doesn't seem to move out of it any time soon, we need a different approach.

My new approach is exposing it to application level. We'd have QWebNotification objects with methods click, close, show, to allow app to notify webkit about the changes in current notification status. My main doubt is how to expose the notifications the first time to application.

My initial idea was adding signals "showNotification" and "cancelNotification" to QQuickWebView, following an approach similar to Gtk and EFL WK1 port implementation proposals. These signals would have as parameter the QWebNotification object, and indicate the life cycle of it (app would be able to handle notifications between show and cancel). The problem is, on WebKit2 there's no reference to PageUIClient on the notification provider. We only have a WKPageRef, so I don't have a way to associate the notification to a QQuickWebView.

So my current idea is somehow expose the notifications using a global object (i.e. signals of the webkit shared object I am proposing for https://bugs.webkit.org/show_bug.cgi?id=105034 ).

[Allan Sandfeld Jensen]

*** Bug 88899 has been marked as a duplicate of this bug. ***

[José Dapena Paz]

Qt port is not in WebKit anymore. Won't fix.