1387482 - Cleanup access to DevTools pref "devtools.inspector.enabled" by the context menu

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

As devtools want to move to an addon, some of the devtools specific preferences might not be available anymore. 

The preference devtools.inspector.enabled is currently used in 2 spots outside of devtools:
- http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/browser/base/content/test/general/contextmenu_common.js#318
- http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/browser/base/content/nsContextMenu.js#315-317

The first one:

> if (Services.prefs.getBoolPref("devtools.inspector.enabled")) {

will throw if DevTools have never been started for the used profile. I say started here, because the default preferences are dynamically loaded by the first initialization of DevTools. Installing devtools is not enough.

The second one:

> DevToolsShim.isInstalled() && gPrefService.getBoolPref("devtools.inspector.enabled", false);

Will never throw but will assume false when DevTools are not installed or if they never have been started.

In any case, we should stop checking DevToolsShim.isInstalled() to decide if the menu item should be displayed. This is one of the entry points for DevTools that should remain available even if DevTools are not installed. We should only hide the menu entry if the user is a DevTools user and explicitly disabled the inspector.

For accessing the preference, we have several options here:

1 - Move the devtools.inspector.enabled preference to the devtools/shim preferences file. This file will remain loaded by Firefox, will be always available etc... The code can stay as it is today, but it's a bit weird to have this devtools.[tool].enabled preference in this file while all the other tools have their preference in another one.

2 - Default to true if the pref is not defined: getBoolPref("devtools.inspector.enabled", true) (the preference is true by default for a new profile)

3 - Stop checking this pref and always display the item. DevTools would be responsible for not crashing if inspectNode is called and the inspector is not enabled. I really wonder who benefits from checking this. The inspector is on by default, it can only be disabled through about:config

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1387482 - default to true when reading pref devtools.inspector.enabled from Firefox;

This preference will be moved inside the devtools addon and won't be always available.
By default consuemrs should consider this to be true, as the inspector is always enabled
by default.

If devtools are not installed, the DevToolsShim will be responsible for redirecting the
call to inspect node to the devtools installation flow.

Review commit: https://reviewboard.mozilla.org/r/164952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164952/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8893866 [details]
Bug 1387482 - default to true when reading pref devtools.inspector.enabled from Firefox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164952/diff/1-2/

Comment 3

[Julian Descottes [:jdescottes]]

I decided to go with option 2 for now. We could also open a dedicated API on the DevToolsShim (isInspectorEnabled?) to handle that. Let me know what you think.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8893866 [details]
Bug 1387482 - default to true when reading pref devtools.inspector.enabled from Firefox;

https://reviewboard.mozilla.org/r/164952/#review171252

Getting rid of a pref would be nice, and I wonder if just checking that DevToolsShim exists would be enough.  It might be nice to eventualy just have the shim inject whatever menus it wants.  In any case, this also also a fine way to fix this.

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)
> Comment on attachment 8893866 [details]
> Bug 1387482 - default to true when reading pref devtools.inspector.enabled
> from Firefox;
> 
> https://reviewboard.mozilla.org/r/164952/#review171252
> 
> Getting rid of a pref would be nice, and I wonder if just checking that
> DevToolsShim exists would be enough.  It might be nice to eventualy just
> have the shim inject whatever menus it wants.  In any case, this also also a
> fine way to fix this.

I agree, in the long run the best will be to have the shim injecting the menu entry and deciding what should be done depending on the DevTools installation status. I will go ahead with the current patch and log a follow up. The reason is that the planning for DevTools becoming an addon is unclear at the moment, so I prefer to go for the least impacting changes for now.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ea77cc3f2e1
default to true when reading pref devtools.inspector.enabled from Firefox;r=mixedpuppy

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ea77cc3f2e1