[webkit-reviews] review granted: [Bug 193226] Web Inspector: Audit: provide a way to determine whether a give node has event listeners : [Attachment 358849] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 193226: Web Inspector: Audit: provide a way to determine whether a give
node has event listeners
https://bugs.webkit.org/show_bug.cgi?id=193226

Attachment 358849: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 358849
  --> https://bugs.webkit.org/attachment.cgi?id=358849
Patch

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

r=me

> LayoutTests/inspector/audit/run-dom.html:13
> +    function evaluateStringForTest(func, target, args) {
> +	   return `DOM.${func}(document.querySelector("#${target}")${args ? ",
" + JSON.stringify(args) : ""})`;
> +    }

This kind of code generation makes the test compact but hurts readability and
comprehension. If this test ever fails someone has to wade through this to
figure out whats going on, when instead there could have been a straightforward
test with a little duplication but much clearer execution. You don't have to
change the test, but this is something to consider in the future.

> LayoutTests/inspector/audit/run-dom.html:53
> +	       let functions = new Map;
> +	       for (let test of tests)
> +		   functions.set(test.func, test);

So this only performs the last `hasEventListeners` test.

> Source/WebCore/inspector/InspectorAuditDOMUtilities.cpp:38
> +#define ERROR_IF_NO_ACTIVE_AUDIT() if (!m_auditAgent.hasActiveAudit())
return Exception { NotAllowedError, "Unable to run outside of an Inspector
Audit"_s };

Style: Make this multiple lines to improve readability.