741325 - Sort the scripts in the menulist by filename

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Comment 1

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

Attachment: v1

WIP. Works. Needs a test.

Comment 2

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

Attachment: v2

We have a test!

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 614728 [details] [diff] [review]
v2

in debugger-view.js:

   contains: function DVS_contains(aUrl) {
+    if (this._tmpScripts.some(function(element) {
+      return element.script.url == aUrl;
+    })) {
+      return true;
+    }

I think this would look better with the .some test on a separate line and a variable used for the truth condition. but that's just a nit.

Same for containsLabel().

+   * Returns the list of labels in the scripts container.
+   * @return array
+   */
+  get scriptLabels() {
+    let labels = [];
+    for (let i = 0, l = this._scripts.itemCount; i < l; i++) {
+      labels.push(this._scripts.getItemAtIndex(i).label);
+    }

what about:

return this._scripts.map(function(script) {
  return script.label;
});

instead?

+    for (let i = 0, l = this._scripts.itemCount; i < l; i++) {
+      if (this._scripts.getItemAtIndex(i).label > aLabel) {
+        this._createScriptElement(aLabel, aScript, i);
+        return;
+      }
+    }

mm. insertion sort. IN THE YEAR 2012!

I feel like there should be a cleverer way of doing this but y'know, I'm not sure it's worth it.

oh what the hell.

You could use a forEach loop and callback with this. Essentially the same thing but cuts down a bit on the extra for loop fiddly bits.

this._scripts.forEach(function(script, index) {
  if (script.label > aLabel) {
    this._createScriptElement(aLabel, aScript, index);
    return;
  }
}, this);

but that won't return early.

Let's just use yours!

(I so wanted to be clever there)

+  /**
+   * Adds all the prepared scripts to the scripts container.
+   * If a script already exists (was previously added), nothing happens.
+   */
+  commitScripts: function DVS_commitScripts() {
+    let newScripts = this._tmpScripts;
+    this._tmpScripts = [];
+
+    if (!newScripts || !newScripts.length) {
+      return;
+    }
+    newScripts.sort(function(a, b) {
+      return a.label.toLowerCase() > b.label.toLowerCase();
+    });
+
+    for (let i = 0, l = newScripts.length; i < l; i++) {
+      let item = newScripts[i];
+      this._createScriptElement(item.label, item.script, -1, true);
+    }
+  },
+

what happens if there are already scripts in the menu? Is that possible? If so, it looks like this won't take them into account and they'll be out of order.

however, it looks like the only time a new script can be added to a set of existing scripts is if _onNewScript gets fired. 

in /test/browser_dbg_scripts-sorting.js

+  urls.sort(function(a, b) {
+    return Math.random() - 0.5;
+  });

why are you randomizing these each time?

head hurt. r+ with try.

Comment 4

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Comment on attachment 614728 [details] [diff] [review]
> v2
> 
> in debugger-view.js:
> 
>    contains: function DVS_contains(aUrl) {
> +    if (this._tmpScripts.some(function(element) {
> +      return element.script.url == aUrl;
> +    })) {
> +      return true;
> +    }
> 
> I think this would look better with the .some test on a separate line and a
> variable used for the truth condition. but that's just a nit.
> 

Ugh.. 

> Same for containsLabel().
> 
> +   * Returns the list of labels in the scripts container.
> +   * @return array
> +   */
> +  get scriptLabels() {
> +    let labels = [];
> +    for (let i = 0, l = this._scripts.itemCount; i < l; i++) {
> +      labels.push(this._scripts.getItemAtIndex(i).label);
> +    }
> 
> what about:
> 
> return this._scripts.map(function(script) {
>   return script.label;
> });
> 
> instead?
> 

I was trying to be polite and match the scriptLocations function which looks and feels the same way. But OK! (yay)

> +    for (let i = 0, l = this._scripts.itemCount; i < l; i++) {
> +      if (this._scripts.getItemAtIndex(i).label > aLabel) {
> +        this._createScriptElement(aLabel, aScript, i);
> +        return;
> +      }
> +    }
> 
> mm. insertion sort. IN THE YEAR 2012!

Insertion sort IN SOME EDGE CASES! ;)
Most of the time addScript will be called with aForceFlag as false, so we lazily sort and add everything. 
The other case with aForceFlag to true is for the unsolicited newScript notification. We gotta' do insertion sort here if we don't want to clear and re-add everything.

> 
> I feel like there should be a cleverer way of doing this but y'know, I'm not
> sure it's worth it.
> 

I wish there was.

> oh what the hell.
> 
> You could use a forEach loop and callback with this. Essentially the same
> thing but cuts down a bit on the extra for loop fiddly bits.
> 

Gotta' love those functional sugar coatings, but I'm afraid of the cost. Probably won't cost much though. I'll give it a shot and see what happens with 100+ scripts.

> this._scripts.forEach(function(script, index) {
>   if (script.label > aLabel) {
>     this._createScriptElement(aLabel, aScript, index);
>     return;
>   }
> }, this);
> 
> but that won't return early.
> 
> Let's just use yours!
> 
> (I so wanted to be clever there)
> 

I feel you.

> +  /**
> +   * Adds all the prepared scripts to the scripts container.
> +   * If a script already exists (was previously added), nothing happens.
> +   */
> +  commitScripts: function DVS_commitScripts() {
> +    let newScripts = this._tmpScripts;
> +    this._tmpScripts = [];
> +
> +    if (!newScripts || !newScripts.length) {
> +      return;
> +    }
> +    newScripts.sort(function(a, b) {
> +      return a.label.toLowerCase() > b.label.toLowerCase();
> +    });
> +
> +    for (let i = 0, l = newScripts.length; i < l; i++) {
> +      let item = newScripts[i];
> +      this._createScriptElement(item.label, item.script, -1, true);
> +    }
> +  },
> +
> 
> what happens if there are already scripts in the menu? Is that possible? If
> so, it looks like this won't take them into account and they'll be out of
> order.
> 

Won't happen.

> however, it looks like the only time a new script can be added to a set of
> existing scripts is if _onNewScript gets fired. 
> 

Exactly.

> in /test/browser_dbg_scripts-sorting.js
> 
> +  urls.sort(function(a, b) {
> +    return Math.random() - 0.5;
> +  });
> 
> why are you randomizing these each time?
> 

Because WE CAN!
But seriously, I don't have a good reason. To make the test misbehave in ways I can't predict and still get green?

> head hurt. r+ with try.

Thank you!

Comment 5

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

(In reply to Victor Porof from comment #4)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> 
> > Same for containsLabel().
> > 
> > +   * Returns the list of labels in the scripts container.
> > +   * @return array
> > +   */
> > +  get scriptLabels() {
> > +    let labels = [];
> > +    for (let i = 0, l = this._scripts.itemCount; i < l; i++) {
> > +      labels.push(this._scripts.getItemAtIndex(i).label);
> > +    }
> > 
> > what about:
> > 
> > return this._scripts.map(function(script) {
> >   return script.label;
> > });
> > 
> > instead?
> > 
> 
> I was trying to be polite and match the scriptLocations function which looks
> and feels the same way. But OK! (yay)
> 

Um, wait no, that's not an array. It's a menulist.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=0d9b08c361de

Comment 7

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

Attachment: v2.01

Making sure this is the latest patch.

Comment 8

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/3bdab8c31492

Comment 9

[Rob Campbell [:rc] (:robcee)]

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

Comment 10

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/9285d5ff5ff0

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9285d5ff5ff0

Comment 12

[(no longer active)]

This patch was in a range which caused a Ts regression, so I backed out the whole range:

https://hg.mozilla.org/mozilla-central/rev/24a6a53c714a

Please reland after investigating and fixing the regression.

Comment 13

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/6fa091e059cc

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6fa091e059cc