Bug 40005 – [Qt] Platform plugin support for Notifications UI

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Yael]

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.

[Yael]

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.

[WebKit Review Bot]

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.

[Yael]

Created attachment 58435 [details]
Patch

Fix style issue.

[Yael]

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.

[Yael]

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.

[Kenneth Rohde Christiansen]

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??

[Kenneth Rohde Christiansen]

(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?

[Yael]

(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.

[Kenneth Rohde Christiansen]

(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.

[Yael]

Created attachment 58550 [details]
Patch

Address review comments

[Kenneth Rohde Christiansen]

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.

[Yael]

Created attachment 58592 [details]
Patch

Update based on comment #11.

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

> 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.

[Yael]

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 :-)

[Kenneth Rohde Christiansen]

Comment on attachment 58596 [details]
Patch

WebKit/qt/examples/platformplugin/WebPlugin.h:93
 +          if (supportsExtension(Notifications))
you still need this here?

[Yael]

(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.

[Yael]

(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.

[Kenneth Rohde Christiansen]

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.

[Yael]

(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.

[Yael]

Committed r61121: <http://trac.webkit.org/changeset/61121>

[Simon Hausmann]

Revision r61121 cherry-picked into qtwebkit-2.1 with commit 033342ddc7af060c22ad42a033a22631ab10a54e