883702 - The list of sources in the debugger should always be sorted alphabetically

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Within each side-menu-widget-group-list, we should order the side-menu-widget-items alphabetically.

In my current use-case, I'm working on three different files named CustomizeMode.jsm, CustomizableUI.jsm, and CustomizableWidget.jsm. Switching between these three files forces me to use the Ctrl+P search since the sidebar isn't sorted and unusable with tons of source files.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

I'm unable to reproduce this with the content debugger, and my perusing through the code has shown me that the items should get sorted using this codepath:

addSource at http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#104 calls ViewHelpers.push(),

which calls _getExpectedIndexForItem and passes the result to _insertItemAt at http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#634

However, whenever I use the Browser Debugger I see items within the "resource gre modules" group are unsorted. However, I can't get the Debugger to pause at breakpoints within this codepath (not sure why).

Victor, can you help me out here?

Comment 2

[Victor Porof [:vporof][:vp]]

The browser debugger has a few bugs at the moment regarding pausing and stepping. I think you've managed to hit bug 857451 (or a friend). Filter on [chrome-debug] in the whiteboard to get a list of all of these.

Regarding sorting, the codepath you found is correct. But sorting should have worked by default...

What I *think* happens is that the initially retrieved batch of sources gets sorted and inserted into the menu correctly [0], but afterwards, subsequent individual unsolicited newSource packets [1] get tangled up and _findExpectedIndexFor gets confused. If I'm reading that code correctly, this likely happens because of the SideMenuWidget's grouping. An index in the widget is relative to a group, versus an index returned by the _findExpectedIndexFor method is relative to the entire menu.

It's subtle, and that's why this isn't always an issue.
A thing of great grandeour is happening. Suppose have the following structure:

+-------------+
|-- group 1 --|
|a            |
|-- group 2 --|
|c            |
+-------------+

You want to insert b into group 2. _findExpectedIndexFor(b) returns 1, because it doesn't care or know about groups, so:

> getItemAtIndex(0) == a, a < b, "don't. stop. me. now.",
> getItemAtIndex(1) == c, c > b, index = 1, "yeah we're having a good time!"

Inserting b into group 2 at index 1 would render the following structure:

+-------------+
|-- group 1 --|
|a            |
|-- group 2 --|
|c            |
|b            |
+-------------+

Sadness!

This is tricky.

FYI, one more thing that's happening, but doesn't actually affect the algorithm above, is that because the groups themselves are also visually sorted inside the menu (so that you get "group 1" before "group 2"), indices don't correspond anymore to the actual visible position of each item in the menu. But we don't care about this now.

Now you might be thinking, whatever, let's make _findExpectedIndexFor not be ignorant of groups and whatnot. But those methods should be compatible with a specific widget interface [1], so we need to find a way of not breaking other widgets (like the BreadcrumbsWidget).

I think getItemAtIndex inside SideMenuWidget could be changed. It'd need to take another param, aGroupName or something:
> getItemAtIndex: function(aIndex, aGroup) {
>   if (aGroup) ...get item at index in that specific group
>   else ...flatten all the things
> },

