Bug 78783

Summary: Bring notifications support to WK1 mac
Product: WebKit Reporter: Jon Lee <jonlee>
Component: New BugsAssignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jberlin, sam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.7   
Attachments:
Description Flags
Bring notifications support to WK1 mac
andersca: review+
Source/WebKit/mac: (Prep work for) Bring notifications support to WK1 mac
andersca: review+
Source/WebKit: Bring notifications support to WK1 mac
none
Bring notifications support to WK1 mac: permission requests
andersca: review+
Bring notifications support to WK1 mac: showing, canceling, removing notifications
none
Bring notifications support to WK1 mac: showing, canceling, removing notifications
none
Bring notifications support to WK1 mac: showing, canceling, removing notifications andersca: review+

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>