Bug 78783 – Bring notifications support to WK1 mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Jon Lee]

<rdar://problem/10610578>

[Jon Lee]

Created attachment 127338 [details]
Bring notifications support to WK1 mac

[Jon Lee]

Created attachment 127339 [details]
Source/WebKit/mac: (Prep work for) Bring notifications support to WK1 mac

[Jon Lee]

Created attachment 127340 [details]
Source/WebKit: Bring notifications support to WK1 mac

[Jon Lee]

Created attachment 127341 [details]
Bring notifications support to WK1 mac: permission requests

[Jon Lee]

Created attachment 127410 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

[Jon Lee]

Comment on attachment 127340 [details]
Source/WebKit: Bring notifications support to WK1 mac

Missing code. Replacement patch has been posted.

[Jon Lee]

Created attachment 127534 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

[Anders Carlsson]

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.

[Jon Lee]

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.

[Jon Lee]

Created attachment 128042 [details]
Bring notifications support to WK1 mac: showing, canceling, removing notifications

[Anders Carlsson]

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.

[Jon Lee]

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.

[Anders Carlsson]

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? :)

[Anders Carlsson]

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

[Jon Lee]

Committed r108409: <http://trac.webkit.org/changeset/108409>