[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