Bug 167080 - Web Inspector: Event Listeners section is missing 'once', 'passive' event listener flags
Summary: Web Inspector: Event Listeners section is missing 'once', 'passive' event lis...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-15 14:40 PST by Brian Burg
Modified: 2017-04-06 15:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.77 KB, patch)
2017-03-10 18:03 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2017-03-11 10:33 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (12.48 KB, patch)
2017-03-11 10:50 PST, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (12.14 KB, patch)
2017-03-13 13:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-01-15 14:40:45 PST
Element.addEventListener has some new flags besides 'bubbling'. These should be shown.
Comment 1 Radar WebKit Bug Importer 2017-01-15 14:41:04 PST
<rdar://problem/30033645>
Comment 2 Devin Rousso 2017-03-10 18:03:31 PST
Created attachment 304109 [details]
Patch
Comment 3 Joseph Pecoraro 2017-03-10 18:44:30 PST
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.
Comment 4 Devin Rousso 2017-03-10 18:50:29 PST
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"?
Comment 5 Joseph Pecoraro 2017-03-10 19:40:48 PST
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.
Comment 6 Devin Rousso 2017-03-11 10:33:00 PST
Created attachment 304165 [details]
Patch
Comment 7 Devin Rousso 2017-03-11 10:50:43 PST
Created attachment 304166 [details]
Patch
Comment 8 Joseph Pecoraro 2017-03-13 11:49:13 PDT
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.
Comment 9 Devin Rousso 2017-03-13 13:11:52 PDT
Created attachment 304289 [details]
Patch
Comment 10 WebKit Commit Bot 2017-03-13 13:12:25 PDT
Comment on attachment 304289 [details]
Patch

Rejecting attachment 304289 [details] from commit-queue.

webkit@devinrousso.com 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.
Comment 11 WebKit Commit Bot 2017-03-13 14:12:46 PDT
Comment on attachment 304289 [details]
Patch

Rejecting attachment 304289 [details] from commit-queue.

webkit@devinrousso.com 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.
Comment 12 WebKit Commit Bot 2017-03-13 14:15:51 PDT
Comment on attachment 304289 [details]
Patch

Rejecting attachment 304289 [details] from commit-queue.

webkit@devinrousso.com 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.
Comment 13 WebKit Commit Bot 2017-03-13 15:27:23 PDT
Comment on attachment 304289 [details]
Patch

Clearing flags on attachment: 304289

Committed r213873: <http://trac.webkit.org/changeset/213873>
Comment 14 WebKit Commit Bot 2017-03-13 15:27:28 PDT
All reviewed patches have been landed.  Closing bug.