Add an interface to platform plugin, so it can override the default behavior of using QSystemTrayIcon for notifications UI. A patch is ready but it depends heavily on the patch at bug #40003, so it will not apply until that patch is committed.
Created attachment 58434 [details] Patch Add an interface to the platform plugin to display UI notifications. Implemented the UI notification in the example platform plugin. This interface is enabled by default, but could be turned off with a build flag. Moved the 10 seconds timer so it closes only QSystemTrayIcon notifications. If a platform plugin exists, it is responsible for closing the UI notifications.
Attachment 58434 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp:41: Alphabetical sorting problem. [build/include_order] [4] WARNING: File exempt from style guide. Skipping: "WebKit/qt/examples/platformplugin/WebNotificationUi.h" WARNING: File exempt from style guide. Skipping: "WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h" WARNING: File exempt from style guide. Skipping: "WebKit/qt/examples/platformplugin/WebNotificationUi.cpp" WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebkitplatformplugin.h" WARNING: File exempt from style guide. Skipping: "WebKit/qt/examples/platformplugin/WebPlugin.cpp" WARNING: File exempt from style guide. Skipping: "WebKit/qt/examples/platformplugin/platformplugin.pro" WARNING: File exempt from style guide. Skipping: "WebKit/qt/examples/platformplugin/WebPlugin.h" Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 58435 [details] Patch Fix style issue.
Created attachment 58484 [details] Patch Found a crash on exit that was a result of GC running during application shut down. Did the following to address the crash on exit: Changed the way NotificationPresenterClientQt is deleted because it is being accessed when GC is deleting the Notification objects. A delete timer was created to ensure that NotificationPresenterClientQt is deleted after final GC was done.
Created attachment 58510 [details] Patch Previous approach did not always delete the NotificationPresenter, depending on how long GC took. In this patch I detach the notifications from the presenter when the presenter is deleted, so when those notifications are GC'd, they would not call the presenter.
WebKit/qt/Api/qwebkitplatformplugin.h:61 + class QWebNotificationUi : public QObject I dont like the Ui part of the name. Isn't this more or less like a notification controller? or notification dispatcher? WebKit/qt/Api/qwebkitplatformplugin.h:68 + virtual void showNotification() = 0; If this class is a notification itself, this should probably just be called show() or appear() WebKit/qt/Api/qwebkitplatformplugin.h:81 + Notifications Maybe we should extend this with SingleSelections as well? WebKit/qt/Api/qwebkitplatformplugin.h:72 + }; What string do I receive??
(In reply to comment #6) > WebKit/qt/Api/qwebkitplatformplugin.h:61 > + class QWebNotificationUi : public QObject > I dont like the Ui part of the name. Isn't this more or less like a notification controller? or notification dispatcher? > > WebKit/qt/Api/qwebkitplatformplugin.h:68 > + virtual void showNotification() = 0; > If this class is a notification itself, this should probably just be called show() or appear() > > WebKit/qt/Api/qwebkitplatformplugin.h:81 > + Notifications > Maybe we should extend this with SingleSelections as well? > > WebKit/qt/Api/qwebkitplatformplugin.h:72 > + }; > What string do I receive?? actually I like the WebCore name, so what about createNotificationPresenter?
(In reply to comment #6) Thank yoy for taking the time to review. > WebKit/qt/Api/qwebkitplatformplugin.h:61 > + class QWebNotificationUi : public QObject > I dont like the Ui part of the name. Isn't this more or less like a notification controller? or notification dispatcher? > I'll change it to QWebNotificationPresenter > WebKit/qt/Api/qwebkitplatformplugin.h:68 > + virtual void showNotification() = 0; > If this class is a notification itself, this should probably just be called show() or appear() I will change it to "show". > > WebKit/qt/Api/qwebkitplatformplugin.h:81 > + Notifications > Maybe we should extend this with SingleSelections as well? > Maks sense to me, but perhaps a separate patch, and also with a build flag, so one can get implement notifications but not the SingleSelect if they want to ? > WebKit/qt/Api/qwebkitplatformplugin.h:72 > + }; > What string do I receive?? This was supposed to be for HTML notifications, if a link was clicked. This is not yet supported, so I will leave it out.
(In reply to comment #8) > (In reply to comment #6) > Thank yoy for taking the time to review. > > WebKit/qt/Api/qwebkitplatformplugin.h:61 > > + class QWebNotificationUi : public QObject > > I dont like the Ui part of the name. Isn't this more or less like a notification controller? or notification dispatcher? > > > I'll change it to QWebNotificationPresenter > > > WebKit/qt/Api/qwebkitplatformplugin.h:68 > > + virtual void showNotification() = 0; > > If this class is a notification itself, this should probably just be called show() or appear() > I will change it to "show". Actually since you are renaming to QWebNotificationPresenter, showNotification makes sense :-) so no need to rename. > > > > > WebKit/qt/Api/qwebkitplatformplugin.h:81 > > + Notifications > > Maybe we should extend this with SingleSelections as well? > > > Maks sense to me, but perhaps a separate patch, and also with a build flag, so one can get implement notifications but not the SingleSelect if they want to ? Yes, not in this patch. > > > WebKit/qt/Api/qwebkitplatformplugin.h:72 > > + }; > > What string do I receive?? > This was supposed to be for HTML notifications, if a link was clicked. This is not yet supported, so I will leave it out. Sounds good.
Created attachment 58550 [details] Patch Address review comments
Comment on attachment 58550 [details] Patch WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:50 + WebNotificationPresenter(const QString& title, const QString& message, const QByteArray& bytes) : QWebNotificationPresenter(title, message, bytes) Put the : part on the next line. WebKit/qt/examples/platformplugin/WebPlugin.h:92 + virtual QWebNotificationPresenter* createNotificationPresenter(const QString& title, const QString& message, const QByteArray& bytes) const { if (supportsExtension(Notifications)) return new WebNotificationPresenter(title, message, bytes); return 0; } I would put the contents part on some other lines. This is barely readable. WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:86 + virtual QWebNotificationPresenter* createNotificationPresenter(const QString& title, const QString& message, const QByteArray&) const = 0; Wouldn't it be better to make this similar to the SelectMethod? so that you can access the info (title, message, etc) from inside the function via an accessor. We never know what is going to be added here in the future.
Created attachment 58592 [details] Patch Update based on comment #11.
Comment on attachment 58592 [details] Patch Almost there! WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:71 + Q_OBJECT We normally indent the Q_OBJECT WebKit/qt/examples/platformplugin/WebPlugin.h:93 + if (supportsExtension(Notifications)) Actually I think we (WebCore) should call supportsExtension(Notifications) before calling createNotificationPresenter(), leaving this code below just return the new WebNotificationsPresenter. WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:46 + : QWebNotificationPresenter() the : part needs to be indented 4 spaces more. WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:35 + bool event(QEvent* ev); This is the header, leave out ev WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:29 + Q_OBJECT Indentation WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:43 + Q_OBJECT Indentation of Q_OBJECT const QString NotificationIconWrapper::title() const 84 { 85 Notification* notification = NotificationPresenterClientQt::notificationPresenter()->notificationForWrapper(this); 86 if (notification) 87 return notification->contents().title(); 88 return QString(); 89 } im a bit confused about this code. When you create the IconWrapper, why can't you already fill it with the title, message etc? Btw the name confused me _ICON_Wrapper, but it contains title, message etc. That doesnt seem like an icon to me
> im a bit confused about this code. When you create the IconWrapper, why can't you already fill it with the title, message etc? What is important there is that we do not copy data unnecessarily, so my comment might be invalid. Cutting down on indirection if possible, is a good thing though. I think the Platform Plugin API is nice now, so later we can fix the implementation if needed.
Created attachment 58596 [details] Patch Address comment #13, with the exception that I don't make a local copy of the strings (as said in comment #14). I agree that the name NotificationIconWrapper does not reflect its purpose anymore. Originally, I wanted it to only wrap QSystemTrayIcon, but now it does much more. How about renaming it NotificationWrapper? However, let's rename it in a separate patch, because this one just keeps growing :-)
Comment on attachment 58596 [details] Patch WebKit/qt/examples/platformplugin/WebPlugin.h:93 + if (supportsExtension(Notifications)) you still need this here?
(In reply to comment #16) > (From update of attachment 58596 [details]) > WebKit/qt/examples/platformplugin/WebPlugin.h:93 > + if (supportsExtension(Notifications)) > you still need this here? Yes, I do :-) I have a build flag that allows people the flexibility of building the platform plugin with SingleSelect, but without Notifications. I need to check if the build flag is enabled before creating the NotificationPresenter.
(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 58596 [details] [details]) > > WebKit/qt/examples/platformplugin/WebPlugin.h:93 > > + if (supportsExtension(Notifications)) > > you still need this here? > > Yes, I do :-) > I have a build flag that allows people the flexibility of building the platform plugin with SingleSelect, but without Notifications. I need to check if the build flag is enabled before creating the NotificationPresenter. To be more specific, tge plugin and WebKit are 2 different components, so I don't think the plugin should rely on the caller to do the check.
the extension support is for the plugin to say whether it implements a feature or not. WebCoreSupport will then check if it does and in that case use the plugin or do some kind of fallback.
(In reply to comment #19) > the extension support is for the plugin to say whether it implements a feature or not. WebCoreSupport will then check if it does and in that case use the plugin or do some kind of fallback. I was just trying to protect from callers who are not using the API properly. Since the only caller is WebCoreSupport anyways, I'll remove the check when committing.
Committed r61121: <http://trac.webkit.org/changeset/61121>
Revision r61121 cherry-picked into qtwebkit-2.1 with commit 033342ddc7af060c22ad42a033a22631ab10a54e