Bug 35503 - [Qt] Implement Desktop Notifications API for QtWebKit
Summary: [Qt] Implement Desktop Notifications API for QtWebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Enhancement
Assignee: Laszlo Gombos
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-02-28 15:05 PST by Laszlo Gombos
Modified: 2010-04-11 09:27 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (18.70 KB, patch)
2010-02-28 16:03 PST, Laszlo Gombos
eric: review-
Details | Formatted Diff | Diff
2nd try (21.08 KB, patch)
2010-03-29 07:14 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2010-02-28 15:05:48 PST
Hook up the code for QtWebKit for ENABLE_NOTIFICAITONS build option.
Comment 1 Laszlo Gombos 2010-02-28 16:03:48 PST
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.
Comment 2 Laszlo Gombos 2010-02-28 16:52:42 PST
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.
Comment 3 Eric Seidel (no email) 2010-03-05 15:14:16 PST
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).
Comment 4 Laszlo Gombos 2010-03-29 07:14:34 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2010-04-08 15:03:27 PDT
Comment on attachment 51906 [details]
2nd try

Fine with me, but you have a spelling error in the ChangeLog "notificationa DRT"
Comment 6 Laszlo Gombos 2010-04-10 00:46:54 PDT
Comment on attachment 51906 [details]
2nd try

Committed as http://trac.webkit.org/changeset/57408 with the change in the ChangeLog suggested by Kenneth.
Comment 7 Csaba Osztrogonác 2010-04-10 01:23:53 PDT
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
Comment 8 WebKit Review Bot 2010-04-10 01:43:58 PDT
http://trac.webkit.org/changeset/57412 might have broken SnowLeopard Intel Release (Tests)
Comment 9 Csaba Osztrogonác 2010-04-10 01:48:02 PDT
(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.
Comment 10 WebKit Review Bot 2010-04-10 01:54:25 PDT
http://trac.webkit.org/changeset/57413 might have broken SnowLeopard Intel Release (Tests)
Comment 11 Laszlo Gombos 2010-04-11 09:27:50 PDT
(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.