Bug 155011 - Add basic support for attributeChanged lifecycle callback
Summary: Add basic support for attributeChanged lifecycle callback
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 154907
  Show dependency treegraph
 
Reported: 2016-03-04 00:43 PST by Ryosuke Niwa
Modified: 2016-03-05 16:27 PST (History)
8 users (show)

See Also:


Attachments
Implements lifecycle callback (39.36 KB, patch)
2016-03-04 01:01 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fix GTK/EFL builds (39.76 KB, patch)
2016-03-04 01:59 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Another GTK/EFL build fix (39.80 KB, patch)
2016-03-04 03:03 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (39.59 KB, patch)
2016-03-04 17:53 PST, Ryosuke Niwa
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-03-04 00:43:23 PST
Add the most basic support for attributeChanged callback:
https://w3c.github.io/webcomponents/spec/custom/#dfn-attribute-changed-callback
Comment 1 Ryosuke Niwa 2016-03-04 01:01:06 PST
Created attachment 272842 [details]
Implements lifecycle callback
Comment 2 Radar WebKit Bug Importer 2016-03-04 01:04:23 PST
<rdar://problem/24972528>
Comment 3 Ryosuke Niwa 2016-03-04 01:59:03 PST
Created attachment 272844 [details]
Fix GTK/EFL builds
Comment 4 Ryosuke Niwa 2016-03-04 03:03:44 PST
Created attachment 272845 [details]
Another GTK/EFL build fix
Comment 5 Ryosuke Niwa 2016-03-04 17:53:50 PST
Created attachment 273064 [details]
Updated for ToT
Comment 6 Antti Koivisto 2016-03-04 22:52:40 PST
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 7 Ryosuke Niwa 2016-03-04 23:01:45 PST
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.
Comment 8 Ryosuke Niwa 2016-03-04 23:50:42 PST
Committed r197611: <http://trac.webkit.org/changeset/197611>
Comment 9 Darin Adler 2016-03-05 12:19:36 PST
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.
Comment 10 Ryosuke Niwa 2016-03-05 16:27:23 PST
(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.