[webkit-changes] [207025] trunk/Source

Mon, 10 Oct 2016 14:33:04 -0700

Title: [207025] trunk/Source
Revision
207025
Author
[email protected]
Date
2016-10-10 14:33:25 -0700 (Mon, 10 Oct 2016)

Log Message

Move NotificationCenter from legacy to new DOM exceptions
https://bugs.webkit.org/show_bug.cgi?id=163217

Reviewed by Chris Dumez.

Source/WebCore:

* Modules/notifications/DOMWindowNotifications.cpp:
(WebCore::DOMWindowNotifications::webkitNotifications): Pass reference to
document instead of pointer. Also use auto and early return.

* Modules/notifications/Notification.cpp:
(WebCore::Notification::Notification): Move checks that can lead to
throwing an exception out of the constructor into the create function.
(WebCore::Notification::create): Changed to use ExceptionOr. Also moved
checks that can lead to throwing an exception here.
(WebCore::Notification::show): Update since NotificationController::client
now returns a reference.
(WebCore::Notification::permission): Ditto.
(WebCore::Notification::requestPermission): Ditto.

* Modules/notifications/Notification.h: Updated for above changes.

* Modules/notifications/NotificationCenter.cpp:
(WebCore::NotificationCenter::create): Take a reference to the script execution
context instead of a pointer.
(WebCore::NotificationCenter::NotificationCenter): Ditto.
(WebCore::NotificationCenter::createNotification): Use ExceptionOr.
(WebCore::NotificationCenter::requestPermission): Take an rvalue reference.

* Modules/notifications/NotificationCenter.h: Use pragma once.
Marked the class final. Updated for changes above.

* Modules/notifications/NotificationCenter.idl: Use non-legacy exceptions.

* Modules/notifications/NotificationController.cpp:
(WebCore::NotificationController::NotificationController): Use a reference rather
than a pointer for m_client, since it is never null.
(WebCore::NotificationController::~NotificationController): Ditto.
(WebCore::NotificationController::clientFrom): Updated for the change above.

* Modules/notifications/NotificationController.h: Updated for the change above.

* Modules/notifications/WorkerGlobalScopeNotifications.cpp:
(WebCore::WorkerGlobalScopeNotifications::webkitNotifications): Updated to
pass a reference for the script execution context. Also added FIXME about the
fact that the client is always null.

Source/WebKit/mac:

* WebView/WebView.mm:
(-[WebView _notificationIDForTesting:]): Pass reference to page rather than pointer.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (207024 => 207025)


--- trunk/Source/WebCore/ChangeLog	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/ChangeLog	2016-10-10 21:33:25 UTC (rev 207025)
@@ -1,3 +1,51 @@
+2016-10-10  Darin Adler  <[email protected]>
+
+        Move NotificationCenter from legacy to new DOM exceptions
+        https://bugs.webkit.org/show_bug.cgi?id=163217
+
+        Reviewed by Chris Dumez.
+
+        * Modules/notifications/DOMWindowNotifications.cpp:
+        (WebCore::DOMWindowNotifications::webkitNotifications): Pass reference to
+        document instead of pointer. Also use auto and early return.
+
+        * Modules/notifications/Notification.cpp:
+        (WebCore::Notification::Notification): Move checks that can lead to
+        throwing an exception out of the constructor into the create function.
+        (WebCore::Notification::create): Changed to use ExceptionOr. Also moved
+        checks that can lead to throwing an exception here.
+        (WebCore::Notification::show): Update since NotificationController::client
+        now returns a reference.
+        (WebCore::Notification::permission): Ditto.
+        (WebCore::Notification::requestPermission): Ditto.
+
+        * Modules/notifications/Notification.h: Updated for above changes.
+
+        * Modules/notifications/NotificationCenter.cpp:
+        (WebCore::NotificationCenter::create): Take a reference to the script execution
+        context instead of a pointer.
+        (WebCore::NotificationCenter::NotificationCenter): Ditto.
+        (WebCore::NotificationCenter::createNotification): Use ExceptionOr.
+        (WebCore::NotificationCenter::requestPermission): Take an rvalue reference.
+
+        * Modules/notifications/NotificationCenter.h: Use pragma once.
+        Marked the class final. Updated for changes above.
+
+        * Modules/notifications/NotificationCenter.idl: Use non-legacy exceptions.
+
+        * Modules/notifications/NotificationController.cpp:
+        (WebCore::NotificationController::NotificationController): Use a reference rather
+        than a pointer for m_client, since it is never null.
+        (WebCore::NotificationController::~NotificationController): Ditto.
+        (WebCore::NotificationController::clientFrom): Updated for the change above.
+
+        * Modules/notifications/NotificationController.h: Updated for the change above.
+
+        * Modules/notifications/WorkerGlobalScopeNotifications.cpp:
+        (WebCore::WorkerGlobalScopeNotifications::webkitNotifications): Updated to
+        pass a reference for the script execution context. Also added FIXME about the
+        fact that the client is always null.
+
 2016-10-10  Antoine Quint  <[email protected]>
 
         [Modern Media Controls] Placards

