[webkit-reviews] review denied: [Bug 79636] Move Notifications APIs from DOMWindow.idl to DOMWindowNotifications.idl : [Attachment 131265] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kentaro Hara <haraken at chromium.org>'s
request for review:
Bug 79636: Move Notifications APIs from DOMWindow.idl to
DOMWindowNotifications.idl
https://bugs.webkit.org/show_bug.cgi?id=79636

Attachment 131265: Patch
https://bugs.webkit.org/attachment.cgi?id=131265&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131265&action=review


> Source/WebCore/notifications/DOMWindowNotifications.cpp:51
> +    if (!window->scriptExecutionContext())
> +	   return 0;

When can this happen?

> Source/WebCore/notifications/DOMWindowNotifications.cpp:53
> +    DOMWindowNotifications* supplement =
static_cast<DOMWindowNotifications*>(Supplement<ScriptExecutionContext>::from(w
indow->scriptExecutionContext(), NotificationCenter::supplementName()));

Why is this a Supplement<ScriptExecutionContext> rather than a
Supplement<DOMWindow> ?

> Source/WebCore/notifications/DOMWindowNotifications.cpp:63
> +    return
DOMWindowNotifications::from(window)->ensureNotificationCenter(window);

One pattern we've be using is for Supplement<X> to have a "back" pointer of
type X* so that lines like these read better:

DOMWindowNotifications::from(window)->webkitNotifications();

> Source/WebCore/notifications/DOMWindowNotifications.cpp:66
> +NotificationCenter*
DOMWindowNotifications::ensureNotificationCenter(DOMWindow* window)

ensureNotificationCenter -> webkitNotifications (to match the JavaScript API).

> Source/WebCore/notifications/DOMWindowNotifications.cpp:98
> +void DOMWindowNotifications::resetNotifications(DOMWindow* window)

You shouldn't need a special function here.  You can just have
DOMWindowNotifications inherit from DOMWindowProperty.	Then you can override
disconnectFrame().

> Source/WebCore/notifications/NotificationCenter.cpp:53
> +PassRefPtr<NotificationCenter> NotificationCenter::create(DOMWindow* window,
NotificationClient* client)
> +{
> +    RefPtr<NotificationCenter> notificationCenter(adoptRef(new
NotificationCenter(window, client)));
> +    notificationCenter->suspendIfNeeded();
> +    return notificationCenter.release();
> +}

You shouldn't need a new constructor.  Nothing about NotificationCenter needs
to change.

> Source/WebCore/notifications/NotificationCenter.cpp:104
> +    if (m_window)
> +	   DOMWindowNotifications::resetNotifications(m_window);
> +}
> +
> +void NotificationCenter::willDetachPage()
> +{
> +    if (m_window)
> +	   DOMWindowNotifications::resetNotifications(m_window);
> +}

DOMWindowNotifications can get these callbacks itself by inheriting from
DOMWindowProperty.  We don't need this extra work to forward the calls around.

> Source/WebCore/notifications/NotificationCenter.cpp:110
> +const AtomicString& NotificationCenter::supplementName()
> +{
> +    DEFINE_STATIC_LOCAL(AtomicString, name, ("NotificationCenter"));
> +    return name;
>  }

NotificationCenter shouldn't be a supplement.  DOMWindowNotification is the
supplement.  NotificationCenter shouldn't know anything about this change.

> Source/WebCore/page/DOMWindow.cpp:-528
> -#if ENABLE(NOTIFICATIONS)
> -    // FIXME: Notifications shouldn't have different disconnection logic
than
> -    // the rest of the DOMWindowProperties.
> -    resetNotifications();
> -#endif

Notice that at the top of this function we call disconnectFrame on all the
DOMWindowProperties.  If we override that in DOMWindowNotification, we can do
all the work we need to do.

> Source/WebCore/page/Frame.cpp:695
> +// FIXME: This will be split out of here to the notifications module.
>  #if ENABLE(NOTIFICATIONS)
>      if (m_domWindow)
> -	   m_domWindow->resetNotifications();
> +	   DOMWindowNotifications::resetNotifications(m_domWindow.get());
>  #endif

This won't be needed when DOMWindowNotifications is a DOMWindowProperty because
DOMWindowProperties get the willDetachPage message that sent out below.

> Source/WebCore/page/Frame.cpp:-743
> -	   if (m_domWindow) {
> +// FIXME: This will be split out of here to the notifications module.
>  #if ENABLE(NOTIFICATIONS)
> -	       m_domWindow->resetNotifications();
> +	   if (m_domWindow)
> +	       DOMWindowNotifications::resetNotifications(m_domWindow.get());
>  #endif
> -	   }

Same here.  Being a DOMWindowProperty lets us get the willDetachPage message
below.

> Source/WebCore/platform/Supplementable.h:52
> +    static void remove(Supplementable<T>* host, const AtomicString& key)
> +    {
> +	   host->removeSupplement(key);
> +    }

I don't think we should introduce the removal feature.	It's not needed. 
Supplements are just lazily allocated storage for a class.  In the same way
that you can't "remove" member variables, there shouldn't be any need to remove
supplements.