Closed Bug 1308912 Opened 3 years ago Closed 3 years ago

Implement support for tools registered locally to a single toolbox

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

Attachments

(2 files)

This issue is extracted from Bug 1300587, which is related to the implementation of the upcoming WebExtensions "devtools.panels.create" API method:

The above API method is the method used by a WebExtensions addon, usually called from a devtools_page (Bug 1291737), it adds the new panel only in the specific toolbox which the WebExtension page that calls the API method is related to.

On the contrary, the internal Firefox DevTools API currently provided is able to register a new tool only globally (for every toolbox already opened and any toolbox opened after the tool has been registered).

This issue has the goal to find and agree on the changes needed on the internal Firefox DevTools API to support tools registered locally to a specific toolbox (which should be added only to that toolbox, and it should handle the tool unregistering and enabling/disabling checkbox events generated from the toolbox options, as the globally registered tool already support).
Assignee: nobody → lgreco
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

Hi Alex,
As briefly discussed on IRC, This is a proposed patch that I'm currently working on as part of the WebExtension DevTools panels API implementation (Bug 1300587).

This patch is an updated version of the patch initially attached to Bug 1300587,
rebased on a more recent mozilla-central tip and with an initial test (I'm going to add more test scenarios, but I though that including an initial version of the tests can be helpful to this first round of feedback on the general approach).

Push to try:
-  https://treeherder.mozilla.org/#/jobs?repo=try&revision=85d660383a0ae6bea96779e14808bc5500f1ed76
Attachment #8799390 - Flags: feedback?(poirot.alex)
Blocks: 1300587
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review83770

::: devtools/client/framework/devtools.js:155
(Diff revision 2)
> +      } else {
> +        // If the tool is not registered globally, notify opened toolboxes
> +        // of the unregistered toolId, so that it can be handled locally
> +        // by the toolbox where the tool has been registered.
> +        this.emit("tool-unregistered", toolId);
> +      }

I think you can cleanup tool-unregistered to accept only a tool id.
https://hg.mozilla.org/mozilla-central/rev/02e5481aca24a6ef5aa54a7e92c139e8d50ab400
Seems to have been removed now (there is no killswitch anymore).

This and all code listening for tool-unregistered is going to be simplier if we always send tool id!

If you could do that in a distinct changeset that would be handy.

::: devtools/client/framework/toolbox-options.js:194
(Diff revision 2)
> +    const toolbox = this.toolbox;
> +
> +    // Signal tool registering/unregistering globally (for the tools registered
> +    // globally) and locally (for the tools registered to a single toolbox).
>      let onCheckboxClick = function (id) {
> -      let toolDefinition = gDevTools._tools.get(id);
> +      let toolDefinition = gDevTools._tools.get(id) || toolbox.toolboxToolDefinitions.get(id);

What about using toolbox.getToolDefinition?

::: devtools/client/framework/toolbox-options.js:254
(Diff revision 2)
>        additionalToolsBox.appendChild(createToolCheckbox(tool));
>      }
>  
> +    // Populating the additional toolbox-specific tools list that came
> +    // from WebExtension add-ons.
> +    for (let tool of this.toolbox.toolboxToolDefinitions.values()) {

nit: I would consider toolboxToolDefinitions more like an internal to the toolbox. May be it would be better to call a getAdditionalTools() like gDevTools?

::: devtools/client/framework/toolbox.js:1217
(Diff revision 2)
>  
>      this._addKeysToWindow();
>    },
>  
>    /**
> +   * Retrieve the map of the tools registered locally to this toolbox.

Here and everywhere you are using "locally". I'm not convinced that's the best word to designate "per toolbox tool".
What about using "Retrieve the map of the tools registered just for this toolbox." or something?
Having said that I don't have a good replacement single word for all the other occurences.

::: devtools/client/framework/toolbox.js:1983
(Diff revision 2)
>     */
>    _toolRegistered: function (event, toolId) {
> -    let tool = gDevTools.getToolDefinition(toolId);
> +    let tool = this.getToolDefinition(toolId);
> +    if (!tool) {
> +      // Ignore if the tool is not found in the global or local maps of registered tools.
> +      return;

I'm wondering, do you see that happening? (calling toolRegistered for an already unregistered tool)

::: devtools/client/framework/toolbox.js:2044
(Diff revision 2)
>      }
> +
> +    // Unregister the locally registered tool, if it was registered locally.
> +    if (this.toolboxToolDefinitions.has(toolId)) {
> +      this.toolboxToolDefinitions.delete(toolId);
> +    }

you are dispatching a tool-unregistered event from toolbox-options, which is going to remove it from the map and then disappear from the toolbox options.
So I'm not sure you want to remove it from the map right here.
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review83776

::: devtools/client/framework/toolbox.js:1251
(Diff revision 2)
> +                      definition.id);
> +    }
> +
> +    this.toolboxToolDefinitions.set(definition.id, definition);
> +    this._buildTabForTool(definition);
> +  },

