The default bug view has changed. See this FAQ.

Sort the scripts in the menulist by filename

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

12 Branch
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [chrome-debug])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 741328
(Assignee)

Updated

5 years ago
Component: Developer Tools: Inspector → Developer Tools: Debugger
QA Contact: developer.tools.inspector → developer.tools.debugger
Priority: -- → P2
(Assignee)

Comment 1

5 years ago
Created attachment 614697 [details] [diff] [review]
v1

WIP. Works. Needs a test.
(Assignee)

Comment 2

5 years ago
Created attachment 614728 [details] [diff] [review]
v2

We have a test!
Attachment #614728 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Attachment #614697 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 741324
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.
Attachment #614728 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 4

5 years ago
(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!
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0d9b08c361de
(Assignee)

Comment 7

5 years ago
Created attachment 615798 [details] [diff] [review]
v2.01

Making sure this is the latest patch.
(Assignee)

Updated

5 years ago
Attachment #615798 - Attachment is patch: true
(Assignee)

Updated

5 years ago
Whiteboard: [chrome-debug] → [chrome-debug][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/3bdab8c31492
Whiteboard: [chrome-debug][land-in-fx-team] → [chrome-debug][fixed-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/f849003d9864
Whiteboard: [chrome-debug][fixed-in-fx-team] → [chrome-debug][backed-out]
https://hg.mozilla.org/integration/fx-team/rev/9285d5ff5ff0
Whiteboard: [chrome-debug][backed-out] → [chrome-debug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9285d5ff5ff0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [chrome-debug][fixed-in-fx-team] → [chrome-debug]
Target Milestone: --- → Firefox 15
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/fx-team/rev/6fa091e059cc
Whiteboard: [chrome-debug] → [chrome-debug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6fa091e059cc
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [chrome-debug][fixed-in-fx-team] → [chrome-debug]
You need to log in before you can comment on or make changes to this bug.