Closed Bug 1309906 Opened 3 years ago Closed 3 years ago

Start using ChildAPIManager for the devtools APIs

Categories

(WebExtensions :: Developer Tools, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Iteration:
53.1 - Nov 28
Tracking Status
firefox53 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Whiteboard: [devtools][devtools.inspectedWindow][devtools.panels] triaged)

Attachments

(2 files, 6 obsolete files)

58 bytes, text/x-review-board-request
kmag
: review+
Details
58 bytes, text/x-review-board-request
kmag
: review+
Details
The goal of this issue is to prepare the Devtools APIs to be "webext-oop" compatible (in particular with the changes that are going to be introduced by the patches attached to "Bug 1287007 - Start using ChildAPIManager for some APIs").
This patch contains only the set of proposed changes to the WebExtensions internals (as changed by applying the patches attached to Bug 1287008) to adapt them for the devtools APIs needs, in particular:

- it defines a new "process type" named "devtools" and its related envTypes "devtools_parent" and "devtools_child" (it doesn't need to be a real process, but even if the extension devtools contexts will live in the same process of the other extension contexts, they will be separated into two different namespaces, "devtools" and "addon", their views will be tracked into two different view "sets", e.g. extension.views and extension.devtoolsViews, and two different SchemaAPIManager instances as well)

- in the main process, a new "extension-proxy-context-load" event is sent when an extension proxy context has been created, which is going to be used in the devtools_page and devtools_panel implementation during their creation/initialization (in particular to be able to associate the newly created devtools proxy contexts with their related toolbox, which will be then used in the devtools APIs implementation, running in the main process, to communicate with the Remote Debugging Server)

- in the child process, the devtools context will receive an additional initialization field (currently named `devtoolsToolboxInfo`) which is going to contains "devtools extension context"-specific initialization data (e.g. the inspectedWindow tabId, or the id of the devtools panel, when the newly created devtools extension context is a devtools panel)

I'm going to add more details about these last two points, in the comments related to the patches that will use these pieces.
Attachment #8800742 - Flags: feedback?(rob)
This patch introduces small changes related to the existent APIs, introducing the new restrictions in the JSON schema files and registering the needed API implementations in the new devtools_child/devtools_parent envTypes.
Attachment #8800746 - Flags: feedback?(rob)
This patch contains the part of Bug 1291737 patches that create the devtools_page context, but using the "webext-oop" way creating and initializing the extension contexts created from the "main process" in the "addon process".

Some additional notes on this patch:

- createWindowlessExtensionPage is supposed to be moved out of DevtoolsPage and shared with ext-backgroundPage.js (I currently defined it in DevtoolsPage only temporarely, mostly to keep the change on the other parts of the internals as small as possible during the feedback phase), it is similar of the one attached to Bug 1291737, slightly changed to make it more "webext-oop" compatible

- DevToolsPage's buildForToolbox initialize the newly created context using the same approach used for the background page in Bug 1287007 patches, but:
  - it sends an additional "devtoolsToolboxInfo: {inspectedWindowTabId: getTabIdForToolbox(toolbox)}" property in the "Extension:InitExtensionView" initialization event
  - it uses a waitForContext promise which listen to the creation of new proxy contexts related to the browser element that has been created for the new extension contexts and resolves when the "devtools_page root context" has been created (but it continues to listen to more contexts created in the same browser element, to be able to associate the toolbox to proxy contexts related to sub-frames of the "devtools_page root context")
Attachment #8800756 - Flags: feedback?(rob)
This patch contains the implementation of the devtools.inspectedWindow API, but splitted into a parent and a child parts, which should better fit in the webext-oop changes.

- the child part of the API implementation contains only the inspectedWindow.tabId getter, it retrives the tabId:
  - directly from the one received during the initialization, if the context is a top level devtools_page context
  - by lazily retrieving (and caching) the tabId from the parent process using a messageManager.sendSyncMessage, the first time it will be requested in a sub-context of the top-level devtools_page context

- the parent part of the API implementation contains the inspectedWindow.eval implementation (the inspectedWindow.reload is not currently included to keep the patch short during this feedback round)

- the parent part of the API implementation currently subscribes a listener to message manager associated to the proxy context to lazily provide the inspectedWindow tabId to the sub-context of the top-level devtools_page context (I'm a bit concerned about this part because the message manager is currently accessible as context.currentMessageManager, which seems to suggest that the message manager can change during the lifetime of the context, and so I'm evaluating alternatives to be able to achieve this without using it)
Attachment #8800769 - Flags: feedback?(rob)
This patch provides the implementation of the devtools.panels.create method and of the related devtools_panel context.

This patch is probably one of the most interesting ones, because it required more changes to fit in the "webext-oop" architecture.

In particular:

- the parent part of the API implementation there is:
  - ParentDevtoolsPanel, which represents the devtools_panel in the parent process (it is responsible of add a panel to the devtools toolbox, add a panel to the devtools toolbox, create the browser element that will load the devtools_panel extension page)
  - the part of devtools.panels.create that needs to be executed in the main process (e.g. create the ParentDevtoolsPanel instance and return to the child part the panel id)

- in the child part of the API implementation there is:
  - ChildDevtoolsPanel, which represents the devtools_panel in the child process (e.g. it is responsible of providing the API object returned to the caller context, it identifies the contentWindow of the panel once created to be able to pass it as parameter in the onShown event listeners, it receives messages from ParentDevtoolsPanel when the panel is shown or hidden)
- the part of the devtools.panels.create that needs to be executed in the child process (e.g. create the ClientDevtoolsPanel instance and pass it to the caller context)

To be able to identify the contentWindow of the newly created panel:
- the parent part sends the panel id to the child part when the top-level context of the devtools_panel is initialized
- the parent part of devtools.panel.create returns the id of the panel to the child part of the same method
- in the child process the panel id is used by ChildDevtoolsPanel to identify the contentWindow of the panel by searching its related context in the extensions.devtoolsView set.
Attachment #8800782 - Flags: feedback?(rob)
This patch contains a workaround for the same issue described in Bug 1075490:

when a devtools toolbox contains a "browser elements with type content", the toolbox docking/undocking feature doesn't work correctly anymore, because of an exception raised by swapFrameLoader in such conditions.

The workaround is similar to the one applied for Bug 1075490 to the SDK "dev/panel" module:

- set on the browser element the "type" to "chrome" and  the "forcemessagemanager" attribute to true (which makes the messageManager available on a browser element even when its type is chrome instead of content.

Unfortunately, once I set "type" to "chrome" on the browser element of the devtools_panel (the devtools_page is not injected into the toolbox and doesn't have this issue), I had to change `getAPILevelForWindow` so that it recognizes the API level of the devtools_panel contexts correctly (more or less as we currently do for the options_ui pages embedded in the "about:addons" page)
Attachment #8800796 - Flags: feedback?(kmaglione+bmo)
Assignee: nobody → lgreco
Depends on: 1287007
Whiteboard: [devtools][devtools.inspectedWindow][devtools.panels] triaged
Comment on attachment 8800742 [details] [diff] [review]
part1__adapt_webext_oop_internals_for_devtools_contexts_and_APIs_.patch

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

::: toolkit/components/extensions/ExtensionChild.jsm
@@ +78,5 @@
> +
> +  generateAPIs(...args) {
> +    if (!this.initialized) {
> +      this.initialized = true;
> +      for (let [/* name */, value] of XPCOMUtils.enumerateCategoryEntries(CATEGORY_EXTENSION_SCRIPTS_ADDON)) {

Most of the APIs in this category are unavailable to devtools, and maybe you'll need to add some devtools-specific APIs that do not fit in the above category.

Add a new category (e.g. webextensions-scripts-devtools) and use that instead.

@@ +236,5 @@
> +    if (envType == "devtools_child") {
> +      this.extension.devtoolsViews.add(this);
> +    } else {
> +      this.extension.views.add(this);
> +    }

Can you generalize the class?
- Rename ExtensionContext to ChildContext
- Add new parameters envType and parentEnvType.
- Add a new class ExtensionContext extends ChildContext, put the above (existing) `viewType == "background"` logic in the constructor
- Add a new class DevtoolsExtensionContext
- Put this.extension.*.add(this) in the constructor of the subclass, and the .delete in the overridden unload method.

(feel free to come up with other names for these classes)

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +705,5 @@
>  
>    // Only used in addon processes.
>    this.views = new Set();
>  
> +  // Only used for devtools views in addon processes.

s/addon processes/devtools processes/ ?
Attachment #8800742 - Flags: feedback?(rob) → feedback+
Attachment #8800746 - Flags: feedback?(rob) → feedback+
Comment on attachment 8800756 [details] [diff] [review]
webext-oop-version-of-Bug_1291737___Implements_the_devtools_page_context_.patch

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

::: browser/components/extensions/ext-devtools.js
@@ +25,5 @@
> +
> +const DevtoolsContextsManager = {
> +  contextDataMap: new Map(),
> +  addContext(context, toolbox) {
> +    if (this.contextDataMap.has(context)) {

context has many meanings, can you add a JSDoc to disambiguate?
Possible candidates include: BaseContext, ProxyContext, ExtensionChildProxyContext

@@ +64,5 @@
> +    if (contextData.contextToolboxTarget) {
> +      return Promise.resolve(contextData.contextToolboxTarget);
> +    }
> +
> +    return Task.spawn(function* asyncConnectLocalTabTartget() {

Tartget -> Target

@@ +65,5 @@
> +      return Promise.resolve(contextData.contextToolboxTarget);
> +    }
> +
> +    return Task.spawn(function* asyncConnectLocalTabTartget() {
> +      contextData.contextToolboxTarget = new TabTarget(contextData.target.tab);

If the item is not in the map above, then contextData is {}. Then contextData.target is undefined so this line will throw.

@@ +95,5 @@
> + * registers a devtools_page and the user opens 3 developer toolbox in 3 webpages,
> + * 3 devtools_page contexts will be created for that add-on).
> + *
> + *  @param {Extension} extension  The extension that owns the devtools_page.
> + *  @param {string}    url        The relative path to the devtools page html page.

Relative to the extension's base URL, I presume?

@@ +106,5 @@
> +    // Map[Devtools target -> {webNav, windowlessBrowser}]
> +    this.devtoolsPageChromeForTarget = new Map();
> +  }
> +
> +  createWindowlessExtensionPage(contextType) {

This looks very similar to the logic in ext-backgroundPage.js.
Any reason for copying instead of reusing?

@@ +140,5 @@
> +
> +      const browser = chromeDoc.createElement("browser");
> +      browser.setAttribute("type", "content");
> +      browser.setAttribute("disableglobalhistory", "true");
> +      browser.setAttribute("webextension-view-type", contextType);

This attribute has been removed in bug 1287007.
The type is now set via IPC as follows: https://reviewboard.mozilla.org/r/77082/diff/9

@@ +172,5 @@
> +      let topLevelContextInitialized = false;
> +
> +      const waitForContext = new Promise(resolve => {
> +        let listener = (event, context) => {
> +          if (context.viewType == "devtools_page" && context.xulBrowser == browser) {

Is it ever possible for the devtools browser ("browser" variable) to become invalid due to a <browser> swap?
I vaguely recall references to browser swaps when I once looked through devtools code.

xulBrowser may change over time due to browser swaps, browser-swapping awareness was added in https://reviewboard.mozilla.org/r/78662/diff/6

::: browser/components/extensions/test/browser/browser_ext_devtools_page.js
@@ +16,5 @@
> +
> +add_task(function* test_devtools_page_runtime_api_messaging() {
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/");
> +
> +  function background_page() {

Make this `background`, then you can simply use `background,` in the `loadExtension` call. This is what we do in most other tests.

@@ +30,5 @@
> +  }
> +
> +  function devtools_page() {
> +    const port = browser.runtime.connect();
> +    port.postMessage("devtools -> background port message");

Can you add a `port.onDisconnect` listener that calls `browser.test.notifyPass` (and change the previous `notifyPass` to `sendMessage`)? This gives confidence that messages can be returned in the other direction and that the test API works in the devtools page.
Attachment #8800756 - Flags: feedback?(rob) → feedback+
Comment on attachment 8800769 [details] [diff] [review]
webext-oop-version-of-Bug_1291737_and_1300584___Implement_devtools_inspectedWindow_API_.patch

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

::: browser/components/extensions/ext-c-devtools-inspectedWindow.js
@@ +9,5 @@
> +} = ExtensionUtils;
> +
> +/**
> + * This module provides the implementation of the devtools.inspectedWindow namespace
> + * executed in the addon process.

Addon process? Really? Isn't it the "devtools process"?

This whole comment can probably be removed, because the function signature provides exactly the same information.

@@ +12,5 @@
> + * This module provides the implementation of the devtools.inspectedWindow namespace
> + * executed in the addon process.
> + */
> +
> +extensions.registerSchemaAPI("devtools.inspectedWindow", "devtools_child", (context) => {

Nit: Remove parentheses around context.

::: browser/components/extensions/ext-devtools-inspectedWindow.js
@@ +11,5 @@
> +} = ExtensionUtils;
> +
> +/**
> + * This module provides the implementation of the devtools.inspectedWindow namespace.
> + */

Similarly, remove this comment because the arguments to registerSchemaAPI tells exactly this.

@@ +13,5 @@
> +/**
> + * This module provides the implementation of the devtools.inspectedWindow namespace.
> + */
> +
> +extensions.registerSchemaAPI("devtools.inspectedWindow", "devtools_parent", (context) => {

And remove parentheses around context.

@@ +20,5 @@
> +  } = require("devtools/shared/fronts/webextension-devtools-api");
> +
> +  let inspectedWindowFront;
> +
> +  const mm = context.currentMessageManager;

Note: This logic falls apart when a browser swap occurs. Unless you are certain that it is not going to happen, it may be a good idea to add a comment about it.
For more context, see the next two separate examples of failures caused by not tracking browser swaps: https://reviewboard.mozilla.org/r/78662/diff/6#field_description and bug 1308421.

An actual fix is to track browser swaps and move over some message listeners, maybe by adding an event to onBrowserChange, or by using the BrowserDocShellFollower that is currently stored in Extension.jsm (maybe put it in ExtensionUtils.jsm?)

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js
@@ +16,5 @@
> +
> +add_task(function* test_devtools_inspectedWindow_tabId() {
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/");
> +
> +  function background_page() {

s/background_page/background/ (previously I explained why).

@@ +28,5 @@
> +  function devtools_page() {
> +    // Test that the available APIs are correctly restricted in devtools pages.
> +    browser.test.assertEq(undefined, browser.runtime.getBackgroundPage,
> +      "The `runtime.getBackgroundPage` API method should be missing in a devtools_page context"
> +    );

To avoid accidentally exposing (or forgetting to expose) APIs, add a new <script> block at the line after test_ext_all_apis.html(*) that creates a devtools page and tests whether the expected APIs are present. Writing this test has helped with identifying bugs before, see bug 1296900.

(*) at <script src="test_ext_all_apis.js"> in http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/components/extensions/test/mochitest/test_ext_all_apis.html#73

@@ +102,5 @@
> +        .then(([evalResult, errorResult]) => {
> +          browser.test.sendMessage("inspectedWindow-eval-promised-result", {
> +            evalResult, errorResult,
> +          });
> +        });

To prevent silent test timeouts, how about the following:

}, e => {
  browser.test.notifyFail(`Unexpected error in test: ${e.message}`);
});
Attachment #8800769 - Flags: feedback?(rob) → feedback+
Comment on attachment 8800782 [details] [diff] [review]
webext-oop-version-of-Bug_1300587___Implements_devtools_panel_context_and_devtools_panel_create_API_method_.patch

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

::: browser/components/extensions/ext-c-devtools-panels.js
@@ +6,5 @@
> +
> +const {EventEmitter} = Cu.import("resource://devtools/shared/event-emitter.js", {});
> +
> +class ChildDevtoolsPanel extends EventEmitter {
> +  constructor(context, {id}) {

Please add JSDoc that specifies the type. E.g. context.messageManager is only defined after setContentWindow is called, which is certainly not going to happen in the parent process (for the lack of a DOMWindow; the message manager is then stored in context.currentMessageManager).

@@ +16,5 @@
> +    this.id = id;
> +
> +    this.mm = context.messageManager;
> +    this.mm.addMessageListener("Extension:DevtoolsPanelShown", this);
> +    this.mm.addMessageListener("Extension:DevtoolsPanelHidden", this);

This implementation can be simplified by using an internal proxied event.

In the implementation of onShown:
listener = (id) => {
   // check if ID matches and fire the onShown event
};
context.childManager.getParentEvent("devtools.panels.onShownInternal").addListener(listener);
// Note: The event is automatically unregistered when the context closes.

In the parent, create a SingletonEventManager as usual (as a property of the panels API) and fire it when needed.

That reduces a lot of complexity involving panel shown/hidden event propagation.

For an example of using proxied events and internal methods, see https://reviewboard.mozilla.org/r/78668/diff/7#field_description
contextMenus.onClicked is internally used to implement the "onclick" property of contextMenus.create/update.
createInternal is an internal method of contextMenus (see context_menus.json in particular).

@@ +76,5 @@
> +  }
> +
> +  close() {
> +    this._panelContext = null;
> +    this.creatorContext = null;

If you decide against using proxied events as I suggested, then you must unregister the Extension:DevtoolsPanel* message listeners here.

(close can be called before the devtools is closed if the extension is unloaded).

@@ +82,5 @@
> +}
> +
> +/**
> + * This module provides the implementation of the devtools.panels namespace.
> + */

Remove useless comment (it's obvious from the call to registerSchemaAPI).

@@ +84,5 @@
> +/**
> + * This module provides the implementation of the devtools.panels namespace.
> + */
> +
> +extensions.registerSchemaAPI("devtools.panels", "devtools_child", (context) => {

Remove parentheses around context.

::: browser/components/extensions/ext-devtools-panels.js
@@ +12,5 @@
> +
> +/* global DevtoolsContextsManager, getTabIdForToolbox */
> +
> +class ParentDevtoolsPanel {
> +  constructor(context, panelOptions) {

Again, please add JSDoc with param types.

@@ +36,5 @@
> +
> +    if (this.toolbox.isReady) {
> +      this.addPanel();
> +    } else {
> +      this.toolbox.once("ready", () => {

What if context is closed before toolbox is ready?

@@ +81,5 @@
> +    browser.setAttribute("disableglobalhistory", "true");
> +    browser.setAttribute("style", "width: 100%; height: 100%;");
> +    browser.setAttribute("transparent", "true");
> +    browser.setAttribute("class", "webextension-devtoolsPanel-browser");
> +    browser.setAttribute("webextension-view-type", "devtools_panel");

Remove this line. It has been replaced by the Extension:InitExtensionView message.

@@ +108,5 @@
> +              },
> +            });
> +          }
> +
> +

Too many newlines?

@@ +112,5 @@
> +
> +          resolve(context);
> +        }
> +      };
> +      extension.on("extension-proxy-context-load", listener);

What if creatorContext is closed soon? Wouldn't you get a stale listener here?

@@ +132,5 @@
> +    return browser;
> +  }
> +
> +  get creatorContextMM() {
> +    return this.creatorContext.xulBrowser.messageManager;

Use this.creatorContext.currentMessageManager (it has the same value).

@@ +166,5 @@
> +}
> +
> +/**
> + * This module provides the implementation of the devtools.panels namespace.
> + */

Remove useless comment.

@@ +168,5 @@
> +/**
> + * This module provides the implementation of the devtools.panels namespace.
> + */
> +
> +extensions.registerSchemaAPI("devtools.panels", "devtools_parent", (context) => {

Remove parentheses around context.

@@ +192,5 @@
> +
> +          const devtoolsPanel = new ParentDevtoolsPanel(context, {
> +            title, icon, url, id,
> +          });
> +          context.callOnClose(devtoolsPanel);

This is not needed, ParentDevtoolsPanel already calls callOnClose with the same parameters.

::: browser/components/extensions/extensions-browser.manifest
@@ +16,5 @@
>  # scripts that must run in the same process as addon code.
>  category webextension-scripts-addon browserAction chrome://browser/content/ext-c-browserAction.js
>  category webextension-scripts-addon contextMenus chrome://browser/content/ext-c-contextMenus.js
>  category webextension-scripts-addon devtools-inspectedWindow chrome://browser/content/ext-c-devtools-inspectedWindow.js
> +category webextension-scripts-addon devtools-panels chrome://browser/content/ext-c-devtools-panels.js

This should use a new category (as I suggested in one of your previous patches), e.g. "webextension-scripts-devtools", with the following heading:

# scripts that must run in the same process as devtools code

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js
@@ +26,5 @@
> +
> +    browser.devtools.panels.create(
> +      "Test Panel", "fake-icon.png", "devtools_panel.html",
> +      panel => {
> +        browser.test.sendMessage("devtools_panel_created");

Move this to the end of the function, to exclude the possibility that gDevTools.showToolbox is called before the panel.onShown/onHidden listeners are added.

@@ +29,5 @@
> +      panel => {
> +        browser.test.sendMessage("devtools_panel_created");
> +
> +        result.panelCreated++;
> +        panel.onShown.addListener(contentWindow => {

Test whether contentWindow makes sense.
Attachment #8800782 - Flags: feedback?(rob) → feedback+
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 8800742 [details] [diff] [review]
part1__adapt_webext_oop_internals_for_devtools_contexts_and_APIs_.patch

Old patch marked as obsolete (patch updated based on the feedback comments and attached as a mozreview request, as attachment 8807527 [details])
Attachment #8800742 - Attachment is obsolete: true
Comment on attachment 8800746 [details] [diff] [review]
part2__provide_existent_APIs_to_the_devtools_contexts_.patch

Old patch marked as obsolete (patch updated based on the feedback comments and attached as a mozreview request, as attachment 8807528 [details])
Attachment #8800746 - Attachment is obsolete: true
Comment on attachment 8800756 [details] [diff] [review]
webext-oop-version-of-Bug_1291737___Implements_the_devtools_page_context_.patch

Old patch marked as obsolete (because it is a modified version of the patches from Bug 1291737, and its review is going to continue on its original bugzilla issue)
Attachment #8800756 - Attachment is obsolete: true
Comment on attachment 8800769 [details] [diff] [review]
webext-oop-version-of-Bug_1291737_and_1300584___Implement_devtools_inspectedWindow_API_.patch

Old patch marked as obsolete (because it is a modified version of the patches from Bug 1300584, and its review is going to continue on its original bugzilla issue)
Attachment #8800769 - Attachment is obsolete: true
Comment on attachment 8800782 [details] [diff] [review]
webext-oop-version-of-Bug_1300587___Implements_devtools_panel_context_and_devtools_panel_create_API_method_.patch

Old patch marked as obsolete (because it is a modified version of the patches from Bug 1300587, and its review is going to continue on its original bugzilla issue)
Attachment #8800782 - Attachment is obsolete: true
Blocks: 1291737
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8807527 [details]
Bug 1309906 - part1: adapt webext-oop internals for devtools contexts and APIs.

https://reviewboard.mozilla.org/r/90686/#review90622

::: toolkit/components/extensions/ExtensionChild.jsm:249
(Diff revision 1)
>  
>      super.unload();
> +  }
> +}
> +
> +class ExtensionContext extends ChildContext {

Please rename this `ExtensionChildContext`. We have too many classes that are called `ExtensionContext`.

::: toolkit/components/extensions/ExtensionChild.jsm:310
(Diff revision 1)
> +    super.unload();
> +    this.extension.devtoolsViews.delete(this);
> +  }
> +}
> +
> +defineLazyGetter(ChildContext.prototype, "messenger", function() {

Please keep these getter definitions with the classes they belong to.
Attachment #8807527 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8807528 [details]
Bug 1309906 - part2: provide existent APIs to the devtools contexts.

https://reviewboard.mozilla.org/r/90688/#review90626

::: toolkit/components/extensions/extensions-toolkit.manifest:26
(Diff revision 1)
> +category webextension-scripts-devtools extension chrome://extensions/content/ext-c-extension.js
> +category webextension-scripts-devtools i18n chrome://extensions/content/ext-i18n.js
> +category webextension-scripts-devtools runtime chrome://extensions/content/ext-c-runtime.js
> +category webextension-scripts-devtools test chrome://extensions/content/ext-c-test.js
> +category webextension-scripts-devtools storage chrome://extensions/content/ext-c-storage.js

This means that we're going to load these scripts twice
Comment on attachment 8800796 [details] [diff] [review]
workaround_issues_with_dock_undock_devtools_toolbox_with_a_devtools_panel_.patch

Patch marked as obsolete (moved to Bug 1300587)
Attachment #8800796 - Attachment is obsolete: true
Attachment #8800796 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8807527 [details]
Bug 1309906 - part1: adapt webext-oop internals for devtools contexts and APIs.

https://reviewboard.mozilla.org/r/90686/#review90622

> Please rename this `ExtensionChildContext`. We have too many classes that are called `ExtensionContext`.

I totally agree, I renamed it to ExtensionChildContext in the updated patch.
Comment on attachment 8807528 [details]
Bug 1309906 - part2: provide existent APIs to the devtools contexts.

https://reviewboard.mozilla.org/r/90688/#review90626

> This means that we're going to load these scripts twice

Yes, the sub-set of API implementations shared between the other extension contexts and the devtools extensions contexts are currently loaded twice.

But what is really forcing us to load them twice is not the category by itself, it is the part of the first patch that creates the new `devtoolsAPIManager`, I did it this way to increase the isolation between the regular APIs and the devtools APIs, and based on the first rounds of feedback on this part of the devtools patches.

If we want to load them once I can apply the following changes:

- on the part 1 patch: remove the `devtoolsAPIManager` and the `webextension-script-devtools` category, apply a small change on the `apiManager` to make it able to load the APIs registered as "addon_child" or "devtools_child", use the `apiManager` instead of `devtoolsAPIManager` to inject the APIs in the DevtoolsExtensionContext
- on the part 2 path: remove from "extension-browser.manifest" all the lines with the `webextension-script-devtools` category (not needed anymore if we share the apiManager with the regular `ExtensionChildContext` instances)
- on the patches from Bug 1300584 and Bug 1300587: change in "extension-browser.manifest" the lines that register the API implementation files that needs to run in the child process from `webextension-script-devtools` to `webextension-script-addon`
Patches rebased on a recent mozilla-central tip and resolved conflicts with some of the changes recently landed.

Kris,
Based on brief discussion on IRC, I've cleared the issue from Comment 20, but I wanted to ask you a confirmation that the second patch is fully reviewed (it is currently marked as r?).

This patches don't currently have any dependencies that block them (it just prepare the internals for Bug 1291737) and I'm a bit worried that if we wait too much this patches are going to conflict with the ongoing refactorings on the same JSMs that are changed by this patch.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8807528 [details]
Bug 1309906 - part2: provide existent APIs to the devtools contexts.

https://reviewboard.mozilla.org/r/90688/#review92056
Attachment #8807528 - Flags: review?(kmaglione+bmo) → review+
(In reply to Luca Greco [:rpl] from comment #28)
> Based on brief discussion on IRC, I've cleared the issue from Comment 20,
> but I wanted to ask you a confirmation that the second patch is fully
> reviewed (it is currently marked as r?).

Sorry, yeah, I forgot to mark it r+ after our conversation.
Flags: needinfo?(kmaglione+bmo)
Iteration: --- → 53.1 - Nov 28
Comment on attachment 8807527 [details]
Bug 1309906 - part1: adapt webext-oop internals for devtools contexts and APIs.

https://reviewboard.mozilla.org/r/90686/#review92752

::: toolkit/components/extensions/ExtensionChild.jsm:903
(Diff revision 4)
>    }
>  
> +  return childManager;
> +});
> +
> +class DevtoolsExtensionContextChild extends ExtensionBaseContextChild {

May as well just call this `DevtoolsContextChild`
Comment on attachment 8807527 [details]
Bug 1309906 - part1: adapt webext-oop internals for devtools contexts and APIs.

https://reviewboard.mozilla.org/r/90686/#review92752

> May as well just call this `DevtoolsContextChild`

Sure, absolutely.

Renamed in the updated patch.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b789e76e68a
part1: adapt webext-oop internals for devtools contexts and APIs. r=kmag
https://hg.mozilla.org/integration/autoland/rev/9509c2c0ad47
part2: provide existent APIs to the devtools contexts. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b789e76e68a
https://hg.mozilla.org/mozilla-central/rev/9509c2c0ad47
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.