Bug 79636 – Move Notifications APIs from DOMWindow.idl to DOMWindowNotifications.idl

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kentaro Hara]

For WebKit modularization, we can move Notifications APIs from DOMWindow.idl to DOMWindowNotifications.idl.

[Kentaro Hara]

Created attachment 130788 [details]
WIP patch

[Kentaro Hara]

Created attachment 130967 [details]
Patch

[Build Bot]

Comment on attachment 130967 [details]
Patch

Attachment 130967 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11906371

[Gustavo Noronha (kov)]

Comment on attachment 130967 [details]
Patch

Attachment 130967 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915356

[Kentaro Hara]

Created attachment 131000 [details]
Patch

[Build Bot]

Comment on attachment 131000 [details]
Patch

Attachment 131000 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11911437

[Kentaro Hara]

Created attachment 131004 [details]
Patch

[Build Bot]

Comment on attachment 131004 [details]
Patch

Attachment 131004 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11903457

[Kentaro Hara]

Created attachment 131010 [details]
Patch

[Kentaro Hara]

Created attachment 131011 [details]
Patch

[Build Bot]

Comment on attachment 131011 [details]
Patch

Attachment 131011 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11903491

[Build Bot]

Comment on attachment 131011 [details]
Patch

Attachment 131011 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11904491

[Kentaro Hara]

Regarding the Mac bot failure, where do I need to add the mangled signatures to other than WebCore.exp.in and WebKit2.order?

[Jon Lee]

