Bug 79636

Summary: Move Notifications APIs from DOMWindow.idl to DOMWindowNotifications.idl
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore Misc.Assignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, beidson, dglazkov, gns, jonlee, ojan, rakuco, vestbo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79633, 82015, 82026, 82027    
Bug Blocks: 79327    
Attachments:
Description Flags
WIP patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
try to remove mangled symbols
none
trying to see mac build
none
trying to see mac build
buildbot: commit-queue-
added xcode file
none
try to see mac build
buildbot: commit-queue-
hopefully fixed the mac build
buildbot: commit-queue-
hopefully fixed the mac build
abarth: commit-queue-
patch for landing none

Description Kentaro Hara 2012-02-26 23:44:48 PST
For WebKit modularization, we can move Notifications APIs from DOMWindow.idl to DOMWindowNotifications.idl.
Comment 1 Kentaro Hara 2012-03-08 00:28:50 PST
Created attachment 130788 [details]
WIP patch
Comment 2 Kentaro Hara 2012-03-08 21:07:01 PST
Created attachment 130967 [details]
Patch
Comment 3 Build Bot 2012-03-08 21:25:36 PST
Comment on attachment 130967 [details]
Patch

Attachment 130967 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11906371
Comment 4 Gustavo Noronha (kov) 2012-03-08 23:05:47 PST
Comment on attachment 130967 [details]
Patch

Attachment 130967 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11915356
Comment 5 Kentaro Hara 2012-03-09 00:31:17 PST
Created attachment 131000 [details]
Patch
Comment 6 Build Bot 2012-03-09 00:57:49 PST
Comment on attachment 131000 [details]
Patch

Attachment 131000 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11911437
Comment 7 Kentaro Hara 2012-03-09 01:09:49 PST
Created attachment 131004 [details]
Patch
Comment 8 Build Bot 2012-03-09 01:38:32 PST
Comment on attachment 131004 [details]
Patch

Attachment 131004 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11903457
Comment 9 Kentaro Hara 2012-03-09 01:50:02 PST
Created attachment 131010 [details]
Patch
Comment 10 Kentaro Hara 2012-03-09 01:52:39 PST
Created attachment 131011 [details]
Patch
Comment 11 Build Bot 2012-03-09 03:09:06 PST
Comment on attachment 131011 [details]
Patch

Attachment 131011 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11903491
Comment 12 Build Bot 2012-03-09 03:45:42 PST
Comment on attachment 131011 [details]
Patch

