[Mac] Basic DRT support for web notifications

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jessie Berlin]

Right now all the notification tests are skipped on Mac.

<rdar://problem/10357639>

[Jon Lee]

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]

Created attachment 161272 [details]
Patch

[Alexey Proskuryakov]

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 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]

(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]

(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 on attachment 161272 [details]
Patch

ok

[Jon Lee]

Committed r127042: <http://trac.webkit.org/changeset/127042>

[WebKit Review Bot]

Re-opened since this is blocked by 95412

[Jon Lee]

Patch for bug 95465 was submitted, and fixes the problem that this patch unearthed. Back to resolved.