Bug 79636

Created attachment 130788 [details] WIP patch

Created attachment 130967 [details] Patch

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

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

Created attachment 131000 [details] Patch

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

Created attachment 131004 [details] Patch

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

Created attachment 131010 [details] Patch

Created attachment 131011 [details] Patch

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

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

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

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

Created attachment 131265 [details] Patch

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

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

Created attachment 131270 [details] Patch

(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 on attachment 131270 [details] Patch Attachment 131270 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11940663

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.

Created attachment 131297 [details] Patch

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

Created attachment 131301 [details] Patch

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

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.

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

Created attachment 131603 [details] Patch

(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 on attachment 131603 [details] Patch Attachment 131603 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11948499

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

(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?

Created attachment 131769 [details] try to remove mangled symbols

(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!

Created attachment 131791 [details] trying to see mac build

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

Created attachment 131845 [details] trying to see mac build

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

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

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

Created attachment 132041 [details] added xcode file

Created attachment 132155 [details] try to see mac build

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

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

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

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

Created attachment 132174 [details] hopefully fixed the mac build

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

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

Created attachment 132178 [details] hopefully fixed the mac build

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

Created attachment 132208 [details] patch for landing

Comment on attachment 132208 [details] patch for landing Clearing flags on attachment: 132208 Committed r110994: <http://trac.webkit.org/changeset/110994>

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

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.

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

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

This appears to have landed cleanly.