[webkit-reviews] review granted: [Bug 44800] Notifications should support a click event : [Attachment 65782] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted John Gregg <johnnyg at google.com>'s
request for review:
Bug 44800: Notifications should support a click event
https://bugs.webkit.org/show_bug.cgi?id=44800

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

------- Additional Comments from David Levin <levin at chromium.org>
Before landing let's get a quick ok from Darin on the public api change.


> Index: WebKitTools/ChangeLog
> ===================================================================
> --- WebKitTools/ChangeLog	(revision 66270)
> +++ WebKitTools/ChangeLog	(working copy)
> @@ -1,3 +1,24 @@
> +2010-08-27  John Gregg  <johnnyg at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Notifications should support a click event.
> +	   Adds necessary hooks to chromium's DRT so that clicks on desktop
notifications
> +	   can be simulated during a layout test.  Requires storing a list of
active
> +	   notifications so that they can be referred to later for clicking.
> +	   https://bugs.webkit.org/show_bug.cgi?id=44800
> +
> +	   * DumpRenderTree/chromium/LayoutTestController.cpp:
> +	   (LayoutTestController::LayoutTestController):
> +	   (LayoutTestController::simulateDesktopNotificationClick):
> +	   * DumpRenderTree/chromium/LayoutTestController.h:
> +	   * DumpRenderTree/chromium/NotificationPresenter.cpp:
> +	   (NotificationPresenter::simulateClick):
> +	   (NotificationPresenter::show):
> +	   (NotificationPresenter::cancel):
> +	   (NotificationPresenter::objectDestroyed):
> +	   * DumpRenderTree/chromium/NotificationPresenter.h:

Please consider switching to a per function comment style here in the future
(and trimming the overall summary as a result).


>  
> Index: WebKitTools/DumpRenderTree/chromium/NotificationPresenter.cpp
> ===================================================================
> --- WebKitTools/DumpRenderTree/chromium/NotificationPresenter.cpp    
(revision 66183)
> +++ WebKitTools/DumpRenderTree/chromium/NotificationPresenter.cpp    
(working copy)
> @@ -50,17 +50,33 @@ void NotificationPresenter::grantPermiss
>      m_allowedOrigins.add(WTF::String(url.GetOrigin().spec().c_str()));
>  }
>  
> +bool NotificationPresenter::simulateClick(const WebString& title)
> +{
> +    WTF::String id(title.data(), title.length());
> +    if (m_activeNotifications.find(id) != m_activeNotifications.end()) {

Please consider a fail fast approach.

    if (m_activeNotifications.find(id) == m_activeNotifications.end())
	return false;
....

> +    WebString identifier;
> +    if (notification.isHTML())
> +	   identifier = notification.url().spec().utf16();
> +    else
> +	   identifier = notification.title();
> +

Consider making a function out of this, since the same thing is done at least
twice.



> Index: LayoutTests/fast/notifications/notifications-click-event.html
> +	       var N = window.webkitNotifications.createNotification("", "New
E-mail", "Meet me tonight at 8!");
> +	       N.onclick = function() { log("PASS: click event fired.");
N.cancel();  }
> +	       N.show();
> +	    if (window.layoutTestController) {

The formatting is a bit odd here due to TABs.