[webkit-reviews] review denied: [Bug 181580] Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener : [Attachment 334793] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 181580: Web Inspector: ASSERT_NOT_REACHED in
PageDebuggerAgent::didAddEventListener when page adds attribute event listener
https://bugs.webkit.org/show_bug.cgi?id=181580

Attachment 334793: Patch

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




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

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

I'd like to see a new version of the patch.

> Source/WebCore/dom/EventTarget.cpp:84
> +	   InspectorInstrumentation::didAddEventListener(*this, eventType,
*listenerPointer, options.capture);

Shouldn't we be comparing all of the options? Add operator== to the struct if
you need to.

> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:181
> +    auto* registeredListener = eventListeners[listenerIndex].get();

This is more idiomatic to write as:

auto position = eventListeners.findMatching[&](...);
if (position == notFound)
    return;
auto registeredListener = eventListeners.at(position);

> LayoutTests/ChangeLog:10
> +	   listener tests into their own test suite, and cleanup the test
utilities

Nit: clean up

> LayoutTests/ChangeLog:11
> +	   for logging asynchronous stack traces. In the future, the catch-all
test

What does 'catch-all test file' refer to?

> LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:18
> +	   document.body.onclick = previousListener;

I don't think we should rely on hoisting like this. Just move previousListener
assignment up higher, or use `let previousListener` to stub it out.

> LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:56
> +    InspectorTest.AsyncStackTrace.addTestCase({

Because you haven't said anything, I have no reason to think that this helper
will be used outside of this test suite. And so, it doesn't need to be in its
own file if that is the case.

> LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:59
> +	   expression: "testAttributeEventListener()"

I can't see any reason for these test cases to be out-of-line. If they are
out-of-line, then I think the convention in most of our tests is to name them
triggerXXX().

> LayoutTests/inspector/debugger/resources/async-stack-trace.js:1
> +TestPage.registerInitializer(() => {

Is this new code, or moved from somewhere else? I don't see a corresponding
deletion. If it is new, you should say something more concrete than "clean up
utilities"; this adds a new test case factory method.

> LayoutTests/inspector/debugger/resources/async-stack-trace.js:4
> +    function getActiveStackTrace() {

Why is this at global scope in the inspector page?

> LayoutTests/inspector/debugger/resources/async-stack-trace.js:19
> +    function logActiveStackTrace() {

Ditto.

> LayoutTests/inspector/debugger/resources/async-stack-trace.js:71
> +	   initializeTestSuite(suite) {

What? This is not okay. We could have more than one suite in the same file, and
this would break.

It's also awkward. We don't need to save the suite in an ivar, it should just
be a parameter to addTestCase.

> LayoutTests/inspector/debugger/resources/async-stack-trace.js:74
> +	   addTestCase({name, description, expression}) {

I would rename this to addTestCaseForAsyncStackTrace or something like that.
Otherwise, they will look the same in a code search. Also, it should be a
static method per the above comment.

> LayoutTests/inspector/debugger/resources/async-stack-trace.js:79
> +		   test(resolve, reject) {

I'd prefer an async function in new code if we don't need to use non-once event
listeners. i.e.,

async test() {
    let done = WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused)
       .then(() => { // do logging })
       .then(() =>
WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed));

    InspectorTest.evaluateInPage(expression);
    await done;
}