Bug 160950 – Add basic support for connected and disconnected callbacks

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Add the support for connected and disconnected callbacks.

[Ryosuke Niwa]

Created attachment 286343 [details]
Adds the basic support

[Ryosuke Niwa]

Created attachment 286344 [details]
Fixed GTK+/EFL builds

[Ryosuke Niwa]

This change looks perf neutral on Dromaeo DOM modify:

Without patch:
DOM Modification:Runs: 714/s stdev=0.0%
appendChild:Runs: 1.655K/s stdev=0.8%
cloneNode:Runs: 281/s stdev=8.5%
createElement:Runs: 1.12K/s stdev=6.5%
createTextNode:Runs: 949/s stdev=2.1%
innerHTML:Runs: 170/s stdev=1.3%
insertBefore:Runs: 1.578K/s stdev=0.5%

With patch:
DOM Modification:Runs: 719/s stdev=0.0%
appendChild:Runs: 1.652K/s stdev=0.4%
cloneNode:Runs: 288/s stdev=7.6%
createElement:Runs: 1.13K/s stdev=5.7%
createTextNode:Runs: 953/s stdev=1.4%
innerHTML:Runs: 168/s stdev=1.1%
insertBefore:Runs: 1.593K/s stdev=0.4%

[Chris Dumez]

Comment on attachment 286344 [details]
Fixed GTK+/EFL builds

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

r=me with comments.

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:155
> +void JSCustomElementInterface::invokeCallback(Element& element, JSObject* callback, ArgumentAdder arguementAdder)

Typo: arguementAdder. Although I don't like this name. Maybe we can call it "addArguments".

Do we really need a template here? If we used "const Function<void(ExecState*, MarkedArgumentBuffer&)>& addArguments = {}" then the call sites that don't want to add arguments would look much nicer.

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:179
> +    arguementAdder(state, args);

Typo: arguementAdder

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:199
> +    invokeCallback(element, m_connectedCallback.get(), [&](ExecState*, MarkedArgumentBuffer&) { });

Don't need the &

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:209
> +    invokeCallback(element, m_disconnectedCallback.get(), [&](ExecState*, MarkedArgumentBuffer&) { });

Don't need the &

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:227
> +        args.append(namespaceURI == nullAtom ? jsNull() : jsStringWithCache(state, attributeName.namespaceURI()));

Can use namespaceURI instead of attributeName.namespaceURI() at the end.

Also we have jsStringOrNull() in JSDOMBinding.h for this purpose so you don't need this ternary.

Ditto for all the ternaries above.

> Source/WebCore/bindings/scripts/IDLAttributes.txt:94
> +CEReactions

Needs to be moved for alphabetical sorting.

> Source/WebCore/dom/LifecycleCallbackQueue.cpp:130
> +        queue->m_items.append(LifecycleQueueItem(LifecycleQueueItem::Type::Connected, element, *elementInterface));

We could probably use { LifecycleQueueItem::Type::Connected, element, *elementInterface } to avoid specifying LifecycleQueueItem explicitly.

> Source/WebCore/dom/LifecycleCallbackQueue.cpp:140
> +        queue->m_items.append(LifecycleQueueItem(LifecycleQueueItem::Type::Disconnected, element, *elementInterface));

Could probably use { } as above.

[Ryosuke Niwa]

Committed r204611: <http://trac.webkit.org/changeset/204611>

[Ryosuke Niwa]

<rdar://problem/28090372>