[webkit-reviews] review granted: [Bug 177451] Web Inspector: provide a way to enable/disable event listeners : [Attachment 323777] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 13 18:03:58 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 177451: Web Inspector: provide a way to enable/disable event listeners
https://bugs.webkit.org/show_bug.cgi?id=177451

Attachment 323777: Patch

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




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

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

r=me

> Source/JavaScriptCore/ChangeLog:9
> +	   Add `setEventListenerDisabled` command that sets the `disabled` flag
on the given event

Instead of "that sets the `disabled` flag" is obscure. There is no public
disabled flag. This should be a higher level description like "that disables or
enables a specific event listener during event dispatch".

> Source/WebCore/inspector/InspectorDOMAgent.h:281
> +	   EventTarget* eventTarget { nullptr };

Having just attended the WebKit Contributors meeting we may want to make this a
RefPtr and avoid having a raw pointer.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:760
> +	   if (event.target == this.domNode ||
event.target.isAncestor(this.domNode))

Style: ===

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:55
> +	   rows.push(this._createDisabledToggleRow());

This should feature check support.

    if (DOMAgent.setEventListenerDisabled)
	rows.push(this._createDisabledToggleRow());

> LayoutTests/inspector/dom/setEventListenerDisabled.html:39
> +		       setTimeout(() => {
> +			   InspectorTest.pass("Click event listener did not
fire.");
> +
> +			   InspectorTest.removeEventListener(listener);
> +			   resolve();
> +		       });

This could be flakey if the page actually did trigger the event. The
`setTimeout` is not safe, you should wait for some event from the page itself.

I suggest:

    function clickBody() {
	document.body.click();
	TestPage.dispatchEventToFrontend("AfterClick");
    }

And here:

    InspectorTest.singleFireEventListener("AfterClick", () => {
	...
    });

This guarantees no race, the setTimeout here doesn't.

> LayoutTests/inspector/dom/setEventListenerDisabled.html:46
> +    suite.addTestCase({
> +	   name: "DOM.setEventListenerDisabled.ReenabledClickEvent",

You're missing a test for:

    DOM.getEventListenersForNode

When the node has a disabled event listener. Should be easy.

> LayoutTests/inspector/dom/setEventListenerDisabled.html:110
> +	       TestPage.dispatchEventToFrontend("Clicked");

Nit: A name like "TestPageDocumentClicked" might be easier to search.


More information about the webkit-reviews mailing list