[webkit-reviews] review denied: [Bug 96171] [Qt] Implement WebNotification in WebKit2 port : [Attachment 162931] Patch to implement web notifications on the WebKit2/qt port

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Olivier Crête
<olivier.crete at collabora.com>'s request for review:
Bug 96171: [Qt] Implement WebNotification in WebKit2 port
https://bugs.webkit.org/show_bug.cgi?id=96171

Attachment 162931: Patch to implement web notifications on the WebKit2/qt port
https://bugs.webkit.org/attachment.cgi?id=162931&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.