Regarding the toolbox-unregistered event and the map cleanup, you may have to add a removeToolboxPanel method.
Would that work for you webext feature?
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

Yes, I think we can move forward with this. There might be some cleanup to help keeping a simple code.
Attachment #8799390 - Flags: feedback?(poirot.alex) → feedback+
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review83770

> I think you can cleanup tool-unregistered to accept only a tool id.
> https://hg.mozilla.org/mozilla-central/rev/02e5481aca24a6ef5aa54a7e92c139e8d50ab400
> Seems to have been removed now (there is no killswitch anymore).
> 
> This and all code listening for tool-unregistered is going to be simplier if we always send tool id!
> 
> If you could do that in a distinct changeset that would be handy.

Sounds great to me.

Added as a separate patch in this mozreview request.

> What about using toolbox.getToolDefinition?

I opted for `toolbox.getToolboxToolDefinition(id)`, how it sounds to you?

> nit: I would consider toolboxToolDefinitions more like an internal to the toolbox. May be it would be better to call a getAdditionalTools() like gDevTools?

Sure, it makes sense, I added it as `toolbox.getAdditionalToolboxTools()`

> Here and everywhere you are using "locally". I'm not convinced that's the best word to designate "per toolbox tool".
> What about using "Retrieve the map of the tools registered just for this toolbox." or something?
> Having said that I don't have a good replacement single word for all the other occurences.

Yeah, "locally" doesn't sound great to me too.

I changed "local"/"locally" into "per-toolbox".

> I'm wondering, do you see that happening? (calling toolRegistered for an already unregistered tool)

Yes, e.g. it can happen in the following scenario:

- you have two toolboxes opened
- you register a per-toolbox tool only on one of the two toolboxes
- switch to the toolbox options view (on the toolbox with the per-toolbox tool registered)
- toggle the per-toolbox tool (disable and re-enable it by clicking two times on the related checkbox)
- all the toolboxes will receive a tool-unregistered and a tool-registered, but only one of the toolboxes has the per-toolbox tool definition in its per-toolbox tools map
  
I added an inline comment to mention the above scenario.

> you are dispatching a tool-unregistered event from toolbox-options, which is going to remove it from the map and then disappear from the toolbox options.
> So I'm not sure you want to remove it from the map right here.

You are right, I removed it from here and added a new `toolbox.removeToolboxTool(toolId)` method to be able remove it explicitly.
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review83776

> Regarding the toolbox-unregistered event and the map cleanup, you may have to add a removeToolboxPanel method.
> Would that work for you webext feature?

I added it as `toolbox.removeToolboxTool(toolId)` and I tried the other devtools API patches (in particular the one that defines `devtools.panels.create(...)`, which is the API method that is going to use this feature) on top of this version and the new version worked successfully (with the required changes applied).

