[webkit-reviews] review denied: [Bug 40005] [Qt] Platform plugin support for Notifications UI : [Attachment 58592] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 40005: [Qt] Platform plugin support for Notifications UI
https://bugs.webkit.org/show_bug.cgi?id=40005

Attachment 58592: Patch
https://bugs.webkit.org/attachment.cgi?id=58592&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
Almost there!

WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:71
 +  Q_OBJECT
We normally indent the Q_OBJECT

WebKit/qt/examples/platformplugin/WebPlugin.h:93
 +	    if (supportsExtension(Notifications))
Actually I think we (WebCore) should call supportsExtension(Notifications)
before calling createNotificationPresenter(), leaving this code below just
return the new WebNotificationsPresenter.

WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:46
 +	: QWebNotificationPresenter()
the : part needs to be indented 4 spaces more.

WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:35
 +	bool event(QEvent* ev);
This is the header, leave out ev

WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:29
 +  Q_OBJECT
Indentation

WebKit/qt/examples/platformplugin/WebNotificationPresenter.h:43
 +  Q_OBJECT
Indentation of Q_OBJECT

 const QString NotificationIconWrapper::title() const
 84 {
 85	Notification* notification =
NotificationPresenterClientQt::notificationPresenter()->notificationForWrapper(
this);
 86	if (notification)
 87	    return notification->contents().title();
 88	return QString();
 89 }

im a bit confused about this code. When you create the IconWrapper, why can't
you already fill it with the title, message etc? 

Btw the name confused me _ICON_Wrapper, but it contains title, message etc.
That doesnt seem like an icon to me