[webkit-reviews] review denied: [Bug 174739] Web Inspector: capture an async stack trace when web content calls addEventListener : [Attachment 316244] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 24 10:08:42 PDT 2017


Brian Burg <bburg at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 174739: Web Inspector: capture an async stack trace when web content calls
addEventListener
https://bugs.webkit.org/show_bug.cgi?id=174739

Attachment 316244: Patch

https://bugs.webkit.org/attachment.cgi?id=316244&action=review




--- Comment #6 from Brian Burg <bburg at apple.com> ---
Comment on attachment 316244
  --> https://bugs.webkit.org/attachment.cgi?id=316244
Patch

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

r- because it needs rebasing.

> Source/WebCore/dom/EventTarget.cpp:-72
> -    return ensureEventTargetData().eventListenerMap.add(eventType,
WTFMove(listener), { options.capture, options.passive, options.once });

This code seems kinda touchy for performance. Maybe you should check with
Ryosuke to learn how to check whether this is a serious perf regression (adding
hooks here on the hot path).

> Source/WebCore/dom/EventTarget.cpp:72
> +    bool listenerIsScriptDebuggable = listener->type() ==
EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();

Move this next to the use site. Also this name is wonky. What about
listenerExecutesJavaScript or listenerCanBeDebugged?

The !listener->wasCreatedFromMarkup() exclusion deserves some explanation here
or in the ChangeLog. Why can't we debug that?

> Source/WebCore/dom/EventTarget.cpp:76
> +    if (listenerIsScriptDebuggable) {

To minimize perf impact you should guard the extra work here (iterating the
listeners) on InspectorInstrumentation::hasFrontends().

> Source/WebCore/dom/EventTarget.cpp:118
> +    auto& listenersForType = eventListeners(eventType);

To minimize perf impact you should guard the extra work here (iterating the
listeners) on InspectorInstrumentation::hasFrontends().

> Source/WebCore/dom/EventTarget.cpp:120
> +	   if (registeredListener->callback() == listener &&
registeredListener->useCapture() == options.capture) {

What is the purpose of this check for usesCapture?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:354
> +void
InspectorInstrumentation::didRegisterEventListenerImpl(InstrumentingAgents&
instrumentingAgents, const RegisteredEventListener& listener,
ScriptExecutionContext& context)

Applies throughout: please rename to add (so we have add/remove method pairs
inside inspector).

> Source/WebCore/inspector/PageDebuggerAgent.cpp:176
> +    int listenerIdentifier = m_nextEventListenerIdentifier++;

Please use an unsigned type for the identifier key. Adding a typedef would be
nice too, so the HashMap type parameters are a little less vague.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:179
> +    didScheduleAsyncCall(scriptState,
static_cast<int>(AsynchronousCallType::EventListener), listenerIdentifier,
listener.isOnce());

what!? don't cast away the enum type here. Do it inside the methods if they
need to use the value for hashing or whatever. Please fix the existing call
sites, this is not good.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:184
> +    auto eventListenerIterator = m_registeredEventListeners.find(&listener);

Nit: you can just do `auto it = ...`. There's only one iterator, naming it is
not helpful.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:195
> +    auto eventListenerIterator = m_registeredEventListeners.find(&listener);

Ditto.


More information about the webkit-reviews mailing list