862363 - Sync the killswitch upon (un)registering of a tool and respect it in Options Panel

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Girish Sharma [:Optimizer]]

In other words, if inspector is disabled (unregistered), the menu item should also go away and come back when it is registered again.

Comment 1

[Girish Sharma [:Optimizer]]

This is a result of a bigger problem - not syncing the killswitch prefs upon (un)registering the tools. For ex., if devtools.inspector.enabled is set to false upon unregistering of inspector, then the context menu entry will automatically go away.

Also, if you manually change the kill switch pref, (for ex, devtools.debugger.enabled) then Options panel does not respect that, and checking/unchecking the tool entry does nothing.

I will fix both in this bug.

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Fixes the two mentioned issues, but one issue still remains unsolved, which in my opinion is too much to worry. The case when someone manually toggles the kill switch pref to false.

Should gDevTools monitor all the killswitches and unregisters the proper tool when someone manually changes the pref ? (I feel no)

Comment 3

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Just noticed that profiler adds itself to the defaultTools list in ToolDefinitions.jsm only when devtools.profiler.enabled is true. After fixing this bug, this leads to a situation when profiler is disabled using options panel now, and after a browser restart, the profiler option is not present in the default tools list.
Since I anyways manage the kill switch now, I removed the check and added the profiler into the default tools list like all other tools.

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 738430 [details] [diff] [review]
patch v0.2

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

::: browser/devtools/framework/gDevTools.jsm
@@ +94,4 @@
>      this._tools.delete(toolId);
>  
>      if (!isQuitApplication) {
> +      this.emit("tool-unregistered", toolId, killswitch);

I'm not hugely keen on adding lots of parameters to events, can we get consumers of this event to look up the killswitch value?

Comment 5

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #4)
> Comment on attachment 738430 [details] [diff] [review]
> patch v0.2
> 
> Review of attachment 738430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +94,4 @@
> >      this._tools.delete(toolId);
> >  
> >      if (!isQuitApplication) {
> > +      this.emit("tool-unregistered", toolId, killswitch);
> 
> I'm not hugely keen on adding lots of parameters to events, can we get
> consumers of this event to look up the killswitch value?

That is only possible by deleting the tool from the |this._tools| after the "tool-unregistered" emit call. Let me try that out if it works correctly, but is it okay to do that ?

Comment 6

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> (In reply to Joe Walker [:jwalker] from comment #4)
...
> > > +      this.emit("tool-unregistered", toolId, killswitch);
> > 
> > I'm not hugely keen on adding lots of parameters to events, can we get
> > consumers of this event to look up the killswitch value?
> 
> That is only possible by deleting the tool from the |this._tools| after the
> "tool-unregistered" emit call. Let me try that out if it works correctly,
> but is it okay to do that ?

How hard would it be to pass the tool round rather than the toolId?

Comment 7

[Girish Sharma [:Optimizer]]

Hard - no. But then we will have to be backward compatible, right ? (passing id should also work) as it will be an API change otherwise. But then:

 - DevTools code will call that emit with the tool definition (which will be okay for killswitch determination) as the only place from where it is emitted is the gDevTools.unregisterTool method.
 - Options Panel can easily retreve the tool as it directly calls the emit method but without deleting the definition, so that is also okay.
 - External tools, if they directly emit the "tool-unregistered" with only the toolid, will fail in a case where their killswitch is not "devtools.<toolid>.enabled"

If the third case is not of much importance, then we can make the change easily.

Also note that bug 862398 also adds an argument in the emit call for "tool-registered" :|

Comment 8

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #7)
> Hard - no. But then we will have to be backward compatible, right ? (passing
> id should also work) as it will be an API change otherwise. But then:
> 
>  - DevTools code will call that emit with the tool definition (which will be
> okay for killswitch determination) as the only place from where it is
> emitted is the gDevTools.unregisterTool method.
>  - Options Panel can easily retreve the tool as it directly calls the emit
> method but without deleting the definition, so that is also okay.
>  - External tools, if they directly emit the "tool-unregistered" with only
> the toolid, will fail in a case where their killswitch is not
> "devtools.<toolid>.enabled"
> 
> If the third case is not of much importance, then we can make the change
> easily.
> 
> Also note that bug 862398 also adds an argument in the emit call for
> "tool-registered" :|

I suggest we take a look at the size of the diff. If it's getting big, then we think again.

The DevTools API is explicitly 'best efforts' WRT backwards compat. Where 'best' is defined as 'not actually malicious'. i.e. we can change it.

Lets: see what effect it has on the size of the patch, and do this with an open eye towards what's happening in bug 862398.

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3 (passing tool itself)

Passing the tool definition, backward compat tool id check and test fixes.

All tests pass locally. Pushed to try : https://tbpl.mozilla.org/?tree=Try&rev=12b01f34de3f

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3.1

Dammit, I misspelled a variable :(

new try at : https://tbpl.mozilla.org/?tree=Try&rev=39ee6fe98e85

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 739101 [details] [diff] [review]
patch v0.3.1

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

Thanks.

::: browser/devtools/framework/Toolbox.jsm
@@ +670,5 @@
>     * Handler for the tool-unregistered event.
>     * @param  {string} event
>     *         Name of the event ("tool-unregistered")
> +   * @param  {string|object} toolId
> +   *         Definition or id of the tool that was unregistered

Please could you make it clear that passing a toolId is a temporary measure and that passing a tool is the correct solution?

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3.2

changed the inline doc comment as requested.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/02e5481aca24

Comment 15

[Dave Camp (:dcamp)]

I'm concerned about the fact that this changes the meaning of killswitch from "whether the devtools team thinks this tool is ready for prime time" to "whether the user wants this tool to show by default".