Bug 44800

Created attachment 65782 [details] Patch

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 @@ > +2010-08-27 John Gregg <johnnyg@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.

API change LGTM

Created attachment 65990 [details] Patch

Updated the patch with the changes you suggested.

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

Comment on attachment 65990 [details] Patch Cleared cq+ since John landed this.

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