(In reply to comment #13)
> Regarding the Mac bot failure, where do I need to add the mangled signatures to other than WebCore.exp.in and WebKit2.order?

Looks like it's missing from the xcodeproj.

[Jon Lee]

Comment on attachment 131011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131011&action=review

> Source/WebCore/WebCore.exp.in:645
> +__ZN7WebCore22DOMWindowNotifications19webkitNotificationsEPNS_9DOMWindowE

Shouldn't this be encapsulated with an #if ENABLE(NOTIFICATIONS)? I'm not sure what proper style is, but I think there is a separate section for it.

> Source/WebCore/notifications/DOMWindowNotifications.cpp:74
> +    if (!m_notificationCenter) {

Maybe it'd be better to reverse the test and do an early return instead.

> Source/WebCore/notifications/DOMWindowNotifications.cpp:101
> +    return;

Not needed.

> Source/WebCore/notifications/DOMWindowNotifications.h:50
> +    static const AtomicString& supplementName();

Is this used externally? If not, it should be private.

> Source/WebCore/notifications/DOMWindowNotifications.h:55
> +    NotificationCenter* notificationCenterSingleton(DOMWindow*);

I don't understand the name. How does the function use a singleton? It seems like ensureNotificationCenter() would be a more appropriate name.

> Source/WebCore/notifications/NotificationCenter.h:56
> +    static const AtomicString& supplementName();

This isn't used anywhere is it?

> Source/WebCore/notifications/NotificationCenter.h:58
>      static PassRefPtr<NotificationCenter> create(ScriptExecutionContext*, NotificationPresenter*);

Is this still constructor still needed now?

[Adam Barth]

Comment on attachment 131011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131011&action=review

> Source/WebCore/notifications/NotificationCenter.cpp:69
>  NotificationCenter::NotificationCenter(ScriptExecutionContext* context, NotificationPresenter* presenter)
>      : ActiveDOMObject(context, this)
> -    , m_notificationPresenter(presenter) {}
> +    , DOMWindowProperty(0)

An object shouldn't need to be both an ActiveDOMObject and a DOMWindowProperty.  ActiveDOMObject is needed when the object uses a ScriptExecutionContext as a context object and DOMWindowProperty is needed when using a Frame as a context object.

> Source/WebCore/notifications/NotificationCenter.h:54
> -class NotificationCenter : public RefCounted<NotificationCenter>, public ActiveDOMObject {
> +class NotificationCenter : public RefCounted<NotificationCenter>, public ActiveDOMObject, public Supplement<ScriptExecutionContext>, public DOMWindowProperty {

This can't be right.  Being RefCounted is incompatible with being a Supplement.  Supplement uses OwnPtr and RefCounted uses RefPtr.

Take a look at how Geolocation solves these problems.  The pattern we've been using is for DOMWindowNotifications to be an OwnPtr'd object (i.e., a Supplement<DOMWindow>) that holds a RefPtr to NotificationCenter.

[Kentaro Hara]

Created attachment 131265 [details]
Patch

[Kentaro Hara]

(In reply to comment #16)
> > Source/WebCore/notifications/NotificationCenter.cpp:69
> >  NotificationCenter::NotificationCenter(ScriptExecutionContext* context, NotificationPresenter* presenter)
> >      : ActiveDOMObject(context, this)
> > -    , m_notificationPresenter(presenter) {}
> > +    , DOMWindowProperty(0)
> 
> An object shouldn't need to be both an ActiveDOMObject and a DOMWindowProperty.  ActiveDOMObject is needed when the object uses a ScriptExecutionContext as a context object and DOMWindowProperty is needed when using a Frame as a context object.

Fixed.

> > Source/WebCore/notifications/NotificationCenter.h:54
> > -class NotificationCenter : public RefCounted<NotificationCenter>, public ActiveDOMObject {
> > +class NotificationCenter : public RefCounted<NotificationCenter>, public ActiveDOMObject, public Supplement<ScriptExecutionContext>, public DOMWindowProperty {
> 
> This can't be right.  Being RefCounted is incompatible with being a Supplement.  Supplement uses OwnPtr and RefCounted uses RefPtr.

Fixed.

[Kentaro Hara]

(In reply to comment #15)
+__ZN7WebCore22DOMWindowNotifications19webkitNotificationsEPNS_9DOMWindowE
> 
> Shouldn't this be encapsulated with an #if ENABLE(NOTIFICATIONS)? I'm not sure what proper style is, but I think there is a separate section for it.

Fixed.

> > Source/WebCore/notifications/DOMWindowNotifications.cpp:74
> > +    if (!m_notificationCenter) {
> 
> Maybe it'd be better to reverse the test and do an early return instead.
> 
> > Source/WebCore/notifications/DOMWindowNotifications.cpp:101
> > +    return;
> 
> Not needed.
> 
> > Source/WebCore/notifications/DOMWindowNotifications.h:50
> > +    static const AtomicString& supplementName();
> 
> Is this used externally? If not, it should be private.
> 
> > Source/WebCore/notifications/DOMWindowNotifications.h:55
> > +    NotificationCenter* notificationCenterSingleton(DOMWindow*);
> 
> I don't understand the name. How does the function use a singleton? It seems like ensureNotificationCenter() would be a more appropriate name.
> 
> > Source/WebCore/notifications/NotificationCenter.h:56
> > +    static const AtomicString& supplementName();
> 
> This isn't used anywhere is it?
> 
> > Source/WebCore/notifications/NotificationCenter.h:58
> >      static PassRefPtr<NotificationCenter> create(ScriptExecutionContext*, NotificationPresenter*);
> 
> Is this still constructor still needed now?

All fixed. Thank you very much for the comments!

[Adam Barth]

Comment on attachment 131265 [details]
Patch

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(window->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.

[Kentaro Hara]

Created attachment 131270 [details]
Patch

[Kentaro Hara]

(In reply to comment #20)
> > Source/WebCore/notifications/DOMWindowNotifications.cpp:51
> > +    if (!window->scriptExecutionContext())
> > +        return 0;
> 
> When can this happen?

Removed.

> > Source/WebCore/notifications/DOMWindowNotifications.cpp:53
> > +    DOMWindowNotifications* supplement = static_cast<DOMWindowNotifications*>(Supplement<ScriptExecutionContext>::from(window->scriptExecutionContext(), NotificationCenter::supplementName()));
> 
> Why is this a Supplement<ScriptExecutionContext> rather than a Supplement<DOMWindow> ?

Fixed.

> > 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).

I guess we cannot overload a static method and a non-static method. I renamed it to ensureWebkitNotifications().

> > 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().

Fixed.

> > 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.

Fixed.

> > 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.

Fixed.

> > 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.

Fixed.

> > 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.

Fixed. 

> > 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.

Fixed. Great clean-up by the way:)

> > 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.

Removed remove().

Thanks for the comments!

[Build Bot]

Comment on attachment 131270 [details]
Patch

Attachment 131270 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940663

[Adam Barth]

Comment on attachment 131270 [details]
Patch

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.

[Kentaro Hara]

Created attachment 131297 [details]
Patch

[Kentaro Hara]

(In reply to comment #24)
> Looks like you also have a build problem on mac too.

Do you have any idea for this?

The mangled signatures that Mac build error (http://queues.webkit.org/results/11940663) complains about are already added to WebCore.exp.in and WebKit2.order...

> > 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.

Fixed.

[Kentaro Hara]

Created attachment 131301 [details]
Patch

[Build Bot]

Comment on attachment 131301 [details]
Patch

Attachment 131301 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940820

[Adam Barth]

Comment on attachment 131301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131301&action=review

Maybe the problem with mac is that the symbol is inside the ENABLE(NOTIFICATIONS) block but needs to be outside?  That doesn't really make sense though.  I'll look more carefully later today.

> Source/WebCore/notifications/DOMWindowNotifications.h:53
> +    void disconnectFrame();

I've been putting OVERRIDE on these since they're easy to break if this function gets changed in the base class.

[Kentaro Hara]

(In reply to comment #28)
> (From update of attachment 131301 [details])
> Attachment 131301 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/11940820

  Undefined symbols for architecture x86_64:
    "__ZN7WebCore22DOMWindowNotifications19webkitNotificationsEPNS_9DOMWindowE", referenced from:
  __ZN7WebCore30jsDOMWindowWebkitNotificationsEPN3JSC9ExecStateENS0_7JSValueERKNS0_10IdentifierE in JSDOMWindow.o
  ld: symbol(s) not found for architecture x86_64


The generated JSDOMWindow.cpp is this:

  #if ENABLE(NOTIFICATIONS)
  JSValue jsDOMWindowWebkitNotifications(ExecState* exec, JSValue slotBase, const Identifier&) {
    ...
    DOMWindow* impl = static_cast<DOMWindow*>(castedThis->impl());
    JSValue result = toJS(exec, castedThis->globalObject(), WTF::getPtr(DOMWindowNotifications::webkitNotifications(impl)));
    return result;
  }
  #endif

Looks sane...

[Kentaro Hara]

Created attachment 131603 [details]
Patch

[Kentaro Hara]

(In reply to comment #29)
> Maybe the problem with mac is that the symbol is inside the ENABLE(NOTIFICATIONS) block but needs to be outside?  That doesn't really make sense though.  I'll look more carefully later today.

I wrote it outside of ENABLE(NOTIFICATIONS). Watching the mac ews-bot. But I think we do not need to add the symbol to WebCore.exp.in, in the first place...

> > Source/WebCore/notifications/DOMWindowNotifications.h:53
> > +    void disconnectFrame();
> 
> I've been putting OVERRIDE on these since they're easy to break if this function gets changed in the base class.

Fixed.

[Build Bot]

Comment on attachment 131603 [details]
Patch

Attachment 131603 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11948499

[Brady Eidson]

The DOMWindow parts of this will likely have to adapt to http://trac.webkit.org/changeset/110570 which includes a FIXME referencing this bug.

[Kentaro Hara]

(In reply to comment #34)
> The DOMWindow parts of this will likely have to adapt to http://trac.webkit.org/changeset/110570 which includes a FIXME referencing this bug.

The concern is going to be removed by this patch, and we can remove FIXME from DOMWindow.cpp, right?

[Kentaro Hara]

Created attachment 131769 [details]
try to remove mangled symbols

[Brady Eidson]

(In reply to comment #35)
> (In reply to comment #34)
> > The DOMWindow parts of this will likely have to adapt to http://trac.webkit.org/changeset/110570 which includes a FIXME referencing this bug.
> 
> The concern is going to be removed by this patch, and we can remove FIXME from DOMWindow.cpp, right?

Yup!

[Kentaro Hara]

Created attachment 131791 [details]
trying to see mac build

[Build Bot]

Comment on attachment 131791 [details]
trying to see mac build

Attachment 131791 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11949868

[WebKit Review Bot]

Comment on attachment 131791 [details]
trying to see mac build

Attachment 131791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11951109

[Early Warning System Bot]

Comment on attachment 131791 [details]
trying to see mac build

Attachment 131791 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11949995

[Early Warning System Bot]

Comment on attachment 131791 [details]
trying to see mac build

Attachment 131791 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11952183

[Kentaro Hara]

Created attachment 131845 [details]
trying to see mac build

[Build Bot]

Comment on attachment 131845 [details]
trying to see mac build

Attachment 131845 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11954143

[Kentaro Hara]

(In reply to comment #44)
> (From update of attachment 131845 [details])
> Attachment 131845 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/11954143

Does anyone have an idea for fixing the mac build failure? I've tried the followings (and all failed):

- Write "__ZN7WebCore22DOMWindowNotifications19webkitNotificationsEPNS_9DOMWindowE" outside of ENABLE(NOTIFICATIONS) in WebCore.exp.in.

- Write "__ZN7WebCore22DOMWindowNotifications19webkitNotificationsEPNS_9DOMWindowE" inside of ENABLE(NOTIFICATIONS) in WebCore.exp.in.


- Do not write "__ZN7WebCore22DOMWindowNotifications19webkitNotificationsEPNS_9DOMWindowE" in WebCore.exp.in.

[Jon Lee]

(In reply to comment #45)
> (In reply to comment #44)
> > (From update of attachment 131845 [details] [details])
> > Attachment 131845 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/11954143
> 
> Does anyone have an idea for fixing the mac build failure? I've tried the followings (and all failed):
> 
The files and derived sources need to be part of the Xcode project.

[Kentaro Hara]

Created attachment 132041 [details]
added xcode file

[Kentaro Hara]

Created attachment 132155 [details]
try to see mac build

[Build Bot]

Comment on attachment 132155 [details]
try to see mac build

Attachment 132155 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11954890

[Adam Barth]

Kentaro, let me try this locally for you.  I have a mac build so I can iterate more quickly to fix the error.

[Kentaro Hara]

(In reply to comment #50)
> Kentaro, let me try this locally for you.  I have a mac build so I can iterate more quickly to fix the error.

Thanks Adam! (I also have a Mac build and tried some ideas, but could not solve it...:-)

[Kentaro Hara]

(In reply to comment #49)
> (From update of attachment 132155 [details])
> Attachment 132155 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/11954890

Ah, this error is different from the previous one. Maybe I've added a wrong xcode target for new files. I guess I can fix it.

[Kentaro Hara]

Created attachment 132174 [details]
hopefully fixed the mac build

[Build Bot]

Comment on attachment 132174 [details]
hopefully fixed the mac build

Attachment 132174 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11958875

[Kentaro Hara]

(In reply to comment #54)
> (From update of attachment 132174 [details])
> Attachment 132174 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/11958875

Though my local Mac build is passing...

[Kentaro Hara]

Created attachment 132178 [details]
hopefully fixed the mac build

[Adam Barth]

Comment on attachment 132178 [details]
hopefully fixed the mac build

View in context: https://bugs.webkit.org/attachment.cgi?id=132178&action=review

> Source/WebCore/notifications/DOMWindowNotifications.cpp:40
> +DOMWindowNotifications::DOMWindowNotifications(DOMWindow *window)

DOMWindow *window => DOMWindow* window

> Source/WebCore/notifications/DOMWindowNotifications.cpp:72
> +    if (m_notificationCenter)
> +        return m_notificationCenter.get();
> +
> +    if (!m_window->isCurrentlyDisplayedInFrame())
> +        return 0;

You've reversed the order of these to checks.  It's important to return 0 when the window is not currently displayed in a frame.

> Source/WebCore/notifications/DOMWindowNotifications.h:50
> +    DOMWindowNotifications(DOMWindow*);

Please add the explicit keyword to one-argument constructor.s

[Kentaro Hara]

Created attachment 132208 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 132208 [details]
patch for landing

Clearing flags on attachment: 132208

Committed r110994: <http://trac.webkit.org/changeset/110994>

[Kentaro Hara]

The patch was rolled out due to Chromium crash (https://mail.google.com/mail/u/0/?ui=2&shva=1#search/crash+report/13629e8e375fb9bf). It seems the patch caused another crash (I couldn't show the link for confidentiality).

[Adam Barth]

I'm inclined to try this again, but in smaller steps.  Kentaro, if you're busy with bindings performance, I can give this bug a try.

[Kentaro Hara]

(In reply to comment #61)
> I'm inclined to try this again, but in smaller steps.  Kentaro, if you're busy with bindings performance, I can give this bug a try.

abarth: Thank you very much. I can have time, but I am happy if you would give a try since I do not fully familiar with the pointer issues.

[Adam Barth]

You can see the commit stream in context on GitHub:
https://github.com/abarth/webkit/compare/2757a48897...notification

[Adam Barth]

This appears to have landed cleanly.