Closed Bug 1394750 Opened 7 years ago Closed 6 years ago

State of devtools installed by addons not remembered

Categories

(WebExtensions :: Developer Tools, defect, P2)

57 Branch
defect

Tracking

(firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified

People

(Reporter: 61.1p57, Assigned: rpl)

References

Details

Attachments

(5 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170825011442

Steps to reproduce:

1. Install https://www.eff.org/files/https-everywhere-test/https-everywhere-2017.7.21.1338-eff.xpi
2. F12, disable https everywhere at "developer tools installed by add-ons" from toolbox options.
3. Close dev panel, then open again.


Actual results:

HTTPS Everywhere is shown on the panel.


Expected results:

HTTPS Everywhere should not be shown.
Please report the issue to the extension author. Thank you!
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
This is a WebExtension, and I think it is not supposed to be able to override user's settings in the first place.
Please check https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Extending_the_developer_tools, the only thing a WE addon can do is to register a dev page via the WE API
Flags: needinfo?(kohei.yoshino)
I'm not very familiar with the APIs honestly.
Component: Untriaged → WebExtensions: Developer Tools
Flags: needinfo?(kohei.yoshino)
Product: Firefox → Toolkit
Can you reopen this bug please?
Flags: needinfo?(kohei.yoshino)
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(kohei.yoshino)
Resolution: INVALID → ---
But I think contacting the extension author is the first step anyway.
Attached file reduction.zip
Confirmed, this isn't the problem with the extension, but the WebExtension API. 

Also a note to the reporter, creating the panel to point the dev-tools.html page which creates another panel means you get a lot of panels.
Status: REOPENED → NEW
Priority: -- → P5
In the DevTools preferences panel, the checkbox related to the DevTools panels created from a WebExtension has currently the same behavior of the ones generated for the integrated tools (and the tools registered globally from a legacy extension),
but the DevTools panels created from a WebExtensions are dynamically created from the devtools_page on a particular DevTools toolbox instance, instead of being registered globally, and the API method doesn't include any id which would allow to identify them individually.

The current behavior of that checkbox (and the related about:config preference) is definitely not that helpful, because the stored preference is associated to the id of the panel and this id has to be dynamically generated, and it has to be different between different instances of the DevTools toolbox, and so it is not going to be valid anymore when the toolbox is closed and re-opened (or, in general, on different instances of the DevTools toolbox). 

The following behaviors would be better and they should be able to cover this scenario as the user expects:

- in the DevTools preference panel, create a single checkbox for every extension that has a devtools_page property in its manifest (as oppose of creating a checkbox for every DevTools panel created by a WebExtension, like it is currently)

- the preference related to the above checkbox in the DevTools preferences panel should be associated to the WebExtensions extension id (instead of being associated to the generated id of the created DevTools panels)

- when the preference associated to the extension is set to false:
  - on the existent DevTools toolboxes, every devtools_page related to the WebExtension extension id should be closed (which will then disable any customization applied from the WebExtension on the developer tools, e.g. the panels added by it) 
  - on newly created DevTools toolboxes, the devtools_page should not be loaded (yet)

- when the preference associated to the extension is set to true:
  - on the existent DevTools toolboxes, the related devtools_page should be loaded (which will then create the related DevTools panels)
  - on newly created DevTools toolboxes, the devtools_page should be loaded when the toolbox is created
Priority: P5 → P2
Comment on attachment 8944728 [details]
Bug 1394750 - Keep track of the active and enabled devtools webextensions.

Hi Julian,
The attached patches contains a set of changes that implements the behavior described in comment 9:

- The extensions listed in the toolbox options represents the devtools_page of the related extension, it is persisted in a preference based on the extension uuid (which is assigned at install time, and so the decision that the user made is going to be still used when the toolbox is re-opened, and across Firefox restarts)

- disabling the extension from the toolbox options has the effect of removing all the "devtools" parts of the extension (e.g. any created devtools panel or inspector sidebar) on all the existent toolboxes

- enabling the extension from the toolbox options has the effect of re-starting the extension devtools page on all the supported toolboxes

- when an extension has been disabled (or uninstalled), its devtools pages instances (one per toolbox) are destroyed and the extension is unlisted from the toolbox options

- when an extension is enabled (or installed), its devtools_page is built for all the existent toolbox which are supported and the toolbox options are refreshed so that the extension is listed (which would also fix Bug 1378107 and make it a bit faster to iterate while developing a devtools panel, because it will not require the developer to close and re-open the toolbox on every extension reload)

- the devtools about:config preferences branch for the extension is removed when the extension is uninstalled

Do you mind to take a brief look to this first patch which contains the changes applied to the devtoools internals?

This set of changes are going to require additional devtools tests and I'd like to get an initial feedback from you (to double-check that the proposed approach sounds reasonable to you, and apply any needed change to it before finalizing the patch by adding the additional tests)
Attachment #8944728 - Flags: feedback?(jdescottes)
Comment on attachment 8944728 [details]
Bug 1394750 - Keep track of the active and enabled devtools webextensions.

https://reviewboard.mozilla.org/r/214880/#review220830

Thanks a lot for the patch (and for the extra explanations)! I have some comments about the shim. And I'd like to discuss moving the preference creation to ext-devtools if possible. Otherwise this is on the right track, I think you can get started on adding tests.

As a follow up, what do you think about moving all the toolbox/extensions related code in a dedicated file outside of toolbox.js. This way we could properly document evertything, explain the differences between registering a webextension and an additional panel etc...

::: devtools/client/framework/toolbox.js:3057
(Diff revision 1)
> +   * An helper function which return true if the extension with the given UUID is
> +   * list as active for the toolbox and has its related devtools about:config preference
> +   * set to true.
> +   * @see browser/components/extensions/ext-devtools.js
> +   */
> +  isWebExtensionEnabled: function (extensionUUID) {

See my comment about getWebExtensionPrefBranch

::: devtools/shim/DevToolsShim.jsm:225
(Diff revision 1)
>      if (!this.isInitialized()) {
>        DevtoolsStartup.initDevTools(reason);
>      }
> +  },
> +
> +  getWebExtensionPrefBranch(extensionUUID) {

It's a bit weird to have this logic on the shim. It doesn't rely on anything else in DevTools. Related to this, toolbox.isWebExtensionEnabled is only used in your second patch, by webextension code. 

Could we move the logic that creates the pref branch/string based on the uuid to ext-devtools.js? When registering a webextension, instead of just giving a uuid + name, we could pass uuid, name, and the enabling pref (which is the only pref interesting for devtools right now).

::: devtools/shim/DevToolsShim.jsm:241
(Diff revision 1)
> +  },
> +
> +  /**
> +   * Iterator that yields each of the toolbox and targets.
> +   */
> +  * [Symbol.iterator]() {

I think I would prefer a getToolboxes API that doesn't rely on iterators and I would move it to the section below (compatibility layer for webextensions) This means also adding the getToolboxes API to devtools.js
That being said, in the second patch this is only called in spots where we already have a toolbox, so it doesn't have to be in the Shim. We could also have a toolbox:getDevToolsGlobal api or something like that. Let's go with the shim option for now. 

Last comment about that, I was tempted to suggest to add [un]registerWebExtension APIs to devtools.js directly and loop on toolboxes over there. But since you have to also build the extension for each toolbox (`devtoolsPageDefinition.buildForToolbox(toolbox);`) that would complexify the picture a bit too much.
Attachment #8944728 - Flags: feedback?(jdescottes) → feedback+
Attachment #8961875 - Flags: review?(jdescottes)
Attachment #8944729 - Flags: review?(aswan)
Attachment #8961876 - Flags: review?(lgreco)
Attachment #8961877 - Flags: review?(aswan)
Comment on attachment 8961876 [details]
Bug 1394750 - Fix typo in WebExtensions devtools panel visibility test.

https://reviewboard.mozilla.org/r/230706/#review236186

Re-newed r+ on this patch imported from Bug 1428976 (marked as a duplicate of this issue), the patch contains a fixed typo in the browser_ext_devtools_panel.js test.
Attachment #8961876 - Flags: review?(lgreco) → review+
Attachment #8944728 - Flags: review?(jdescottes)
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review236706

::: browser/components/extensions/ext-devtools.js:457
(Diff revision 3)
> +      return;
> +    }
> +
> +    this.devtoolsPrefBranch.removeObserver("enabled", this);
> +
> +    if (shutdownReason === "ADDON_UNINSTALL") {

This should be done from onUninstall(), there are a few corner cases where we don't get a shutdown reason of ADDON_UNINSTALL (for instance, disable the extension and then uninstall it while it is disabled)
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review238012

Some comments below, there is also a lot of stuff here about how the api interacts with devtools that I don't really know much about, would probably be good to have somebody from the devtools team take a look.

::: browser/components/extensions/ext-devtools.js:419
(Diff revision 4)
> +  isDevToolsPageDisabled() {
> +    return !this.devtoolsPrefBranch.getBoolPref("enabled", false);
> +  }
> +
> +  /**
> +   * Observes the changed preferences on the DevTools about:config preferemces branch

typo in "preferences".
Also, this is a nit but in comments generally I think the term "about:config preference(s)" is unnecessary and just "preference(s)" is sufficient.

::: browser/components/extensions/ext-devtools.js:434
(Diff revision 4)
> +    if (subject !== this.devtoolsPrefBranch || prefName !== "enabled") {
> +      return;
> +    }
> +
> +    if (this.isDevToolsPageDisabled()) {
> +      // Shutdown the extension devtools page from all the existent toolboxes.

nit: this comment (and the one below) seem redundant since the names of the methods being called describe the same thing.

::: browser/components/extensions/ext-devtools.js:456
(Diff revision 4)
> +    // If the preference branch has never been initialized there is nothing
> +    // to cleanup.
> +    if (!this._devtoolsPrefBranch) {
> +      return;
> +    }

how would this happen?

::: browser/components/extensions/ext-devtools.js:483
(Diff revision 4)
> +
>      // Create and register a new devtools_page definition as specified in the
>      // "devtools_page" property in the extension manifest.
> -    let devtoolsPageDefinition = new DevToolsPageDefinition(extension, manifest.devtools_page);
> +    let devtoolsPageDefinition = new DevToolsPageDefinition(extension, manifest.devtools_page,
> +                                                            () => this.isDevToolsDisabled());
>      devtoolsPageDefinitionMap.set(extension, devtoolsPageDefinition);

It doesn't have to happen here, but can we just store this in the instance of the API class rather than in a global Map?
Comment on attachment 8944728 [details]
Bug 1394750 - Keep track of the active and enabled devtools webextensions.

https://reviewboard.mozilla.org/r/214880/#review238840

Thanks for the patch Luca! A few comments and nits, but overall this looks good to me.

::: devtools/client/framework/devtools.js:698
(Diff revision 3)
>      //   for (let [target, toolbox] of this._toolboxes) toolbox.destroy();
>      // Is taken care of by the gDevToolsBrowser.forgetBrowserWindow
>    },
>  
>    /**
> +   * Return the array of the existent toolboxes.

The documentation could mention that this returns an array of [target, toolbox] pairs, instead of an array of toolboxes.

nit: existent -> existing (I guess?)

::: devtools/client/framework/test/browser_toolbox_options.js:249
(Diff revision 3)
> +    modifiedPrefs.push(ext.pref);
> +  }
> +
> +  // Turn each registered WebExtension to disabled.
> +  for (let node of webExtensionNodes) {
> +    toggleWebExtension(node);

Even if this is purely synchronous, is there any event we can listen to in order to be future-proof?

::: devtools/client/framework/test/browser_toolbox_options.js:266
(Diff revision 3)
> +  }
> +
> +  // Check that all the registered Extension are now enabled again.
> +  for (let ext of toggleableWebExtensions) {
> +    ok(toolbox.isWebExtensionEnabled(ext.uuid),
> +       `The registered WebExtension ${ext.name} has been disabled`);

nit: has been disabled -> has been enabled

::: devtools/client/framework/test/browser_toolbox_options.js:270
(Diff revision 3)
> +  for (let ext of toggleableWebExtensions) {
> +    toolbox.unregisterWebExtension(ext.uuid);
> +  }

Since the test only adds one webextension, maybe we should only test this one, and unregister only this one. It's unlikely that we would have other extensions already enabled before going into this test, but the test should still only assert and cleanup what the test controls.

::: devtools/client/framework/toolbox-options.js:231
(Diff revision 3)
>        // Set the kill switch pref boolean to true
>        Services.prefs.setBoolPref(toolDefinition.visibilityswitch, this.checked);
>        gDevTools.emit(this.checked ? "tool-registered" : "tool-unregistered", id);
>      };
>  
> -    let createToolCheckbox = tool => {
> +    let createToolCheckbox = (tool, onClick = onCheckboxClick) => {

Rather than passing a different callback for some of the tools, what about:
- always use onCheckboxClick
- pass the tooldefinition to onCheckboxClick rather than just the id
- add isWebExtension to the tooldefinition when appropriate
- in onCheckboxClick, read isWebExtension and process regular tools and webextension tools differently (the main difference should be about emitting the "tool-(un)registered" event?)

::: devtools/client/framework/toolbox-options.js:280
(Diff revision 3)
> +    // Populating the additional tools list.
>      let atleastOneAddon = false;
>      for (let tool of gDevTools.getAdditionalTools()) {
>        atleastOneAddon = true;
>        additionalToolsBox.appendChild(createToolCheckbox(tool));
>      }

I know we already discussed about that, but to be clear, WebExtensions _never_ register tools on the global devtools, so this code might be candidate for removal, right? Can I file a follow up for that?

::: devtools/client/framework/toolbox-options.js:287
(Diff revision 3)
>      for (let tool of gDevTools.getAdditionalTools()) {
>        atleastOneAddon = true;
>        additionalToolsBox.appendChild(createToolCheckbox(tool));
>      }
>  
> -    // Populating the additional toolbox-specific tools list that came
> +    // Populating the additional tools that came

Just as a comment, the line-length in devtools/ is 90 chars, so this comment (and a few others) actually fits on one line.

::: devtools/client/framework/toolbox.js:3120
(Diff revision 3)
>      }
>  
>      return netPanel.panelWin.Netmonitor.fetchResponseContent(requestId);
> -  }
> +  },
> +
> +  // Support management of installed WebExtensions that provides a devtools_page.

nit: provides -> provide

::: devtools/client/framework/toolbox.js:3138
(Diff revision 3)
> +      return {uuid, name, pref};
> +    });
> +  },
> +
> +  /**
> +   * Add a WebExtensions to the list of the active extensions (given the extension UUID,

nit: WebExtensions -> WebExtension
an unique -> a unique (1 line below)

Same comments for unregisterWebExtension

::: devtools/client/framework/toolbox.js:3167
(Diff revision 3)
> +    this._webExtensions.delete(extensionUUID);
> +    this.emit("webextension-unregistered", extensionUUID);
> +  },
> +
> +  /**
> +   * An helper function which return true if the extension with the given UUID is

nit: an helper -> a helper, which return -> which returns, is list -> is listed (line below)
Attachment #8944728 - Flags: review?(jdescottes) → review+
Comment on attachment 8961875 [details]
Bug 1394750 - Fix selected devtools panel when an Extension DevTools panel is unloaded.

https://reviewboard.mozilla.org/r/230704/#review238872

As discussed, I would prefer to keep things simple for now and if we don't find an index, simply pick the first tool.
My main motivation is that tab ordering might be reworked soon.
Attachment #8961875 - Flags: review?(jdescottes)
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review238012

Sure, they are all related to the changes that I've done on the devtools side, I'll ask jdescottes to give another look and his explicit sign-off on this patch too.

> typo in "preferences".
> Also, this is a nit but in comments generally I think the term "about:config preference(s)" is unnecessary and just "preference(s)" is sufficient.

I'm also in doubt if it can be confused with the "preferences" as in "about:preferences", but I agree that the context is enough to make it clear on its own.
Attachment #8944729 - Flags: review?(aswan) → review?(jdescottes)
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review238012

> how would this happen?

I looked again and it should not actually happen, I guess that I was just too defensive because of it being created lazily, but it actually always created as soon as an extension with a devtools_page property in the manifest is being loaded.

I've removed the redundant check for now (but I'm thinking that I should also make the devtools branch property in the class constructor instead of lazily create it from the getter, it would make this much easier to follow).

> It doesn't have to happen here, but can we just store this in the instance of the API class rather than in a global Map?

Yeah, I think that we can do that if we also change the way we lazily initialize the devtools API (which is currently happening when we create the first devtools page definition from a devtools_page manifest entry, from the global initDevTools function which is accessing that map: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/browser/components/extensions/parent/ext-devtools.js#293), which to be fair I would really like to, I'm going to file a follow up for that.
Attachment #8961875 - Attachment is obsolete: true
Comment on attachment 8944728 [details]
Bug 1394750 - Keep track of the active and enabled devtools webextensions.

https://reviewboard.mozilla.org/r/214880/#review238840

> I know we already discussed about that, but to be clear, WebExtensions _never_ register tools on the global devtools, so this code might be candidate for removal, right? Can I file a follow up for that?

Yes, the WebExtensions APIs cannot register a panel on the global devtools (or any other tool that is registered from the devtools API namespace, because the devtools API namespace is only available once a toolbox is open, and it is unlikely that we would ever change that).

But it is worth to mention that there is at least one "test-only user" that register a tool globally:
https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/browser/components/extensions/test/browser/browser_ext_devtools_panel.js#44-69

In browser_ext_devtools_panel.js (which is also refactored in the last patch in this queue), I've added a "blank panel" some time ago, because loading the full webconsole or the inspector panels may require some time and being pretty "expensive", and so it was making that test file to fail intermittently (especially in debug builds). 
Given that the integrated devtool panels that were triggering the failures weren't really part of the test, I opted to introduce a "blank panel" registered globally in the test file.

Nevertheless, I don't need that "blank panel" to be part of the toolbox options (I just need an empty panel to select when the toolbox is opened), and so if we would like to candidate for removal this "section" of the generated toolbox options, that would not affect this "test-only" usage.
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review238930

LGTM, a couple of comments, no issue.

::: browser/components/extensions/ext-devtools.js:299
(Diff revision 7)
>    }
>  }
>  
> +// Get the devtools preference given the extensionId.
> +function getDevToolsPrefBranchName(extensionId) {
> +  return `devtools.webextensions.'${extensionId}'`;

Just checking because it is a bit out of the ordinary: this results in preference names such as:

`devtools.webextensions.'{ff0cf743-dcf3-4097-ae4c-d872c18f7b4e}'.enabled`

If this is expected, is there a technical reason why it's better to wrap the uuid as `'{uuid}'` ?

::: browser/components/extensions/ext-devtools.js:509
(Diff revision 7)
> +    }
> +
> +    // Iterate over the existent toolboxes and create the devtools page for them
> +    // (if the toolbox target is supported).
> +    for (let [, toolbox] of DevToolsShim.getToolboxes()) {
> +      if (!toolbox.target.isLocalTab) {

In theory we could iterate on [target, toolbox] rather than [, toolbox] and then fetch toolbox.target. But I wonder if we shouldn't make getToolboxes simply return an array of toolboxes. That's follow-up material.
Attachment #8944729 - Flags: review?(jdescottes) → review+
Comment on attachment 8944728 [details]
Bug 1394750 - Keep track of the active and enabled devtools webextensions.

https://reviewboard.mozilla.org/r/214880/#review238840

> Even if this is purely synchronous, is there any event we can listen to in order to be future-proof?

Toggling the checkbox related to a WebExtension just toggle its preference (everything else that really happens when there is an actual webExtension doesn't really exist in this test, where the installed extension is actually mocked, but it is tested in the WebExtensions test changed in the last patch of this set), the only thing that we could listen for currently is the toggled preference, but I wasn't sure if it was really worth it.

> Since the test only adds one webextension, maybe we should only test this one, and unregister only this one. It's unlikely that we would have other extensions already enabled before going into this test, but the test should still only assert and cleanup what the test controls.

I'm actually thinking that it would make sense if I register two "mocked" WebExtensions in this test, so that we check here that toggling the checkbox for one doesn't affect the other unrelated one, as expected.

How that would sound to you?

> Rather than passing a different callback for some of the tools, what about:
> - always use onCheckboxClick
> - pass the tooldefinition to onCheckboxClick rather than just the id
> - add isWebExtension to the tooldefinition when appropriate
> - in onCheckboxClick, read isWebExtension and process regular tools and webextension tools differently (the main difference should be about emitting the "tool-(un)registered" event?)

yeah, that sounds pretty good to me, I'm applying this change.
(In reply to Luca Greco [:rpl] from comment #46)
> Comment on attachment 8944728 [details]
> Bug 1394750 - Keep track of the active and enabled devtools webextensions.
> 
> https://reviewboard.mozilla.org/r/214880/#review238840
> 
> > Even if this is purely synchronous, is there any event we can listen to in order to be future-proof?
> 
> Toggling the checkbox related to a WebExtension just toggle its preference
> (everything else that really happens when there is an actual webExtension
> doesn't really exist in this test, where the installed extension is actually
> mocked, but it is tested in the WebExtensions test changed in the last patch
> of this set), the only thing that we could listen for currently is the
> toggled preference, but I wasn't sure if it was really worth it.
> 

I see, fine by me to keep it as is then.
 
> > Since the test only adds one webextension, maybe we should only test this one, and unregister only this one. It's unlikely that we would have other extensions already enabled before going into this test, but the test should still only assert and cleanup what the test controls.
> 
> I'm actually thinking that it would make sense if I register two "mocked"
> WebExtensions in this test, so that we check here that toggling the checkbox
> for one doesn't affect the other unrelated one, as expected.
> 
> How that would sound to you?
> 

Sounds good, go ahead!
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review238930

> Just checking because it is a bit out of the ordinary: this results in preference names such as:
> 
> `devtools.webextensions.'{ff0cf743-dcf3-4097-ae4c-d872c18f7b4e}'.enabled`
> 
> If this is expected, is there a technical reason why it's better to wrap the uuid as `'{uuid}'` ?

That's something that I'm not happy of, and I'm still thinking about it.
Initially the preference was based of the extension uuid, and it is automatically generated and so it doesn't have any "." in it for sure.

During aswan review, he noticed that we should actually remove the preference branch from the lifecycle onUninstalled static method, which received just the extensionId (it can't receive the extension object because the extension has already been stopped).

The extensionId can be assigned in the manifest.json and it can be in an "email"-like form, and so it can have "." characters in it, from a behavior point of view it doesn't make any difference with the current implementation, but a user may find hard to understand which part of the preference name is the extension id, and so I opted to quote it to help their readability.

I think that it would be actually better to go back to use the extension uuid, and retrieve the one we need from the UUIDMap, using the extensionId (basicallly using `let uuid = UUIDMap.get(addon.id, false);` as we do here: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/toolkit/components/extensions/Extension.jsm#207).

> In theory we could iterate on [target, toolbox] rather than [, toolbox] and then fetch toolbox.target. But I wonder if we shouldn't make getToolboxes simply return an array of toolboxes. That's follow-up material.

Actually, I think we should change it right now, it makes getToolboxes much easier to get right without even reading its jsdocs.

I've applied this change.
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review239036

I agree that the pref branch handling would be easier to follow if you just set it up in `onManifestEntry()` instead of the lazy getter.
I'm also finding the terminology here kind of confusing -- the term "DevToolsPage" seems to mean one thing for `CreateDevToolsPage()` and `destroyDevToolsPage()` and something else for `buildDevToolsPages()` and `shutdownDevToolsPages()`?  I don't have enough context here to suggest better names though.
Attachment #8944729 - Flags: review?(aswan) → review+
Comment on attachment 8961877 [details]
Bug 1394750 - Refactoring WebExtensions devtools panel tests.

https://reviewboard.mozilla.org/r/230708/#review239046

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:132
(Diff revision 6)
> +    files = {
> +      ...files,

nit: how about either
```
files["devtools_panel.html"] = ...;
...
```

or

```
Object.assign(files, {
  "devtools_panel.html": ...,
  ...
});
```

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:149
(Diff revision 6)
>    });
>  
>    // Ensure that the initial value of the devtools theme is "light".
>    await SpecialPowers.pushPrefEnv({set: [[DEVTOOLS_THEME_PREF, "light"]]});
> +  registerCleanupFunction(function() {
> +    Preferences.reset(DEVTOOLS_THEME_PREF);

await SpecialPowers.popPrefEnv()

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:270
(Diff revision 6)
> +    const {
> +      target, panelId, isFirstPanelLoad, expectedResults,
> +    } = params;

nit: can just move these to the left side of the arrow on the line above

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:380
(Diff revision 6)
> +
> +  // Close and Re-open the toolbox to verify that the toolbox doesn't load the
> +  // devtools_page and the devtools panel.
> +  info("Re-open the toolbox and expect no extension devtools panel");
> +  await closeToolboxForTab(tab);
> +  ({toolbox, target} = (await openToolboxForTab(tab)));

nit: unnecessary parantheses around the await expression

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:390
(Diff revision 6)
> +
> +  // Close and Re-open the toolbox to verify that the toolbox does load the
> +  // devtools_page and the devtools panel again.
> +  info("Restart the toolbox and enable the extension devtools panel");
> +  await closeToolboxForTab(tab);
> +  ({toolbox, target} = (await openToolboxForTab(tab)));

nit: the parentheses around the await expression are unnecessary
Attachment #8961877 - Flags: review?(aswan) → review+
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Hi Andrew, I applied the change we agreed on the preferences branch property initialization, the one related to Comment 21,
and also moved back to use the extension UUID into the preference name.

The main reason to move back from the extension id to the extension UUID is that I'm not really happy that the extension id may contains "." chars and make the preference branch name harder to read, the only reasons for changing it into use the extension id was that the lifecycle onUninstall static method only receives the extensionId and it has to be able to remove the preference branch.
The extension uuid, even if not currently available from the underlying "uninstall" event (yet), can still be retrieved from the UUIDMap and so I added some additional changes to this patch to add the uuid directly as a second parameter of the static onUninstall method (because I'm pretty sure that it could be also helpful for other APIs).

I'm a bit concerned that we didn't have yet detailed a plan for Bug 1372288, and this preference name is supposed to be the same for the extension from the time it has been installed and started once to the time it has been finally uninstalled, 
it would be totally fine if we can assume that the extension uuid itself will still be generated once per addon installation (and the fix for Bug 1372288 will be actually impacting only the origin of the extension urls, or just the ones in the web_accessible_resources list), what do you think?

Do you mind to take a look at the diff interdiff? https://reviewboard.mozilla.org/r/214878/diff/9-10/
(related to the changes needed to add the extension uuid to the onUninstall lifecycle method,
in particular the new changes applied to Extension.jsm and ExtensionParent.jsm).

As a side note: I've not yet rebased these patches to keep the interdiffs of the last updates as readable as possible (but I'm going to do it next, because some of the files in this set of patches has recently changed location and there maybe other conflicts to solve), and I'm also going to merge attachment 8961877 [details] (the refactoring on the test cases) into attachment 8961876 [details] (the patch that applies the changes to ext-devtools and the other WebExtensions internals), because I kept them separated to make the review easier but it would be better to have both in a single more self-contained commit (which applies the changes and pass all the existent tests).
Flags: needinfo?(aswan)
sorry, typo in the second attachment number, I meant: attachment 8961877 [details] into attachment 8944729 [details] (this is the correct attachment number for the patch that applies the changes to ext-devtools and Extension.jsm/ExtensionParent.jsm)
(In reply to Luca Greco [:rpl] from comment #59)
> The main reason to move back from the extension id to the extension UUID is
> that I'm not really happy that the extension id may contains "." chars and
> make the preference branch name harder to read,

Are we expecting that users will be setting (or flipping or even just looking at) this preference manually?
If so, then the local uuid makes it pretty much inaccessible to them.  If not, then I don't think the presence of dots is really a concern.
(In reply to Andrew Swan [:aswan] from comment #61)
> (In reply to Luca Greco [:rpl] from comment #59)
> > The main reason to move back from the extension id to the extension UUID is
> > that I'm not really happy that the extension id may contains "." chars and
> > make the preference branch name harder to read,
> 
> Are we expecting that users will be setting (or flipping or even just
> looking at) this preference manually?
> If so, then the local uuid makes it pretty much inaccessible to them.  If
> not, then I don't think the presence of dots is really a concern.

No, the users are expecting to toggle it from the toolbox options panel
(where there is a checkbox for every extension with a devtools_page defined in its manifest,
and when clicked it toggles the related preference).
Chatted with Luca on IRC, I think we got to a resolution (continuing to use the extension's ID)
Flags: needinfo?(aswan)
Attachment #8961877 - Attachment is obsolete: true
Blocks: 1451795
Comment on attachment 8944729 [details]
Bug 1394750 - Allow the webextension devtools_page to be disabled separately from the entire extension.

https://reviewboard.mozilla.org/r/214882/#review238012

> Yeah, I think that we can do that if we also change the way we lazily initialize the devtools API (which is currently happening when we create the first devtools page definition from a devtools_page manifest entry, from the global initDevTools function which is accessing that map: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/browser/components/extensions/parent/ext-devtools.js#293), which to be fair I would really like to, I'm going to file a follow up for that.

Follow up filed as Bug 1451795.
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/2e6cd6aa34e6
Fix typo in WebExtensions devtools panel visibility test. r=rpl
https://hg.mozilla.org/integration/autoland/rev/0cc9c6fa4ec2
Keep track of the active and enabled devtools webextensions. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f6a0fec5879a
Allow the webextension devtools_page to be disabled separately from the entire extension. r=aswan,jdescottes
https://hg.mozilla.org/mozilla-central/rev/2e6cd6aa34e6
https://hg.mozilla.org/mozilla-central/rev/0cc9c6fa4ec2
https://hg.mozilla.org/mozilla-central/rev/f6a0fec5879a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1368332
Attached image Bug1394750.gif
I can reproduce the issue on Firefox 59.0.2 (20180323154952) under Win 7 64-bit and Mac OS X 10.13.2 with the extensions from the Description and comment4.

After you disable the extensions from the “Toolbox Options” and you reopen the devtools panel, the extensions are loaded again.

This issue is verified as fixed on Firefox 61.0a1 (20180415220108) under Win 7 64-bit and Mac OS X 10.13.2.

Please see the attached video.
Status: RESOLVED → VERIFIED
See Also: → 1455573
Product: Toolkit → WebExtensions
See Also: → 1722370
You need to log in before you can comment on or make changes to this bug.