Attachment 131011 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11904491
Comment 13 Kentaro Hara 2012-03-09 04:08:39 PST
Regarding the Mac bot failure, where do I need to add the mangled signatures to other than WebCore.exp.in and WebKit2.order?
Comment 14 Jon Lee 2012-03-09 06:43:55 PST
(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.
Comment 15 Jon Lee 2012-03-09 07:45:14 PST
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?
Comment 16 Adam Barth 2012-03-09 09:39:39 PST
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.
Comment 17 Kentaro Hara 2012-03-11 18:08:42 PDT
Created attachment 131265 [details]
Patch
Comment 18 Kentaro Hara 2012-03-11 18:09:33 PDT
(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.
Comment 19 Kentaro Hara 2012-03-11 18:10:15 PDT
(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!
Comment 20 Adam Barth 2012-03-11 18:22:42 PDT
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.
Comment 21 Kentaro Hara 2012-03-11 19:07:09 PDT
Created attachment 131270 [details]
Patch
Comment 22 Kentaro Hara 2012-03-11 19:09:36 PDT
(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!
Comment 23 Build Bot 2012-03-11 19:38:30 PDT
Comment on attachment 131270 [details]
Patch

Attachment 131270 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940663
Comment 24 Adam Barth 2012-03-11 20:28:19 PDT
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.
Comment 25 Kentaro Hara 2012-03-12 02:06:45 PDT
Created attachment 131297 [details]
Patch
Comment 26 Kentaro Hara 2012-03-12 02:10:03 PDT
(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.
Comment 27 Kentaro Hara 2012-03-12 02:34:22 PDT
Created attachment 131301 [details]
Patch
Comment 28 Build Bot 2012-03-12 04:24:10 PDT
Comment on attachment 131301 [details]
Patch

Attachment 131301 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940820
Comment 29 Adam Barth 2012-03-12 10:41:25 PDT
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.
Comment 30 Kentaro Hara 2012-03-12 17:22:09 PDT
(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...
Comment 31 Kentaro Hara 2012-03-13 06:49:06 PDT
Created attachment 131603 [details]
Patch
Comment 32 Kentaro Hara 2012-03-13 06:50:57 PDT
(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.
Comment 33 Build Bot 2012-03-13 07:14:07 PDT
Comment on attachment 131603 [details]
Patch

Attachment 131603 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11948499
Comment 34 Brady Eidson 2012-03-13 08:58:16 PDT
The DOMWindow parts of this will likely have to adapt to http://trac.webkit.org/changeset/110570 which includes a FIXME referencing this bug.
Comment 35 Kentaro Hara 2012-03-13 18:41:30 PDT
(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?
Comment 36 Kentaro Hara 2012-03-13 18:46:10 PDT
Created attachment 131769 [details]
try to remove mangled symbols
Comment 37 Brady Eidson 2012-03-13 18:55:00 PDT
(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!
Comment 38 Kentaro Hara 2012-03-13 22:50:37 PDT
Created attachment 131791 [details]
trying to see mac build
Comment 39 Build Bot 2012-03-14 00:47:46 PDT
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
Comment 40 WebKit Review Bot 2012-03-14 01:32:28 PDT
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
Comment 41 Early Warning System Bot 2012-03-14 05:28:57 PDT
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
Comment 42 Early Warning System Bot 2012-03-14 06:03:41 PDT
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
Comment 43 Kentaro Hara 2012-03-14 07:54:54 PDT
Created attachment 131845 [details]
trying to see mac build
Comment 44 Build Bot 2012-03-14 08:39:44 PDT
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
Comment 45 Kentaro Hara 2012-03-14 08:57:42 PDT
(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.
Comment 46 Jon Lee 2012-03-14 09:20:41 PDT
(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.
Comment 47 Kentaro Hara 2012-03-15 07:18:51 PDT
Created attachment 132041 [details]
added xcode file
Comment 48 Kentaro Hara 2012-03-15 17:07:56 PDT
Created attachment 132155 [details]
try to see mac build
Comment 49 Build Bot 2012-03-15 18:24:15 PDT
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
Comment 50 Adam Barth 2012-03-15 18:25:34 PDT
Kentaro, let me try this locally for you.  I have a mac build so I can iterate more quickly to fix the error.
Comment 51 Kentaro Hara 2012-03-15 18:36:08 PDT
(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...:-)
Comment 52 Kentaro Hara 2012-03-15 18:37:51 PDT
(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.
Comment 53 Kentaro Hara 2012-03-15 18:46:56 PDT
Created attachment 132174 [details]
hopefully fixed the mac build
Comment 54 Build Bot 2012-03-15 19:10:34 PDT
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
Comment 55 Kentaro Hara 2012-03-15 19:19:01 PDT
(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...
Comment 56 Kentaro Hara 2012-03-15 19:35:39 PDT
Created attachment 132178 [details]
hopefully fixed the mac build
Comment 57 Adam Barth 2012-03-15 22:32:08 PDT
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
Comment 58 Kentaro Hara 2012-03-15 23:14:52 PDT
Created attachment 132208 [details]
patch for landing
Comment 59 WebKit Review Bot 2012-03-16 05:50:17 PDT
Comment on attachment 132208 [details]
patch for landing

Clearing flags on attachment: 132208

Committed r110994: <http://trac.webkit.org/changeset/110994>
Comment 60 Kentaro Hara 2012-03-20 16:53:22 PDT
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).
Comment 61 Adam Barth 2012-03-21 09:29:28 PDT
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.
Comment 62 Kentaro Hara 2012-03-21 16:37:09 PDT
(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.
Comment 63 Adam Barth 2012-03-23 00:12:06 PDT
You can see the commit stream in context on GitHub:
https://github.com/abarth/webkit/compare/2757a48897...notification
Comment 64 Adam Barth 2012-03-23 17:21:31 PDT
This appears to have landed cleanly.