Bug 167080 – Web Inspector: Event Listeners section is missing 'once', 'passive' event listener flags

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Element.addEventListener has some new flags besides 'bubbling'. These should be shown.

[Radar WebKit Bug Importer]

<rdar://problem/30033645>

[Devin Rousso]

Created attachment 304109 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 304109 [details]
Patch

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

Awesome!!

r-, this needs a test.

When the user has an element selected that shows a "Once" listener, and it fires, does the sidebar then update when the event listener get removed? Should we do this?

> Source/JavaScriptCore/inspector/protocol/DOM.json:85
> +                { "name": "isPassive", "type": "boolean", "optional": true, "description": "<code>EventListener</code>'s isPassive." },
> +                { "name": "isOnce", "type": "boolean", "optional": true, "description": "<code>EventListener</code>'s isOnce." }

We normally drop the "is" prefix.

[Devin Rousso]

Comment on attachment 304109 [details]
Patch

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

> When the user has an element selected that shows a "Once" listener, and it
> fires, does the sidebar then update when the event listener get removed?
> Should we do this?

So I just tested this, and it doesn't remove it.  Unfortunately, it also seems that if an event listener is removed via removeEventListener, the sidebar doesn't update either.  I think this would be better addressed in a followup.

>> Source/JavaScriptCore/inspector/protocol/DOM.json:85
>> +                { "name": "isOnce", "type": "boolean", "optional": true, "description": "<code>EventListener</code>'s isOnce." }
> 
> We normally drop the "is" prefix.

I was trying to match the existing "isAttribute" and the getter methods of RegisteredEventListener.  Would you prefer I change it to "passive" and "once"?

[Joseph Pecoraro]

Comment on attachment 304109 [details]
Patch

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

>>> Source/JavaScriptCore/inspector/protocol/DOM.json:85
>>> +                { "name": "isOnce", "type": "boolean", "optional": true, "description": "<code>EventListener</code>'s isOnce." }
>> 
>> We normally drop the "is" prefix.
> 
> I was trying to match the existing "isAttribute" and the getter methods of RegisteredEventListener.  Would you prefer I change it to "passive" and "once"?

Hmm. ~105 non-is-prefixed booleans, 28 isPrefixed. So yeah, lets still drop the "is" here. The old "isAttribute" name is unfortunate better to keep it around for backwards compatibility.

[Devin Rousso]

Created attachment 304165 [details]
Patch

[Devin Rousso]

Created attachment 304166 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 304166 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * inspector/dom-debugger/dom-agent-expected.txt: Added.
> +        * inspector/dom-debugger/dom-agent.html: Added.

(1) This name is too general. You are only testing getEventListenersForNode not the entire DOM Agent.
(2) This is in the 'dom' domain, not the 'dom-debugger' domain.

I'd suggest:

    LayoutTests/inspector/dom/getEventListenersForNode.html

Following the pattern of:

    LayoutTests/inspector/<domain>/<command>.html

> LayoutTests/inspector/dom/dom-agent.html:9
> +    let suite = InspectorTest.createAsyncSuite("DOMAgent");

Yeah, name this suite "DOM.getEventListenersForNode". You can name the test the same thing or I sometimes add ".Basic".

> LayoutTests/inspector/dom/dom-agent.html:23
> +                    InspectorTest.log(`Node ID: ${eventListener.nodeId}`);

You could probably have a nice string output for the element to get: "div#x", "body", and "#document".

I wonder if the nodeId itself might introduce flakiness (if not now maybe in the future).

> LayoutTests/inspector/dom/dom-agent.html:65
> +        element.addEventListener("a", (event) => {});

Giving a more complete name like "alpha" instead of "a" makes it easier to read the test results and makes it easier to search for if something fails related to this.

[Devin Rousso]

Created attachment 304289 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304289 [details]
Patch

Rejecting attachment 304289 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 304289 [details]
Patch

Rejecting attachment 304289 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 304289 [details]
Patch

Rejecting attachment 304289 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 304289 [details]
Patch

Clearing flags on attachment: 304289

Committed r213873: <http://trac.webkit.org/changeset/213873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.