Closed Bug 1136155 Opened 6 years ago Closed 6 years ago

The event listener viewer in the inspector should alpha-sort the events

Categories

(DevTools :: Inspector, defect, P2)

x86
macOS
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: kats, Assigned: miker)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(2 files, 2 obsolete files)

Attached image Screenshot
See attached screenshot. When there are hundreds of listeners it's hard to find the one you're looking for. Having the events sorted would be most useful.
Attached patch Hacky patch (obsolete) — Splinter Review
Here's my hacky local patch to accomplish this.
Attachment #8568591 - Flags: review?(jryans)
Comment on attachment 8568591 [details] [diff] [review]
Hacky patch

Review of attachment 8568591 [details] [diff] [review]:
-----------------------------------------------------------------

I think Mike should review this.
Attachment #8568591 - Flags: review?(jryans) → review?(mratcliffe)
Comment on attachment 8568591 [details] [diff] [review]
Hacky patch

Review of attachment 8568591 [details] [diff] [review]:
-----------------------------------------------------------------

Also, all of the browser/devtools/markupview/test/browser_markupview_events*.js tests will need their TEST_DATA arrays sorting... I am happy if we just array.sort() them.

Can you make those changes and then I will r+ the patch.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +1157,5 @@
>      };
>      let doc = this._tooltip.doc;
>      let container = doc.createElement("vbox");
>      container.setAttribute("id", "devtools-tooltip-events-container");
> +    this._eventListenerInfos.sort(function(a, b) {

I like the patch but we should sort the events further up the chain.

The best place to do this is at the end of `getEventListeners()` in inspector.js just before returning the events array:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#409

Also, a small nit... type will always be a string so you can just use:
```
return a.type.localeCompare(b.type);
```
Attachment #8568591 - Flags: review?(mratcliffe) → review-
Splinter!

If you make all of the changes I mention in comment 3 I will r+ the patch.
Assignee: nobody → bugmail.mozilla
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> The best place to do this is at the end of `getEventListeners()` in
> inspector.js just before returning the events array:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> inspector.js#409

I gave this a shot but it didn't seem to have the desired effect. I also added console.log statements there and my output didn't show up in either the gecko console output or the webide console output.
Flags: needinfo?(mratcliffe)
Summary: The event listener viewer in the WebIDE inspector should alpha-sort the events → The event listener viewer in the inspector should alpha-sort the events
Whiteboard: [devedition-40][difficulty=easy]
Assignee: bugmail.mozilla → nobody
Priority: -- → P2
Flags: needinfo?(mratcliffe)
Took a few minutes to get this working but then decided I may as well fix the tests.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da811ca88bb8
Assignee: nobody → mratcliffe
Attachment #8568591 - Attachment is obsolete: true
Attachment #8593437 - Flags: review?(pbrosset)
Comment on attachment 8593437 [details] [diff] [review]
1136155-sort-event-listeners.patch

Review of attachment 8593437 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes look fine to me, and testing the patch locally, it seems to work fine. But I have test failures when running all of the browser_markupview_events* tests locally:
./mach mochitest-devtools browser/devtools/markupview/test/browser_markupview_events*
leads to 10 failures, here's one as an example:
969 INFO TEST-UNEXPECTED-FAIL | browser/devtools/markupview/test/browser_markupview_events_jquery_2.1.1.js | type matches for DIV#testdiv - Got click, expected keydown
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:923
    chrome://mochitests/content/browser/browser/devtools/markupview/test/helper_events_test_runner.js:checkEventsForNode:61
    self-hosted:next:625
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
    Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
    this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
    this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
    Editor.prototype.appendTo/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/sourceeditor/editor.js:338:7

It looks like there are still some test corrections to do.
Attachment #8593437 - Flags: review?(pbrosset)
Must have missed that file.

I would expect a green try now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a78b7b088f24
Attachment #8593437 - Attachment is obsolete: true
Attachment #8593989 - Flags: review?(pbrosset)
Comment on attachment 8593989 [details] [diff] [review]
1136155-sort-event-listeners.patch

Review of attachment 8593989 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a green try.
Attachment #8593989 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/integration/fx-team/rev/7625c513d47f
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7625c513d47f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.