Then, _findExpectedIndexFor inside ViewHelpers.jsm would change:
> ...
>   if (...(this.getItemAtIndex(i, aItem._description), aItem) > 0) {
> ...

and also getItemAtIndex inside ViewHelpers.jsm:
> return ...(this._container.getItemAtIndex(aIndex, aDescription));

[0] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1166
[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#1126
[2] https://gist.github.com/victorporof/5749386

Comment 3

[Eddy Bruel [:ejpbruel]]

Hi Jared, out of curiosity: why would you intentionally create a duplicate of this bug?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Bug 1130392 is concerned with the properties of an object, whereas this bug is concerned with the file names of the JS sources.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Any update on this, Brandon?

Comment 6

[bmax1337]

Jared, 

I have ran into a problem and am needinfo? victor. Hopefully it will be finished by tomorrow.

I have followed the steps that victor provided earlier and have come to a problem with GetItemAtIndex in SideMenuWidget. I'm not sure how to return an index at a specific group. I have tried using his._getMenuGroupForName(aGroup) and  then accessing _list.children[aIndex] or something a long those lines but _list doesn't always exist, so it will throw errors, especially for sources when loading the browser toolbox. When opening the browser toolbox it throws errors but as a result the sources are ordered correctly. 

Important pieces of code are here:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/SideMenuWidget.jsm#135
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#1571 <-- this is true problem because when getItemAtIndex in SideMenuWidget returns null or some sort the _currentSortPredicate is incorrect. (correct me if I'm wrong on any of this.)

Thanks,
Brandon.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

For SideMenuWidget's getItemAtIndex, would this work?

getItemAtIndex: function(aIndex, aGroup) {
  if (aGroup) {
    let group = this.getMenuGroupForName(aGroup)
    let container = group._list.childNodes[aIndex];
    if (container) {
      // If container exists, get the _target inside of it
      // (childNodes[0] because it's the only child? Maybe?)
      let target = container.childNodes[0]
      let item = this._itemsByElement.get(target)
      return item;
    }
  }
  
  return this._orderedMenuElementsArray[aIndex];
},

Comment 8

[Victor Porof [:vporof][:vp]]

Sorry, I've been really busy, will get to this asap.

Comment 9

[bmax1337]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> For SideMenuWidget's getItemAtIndex, would this work?
> 
> getItemAtIndex: function(aIndex, aGroup) {
>   if (aGroup) {
>     let group = this.getMenuGroupForName(aGroup)
>     let container = group._list.childNodes[aIndex];
>     if (container) {
>       // If container exists, get the _target inside of it
>       // (childNodes[0] because it's the only child? Maybe?)
>       let target = container.childNodes[0]
>       let item = this._itemsByElement.get(target)
>       return item;
>     }
>   }
>   
>   return this._orderedMenuElementsArray[aIndex];
> },

Jordan,
Thanks for responding to this. I tried code very very similar to this in the beginning and this is why I am looking for help from victor. The problem with the code above is when it returns an item, when aGroup is present, it falls back into this function:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#1402 
and from here it sorts it using this function:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#1753 
where it then throws an error -- notify event 'newSource' threw an exception: TypeError: aFirst is null.

I believe this is happening because getItemForElement is returning null? 

Any help is appreciated.
Thanks,
Brandon.

Comment 10

[Victor Porof [:vporof][:vp]]

(In reply to bmax1337 from comment #6)
> 
> I have followed the steps that victor provided earlier and have come to a
> problem with GetItemAtIndex in SideMenuWidget. I'm not sure how to return an
> index at a specific group. I have tried using
> his._getMenuGroupForName(aGroup) and  then accessing _list.children[aIndex]
> or something a long those lines but _list doesn't always exist, so it will
> throw errors, especially for sources when loading the browser toolbox. When
> opening the browser toolbox it throws errors but as a result the sources are
> ordered correctly. 

> Important pieces of code are here:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> widgets/SideMenuWidget.jsm#135
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> widgets/ViewHelpers.jsm#1571 <-- this is true problem because when
> getItemAtIndex in SideMenuWidget returns null or some sort the
> _currentSortPredicate is incorrect. (correct me if I'm wrong on any of this.)
> 
> Thanks,
> Brandon.

Sorry about being a few days late on this, we've all been very busy releasing a new performance tool :)

Regarding your questions,

The `_list` property should always exist on the SideMenuWidget, because it's created on instantiation, so I don't understand your assertion about how "_list doesn't always exist". Regarding "_currentSortPredicate being incorrect", I'm not sure what you mean by "incorrect", as it can never be null and it should only work on Items, not DOM elements (see the documentation about the vocabulary inside ViewHelpers.jsm). So if `getItemAtIndex` always returns an item or null, then there shouldn't be any problems.

The SideMenuWidget is not a WidgetMethods instance. This might be a common source of confusion. They're two separate objects composed at runtime, so it would be incorrect to access `_getMenuItemForGroup` inside WidgetMethods, for example, because it's a private method defined on the SideMenuWidget. We should access only the exposed public methods for a "widget", these are defined in the documentation for WidgetMethods as well, inside ViewHelpers.jsm. In this case, it's *only* `getItemAtIndex`.

Therefore, the correct course of action here is to:

1. First allow getItemAtIndex on WidgetMethods to pass in a group name. This will require the public "widget" interface to be extended, so getItemAtIndex inside SideMenuWidget also needs to be extended.
  1.1: make `getItemAtIndex` inside WidgetMethods accept a second param, which can be the group name
  1.2: this in turn calls `getItemAtIndex` inside a "widget", so make `getItemAtIndex` inside SideMenuWidget also accept a second param, which is the group name
  1.3: accessing a group here is easy, via the `_orderedGroupElementsArray` list

2. Make all `getItemAtIndex` callsites inside WidgetMethods handle this second param, especially `_findExpectedIndexFor` which is called when inserting items in the menu.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Hi Brandon, does comment #10 help you out enough here?

Comment 12

[bmax1337]

Sorry, I've been super busy with school and events lately. I will have an update this weekend.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Brandon, any update here?

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Closing old needinfo request...

Comment 15

[Nick Alexander :nalexander [he/him]]

Can we just make this happen?  I've been using (and loving!) the JS Debugger for months/years, but it's almost impossible to use it for debugging Fennec's chrome process due to the hundreds of randomly sorted files.  I think the team expects searching to be ubiquitous... but it rarely works for me.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Nick Alexander :nalexander from comment #15)
> Can we just make this happen?

Yes, you described the heart of the issue. Would you like to work on it? You can probably pick up where Brandon left off at. Although there is no patch attached, the discussion in this bug should describe enough detail to get most of the way.

Comment 17

[James Long (:jlongster)]

FWIW, we're pretty close to starting work on a tree view of the sources instead. See bug 987685. A lot of work will happen in Q1 (not sure if it will land or not though).

Comment 18

[Tom Tromey :tromey]

The new debugger has a tree view, so I'm closing this.