Notifications should support a click event

[John Gregg]

Notifications should support a click event

[John Gregg]

Created attachment 65782 [details]
Patch

[David Levin]

Comment on attachment 65782 [details]
Patch

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 @@
> [phone]  John Gregg  <[e-mail]>
> +
> +        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.

[Darin Fisher (:fishd, Google)]

API change LGTM

[John Gregg]

Created attachment 65990 [details]
Patch

[John Gregg]

Updated the patch with the changes you suggested.

[John Gregg]

Committed r66470: <http://trac.webkit.org/changeset/66470>

[David Levin]

Comment on attachment 65990 [details]
Patch

Cleared cq+ since John landed this.

[Ademar Reis]

Revision r66470 cherry-picked into qtwebkit-2.1 with commit d11109dced53426ff19d0e89c133cb49fac0a2c8