Bug 155011

Created attachment 272842 [details] Implements lifecycle callback

<rdar://problem/24972528>

Created attachment 272844 [details] Fix GTK/EFL builds

Created attachment 272845 [details] Another GTK/EFL build fix

Created attachment 273064 [details] Updated for ToT

Comment on attachment 273064 [details] Updated for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=273064&action=review > Source/WebCore/bindings/scripts/IDLAttributes.txt:94 > +NeedsLifecycleProcessingStack Lifecycle of what? This should be named "NeedsCustomElementLifecycleProcessingStack" or probably just "NeedsCustomElementLifecycleProcessing". Or maybe "HasCustomElementLifecycleCallbacks" which answers 'why?' > Source/WebCore/dom/LifecycleCallbackQueue.cpp:43 > + Invalid, Invalid type is never used and doesn't seem like something you would use later either. You should just remove it. > Source/WebCore/dom/LifecycleCallbackQueue.cpp:63 > + Type m_type { Type::Invalid }; The only constructor initializes this explicitly. > Source/WebCore/dom/LifecycleCallbackQueue.h:75 > + WEBCORE_EXPORT static LifecycleCallbackQueue* ensureCurrentQueue(); Why WEBCORE_EXPORTs? The stuff here seems WebCore internal. > Source/WebCore/dom/LifecycleCallbackQueue.h:85 > + WEBCORE_EXPORT static CustomElementLifecycleProcessingStack* s_currentProcessingStack; You should probably just access this from non-inline functions rather than exporting. Not a big fan of these sort of static-rooted stacks but I suppose it works fine here.

Comment on attachment 273064 [details] Updated for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=273064&action=review >> Source/WebCore/bindings/scripts/IDLAttributes.txt:94 >> +NeedsLifecycleProcessingStack > > Lifecycle of what? This should be named "NeedsCustomElementLifecycleProcessingStack" or probably just "NeedsCustomElementLifecycleProcessing". Or maybe "HasCustomElementLifecycleCallbacks" which answers 'why?' Will use InvokesCustomElementLifecycleCallbacks. >> Source/WebCore/dom/LifecycleCallbackQueue.cpp:43 >> + Invalid, > > Invalid type is never used and doesn't seem like something you would use later either. You should just remove it. Yeah, just realized that. Will remove. >> Source/WebCore/dom/LifecycleCallbackQueue.h:75 >> + WEBCORE_EXPORT static LifecycleCallbackQueue* ensureCurrentQueue(); > > Why WEBCORE_EXPORTs? The stuff here seems WebCore internal. Yeah, it'll be used in the future but will remove for now. >> Source/WebCore/dom/LifecycleCallbackQueue.h:85 >> + WEBCORE_EXPORT static CustomElementLifecycleProcessingStack* s_currentProcessingStack; > > You should probably just access this from non-inline functions rather than exporting. > > Not a big fan of these sort of static-rooted stacks but I suppose it works fine here. Can't do that because of the performance. Being able to inline all code in the constructor & destructor is very important. Having said that, this variable doesn't need to be exported for now so I'll just remove WEBCORE_EXPORT.

Committed r197611: <http://trac.webkit.org/changeset/197611>

Comment on attachment 273064 [details] Updated for ToT View in context: https://bugs.webkit.org/attachment.cgi?id=273064&action=review > Source/WebCore/dom/LifecycleCallbackQueue.h:34 > +#include <wtf/text/AtomicString.h> We should be able to use Forward.h instead of AtomicString.h. > Source/WebCore/dom/LifecycleCallbackQueue.h:57 > +class LifecycleCallbackQueue { > + WTF_MAKE_NONCOPYABLE(LifecycleCallbackQueue); > +public: > + LifecycleCallbackQueue(); > + ~LifecycleCallbackQueue(); > + > + static void enqueueAttributeChangedCallback(Element&, JSCustomElementInterface&, > + const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue); > + > + void invokeAll(); > + > +private: > + Vector<LifecycleQueueItem> m_items; > +}; Can we forward-declare this and define this class only inside the cpp file instead? I’m not sure it makes sense that enqueueAttributeChangedCallback is a static class member of a class that we never need house directly. > Source/WebCore/dom/LifecycleCallbackQueue.h:82 > + LifecycleCallbackQueue* m_queue { nullptr }; This should be std::unique_ptr.

(In reply to comment #9) > Comment on attachment 273064 [details] > Updated for ToT > > View in context: > https://bugs.webkit.org/attachment.cgi?id=273064&action=review > > > Source/WebCore/dom/LifecycleCallbackQueue.h:34 > > +#include <wtf/text/AtomicString.h> > > We should be able to use Forward.h instead of AtomicString.h. > > > Source/WebCore/dom/LifecycleCallbackQueue.h:57 > > +class LifecycleCallbackQueue { > > + WTF_MAKE_NONCOPYABLE(LifecycleCallbackQueue); > > +public: > > + LifecycleCallbackQueue(); > > + ~LifecycleCallbackQueue(); > > + > > + static void enqueueAttributeChangedCallback(Element&, JSCustomElementInterface&, > > + const QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue); > > + > > + void invokeAll(); > > + > > +private: > > + Vector<LifecycleQueueItem> m_items; > > +}; > > Can we forward-declare this and define this class only inside the cpp file > instead? I’m not sure it makes sense that enqueueAttributeChangedCallback is > a static class member of a class that we never need house directly. No, because we use static function: enqueueAttributeChangedCallback in Element.cpp. > > Source/WebCore/dom/LifecycleCallbackQueue.h:82 > > + LifecycleCallbackQueue* m_queue { nullptr }; > > This should be std::unique_ptr. We can't for performance reasons. It's very important that the destructor of this function doesn't generate the code to deallocate the queue.