(basically the checkbox from the toolbox options removes the tool but do not unregister it from the toolbox, but when the devtools_page is destroyed, or the entire addon disabled/uninstalled, the per-toolbox tool is explicitly removed from the toolbox)
Comment on attachment 8801706 [details]
Bug 1308912 - Simplify tool-unregistered by only accepting a string toolId.

https://reviewboard.mozilla.org/r/86386/#review85066

::: devtools/client/framework/devtools.js:141
(Diff revision 1)
> +      // TODO(rpl): log a deprecation message here?
>        toolId = tool.id;

I'm a bit unsure if it would be more reasonable to start by logging a deprecation error message here or just raising an exception (I would like to take a look at how much is this directly used from addons).

What is the preferred way to produce a deprecated warning in this part of the devtools? (e.g. just using  a `console.warn` or is there a devtools-specific helper?)
Attachment #8801706 - Flags: review?(poirot.alex)
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

Besides the comments already added in Comment 9 and Comment 10,
the updated version of this patch:

- changes the `_toolUnregistered(...)` private helper method, by extracting the new `_removeToolById(...)` private helper method (which is reused in the new `removeToolboxTool(...)` method)

- emits the following new toolbox events:
  - 'toolbox-tool-added' (sent after a per-toolbox tool definition has been added to a toolbox and the tool instance has been created)
  - 'toolbox-tool-removed' (sent after a per-toolbox tool instance its definition has been removed from the toolbox)
  - 'tool-instance-removed' (send after a tool instance, registered globally or per-toolbox, has been removed from the toolbox)
Attachment #8799390 - Flags: review?(poirot.alex)
Comment on attachment 8801706 [details]
Bug 1308912 - Simplify tool-unregistered by only accepting a string toolId.

https://reviewboard.mozilla.org/r/86386/#review85924

I imagine you can also get rid of this duck typing:
http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#733

::: devtools/client/framework/toolbox.js:1905
(Diff revision 1)
>    /**
>     * 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. Passing the

Please update the doc type

::: devtools/client/framework/toolbox.js:1911
(Diff revision 1)
>     *         tool id should be avoided as it is a temporary measure.
>     */
>    _toolUnregistered: function (event, toolId) {
>      if (typeof toolId != "string") {
> -      toolId = toolId.id;
> +      throw new Error("Unexpected non-string toolId received.");
>      }

Given that tool-unregistered is emitted by gDevTools, i.e. our codebase, we can get rid of the type check + throw. We know it is going to be a string.
Attachment #8801706 - Flags: review?(poirot.alex) → review+
(In reply to Luca Greco [:rpl] from comment #9)
> Added as a separate patch in this mozreview request.

Thanks!

> > What about using toolbox.getToolDefinition?
> 
> I opted for `toolbox.getToolboxToolDefinition(id)`, how it sounds to you?
...
> Sure, it makes sense, I added it as `toolbox.getAdditionalToolboxTools()`

I would omit the "Toolbox" in both function names. It looks redundant as we are calling that function on a toolbox object...
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review85944

I have to go, I haven't reviewed the whole patch again, but here is a bunch of comments.

::: devtools/client/framework/toolbox-options.js:194
(Diff revision 3)
> +    const toolbox = this.toolbox;
> +
> +    // Signal tool registering/unregistering globally (for the tools registered
> +    // globally) and per toolbox (for the tools registered to a single toolbox).
>      let onCheckboxClick = function (id) {
> -      let toolDefinition = gDevTools._tools.get(id);
> +      let toolDefinition = gDevTools._tools.get(id) || toolbox.getToolboxToolDefinition(id);

You don't need getToolboxToolDefiniton,
This line is equivalent to
  let toolDefinition = toolbox.getToolDefinition(id);

::: devtools/client/framework/toolbox.js:1281
(Diff revision 3)
> +                      toolId);
> +    }
> +
> +    this._removeToolById(toolId);
> +    this.toolboxToolDefinitions.delete(toolId);
> +    this.emit("toolbox-tool-removed", toolId);

I don't see much value in this event given it is sent synchronously when removeToolboxTool returns.
Same thing applies for toolbox-tool-added.

::: devtools/client/framework/toolbox.js:2050
(Diff revision 3)
>          key.parentNode.removeChild(key);
>        }
>      }
> +
> +    // The tool instance has been removed from the toolbox.
> +    this.emit("tool-instance-removed", toolId);

