<rdar://problem/10610578>
Created attachment 127338 [details] Bring notifications support to WK1 mac
Created attachment 127339 [details] Source/WebKit/mac: (Prep work for) Bring notifications support to WK1 mac
Created attachment 127340 [details] Source/WebKit: Bring notifications support to WK1 mac
Created attachment 127341 [details] Bring notifications support to WK1 mac: permission requests
Created attachment 127410 [details] Bring notifications support to WK1 mac: showing, canceling, removing notifications
Comment on attachment 127340 [details] Source/WebKit: Bring notifications support to WK1 mac Missing code. Replacement patch has been posted.
Created attachment 127534 [details] Bring notifications support to WK1 mac: showing, canceling, removing notifications
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 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.
Created attachment 128042 [details] Bring notifications support to WK1 mac: showing, canceling, removing notifications
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 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 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 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
Committed r108409: <http://trac.webkit.org/changeset/108409>