Bug 44836 – [Qt] Support click event for notifications

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Yael]

Notifications should support a click event.

[Ademar Reis]

Now that Bug #44800 has been fixed, is anything else needed on the Qt side?

This is the only bug left blocking Bug #39995

[Yael]

(In reply to comment #1)
> Now that Bug #44800 has been fixed, is anything else needed on the Qt side?
> 
> This is the only bug left blocking Bug #39995

There are more requirements coming for Notificatons in Qt. I updated the list in #3995 recently.

[Yael]

(In reply to comment #2)
> (In reply to comment #1)
> > Now that Bug #44800 has been fixed, is anything else needed on the Qt side?
> > 
> > This is the only bug left blocking Bug #39995
> 
> There are more requirements coming for Notificatons in Qt. I updated the list in #3995 recently.
That is bug #39995, of course :-)

[Ademar Reis]

OK, so bug #39995 has some tasks left, but I guess we can at least close this one, right? Or do we still need something on the Qt side to support click events for notifications?

[Yael]

(In reply to comment #4)
> OK, so bug #39995 has some tasks left, but I guess we can at least close this one, right? Or do we still need something on the Qt side to support click events for notifications?

Yes, I am currently working on the Qt implementation of the click event.

[Yael]

Created attachment 66833 [details]
Patch

Add support for sending a click event when either QSystemTrayIcon is clicked, or the platform plugin's notification is clicked.
Also added a new method NotificationWrapper::openerPageUrl(), so that if the platform plugin wants to open the same url in a new page, when the user clicks the notification, it can do that too.
I added that somewhat related method here since I did not want to bump the plugin version twice.

[Andreas Kling]

Comment on attachment 66833 [details]
Patch

> +    if (notification && notification->scriptExecutionContext())
> +        sendEvent(notification, eventNames().clickEvent);

Redundancy: sendEvent() will also check notification->scriptExecutionContext() before proceeding.

> +    if (ev->type() == QEvent::MouseButtonRelease) {
> +        emit notificationClicked();

Are you sure the click event should fire for all mouse buttons?

[Yael]

(In reply to comment #7)
Thank you for the review, Andreas.
> (From update of attachment 66833 [details])
> > +    if (notification && notification->scriptExecutionContext())
> > +        sendEvent(notification, eventNames().clickEvent);
> 
> Redundancy: sendEvent() will also check notification->scriptExecutionContext() before proceeding.
I'll remove the second part of the check. However, the first part is still needed.
> 
> > +    if (ev->type() == QEvent::MouseButtonRelease) {
> > +        emit notificationClicked();
> 
> Are you sure the click event should fire for all mouse buttons?
This is _example_ code, and I don't think we care much about that.

[Andreas Kling]

Comment on attachment 66833 [details]
Patch

r=me

[Yael]

Created attachment 67194 [details]
Patch

Remove redundnt check for notification->ScriptExecutionContext.

[WebKit Commit Bot]

Comment on attachment 67194 [details]
Patch

Clearing flags on attachment: 67194

Committed r67272: <http://trac.webkit.org/changeset/67272>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r67272 cherry-picked into qtwebkit-2.1 with commit 7cb97137ea15daf78870cf1b4028e0bb17784c56