I tend to dislike seing code used only for tests.
Also, like other events, it is sent synchronously when calling removeToolboxTool... so it doesn't look much useful.

Could you listen for the event sent when the panel is destroyed? You are not returning a panel from definition.build:
 toolbox.addToolboxTool({
    id: TOOL_ID,
    label: "per-toolbox Test Tool",
    inMenu: true,
    isTargetSupported: () => true,
    build: function () {
     // HERE! you could return a panel object with destroy method and assert it gets called
    },
    key: "t"
  });

::: devtools/client/framework/toolbox.js:2066
(Diff revision 3)
> +    let tool = this.getToolDefinition(toolId);
> +    if (!tool) {
> +      // Ignore if the tool is not found in the global or
> +      // per-toolbox maps of registered tools (e.g. when a per-toolbox tool
> +      // has been toggle in the toolbox options view, every toolbox will receive
> +      // the toolbox-register and toolbox-unregister events).

The first part of the comment is useless, it is redundant with the code, the second part wich is minimized between the parentheses is much more useful!
Attachment #8799390 - Flags: review?(poirot.alex)
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review86228

I took some time to read the read, here is some more comments.

::: devtools/client/framework/toolbox.js:1260
(Diff revision 3)
> +    }
> +
> +    if (this.toolboxToolDefinitions.has(definition.id)) {
> +      throw new Error("Tool definition already registered to this toolbox: " +
> +                      definition.id);
> +    }

nit, optional: you may merge these two into: 
if (this.isToolRegistered(definition.id))
  throw "already registered"
The error message would be less precise.

