[webkit-reviews] review denied: [Bug 35503] [Qt] Implement Desktop Notifications API for QtWebKit : [Attachment 49705] proposed patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Laszlo Gombos
<laszlo.1.gombos at nokia.com>'s request for review:
Bug 35503: [Qt] Implement Desktop Notifications API for QtWebKit
https://bugs.webkit.org/show_bug.cgi?id=35503

Attachment 49705: proposed patch
https://bugs.webkit.org/attachment.cgi?id=49705&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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).