866138 - Make options panel less hacky

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Girish Sharma [:Optimizer]]

There are things that can be improved:

1) Try to convert the options panel into a real tool, without creating menu items or global shortcuts. Also not allowing options panel to be disabled via the options panel.
2) Remove the dependency of the disabledTools pref json. Since the killswitches are synced now, we should be directly using them to disable or enable any tool.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

This patch:

1) Removes the dependency of the disabledTools json pref and uses the killswitch pref for each tool individually.
2) Converts the options panel into a tool with 0th ordinal. Makes sure that no menu item, shortcut and options panel entry is made for the options tool.

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 744090 [details] [diff] [review]
patch v0.1

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

We need to change how we use killswitches, right?

Comment 3

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 744090 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 744090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We need to change how we use killswitches, right?

bug 867397 suggests that we change the meaning of killswitches to visibility switches. As per the current logic, that would mean just a rename of a variable from killswitch to visibilityswitch.

I can do that in this patch also, if it is decided for final.

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 744090 [details] [diff] [review]
patch v0.1

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

It looks a bit like toolbox-options.js has DOS line endings (at least it does on my system), please could you check that it doesn't any more.
It's up to you if you'd like to rename the variable here (and make bug 867397 depend on this), or as a patch to bug 867397.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Changed the variable name. Changes the inline docs explaining the variable. Removes the passed killswitch in "tool-unregistered" event.

Now that I do not need the pref, we can simply go with passing the tool id in the gDevTools.unregisterTool method. But I did not make that change as it does not harm to have both the options.

Did this try  that was green : https://tbpl.mozilla.org/?tree=Try&rev=c8b093ab9dc4

Only the killswitch variable name changed after the try, and all tests pass locally.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Fixed a type in toolUnregistered method.

Comment 7

[Joe Walker [:jwalker]]

Attachment: patch v0.4

Rebased.

I also took the liberty of putting the tools in ordinal order in main.js (was there a good reason to have the debugger and inspector swapped around?

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #7)
> Created attachment 746989 [details] [diff] [review]
> patch v0.4
> 
> Rebased.
> 
> I also took the liberty of putting the tools in ordinal order in main.js
> (was there a good reason to have the debugger and inspector swapped around?

No idea :)

Comment 9

[Joe Walker [:jwalker]]

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=7ac21da57bbe

Comment 10

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=9ae9cc383d0f
https://hg.mozilla.org/integration/fx-team/rev/9ae9cc383d0f

Comment 11

[Joe Walker [:jwalker]]

https://hg.mozilla.org/mozilla-central/rev/9ae9cc383d0f

Comment 12

[Robert Kaiser]

Comment on attachment 746989 [details] [diff] [review]
patch v0.4

>-#toolbox-tab-options {
>-  list-style-image: url("chrome://browser/skin/devtools/option-icon.png");
>-  -moz-image-region: rect(0px 16px 16px 0px);
>-}

Should have remove that .png from jar.mn and the repository as well, I guess. ;-)

Comment 13

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

(In reply to Robert Kaiser (:[e-mail]) from comment #12)

> Should have remove that .png from jar.mn and the repository as well, I
> guess. ;-)

It's used in .devtools-option-toolbarbutton from devtools common.css.

Comment 14

[Robert Kaiser]

(In reply to Victor Porof [:vp] from comment #13)
> It's used in .devtools-option-toolbarbutton from devtools common.css.

Yes, sorry, just figured that out myself. Should look deeper before saying such things.