Bug 78783 - Bring notifications support to WK1 mac
Summary: Bring notifications support to WK1 mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-16 00:38 PST by Jon Lee
Modified: 2012-02-21 16:00 PST (History)
5 users (show)

See Also:


Attachments
Bring notifications support to WK1 mac (1.19 KB, patch)
2012-02-16 02:38 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff
Source/WebKit/mac: (Prep work for) Bring notifications support to WK1 mac (11.74 KB, patch)
2012-02-16 02:39 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff
Source/WebKit: Bring notifications support to WK1 mac (28.47 KB, patch)
2012-02-16 02:39 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Bring notifications support to WK1 mac: permission requests (6.95 KB, patch)
2012-02-16 02:39 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff
Bring notifications support to WK1 mac: showing, canceling, removing notifications (30.88 KB, patch)
2012-02-16 10:54 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Bring notifications support to WK1 mac: showing, canceling, removing notifications (31.01 KB, patch)
2012-02-16 23:47 PST, Jon Lee
no flags Details | Formatted Diff | Diff
Bring notifications support to WK1 mac: showing, canceling, removing notifications (30.47 KB, patch)
2012-02-21 14:10 PST, Jon Lee
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2012-02-16 00:38:55 PST
<rdar://problem/10610578>
Comment 1 Jon Lee 2012-02-16 02:38:57 PST
Created attachment 127338 [details]
Bring notifications support to WK1 mac
Comment 2 Jon Lee 2012-02-16 02:39:00 PST
Created attachment 127339 [details]
Source/WebKit/mac: (Prep work for) Bring notifications support to WK1 mac
Comment 3 Jon Lee 2012-02-16 02:39:03 PST
Created attachment 127340 [details]
Source/WebKit: Bring notifications support to WK1 mac
Comment 4 Jon Lee 2012-02-16 02:39:06 PST
Created attachment 127341 [details]
Bring notifications support to WK1 mac: permission requests
Comment 5 Jon Lee 2012-02-16 10:54:35 PST
Created attachment 127410 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications
Comment 6 Jon Lee 2012-02-16 10:56:12 PST
Comment on attachment 127340 [details]
Source/WebKit: Bring notifications support to WK1 mac

Missing code. Replacement patch has been posted.
Comment 7 Jon Lee 2012-02-16 23:47:55 PST
Created attachment 127534 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications
Comment 8 Anders Carlsson 2012-02-17 09:29:02 PST
Comment on attachment 127534 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

View in context: https://bugs.webkit.org/attachment.cgi?id=127534&action=review

> Source/WebKit/mac/WebView/WebNotification.mm:45
> +    WebNotificationInternal *_internal;

Can't this just be a RefPtr<Notification> instead? Then you wouldn't have to worry about the lifecycle management and casting between WebNotificationInternal and WebCore::Notification.

> Source/WebKit/mac/WebView/WebViewPrivate.h:2
> - * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> + * Copyright (C) 2005-2012 Apple Inc. All rights reserved.

I don't think we're supposed to use ranges for copyright years.
Comment 9 Jon Lee 2012-02-17 09:40:33 PST
Comment on attachment 127534 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

View in context: https://bugs.webkit.org/attachment.cgi?id=127534&action=review

>> Source/WebKit/mac/WebView/WebNotification.mm:45
>> +    WebNotificationInternal *_internal;
> 
> Can't this just be a RefPtr<Notification> instead? Then you wouldn't have to worry about the lifecycle management and casting between WebNotificationInternal and WebCore::Notification.

Sure. I was originally basing this on other code that do something similar. But it's easy to convert this to a RefPtr.

>> Source/WebKit/mac/WebView/WebViewPrivate.h:2
>> + * Copyright (C) 2005-2012 Apple Inc. All rights reserved.
> 
> I don't think we're supposed to use ranges for copyright years.

Whoops, this change slipped by. It wouldn't be correct, anyway.
Comment 10 Jon Lee 2012-02-21 14:10:32 PST
Created attachment 128042 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications
Comment 11 Anders Carlsson 2012-02-21 14:17:01 PST
Comment on attachment 128042 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

View in context: https://bugs.webkit.org/attachment.cgi?id=128042&action=review

> Source/WebKit/mac/WebCoreSupport/WebNotificationClient.mm:73
> +    NotificationContextMap::iterator it = m_notificationContextMap.find(notification->scriptExecutionContext());
> +    if (it == m_notificationContextMap.end()) {
> +        pair<NotificationContextMap::iterator, bool> addedPair = m_notificationContextMap.add(notification->scriptExecutionContext(), Vector<RetainPtr<WebNotification> >());
> +        it = addedPair.first;
> +    }
> +    it->second.append(webNotification);

I think you can simplify this to always call add:

NotificationContextMap::iterator it = m_notificationContextMap.add(notification->scriptExecutionContext(), Vector<RetainPtr<WebNotification> >()).first();
addedPair.append(webNotification);

> Source/WebKit/mac/WebView/WebNotification.mm:73
> +- (id)init

Please add an extra newline before the method definition.
Comment 12 Jon Lee 2012-02-21 14:25:19 PST
Comment on attachment 128042 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

View in context: https://bugs.webkit.org/attachment.cgi?id=128042&action=review

>> Source/WebKit/mac/WebCoreSupport/WebNotificationClient.mm:73
>> +    it->second.append(webNotification);
> 
> I think you can simplify this to always call add:
> 
> NotificationContextMap::iterator it = m_notificationContextMap.add(notification->scriptExecutionContext(), Vector<RetainPtr<WebNotification> >()).first();
> addedPair.append(webNotification);

Done.

>> Source/WebKit/mac/WebView/WebNotification.mm:73
>> +- (id)init
> 
> Please add an extra newline before the method definition.

Done.
Comment 13 Anders Carlsson 2012-02-21 15:05:21 PST
Comment on attachment 127341 [details]
Bring notifications support to WK1 mac: permission requests

View in context: https://bugs.webkit.org/attachment.cgi?id=127341&action=review

> Source/WebKit/mac/WebCoreSupport/WebNotificationClient.mm:136
> +        // execute callback

I don't understand this comment. Should the callback be executed or not? :)
Comment 14 Anders Carlsson 2012-02-21 15:13:01 PST
Comment on attachment 127339 [details]
Source/WebKit/mac: (Prep work for) Bring notifications support to WK1 mac

View in context: https://bugs.webkit.org/attachment.cgi?id=127339&action=review

> Source/WebKit/mac/WebView/WebPreferences.h:443
> +/*!
> +    @method setNotificationsEnabled
> +    @param flag
> +*/
> +- (void)setNotificationsEnabled:(BOOL)flag;
> +/*!
> +    @method notificationsEnabled
> +    @param flag
> +*/
> +- (BOOL)notificationsEnabled;

These need to go in WebPreferencesPrivate.h
Comment 15 Jon Lee 2012-02-21 16:00:42 PST
Committed r108409: <http://trac.webkit.org/changeset/108409>