::: devtools/client/framework/toolbox.js:1982
(Diff revision 3)
> +   *
> +   * @returns {bool}
> +   *         Returns true if the tool is registered globally or on this toolbox.
>     */
>    isToolRegistered: function (toolId) {
> -    return gDevTools.getToolDefinitionMap().has(toolId);
> +    // Check if the toolId is in the global or the per-toolbox map of registered tools.

This comment is very redundant with the method doc.

::: devtools/client/framework/toolbox.js:2002
(Diff revision 3)
> -    // instead of gDevTools
> -    this.emit("tool-registered", toolId);
>    },
>  
>    /**
> -   * Handler for the tool-unregistered event.
> +   * Internal helper that removes a loaded tool from the toolbox.

We can tweak this comment to say why it is needed.
To something like:
"Removes a loaded tool panel and tab from the toolbox without removing its definition so that it can still listed in options and re-added later"
This method is the opposite of _buildTabForTool+loadTool, we may be able to find a nice opposite name? removeTabAndPanelForTool? removeTabAndUnloadTool? unloadTool?
Or split this method in two (unloadTool [remove the panel] and removeTabForTool) and call both?

::: devtools/client/framework/toolbox.js:2079
(Diff revision 3)
> +
> +  /**
> +   * Handler for the tool-unregistered event.
> +   * @param  {string} event
> +   *         Name of the event ("tool-unregistered")
> +   * @param  {string|object} toolId

This is now only "string"

::: devtools/client/framework/toolbox.js:2081
(Diff revision 3)
> +   * 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. Passing the
> +   *         tool id should be avoided as it is a temporary measure.

The comment needs to be updated.
(In reply to Alexandre Poirot [:ochameau] from comment #16)
> I took some time to read the read, here is some more comments.

s/read the read/read the rest/!
I'm giving a beer to the dev that implements this. 5$ on Bountysource :) I know it's not much but I can't wait for Chrome DevTools extensions to work in Firefox also :) I might increase the amount in the future.
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review85944

> You don't need getToolboxToolDefiniton,
> This line is equivalent to
>   let toolDefinition = toolbox.getToolDefinition(id);

that's true, suggestion applied in the updated patch.

> I don't see much value in this event given it is sent synchronously when removeToolboxTool returns.
> Same thing applies for toolbox-tool-added.

I've updated this patch and removed these new events (and reworked the test file to do not use any event to sync the test steps).

> I tend to dislike seing code used only for tests.
> Also, like other events, it is sent synchronously when calling removeToolboxTool... so it doesn't look much useful.
> 
> Could you listen for the event sent when the panel is destroyed? You are not returning a panel from definition.build:
>  toolbox.addToolboxTool({
>     id: TOOL_ID,
>     label: "per-toolbox Test Tool",
>     inMenu: true,
>     isTargetSupported: () => true,
>     build: function () {
>      // HERE! you could return a panel object with destroy method and assert it gets called
>     },
>     key: "t"
>   });

I've reworked the test case to use promises resolved when the addon toolbox has been build and destroyed (and cleaned it a bit)

> The first part of the comment is useless, it is redundant with the code, the second part wich is minimized between the parentheses is much more useful!

I removed the redundant part and turned the part in the parentheses as the actual inline comment.
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review86228

> nit, optional: you may merge these two into: 
> if (this.isToolRegistered(definition.id))
>   throw "already registered"
> The error message would be less precise.

it sounds reasonable to me, I applied the suggestion to the updated patch.

> We can tweak this comment to say why it is needed.
> To something like:
> "Removes a loaded tool panel and tab from the toolbox without removing its definition so that it can still listed in options and re-added later"
> This method is the opposite of _buildTabForTool+loadTool, we may be able to find a nice opposite name? removeTabAndPanelForTool? removeTabAndUnloadTool? unloadTool?
> Or split this method in two (unloadTool [remove the panel] and removeTabForTool) and call both?

In the updated patch I added the suggestion brief description of it in the function inline comment and renamed it to `unloadTool`.
Comment on attachment 8801706 [details]
Bug 1308912 - Simplify tool-unregistered by only accepting a string toolId.

https://reviewboard.mozilla.org/r/86386/#review85066

> I'm a bit unsure if it would be more reasonable to start by logging a deprecation error message here or just raising an exception (I would like to take a look at how much is this directly used from addons).
> 
> What is the preferred way to produce a deprecated warning in this part of the devtools? (e.g. just using  a `console.warn` or is there a devtools-specific helper?)

I opted to log a deprecation message here (using the "Deprecated.jsm" javascript module), 
is this the correct way to log this kind of the warning messages from the devtools framework code? 

(I "dxr-ed" the devtools internals for any existent use of warning logging, and I found that the "Deprecated.jsm" is used to warn the addon developers of the old devtools jsm module urls that have been deprecated)
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review90124

Looks good, thanks Luca!

::: devtools/client/framework/toolbox.js:1293
(Diff revision 4)
> +   * @return {boolean}
> +   *
> +   */
> +  hasAdditionalToolboxTool(toolId) {
> +    return this.toolboxToolDefinitions.has(toolId);
> +  },

Did you saw comment 14?
I'm wondering if "Toolbox" is really necessary is all these methods: toolboxToolDefitinion, getAdditionalToolboxTools, hasAdditionalToolboxTool, getToolboxToolDefinition, addToolboxTool, removeToolboxTool.

* In getAdditionalToolboxTools, hasAdditionalToolboxTool, "Toolbox" looks redundant with "Additional".
May be it is really justified in addToolboxTool/removeToolboxTool.

* hasAdditionalToolboxTool is only used in test and could be replaced with isToolRegistered(). This helper method is then not justified for removeToolboxTool.
* getToolboxToolDefintion isn't used anywhere?

