Bug 79492 - [Mac] Basic DRT support for web notifications
Summary: [Mac] Basic DRT support for web notifications
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 95093 95412
Blocks: 81048 81697 77969 95232
  Show dependency treegraph
 
Reported: 2012-02-24 08:15 PST by Jessie Berlin
Modified: 2012-08-30 13:04 PDT (History)
5 users (show)

See Also:


Attachments
Patch (22.31 KB, patch)
2012-08-29 11:20 PDT, Jon Lee
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Berlin 2012-02-24 08:15:58 PST
Right now all the notification tests are skipped on Mac.

<rdar://problem/10357639>
Comment 1 Jon Lee 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.
Comment 2 Jon Lee 2012-08-29 11:20:52 PDT
Created attachment 161272 [details]
Patch
Comment 3 Alexey Proskuryakov 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.
Comment 4 Alexey Proskuryakov 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?
Comment 5 Jon Lee 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.
Comment 6 Jon Lee 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.
Comment 7 Alexey Proskuryakov 2012-08-29 12:39:25 PDT
Comment on attachment 161272 [details]
Patch

ok
Comment 8 Jon Lee 2012-08-29 13:31:50 PDT
Committed r127042: <http://trac.webkit.org/changeset/127042>
Comment 9 WebKit Review Bot 2012-08-29 18:13:27 PDT
Re-opened since this is blocked by 95412
Comment 10 Jon Lee 2012-08-30 13:04:28 PDT
Patch for bug 95465 was submitted, and fixes the problem that this patch unearthed. Back to resolved.