Modified: trunk/Source/WebCore/Modules/notifications/DOMWindowNotifications.cpp (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/DOMWindowNotifications.cpp	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/DOMWindowNotifications.cpp	2016-10-10 21:33:25 UTC (rev 207025)
@@ -101,23 +101,24 @@
 NotificationCenter* DOMWindowNotifications::webkitNotifications()
 {
     if (!m_window->isCurrentlyDisplayedInFrame())
-        return 0;
+        return nullptr;
 
     if (m_notificationCenter)
         return m_notificationCenter.get();
 
-    Document* document = m_window->document();
+    auto* document = m_window->document();
     if (!document)
-        return 0;
+        return nullptr;
     
-    Page* page = document->page();
+    auto* page = document->page();
     if (!page)
-        return 0;
+        return nullptr;
 
-    NotificationClient* provider = NotificationController::clientFrom(page);
-    if (provider) 
-        m_notificationCenter = NotificationCenter::create(document, provider);    
+    auto* provider = NotificationController::clientFrom(*page);
+    if (!provider)
+        return nullptr;
 
+    m_notificationCenter = NotificationCenter::create(*document, provider);
     return m_notificationCenter.get();
 }

Modified: trunk/Source/WebCore/Modules/notifications/Notification.cpp (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/Notification.cpp	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/Notification.cpp	2016-10-10 21:33:25 UTC (rev 207025)
@@ -50,22 +50,13 @@
 
 #if ENABLE(LEGACY_NOTIFICATIONS)
 
-Notification::Notification(const String& title, const String& body, const String& iconURI, ScriptExecutionContext& context, ExceptionCode& ec, NotificationCenter& notificationCenter)
+Notification::Notification(const String& title, const String& body, URL&& iconURL, ScriptExecutionContext& context, NotificationCenter& notificationCenter)
     : ActiveDOMObject(&context)
+    , m_icon(WTFMove(iconURL))
     , m_title(title)
     , m_body(body)
     , m_notificationCenter(&notificationCenter)
 {
-    if (m_notificationCenter->checkPermission() != NotificationClient::PermissionAllowed) {
-        ec = SECURITY_ERR;
-        return;
-    }
-
-    m_icon = iconURI.isEmpty() ? URL() : scriptExecutionContext()->completeURL(iconURI);
-    if (!m_icon.isEmpty() && !m_icon.isValid()) {
-        ec = SYNTAX_ERR;
-        return;
-    }
 }
 
 #endif
@@ -93,11 +84,18 @@
 
 #if ENABLE(LEGACY_NOTIFICATIONS)
 
-Ref<Notification> Notification::create(const String& title, const String& body, const String& iconURI, ScriptExecutionContext& context, ExceptionCode& ec, NotificationCenter& provider)
+ExceptionOr<Ref<Notification>> Notification::create(const String& title, const String& body, const String& iconURL, ScriptExecutionContext& context, NotificationCenter& provider)
 { 
-    auto notification = adoptRef(*new Notification(title, body, iconURI, context, ec, provider));
+    if (provider.checkPermission() != NotificationClient::PermissionAllowed)
+        return Exception { SECURITY_ERR };
+
+    URL completedIconURL = iconURL.isEmpty() ? URL() : context.completeURL(iconURL);
+    if (!completedIconURL.isEmpty() && !completedIconURL.isValid())
+        return Exception { SYNTAX_ERR };
+
+    auto notification = adoptRef(*new Notification(title, body, WTFMove(completedIconURL), context, provider));
     notification.get().suspendIfNeeded();
-    return notification;
+    return WTFMove(notification);
 }
 
 #endif
@@ -146,7 +144,7 @@
         auto* page = downcast<Document>(*scriptExecutionContext()).page();
         if (!page)
             return;
-        if (NotificationController::from(page)->client()->checkPermission(scriptExecutionContext()) != NotificationClient::PermissionAllowed) {
+        if (NotificationController::from(page)->client().checkPermission(scriptExecutionContext()) != NotificationClient::PermissionAllowed) {
             dispatchErrorEvent();
             return;
         }
@@ -224,7 +222,7 @@
 
 String Notification::permission(Document& document)
 {
-    return permissionString(NotificationController::from(document.page())->client()->checkPermission(&document));
+    return permissionString(NotificationController::from(document.page())->client().checkPermission(&document));
 }
 
 String Notification::permissionString(NotificationClient::Permission permission)
@@ -243,7 +241,7 @@
 
 void Notification::requestPermission(Document& document, RefPtr<NotificationPermissionCallback>&& callback)
 {
-    NotificationController::from(document.page())->client()->requestPermission(&document, WTFMove(callback));
+    NotificationController::from(document.page())->client().requestPermission(&document, WTFMove(callback));
 }
 
 #endif

Modified: trunk/Source/WebCore/Modules/notifications/Notification.h (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/Notification.h	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/Notification.h	2016-10-10 21:33:25 UTC (rev 207025)
@@ -35,14 +35,12 @@
 
 #include "ActiveDOMObject.h"
 #include "EventTarget.h"
+#include "ExceptionOr.h"
 #include "NotificationClient.h"
+#include "Timer.h"
 #include "URL.h"
 #include "WritingMode.h"
 
-#if ENABLE(NOTIFICATIONS)
-#include "Timer.h"
-#endif
-
 namespace WebCore {
 
 class Document;
@@ -55,7 +53,7 @@
     WTF_MAKE_FAST_ALLOCATED;
 public:
 #if ENABLE(LEGACY_NOTIFICATIONS)
-    static Ref<Notification> create(const String& title, const String& body, const String& iconURL, ScriptExecutionContext&, ExceptionCode&, NotificationCenter&);
+    static ExceptionOr<Ref<Notification>> create(const String& title, const String& body, const String& iconURL, ScriptExecutionContext&, NotificationCenter&);
 #endif
 
 #if ENABLE(NOTIFICATIONS)
@@ -111,7 +109,7 @@
 
 private:
 #if ENABLE(LEGACY_NOTIFICATIONS)
-    Notification(const String& title, const String& body, const String& iconURL, ScriptExecutionContext&, ExceptionCode&, NotificationCenter&);
+    Notification(const String& title, const String& body, URL&& iconURL, ScriptExecutionContext&, NotificationCenter&);
 #endif
 
 #if ENABLE(NOTIFICATIONS)

Modified: trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/NotificationCenter.cpp	2016-10-10 21:33:25 UTC (rev 207025)
@@ -35,6 +35,7 @@
 
 #include "NotificationCenter.h"
 
+#include "ExceptionCode.h"
 #include "Notification.h"
 #include "ScriptExecutionContext.h"
 #include "SecurityOrigin.h"
@@ -42,7 +43,7 @@
 
 namespace WebCore {
 
-Ref<NotificationCenter> NotificationCenter::create(ScriptExecutionContext* context, NotificationClient* client)
+Ref<NotificationCenter> NotificationCenter::create(ScriptExecutionContext& context, NotificationClient* client)
 {
     auto notificationCenter = adoptRef(*new NotificationCenter(context, client));
     notificationCenter->suspendIfNeeded();
@@ -49,8 +50,8 @@
     return notificationCenter;
 }
 
-NotificationCenter::NotificationCenter(ScriptExecutionContext* context, NotificationClient* client)
-    : ActiveDOMObject(context)
+NotificationCenter::NotificationCenter(ScriptExecutionContext& context, NotificationClient* client)
+    : ActiveDOMObject(&context)
     , m_client(client)
     , m_timer([this]() { timerFired(); })
 {
@@ -58,13 +59,11 @@
 
 #if ENABLE(LEGACY_NOTIFICATIONS)
 
-RefPtr<Notification> NotificationCenter::createNotification(const String& iconURI, const String& title, const String& body, ExceptionCode& ec)
+ExceptionOr<Ref<Notification>> NotificationCenter::createNotification(const String& iconURI, const String& title, const String& body)
 {
-    if (!m_client || !scriptExecutionContext()) {
-        ec = INVALID_STATE_ERR;
-        return nullptr;
-    }
-    return Notification::create(title, body, iconURI, *scriptExecutionContext(), ec, *this);
+    if (!m_client || !scriptExecutionContext())
+        return Exception { INVALID_STATE_ERR };
+    return Notification::create(title, body, iconURI, *scriptExecutionContext(), *this);
 }
 
 int NotificationCenter::checkPermission()
@@ -85,7 +84,7 @@
     return m_client->checkPermission(scriptExecutionContext());
 }
 
-void NotificationCenter::requestPermission(const RefPtr<VoidCallback>& callback)
+void NotificationCenter::requestPermission(RefPtr<VoidCallback>&& callback)
 {
     if (!m_client || !scriptExecutionContext())
         return;
@@ -103,12 +102,12 @@
         });
         return;
     case SecurityOrigin::Ask:
-        m_client->requestPermission(scriptExecutionContext(), callback.get());
+        m_client->requestPermission(scriptExecutionContext(), WTFMove(callback));
         return;
     }
 
     ASSERT_NOT_REACHED();
-    m_client->requestPermission(scriptExecutionContext(), callback.get());
+    m_client->requestPermission(scriptExecutionContext(), WTFMove(callback));
 }
 
 #endif

Modified: trunk/Source/WebCore/Modules/notifications/NotificationCenter.h (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/NotificationCenter.h	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/NotificationCenter.h	2016-10-10 21:33:25 UTC (rev 207025)
@@ -29,11 +29,10 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef NotificationCenter_h
-#define NotificationCenter_h
+#pragma once
 
 #include "ActiveDOMObject.h"
-#include "ExceptionCode.h"
+#include "ExceptionOr.h"
 #include "Timer.h"
 #include <wtf/RefCounted.h>
 
@@ -45,15 +44,15 @@
 class NotificationClient;
 class VoidCallback;
 
-class NotificationCenter : public RefCounted<NotificationCenter>, private ActiveDOMObject {
+class NotificationCenter final : public RefCounted<NotificationCenter>, private ActiveDOMObject {
 public:
-    static Ref<NotificationCenter> create(ScriptExecutionContext*, NotificationClient*);
+    static Ref<NotificationCenter> create(ScriptExecutionContext&, NotificationClient*);
 
 #if ENABLE(LEGACY_NOTIFICATIONS)
-    RefPtr<Notification> createNotification(const String& iconURI, const String& title, const String& body, ExceptionCode&);
+    ExceptionOr<Ref<Notification>> createNotification(const String& iconURL, const String& title, const String& body);
 
     int checkPermission();
-    void requestPermission(const RefPtr<VoidCallback>&);
+    void requestPermission(RefPtr<VoidCallback>&&);
 #endif
 
     NotificationClient* client() const { return m_client; }
@@ -61,11 +60,11 @@
     using ActiveDOMObject::hasPendingActivity;
 
 private:
-    NotificationCenter(ScriptExecutionContext*, NotificationClient*);
+    NotificationCenter(ScriptExecutionContext&, NotificationClient*);
 
-    void stop() override;
-    const char* activeDOMObjectName() const override;
-    bool canSuspendForDocumentSuspension() const override;
+    void stop() final;
+    const char* activeDOMObjectName() const final;
+    bool canSuspendForDocumentSuspension() const final;
 
     void timerFired();
 
@@ -77,5 +76,3 @@
 } // namespace WebCore
 
 #endif // ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
-
-#endif // NotificationCenter_h

Modified: trunk/Source/WebCore/Modules/notifications/NotificationCenter.idl (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/NotificationCenter.idl	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/NotificationCenter.idl	2016-10-10 21:33:25 UTC (rev 207025)
@@ -34,7 +34,7 @@
     Conditional=LEGACY_NOTIFICATIONS,
     NoInterfaceObject,
 ] interface NotificationCenter {
-   [MayThrowLegacyException] Notification createNotification(DOMString iconUrl, DOMString title, DOMString body);
+   [MayThrowException] Notification createNotification(DOMString iconUrl, DOMString title, DOMString body);
 
    long checkPermission();
    void requestPermission(optional VoidCallback? callback);

Modified: trunk/Source/WebCore/Modules/notifications/NotificationController.cpp (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/NotificationController.cpp	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/NotificationController.cpp	2016-10-10 21:33:25 UTC (rev 207025)
@@ -33,21 +33,22 @@
 namespace WebCore {
 
 NotificationController::NotificationController(NotificationClient* client)
-    : m_client(client)
+    : m_client(*client)
 {
+    ASSERT(client);
 }
 
 NotificationController::~NotificationController()
 {
-    if (m_client)
-        m_client->notificationControllerDestroyed();
+    m_client.notificationControllerDestroyed();
 }
 
-NotificationClient* NotificationController::clientFrom(Page* page)
+NotificationClient* NotificationController::clientFrom(Page& page)
 {
-    if (NotificationController* controller = NotificationController::from(page))
-        return controller->client();
-    return 0;
+    auto* controller = NotificationController::from(&page);
+    if (!controller)
+        return nullptr;
+    return &controller->client();
 }
 
 const char* NotificationController::supplementName()

Modified: trunk/Source/WebCore/Modules/notifications/NotificationController.h (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/NotificationController.h	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/NotificationController.h	2016-10-10 21:33:25 UTC (rev 207025)
@@ -23,14 +23,12 @@
 * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
 */
 
-#ifndef NotificationController_h
-#define NotificationController_h
+#pragma once
 
 #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
 
 #include "Page.h"
 #include <wtf/Forward.h>
-#include <wtf/Noncopyable.h>
 
 namespace WebCore {
 
@@ -38,7 +36,6 @@
 
 class NotificationController : public Supplement<Page> {
     WTF_MAKE_FAST_ALLOCATED;
-    WTF_MAKE_NONCOPYABLE(NotificationController);
 public:
     explicit NotificationController(NotificationClient*);
     ~NotificationController();
@@ -45,16 +42,14 @@
 
     static const char* supplementName();
     static NotificationController* from(Page* page) { return static_cast<NotificationController*>(Supplement<Page>::from(page, supplementName())); }
-    WEBCORE_EXPORT static NotificationClient* clientFrom(Page*);
+    WEBCORE_EXPORT static NotificationClient* clientFrom(Page&);
 
-    NotificationClient* client() { return m_client; }
-    
+    NotificationClient& client() { return m_client; }
+
 private:
-    NotificationClient* m_client;
+    NotificationClient& m_client;
 };
 
 } // namespace WebCore
 
 #endif // ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
-
-#endif // NotificationController_h

Modified: trunk/Source/WebCore/Modules/notifications/WorkerGlobalScopeNotifications.cpp (207024 => 207025)


--- trunk/Source/WebCore/Modules/notifications/WorkerGlobalScopeNotifications.cpp	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebCore/Modules/notifications/WorkerGlobalScopeNotifications.cpp	2016-10-10 21:33:25 UTC (rev 207025)
@@ -68,8 +68,11 @@
 
 NotificationCenter* WorkerGlobalScopeNotifications::webkitNotifications()
 {
+    // FIXME: As of this writing, this always passes nullptr for the client.
+    // If it wasn't for that, the notification center create function could be taking a reference.
+    // How is it useful to create a notification center with no client?
     if (!m_notificationCenter)
-        m_notificationCenter = NotificationCenter::create(m_context, m_context->thread().getNotificationClient());
+        m_notificationCenter = NotificationCenter::create(*m_context, m_context->thread().getNotificationClient());
     return m_notificationCenter.get();
 }

Modified: trunk/Source/WebKit/mac/ChangeLog (207024 => 207025)


--- trunk/Source/WebKit/mac/ChangeLog	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebKit/mac/ChangeLog	2016-10-10 21:33:25 UTC (rev 207025)
@@ -1,3 +1,13 @@
+2016-10-10  Darin Adler  <[email protected]>
+
+        Move NotificationCenter from legacy to new DOM exceptions
+        https://bugs.webkit.org/show_bug.cgi?id=163217
+
+        Reviewed by Chris Dumez.
+
+        * WebView/WebView.mm:
+        (-[WebView _notificationIDForTesting:]): Pass reference to page rather than pointer.
+
 2016-10-09  Wenson Hsieh  <[email protected]>
 
         Support InputEvent.inputType for the new InputEvent spec

Modified: trunk/Source/WebKit/mac/WebView/WebView.mm (207024 => 207025)


--- trunk/Source/WebKit/mac/WebView/WebView.mm	2016-10-10 21:17:47 UTC (rev 207024)
+++ trunk/Source/WebKit/mac/WebView/WebView.mm	2016-10-10 21:33:25 UTC (rev 207025)
@@ -9002,9 +9002,12 @@
 - (uint64_t)_notificationIDForTesting:(JSValueRef)jsNotification
 {
 #if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
+    auto* page = _private->page;
+    if (!page)
+        return 0;
     JSContextRef context = [[self mainFrame] globalContext];
-    WebCore::Notification* notification = JSNotification::toWrapped(toJS(toJS(context), jsNotification));
-    return static_cast<WebNotificationClient*>(NotificationController::clientFrom(_private->page))->notificationIDForTesting(notification);
+    auto* notification = JSNotification::toWrapped(toJS(toJS(context), jsNotification));
+    return static_cast<WebNotificationClient*>(NotificationController::clientFrom(*page))->notificationIDForTesting(notification);
 #else
     return 0;
 #endif

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to