Bug 44836

Now that Bug #44800 has been fixed, is anything else needed on the Qt side? This is the only bug left blocking Bug #39995

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

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

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?

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

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.

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?

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

Comment on attachment 66833 [details] Patch r=me

Created attachment 67194 [details] Patch Remove redundnt check for notification->ScriptExecutionContext.

Comment on attachment 67194 [details] Patch Clearing flags on attachment: 67194 Committed r67272: <http://trac.webkit.org/changeset/67272>

All reviewed patches have been landed. Closing bug.

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