915874 - [toolbox] Clean up toolbox.js

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brandon Benvie [:benvie]]

There's some gratuitous inefficiencies as well as a lot of inter-mixing of arrow functions with normal.bind(this) ones and just some plain nutty stuff going in toolbox.js that's driving me bonkers. This bug is to fix it.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: bug-915875.patch

Cleans up toolbox.js, also with a minor modification to gDevTools.jsm.

Let me know if you can't review this and I'll find someone else.

https://tbpl.mozilla.org/?tree=Try&rev=3ee9688f81ae

Comment 2

[Girish Sharma [:Optimizer]]

Comment on attachment 804129 [details] [diff] [review]
bug-915875.patch

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

Too much OCD going around :)
You might wanna pass it through try again with the perma orange cause backed out.

::: browser/devtools/framework/toolbox.js
@@ +132,5 @@
>     * likely that we're going to want to get the panel that we've just made
>     * visible
>     */
> +  getCurrentPanel: function() {
> +    return this.getPanel(this.currentToolId);

You can directly do 

this._tools.get(this.currentToolId)

right ?

@@ +319,5 @@
>      }
>  
>      let sideEnabled = Services.prefs.getBoolPref(this._prefs.SIDE_ENABLED);
>  
> +    for (let type in Toolbox.HostType) {

let position of Toolbox.HostType ?

Comment 3

[Brandon Benvie [:benvie]]

Thanks for the review!

> ::: browser/devtools/framework/toolbox.js
> @@ +132,5 @@
> >     * likely that we're going to want to get the panel that we've just made
> >     * visible
> >     */
> > +  getCurrentPanel: function() {
> > +    return this.getPanel(this.currentToolId);
> 
> You can directly do 
> 
> this._tools.get(this.currentToolId)

Yeah I had that originally but changed it. However, I think you're right, better to do it directly rather than another level of indirection.

> >      }
> >  
> >      let sideEnabled = Services.prefs.getBoolPref(this._prefs.SIDE_ENABLED);
> >  
> > +    for (let type in Toolbox.HostType) {
> 
> let position of Toolbox.HostType ?

Can't use for-of on a plain object (they don't define "iterator".


https://tbpl.mozilla.org/?tree=Try&rev=9f2a90a48dcb

Comment 4

[Brandon Benvie [:benvie]]

Trying this again, not getting failures when I test locally.

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

Comment 5

[Brandon Benvie [:benvie]]

Attachment: bug-915874.patch

Rebases. Let's do another try run just to make sure...

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

Comment 6

[Brandon Benvie [:benvie]]

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

Comment 7

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

This landed with the wrong bug number (bug 915875): https://hg.mozilla.org/mozilla-central/rev/ab4ffa5b2428