Last Comment Bug 741325 - Sort the scripts in the menulist by filename
: Sort the scripts in the menulist by filename
Status: RESOLVED FIXED
[chrome-debug]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: 12 Branch
: All All
: P2 normal (vote)
: Firefox 15
Assigned To: Victor Porof [:vporof][:vp]
:
: James Long (:jlongster)
Mentors:
Depends on: 741324
Blocks: 741328
  Show dependency treegraph
 
Reported: 2012-04-02 03:03 PDT by Victor Porof [:vporof][:vp]
Modified: 2012-05-04 07:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (4.41 KB, patch)
2012-04-13 00:15 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (12.77 KB, patch)
2012-04-13 04:23 PDT, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review
v2.01 (12.77 KB, patch)
2012-04-17 11:17 PDT, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-04-02 03:03:38 PDT

    
Comment 1 Victor Porof [:vporof][:vp] 2012-04-13 00:15:42 PDT
Created attachment 614697 [details] [diff] [review]
v1

WIP. Works. Needs a test.
Comment 2 Victor Porof [:vporof][:vp] 2012-04-13 04:23:51 PDT
Created attachment 614728 [details] [diff] [review]
v2

We have a test!
Comment 3 Rob Campbell [:rc] (:robcee) 2012-04-13 10:13:31 PDT
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] 2012-04-13 10:36:53 PDT
(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] 2012-04-13 10:48:42 PDT
(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] 2012-04-13 21:04:08 PDT
https://tbpl.mozilla.org/?tree=Try&rev=0d9b08c361de
Comment 7 Victor Porof [:vporof][:vp] 2012-04-17 11:17:45 PDT
Created attachment 615798 [details] [diff] [review]
v2.01

Making sure this is the latest patch.
Comment 8 Rob Campbell [:rc] (:robcee) 2012-04-23 09:04:14 PDT
https://hg.mozilla.org/integration/fx-team/rev/3bdab8c31492
Comment 9 Rob Campbell [:rc] (:robcee) 2012-04-23 15:26:51 PDT
https://hg.mozilla.org/integration/fx-team/rev/f849003d9864
Comment 10 Rob Campbell [:rc] (:robcee) 2012-04-25 08:22:27 PDT
https://hg.mozilla.org/integration/fx-team/rev/9285d5ff5ff0
Comment 11 Tim Taubert [:ttaubert] 2012-04-27 05:49:31 PDT
https://hg.mozilla.org/mozilla-central/rev/9285d5ff5ff0
Comment 12 :Ehsan Akhgari 2012-05-02 13:25:01 PDT
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) 2012-05-03 05:55:39 PDT
https://hg.mozilla.org/integration/fx-team/rev/6fa091e059cc
Comment 14 Tim Taubert [:ttaubert] 2012-05-04 07:27:31 PDT
https://hg.mozilla.org/mozilla-central/rev/6fa091e059cc

Note You need to log in before you can comment on or make changes to this bug.