[webkit-reviews] review denied: [Bug 79636] Move Notifications APIs from DOMWindow.idl to DOMWindowNotifications.idl : [Attachment 131270] 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 131270: Patch
https://bugs.webkit.org/attachment.cgi?id=131270&action=review

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


Looks like you also have a build problem on mac too.

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

We shouldn't need to pass window here anymore, right?  DOMWindowNotifications
has m_window

> Source/WebCore/notifications/DOMWindowNotifications.cpp:95
> +NotificationCenter* DOMWindowNotifications::notificationCenter(DOMWindow*
window)
> +{
> +    return m_notificationCenter.get();
> +}
> +
> +void DOMWindowNotifications::clearNotificationCenter(DOMWindow* window)
> +{
> +    m_notificationCenter = 0;
> +}

These functions aren't needed.

> Source/WebCore/notifications/DOMWindowNotifications.cpp:101
> +    DOMWindowNotifications* supplement =
static_cast<DOMWindowNotifications*>(Supplement<DOMWindow>::from(m_window,
supplementName()));
> +    if (!supplement)
> +	   return;

Why do we need to do this?  DOMWindowNotifications::disconnectFrame isn't a
static function.  We can use use |this|.

> Source/WebCore/notifications/DOMWindowNotifications.cpp:105
> +    NotificationCenter* notificationCenter =
supplement->notificationCenter(m_window);
> +    if (!notificationCenter)
> +	   return;

Why not just:

if (m_notificationCenter) {
    m_notificationCenter->disconnectFrame();
    m_notificationCenter = 0;
}

?

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

After you change DOMWindowNotifications::disconnectFrame to use |this|, you'll
only need the supplementName in DOMWindowNotifications::from, which means you
can remove this function too.