Bug 160950 - Add basic support for connected and disconnected callbacks
Summary: Add basic support for connected and disconnected callbacks
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
Keywords: InRadar
Depends on:
Blocks: 154907
  Show dependency treegraph
Reported: 2016-08-17 16:13 PDT by Ryosuke Niwa
Modified: 2016-09-01 16:16 PDT (History)
10 users (show)

See Also:

Adds the basic support (36.64 KB, patch)
2016-08-17 16:26 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed GTK+/EFL builds (35.74 KB, patch)
2016-08-17 17:02 PDT, Ryosuke Niwa
cdumez: 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-08-17 16:13:33 PDT
Add the support for connected and disconnected callbacks.
Comment 1 Ryosuke Niwa 2016-08-17 16:26:59 PDT
Created attachment 286343 [details]
Adds the basic support
Comment 2 Ryosuke Niwa 2016-08-17 17:02:32 PDT
Created attachment 286344 [details]
Fixed GTK+/EFL builds
Comment 3 Ryosuke Niwa 2016-08-17 17:17:42 PDT
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%
Comment 4 Chris Dumez 2016-08-18 13:31:17 PDT
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.
Comment 5 Ryosuke Niwa 2016-08-18 15:21:37 PDT
Committed r204611: <http://trac.webkit.org/changeset/204611>
Comment 6 Ryosuke Niwa 2016-09-01 16:16:31 PDT