Closed Bug 938172 Opened 6 years ago Closed 6 years ago

Do not allow disabling of "core" developer tools from options panel

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 28

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file, 1 obsolete file)

As I discovered in Bug 862558, there are lots of tricky edge cases when users disable important panels.  I believe that we should define a core set of tools that are not disable-able.  This will have the added bonus of removing some options from our option panel UI.

Here is the proposed set of core tools:

* Console
* Inspector
* Debugger
* Style Editor
* Network Panel

Other ones that could still be disabled if you'd like:

* Shader Editor (off by default)
* Profiler (on by default)
* Scratchpad (on by default)
Style Editor is not necessary I think.

Anyway, I suggest that we add a property in the tool definition.
I would say that the sole purpose of allowing the removal of all the tools was space in the tabs toolbar. Now if we force 5 tools to be there always, then a lot of space will be occupied by those + gcli buttons on the right. This is specially bothersome when the tools are docked to the side.

Also, the number of core tools will keep on increasing with JSTerm in near future, Storage Inspector also in line and at least one performance related tool.

That will make total of 8 tools always there, adding the three which are written below (shader editor, profiler and scratchpad) makes total of 11 tools. Even on my 1600 px screen width, 10 tools is the max I can have.

I do not see a valid reason to force 5-8 tools to be there permanently.

I assume that the main and foremost requirement of this bug is to support split view console. So may be we can only force Console to be present always because every other tool is pretty much independent.

What say ?
Is there any other use case for forcing "core" tools ?
(In reply to Paul Rouget [:paul] from comment #1)

> Anyway, I suggest that we add a property in the tool definition.

Sounds good, this way the list could be customized easily.  We have an existing property called visibilityswitch that is a string containing the pref to check.  We could maybe define that as 'false' for core tools and update the code that references it.
(In reply to Girish Sharma [:Optimizer] from comment #2)
> I would say that the sole purpose of allowing the removal of all the tools
> was space in the tabs toolbar. Now if we force 5 tools to be there always,
> then a lot of space will be occupied by those + gcli buttons on the right.
> This is specially bothersome when the tools are docked to the side.
> 
> Also, the number of core tools will keep on increasing with JSTerm in near
> future, Storage Inspector also in line and at least one performance related
> tool.
> 
> That will make total of 8 tools always there, adding the three which are
> written below (shader editor, profiler and scratchpad) makes total of 11
> tools. Even on my 1600 px screen width, 10 tools is the max I can have.
> 
> I do not see a valid reason to force 5-8 tools to be there permanently.
> 
> I assume that the main and foremost requirement of this bug is to support
> split view console. So may be we can only force Console to be present always
> because every other tool is pretty much independent.
> 
> What say ?
> Is there any other use case for forcing "core" tools ?

The list could change - I'd say the bare minimum would be:

* Console
* Inspector
* Debugger

I believe that there may be portions of the debugger that are necessary for other tools, and there will be for the inspector soon (like in Bug 757866).

Another part of the reasoning behind enforcing non-hidable tools is to limit the number of options that we present to users.  Having so many options that are unlikely to be chosen can be overwhelming or make more important options less visible.
Attached patch core-tools.patch (obsolete) — Splinter Review
Here is a patch that prevents disabling of tools that don't have a visibilityswitch set.  It ends up being this list:

* Console
* Inspector
* Debugger
* Style Editor

The only controversial one in the list is Style Editor - and it really needs to be in this list since the links on the right of the rule view from the Inspector tab navigate you directly to the Style Editor.

Ran the framework/ tests locally, and pushed to try for the rest: https://tbpl.mozilla.org/?tree=Try&rev=0e9cebaacbfb
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attached patch core-tools.patchSplinter Review
This patch is like the first one, but it also forces all additional tools to be toggleable.

This was not as simple as it seemed.  There was a bug that came up when the browser was opened with the -jsconsole flag.  In order to make this work as expected, we needed to modify devtools-clhandler.js to not call devtools.main("main") and instead to just import gDevTools.

I left a comment in registerTool explaining why this was needed, but it seemed to be some sort of issue with the two files both importing each other - and having it imported a second time by the command line handler caused some bugs with the defaultTools array not being the object you would expect at the time.

The jsconsole seems to work fine with these changes, and browser/framework/ tests pass again, but I went ahead and pushed to try with all tests enabled, since I changed the code for the command line flag: https://tbpl.mozilla.org/?tree=Try&rev=2ab835a7c440
Attachment #831704 - Attachment is obsolete: true
Attachment #831887 - Flags: review?(dcamp)
Attachment #831887 - Flags: review?(dcamp) → review+
Whiteboard: [ → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/412718e24859
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Keywords: verifyme
User agents:
[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
[2] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
[3] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0

I was able to confirm the fix for this issue on:
- the latest Beta (Build ID: 20140210161136) [1] 
- the latest Aurora (Build ID: 20140210004002) [2]
- the latest Nightly (Build ID: 20140210030201) [3]
Status: RESOLVED → VERIFIED
Keywords: verifyme
See Also: → 1046019
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.