Summary: | Notifications should support a click event | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Gregg <johnnyg> | ||||||
Component: | New Bugs | Assignee: | John Gregg <johnnyg> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ademar, fishd | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Other | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 44836 | ||||||||
Attachments: |
|
Description
John Gregg
2010-08-27 15:52:30 PDT
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.
|