[Webkit-unassigned] [Bug 96171] [Qt] Implement WebNotification in WebKit2 port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96171


Simon Hausmann <hausmann at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #162931|review?                     |review-
               Flag|                            |




--- Comment #4 from Simon Hausmann <hausmann at webkit.org>  2012-09-09 11:31:20 PST ---
(From update of attachment 162931)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.