Bug 35503 – [Qt] Implement Desktop Notifications API for QtWebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Laszlo Gombos]

Hook up the code for QtWebKit for ENABLE_NOTIFICAITONS build option.

[Laszlo Gombos]

Created attachment 49705 [details]
proposed patch

Map WebKit notifications to Qt's SystemTray API and implement DRT tracing.

This patch does not implement the security part of WebKit notifications.

[Laszlo Gombos]

The first patch provides a very basic implementation and satisfies the build scripts. The complete solution (following additional patches) would need to address:
 - DRT tests for Qt port (make sure tests not executed for other ports as most ports do not implement this feature yet).
 - implement security
 - possible provide a Delegate API for QtWebKit to delegate the notification mechanism.

To keep the patch size low, I figured the patch is complete enough for a review round.

[Eric Seidel (no email)]

Comment on attachment 49705 [details]
proposed patch

Is this really supposed to return a new one?
#if ENABLE(NOTIFICATIONS)
 436 NotificationPresenter* ChromeClientQt::notificationPresenter() const
 437 {
 438     return new NotificationPresenterClientQt; 
 439 }
 440 #endif

based on that method signature, I woudl expect it to return a reference to one.  I expect you're leaking here.

Spacing:
void NotificationPresenterClientQt::notificationObjectDestroyed(Notification* notification)
 92 {
 93       notImplemented();
 94 
 95 }

Spacing:
void NotificationPresenterClientQt::requestPermission(SecurityOrigin* origin, PassRefPtr<VoidCallback> callback)
 98 {
 99   
 100     if (dumpNotification)

r- for what appears to be a leak (or at least would confuse other developers into causing a leak).

[Laszlo Gombos]

Created attachment 51906 [details]
2nd try

Thanks Eric for the review. 

Good catch on the leak. I changed the patch so that now each instance QWebPage of owns (creates and destroys) an instance of NotificationPresenterClientQt.

[Kenneth Rohde Christiansen]

Comment on attachment 51906 [details]
2nd try

Fine with me, but you have a spelling error in the ChangeLog "notificationa DRT"

[Laszlo Gombos]

Comment on attachment 51906 [details]
2nd try

Committed as http://trac.webkit.org/changeset/57408 with the change in the ChangeLog suggested by Kenneth.

[Csaba Osztrogonác]

Next time please keep your eyes on bots or don't commit before you go offline!

First I had to kick (make clean build) our bots because build system
modifications usually make all test crash. Makefiles can't handle
only build flag modifications.

Second I had to modify 3 expected files (which you forgot) to make 
our very sad bots happy: http://trac.webkit.org/changeset/57412

[WebKit Review Bot]

http://trac.webkit.org/changeset/57412 might have broken SnowLeopard Intel Release (Tests)

[Csaba Osztrogonác]

(In reply to comment #8)
> http://trac.webkit.org/changeset/57412 might have broken SnowLeopard Intel
> Release (Tests)

false positive alarm, it was an absolutely unrelated patch.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57413 might have broken SnowLeopard Intel Release (Tests)

[Laszlo Gombos]

(In reply to comment #7)

Ossy, thanks for you help !

> Next time please keep your eyes on bots or don't commit before you go offline!

I did just that but I obviously missed the issues with the Qt bot.
 
> First I had to kick (make clean build) our bots because build system
> modifications usually make all test crash. Makefiles can't handle
> only build flag modifications.

First, the build should fail instead of creating an unusable binary. Had the build fail, I would have caught it before even committing. Any idea how to fix this ? 

Second, there should be a way for committees to kick the bot.

> Second I had to modify 3 expected files (which you forgot) to make 
> our very sad bots happy: http://trac.webkit.org/changeset/57412

Sorry I missed that, thanks for your help.