RESOLVED FIXED 79492
[Mac] Basic DRT support for web notifications 
https://bugs.webkit.org/show_bug.cgi?id=79492
Summary [Mac] Basic DRT support for web notifications
Jessie Berlin
Reported 2012-02-24 08:15:58 PST
Right now all the notification tests are skipped on Mac. <rdar://problem/10357639>
Attachments
Patch (22.31 KB, patch)
2012-08-29 11:20 PDT, Jon Lee 		ap: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jon Lee
Comment 1 2012-08-27 17:01:49 PDT
This does not include any additional output for platform events (platform showed the notification, user clicked on a notification, etc). That is tracked in another bug under master bug 77969.
Jon Lee
Comment 2 2012-08-29 11:20:52 PDT
Created attachment 161272 [details] Patch
Alexey Proskuryakov
Comment 3 2012-08-29 11:37:31 PDT
Comment on attachment 161272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161272&action=review > Tools/DumpRenderTree/mac/MockWebNotificationProvider.h:39 > +typedef HashMap<uint64_t, WebView *> NotificationViewMap; We don't want to retain the web views? > Tools/DumpRenderTree/mac/MockWebNotificationProvider.h:42 > + HashSet<WebView *> _registeredWebViews; Ditto. > Tools/DumpRenderTree/mac/MockWebNotificationProvider.mm:88 > + uint64_t id = [notificationID unsignedLongLongValue]; I'm surprised that "id" doesn't cause a build failure.
Alexey Proskuryakov
Comment 4 2012-08-29 11:38:17 PDT
Comment on attachment 161272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161272&action=review > Tools/DumpRenderTree/mac/MockWebNotificationProvider.mm:58 > +- (void)registerWebView:(WebView *)webView > +{ > + ASSERT(!_registeredWebViews.contains(webView)); > + _registeredWebViews.add(webView); > +} > + > +- (void)unregisterWebView:(WebView *)webView > +{ > + ASSERT(_registeredWebViews.contains(webView)); > + _registeredWebViews.remove(webView); > +} I don't see where these are called from. Is that in a separate patch?
Jon Lee
Comment 5 2012-08-29 11:50:56 PDT
(In reply to comment #3) > (From update of attachment 161272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161272&action=review > > > Tools/DumpRenderTree/mac/MockWebNotificationProvider.h:39 > > +typedef HashMap<uint64_t, WebView *> NotificationViewMap; > > We don't want to retain the web views? I didn't think this was necessary. Between tests we clear out all the maps anyway, and if there is any bad access, that's an error in the provider implementation. > > > Tools/DumpRenderTree/mac/MockWebNotificationProvider.h:42 > > + HashSet<WebView *> _registeredWebViews; > > Ditto. > > > Tools/DumpRenderTree/mac/MockWebNotificationProvider.mm:88 > > + uint64_t id = [notificationID unsignedLongLongValue]; > > I'm surprised that "id" doesn't cause a build failure. It does not, at least on my machine.
Jon Lee
Comment 6 2012-08-29 11:52:16 PDT
(In reply to comment #4) > (From update of attachment 161272 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161272&action=review > > > Tools/DumpRenderTree/mac/MockWebNotificationProvider.mm:58 > > +- (void)registerWebView:(WebView *)webView > > +{ > > + ASSERT(!_registeredWebViews.contains(webView)); > > + _registeredWebViews.add(webView); > > +} > > + > > +- (void)unregisterWebView:(WebView *)webView > > +{ > > + ASSERT(_registeredWebViews.contains(webView)); > > + _registeredWebViews.remove(webView); > > +} > > I don't see where these are called from. Is that in a separate patch? No, these methods are called in WebKit, when the provider is added to the web view. It is part of the WebNotificationProvider protocol.
Alexey Proskuryakov
Comment 7 2012-08-29 12:39:25 PDT
Comment on attachment 161272 [details] Patch ok
Jon Lee
Comment 8 2012-08-29 13:31:50 PDT
Committed r127042: <http://trac.webkit.org/changeset/127042>
WebKit Review Bot
Comment 9 2012-08-29 18:13:27 PDT
Re-opened since this is blocked by 95412
Jon Lee
Comment 10 2012-08-30 13:04:28 PDT
Patch for bug 95465 was submitted, and fixes the problem that this patch unearthed. Back to resolved.
Note You need to log in before you can comment on or make changes to this bug.