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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Hacky patch

Here's my hacky local patch to accomplish this.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8568591 [details] [diff] [review]
Hacky patch

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

I think Mike should review this.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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);
```

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Splinter!

If you make all of the changes I mention in comment 3 I will r+ the patch.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136155-sort-event-listeners.patch

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I used http://shapeshed.com/examples/HTML5-video-element/ as a test page.

Comment 9

[Patrick Brosset <:pbro>]

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.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136155-sort-event-listeners.patch

Must have missed that file.

I would expect a green try now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a78b7b088f24

Comment 11

[Patrick Brosset <:pbro>]

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.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/7625c513d47f

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7625c513d47f