::: devtools/client/framework/toolbox.js:2030
(Diff revision 4)
>    },
>  
>    /**
> -   * Handler for the tool-unregistered event.
> -   * @param  {string} event
> -   *         Name of the event ("tool-unregistered")
> +   * Internal helper that removes a loaded tool from the toolbox,
> +   * it removes a loaded tool panel and tab from the toolbox without removing
> +   * its definition, so that it can still listed in options and re-added later.

still listed => still be listed

::: devtools/client/framework/toolbox.js:2033
(Diff revision 4)
> -   * Handler for the tool-unregistered event.
> -   * @param  {string} event
> -   *         Name of the event ("tool-unregistered")
> +   * Internal helper that removes a loaded tool from the toolbox,
> +   * it removes a loaded tool panel and tab from the toolbox without removing
> +   * its definition, so that it can still listed in options and re-added later.
> +   *
>     * @param  {string} toolId
> -   *         id of the tool that was unregistered.
> +   *         Id of the tool that was registered

"Id of the tool to be removed"

::: devtools/client/framework/toolbox.js:2078
(Diff revision 4)
>          key.parentNode.removeChild(key);
>        }
>      }
> +
> +    // The tool instance has been removed from the toolbox.
> +    this.emit("tool-instance-removed", toolId);

This is no longer used.
Attachment #8799390 - Flags: review?(poirot.alex) → review+
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review90124

> Did you saw comment 14?
> I'm wondering if "Toolbox" is really necessary is all these methods: toolboxToolDefitinion, getAdditionalToolboxTools, hasAdditionalToolboxTool, getToolboxToolDefinition, addToolboxTool, removeToolboxTool.
> 
> * In getAdditionalToolboxTools, hasAdditionalToolboxTool, "Toolbox" looks redundant with "Additional".
> May be it is really justified in addToolboxTool/removeToolboxTool.
> 
> * hasAdditionalToolboxTool is only used in test and could be replaced with isToolRegistered(). This helper method is then not justified for removeToolboxTool.
> * getToolboxToolDefintion isn't used anywhere?

In the updated patch, I renamed all the new helpers into `getAdditionalTools`/`addAdditionalTool`/..., so that the method name is explicit by not redundant.

I've also removed `getToolboxToolDefinition`, because it is not used anymore, but I'm a bit unsure about removing the `hasAdditionalTool` method:

- if we opt to use `isToolRegistered` in the `removeAdditionalTool` method, the method can unload from the toolbox even tools that are registered globally
- I removed usage of the `additionalToolDefinitions` getter in the tests, because the getter looks more like an internal  implementation detail than an exposed API

if the above reasons doesn't seem enough to keep the `hasAdditionalTool` method, I'm going to change `removeAdditionalTool` and the tests to use `additionalToolDefinitions` getter instead.
Comment on attachment 8799390 [details]
Bug 1308912 - Add support for addon tools registered to a specific DevTools toolbox.

https://reviewboard.mozilla.org/r/84580/#review90124

> In the updated patch, I renamed all the new helpers into `getAdditionalTools`/`addAdditionalTool`/..., so that the method name is explicit by not redundant.
> 
> I've also removed `getToolboxToolDefinition`, because it is not used anymore, but I'm a bit unsure about removing the `hasAdditionalTool` method:
> 
> - if we opt to use `isToolRegistered` in the `removeAdditionalTool` method, the method can unload from the toolbox even tools that are registered globally
> - I removed usage of the `additionalToolDefinitions` getter in the tests, because the getter looks more like an internal  implementation detail than an exposed API
> 
> if the above reasons doesn't seem enough to keep the `hasAdditionalTool` method, I'm going to change `removeAdditionalTool` and the tests to use `additionalToolDefinitions` getter instead.

typo: "explicit by not redundant" => "explicit but not redundant"
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df6ef63c73e3
Simplify tool-unregistered by only accepting a string toolId. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/381c0d6fd9b9
Add support for addon tools registered to a specific DevTools toolbox. r=ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/df6ef63c73e3
https://hg.mozilla.org/mozilla-central/rev/381c0d6fd9b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.