1300587 - Implements devtools_panel context and devtools.panel.create API method

Assignee

Description

•

8 years ago

The goal of this issue is to provide an initial subset of the devtools.panel API, in particular the devtools.panel.create method (which is needed to register new panels to the toolbox associated to a devtools page instance) and the Panel's onShown/onHidden API events (which are often used to lower the cpu usage of a non visible addon devtools panel, or to exchange data between the devtools page and the devtools panel, because the onShown event will provide a reference to the devtools panel contentWindow).

In the WebExtension devtools API, the devtools page can add an addon panel to just the toolbox which is associated to the devtools page instance, without actually register the panel globally for every opened toolbox.

For the above reason, this issue is going to contain a proposal to optionally register a panel only to a particular toolbox (and to support this "toolbox"-specific tools in the preferences of the toolbox, as it is currently supported for the panels registered globally, e.g. list the panels registered by addons and enable/disable of this panels from the toolbox preferences), to better map the lifecycle describe above to the API provided by the internal devtools toolbox API.

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Blocks: 1211859

Depends on: 1291737, 1300584

Priority: -- → P1

Whiteboard: [devtools][devtools.panels] triaged

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

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

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

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

Bob Silverberg [:bsilverberg]

Comment 5

•

8 years ago

mozreview-review

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

https://reviewboard.mozilla.org/r/76820/#review77034

::: browser/components/extensions/ext-devtools-panels.js:21
(Diff revision 2)
> +
> +    this.visible = false;
> +    this.toolbox = toolbox;
> +
> +    if (!this.toolbox) {
> +      // This should happen when this constructor is called with a valid

Did you mean `without a valid` here?

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:62
(Diff revision 2)
> +       </head>
> +       <body>
> +         <script src="devtools_page.js"></script>
> +       </body>
> +      </html>`,
> +      "devtools_page.js": `new ${devtools_page}();`,

This could use the simpler syntax (i.e., without the template string and `new`).

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

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

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

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

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 8

•

8 years ago

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Marking this patch as obsolete, because it has been moved (and updated) into a separate bugzilla issue (Bug 1308912).

Attachment #8788247 - Attachment is obsolete: true

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Depends on: 1308912

Sebastian Zartner [:sebo]

Updated

•

8 years ago

Keywords: dev-doc-needed

Andrei Petcu

Comment 9

•

8 years ago

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 hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/4-5/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/4-5/

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Assignee: nobody → lgreco

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/5-6/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/5-6/

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/6-7/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/6-7/

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/7-8/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/7-8/

Andy McKay

Updated

•

8 years ago

webextensions: --- → +

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/8-9/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/8-9/

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/9-10/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/9-10/

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Attachment #8788247 - Flags: review?(kmaglione+bmo)

Attachment #8788248 - Flags: review?(kmaglione+bmo)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

No longer depends on: 1300584

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 24

•

8 years ago

I removed Bug 1300584 wrong the dependencies, this patches only uses `inspectedWindow.tabId` in the new tests, which has been already provided by Bug 1291737, and so it doesn't depend from Bug 1300584 that introduces `inspectedWindow.eval` and `inspectedWindow.reload`.

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 25

•

8 years ago

In reply to Luca Greco [:rpl] from comment #24)
> I removed Bug 1300584 wrong the dependencies
s/wrong the/from the/

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/10-11/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/10-11/

Kris Maglione [:kmag]

Comment 28

•

8 years ago

mozreview-review

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review110916

::: browser/components/extensions/ext-c-devtools-panels.js:22
(Diff revision 11)
> +    this.creatorContext = context;
> +    this.creatorContext.callOnClose(this);

s/creatorContext/context/

::: browser/components/extensions/ext-c-devtools-panels.js:33
(Diff revision 11)
> +    this.mm.addMessageListener("Extension:DevToolsPanelShown", this);
> +    this.mm.addMessageListener("Extension:DevToolsPanelHidden", this);
> +  }
> +
> +  get panelContext() {
> +    if (this._panelContext) {

Please either set this property to null in the constructor, of just use `defineLazyGetter` for this property.

::: browser/components/extensions/ext-c-devtools-panels.js:38
(Diff revision 11)
> +      if (view.viewType == "devtools_panel" &&
> +          view.devtoolsToolboxInfo.toolboxPanelId == this.id) {

s/==/&=/

::: browser/components/extensions/ext-c-devtools-panels.js:41
(Diff revision 11)
> +
> +    for (let view of this.creatorContext.extension.devtoolsViews) {
> +      if (view.viewType == "devtools_panel" &&
> +          view.devtoolsToolboxInfo.toolboxPanelId == this.id) {
> +        this._panelContext = view;
> +        break;

return view;

::: browser/components/extensions/ext-c-devtools-panels.js:45
(Diff revision 11)
> +        this._panelContext = view;
> +        break;
> +      }
> +    }
> +
> +    return this._panelContext;

No need for a return here.

::: browser/components/extensions/ext-c-devtools-panels.js:49
(Diff revision 11)
> +
> +    return this._panelContext;
> +  }
> +
> +  receiveMessage({name, data}) {
> +    if (!this.panelContext && !this.panelContext.contentWindow) {

s/&&/||/ I think.

(If I'm wrong, then something else is very wrong, because the RHS will throw if the LHS is true.)

::: browser/components/extensions/ext-c-devtools-panels.js:64
(Diff revision 11)
> +    if (panelId !== this.id) {
> +      return;
> +    }

We should probably just check this for all messages at the top of `receiveMessage`.

::: browser/components/extensions/ext-c-devtools-panels.js:74
(Diff revision 11)
> +
> +    // Ensure that the onShown event is fired when the panel document has
> +    // been fully loaded.
> +    let firedShown = false;
> +    const onPanelLoaded = () => {
> +      if (!firedShown) {

Why is this necessary? It's only possible for this to be called once.

::: browser/components/extensions/ext-c-devtools-panels.js:75
(Diff revision 11)
> +    // Ensure that the onShown event is fired when the panel document has
> +    // been fully loaded.
> +    let firedShown = false;
> +    const onPanelLoaded = () => {
> +      if (!firedShown) {
> +        this.emit("shown", this.panelContext && this.panelContext.contentWindow);

If it's possible for `panelContext` to be `null` here (is it?), then we should probably not emit "shown" at all.

::: browser/components/extensions/ext-c-devtools-panels.js:82
(Diff revision 11)
> +      }
> +    };
> +
> +    this.panelContext.contentWindow.addEventListener("load", onPanelLoaded, {once: true});
> +
> +    if (document.readyState == "complete") {

s/==/&=/g

::: browser/components/extensions/ext-c-devtools-panels.js:83
(Diff revision 11)
> +    };
> +
> +    this.panelContext.contentWindow.addEventListener("load", onPanelLoaded, {once: true});
> +
> +    if (document.readyState == "complete") {
> +      this.panelContext.contentWindow.removeEventListener("load", onPanelLoaded, {once: true});

Please only add the event listener if the ready state is not complete, rather than removing it if it is.

Also, we have a `promiseDocumentLoaded` helper in `ExtensionUtils` for this.

::: browser/components/extensions/ext-c-devtools-panels.js:98
(Diff revision 11)
> +    this.emit("hidden");
> +  }
> +
> +  api() {
> +    return {
> +      onShown: new ExtensionUtils.SingletonEventManager(

Please destructure `SingletonEventManager` from `ExtensionUtils` at the top of this script, as we do elsewhere.

::: browser/components/extensions/ext-c-devtools-panels.js:100
(Diff revision 11)
> +
> +  api() {
> +    return {
> +      onShown: new ExtensionUtils.SingletonEventManager(
> +        this.creatorContext, "devtoolsPanel.onShown", fire => {
> +          const listener = (what, panelContentWindow) => {

s/what/event/ or eventName, please.

::: browser/components/extensions/ext-c-devtools-panels.js:112
(Diff revision 11)
> +        }).api(),
> +
> +      onHidden: new ExtensionUtils.SingletonEventManager(
> +        this.creatorContext, "devtoolsPanel.onHidden", fire => {
> +          const listener = (what) => {
> +            fire.asyncWithoutClone();

s/asyncWithoutClone/async/ please. `asyncWithoutClone` should only be used when we're passing arguments that can't be cloned.

::: browser/components/extensions/ext-c-devtools-panels.js:137
(Diff revision 11)
> +
> +extensions.registerSchemaAPI("devtools.panels", "devtools_child", context => {
> +  return {
> +    devtools: {
> +      panels: {
> +        create(title, icon, url) {

Please use an async function for this.

::: browser/components/extensions/ext-c-devtools-panels.js:138
(Diff revision 11)
> +extensions.registerSchemaAPI("devtools.panels", "devtools_child", context => {
> +  return {
> +    devtools: {
> +      panels: {
> +        create(title, icon, url) {
> +          return context.childManager.callParentAsyncFunction(

`let panelId = await ...`

::: browser/components/extensions/ext-devtools-panels.js:16
(Diff revision 11)
> +
> +const {
> +  watchExtensionProxyContextLoad,
> +} = ExtensionParent;
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");

Please group this with the `ExtensionParent` import above.

::: browser/components/extensions/ext-devtools-panels.js:28
(Diff revision 11)
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "gDevTools",
> +                                  "resource://devtools/client/framework/gDevTools.jsm");
> +
> +/* global getTargetTabIdForToolbox */

This should be defined in `.eslintrc.js`

::: browser/components/extensions/ext-devtools-panels.js:33
(Diff revision 11)
> +/* global getTargetTabIdForToolbox */
> +
> +/**
> + * Represents an addon devtools panel in the main process.
> + *
> + * @param {ExtensionChildProxyContext}

Missing parameter name. ESLint shouldn't even allow this.

::: browser/components/extensions/ext-devtools-panels.js:34
(Diff revision 11)
> +
> +/**
> + * Represents an addon devtools panel in the main process.
> + *
> + * @param {ExtensionChildProxyContext}
> + *   A devtools extension proxy context running in a main process.

Please align descriptions with the opening `{` of the type.

::: browser/components/extensions/ext-devtools-panels.js:35
(Diff revision 11)
> +/**
> + * Represents an addon devtools panel in the main process.
> + *
> + * @param {ExtensionChildProxyContext}
> + *   A devtools extension proxy context running in a main process.
> + * @param {object} panelOptions

s/panelOptions/options/

::: browser/components/extensions/ext-devtools-panels.js:48
(Diff revision 11)
> + *   The url of the addon devtools panel, relative to the extension base URL.
> + */
> +class ParentDevToolsPanel {
> +  constructor(context, panelOptions) {
> +    const toolbox = context.devToolsToolbox;
> +

Nit: Remove empty line.

::: browser/components/extensions/ext-devtools-panels.js:61
(Diff revision 11)
> +    this.viewType = "devtools_panel";
> +
> +    this.visible = false;
> +    this.toolbox = toolbox;
> +
> +    this.creatorContext = context;

s/creatorContext/context/

::: browser/components/extensions/ext-devtools-panels.js:71
(Diff revision 11)
> +    this.id = this.panelOptions.id;
> +
> +    this.onToolboxPanelSelect = this.onToolboxPanelSelect.bind(this);
> +    this.onToolboxReady = this.onToolboxReady.bind(this);
> +
> +    this.isPanelAdded = false;

s/isP/p/

::: browser/components/extensions/ext-devtools-panels.js:76
(Diff revision 11)
> +    this.isPanelAdded = false;
> +
> +    this.toolbox.once("ready", this.onToolboxReady);
> +
> +    if (this.toolbox.isReady) {
> +      this.toolbox.off("ready", this.onToolboxReady);

Again, please only add the listener when necessary, rather than removing it when not.

::: browser/components/extensions/ext-devtools-panels.js:80
(Diff revision 11)
> +    if (this.toolbox.isReady) {
> +      this.toolbox.off("ready", this.onToolboxReady);
> +      this.onToolboxReady();
> +    }
> +
> +    this.unwatchExtensionProxyContextLoad = null;

Please just use `context.callOnClose`, or add an array of cleanup functions, for this kind of thing.

::: browser/components/extensions/ext-devtools-panels.js:140
(Diff revision 11)
> +      browser.setAttribute("remote", "true");
> +      browser.setAttribute("remoteType", E10SUtils.EXTENSION_REMOTE_TYPE);
> +      awaitFrameLoader = promiseEvent(browser, "XULFrameLoaderCreated");
> +    }
> +
> +    let topLevelContextInitialized = false;

Less verbose variable names, please.

::: browser/components/extensions/ext-devtools-panels.js:168
(Diff revision 11)
> +  get creatorContextMM() {
> +    return this.creatorContext.parentMessageManager;
> +  }

Please just use `this.context.parentMessageManager` directly.

::: browser/components/extensions/ext-devtools-panels.js:243
(Diff revision 11)
> +          }
> +
> +          icon = context.extension.baseURI.resolve(icon);
> +          url = context.extension.baseURI.resolve(url);
> +
> +          const baseId = context.extension.id + "-" + context.contextId + "-" + nextPanelId++;

Template string, please.

::: browser/components/extensions/ext-devtools-panels.js:244
(Diff revision 11)
> +
> +          icon = context.extension.baseURI.resolve(icon);
> +          url = context.extension.baseURI.resolve(url);
> +
> +          const baseId = context.extension.id + "-" + context.contextId + "-" + nextPanelId++;
> +          const id = makeWidgetId(baseId) + "-devtools-panel";

Template string please.

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:5
(Diff revision 11)
> +XPCOMUtils.defineLazyModuleGetter(this, "gDevTools",
> +                                  "resource://devtools/client/framework/gDevTools.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "devtools",
> +                                  "resource://devtools/shared/Loader.jsm");

Please keep sorted.

Attachment #8788247 - Flags: review?(kmaglione+bmo)

Kris Maglione [:kmag]

Comment 29

•

8 years ago

mozreview-review

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

https://reviewboard.mozilla.org/r/76820/#review110918

::: browser/components/extensions/ext-devtools-panels.js:141
(Diff revision 11)
>        browser.setAttribute("remoteType", E10SUtils.EXTENSION_REMOTE_TYPE);
>        awaitFrameLoader = promiseEvent(browser, "XULFrameLoaderCreated");
> +    } else {
> +      // NOTE: Using a content iframe here breaks the devtools panel
> +      // switching between docked and undocked mode,
> +      // because of a swapFrameLoader exception (see Bug 1075490).

s/Bug/bug/

::: browser/components/extensions/ext-devtools-panels.js:143
(Diff revision 11)
> +    } else {
> +      // NOTE: Using a content iframe here breaks the devtools panel
> +      // switching between docked and undocked mode,
> +      // because of a swapFrameLoader exception (see Bug 1075490).
> +      browser.setAttribute("type", "chrome");
> +      browser.setAttribute("forcemessagemanager", true);

Hm... This worries me, but I guess I can live with it for now, as long as it's behind a `!RELEASE_OR_BETA` flag.

::: toolkit/components/extensions/ExtensionManagement.jsm:293
(Diff revision 11)
>        return FULL_PRIVILEGES;
>      }
>  
> +    // NOTE: Special handling for devtools panels using a chrome iframe here
> +    // for the devtools panel, it is needed because a content iframe breaks
> +    // switching between docked and undocked mode (see Bug 1075490).

s/Bug/bug/

::: toolkit/components/extensions/ExtensionManagement.jsm:296
(Diff revision 11)
> +    // NOTE: Special handling for devtools panels using a chrome iframe here
> +    // for the devtools panel, it is needed because a content iframe breaks
> +    // switching between docked and undocked mode (see Bug 1075490).
> +    let devtoolsBrowser = parentDocument.querySelector(
> +      "browser[webextension-view-type='devtools_panel']"
> +    );

Nit: Please move to previous line.

::: toolkit/components/extensions/ExtensionManagement.jsm:297
(Diff revision 11)
> +    // for the devtools panel, it is needed because a content iframe breaks
> +    // switching between docked and undocked mode (see Bug 1075490).
> +    let devtoolsBrowser = parentDocument.querySelector(
> +      "browser[webextension-view-type='devtools_panel']"
> +    );
> +    if (devtoolsBrowser && devtoolsBrowser.contentWindow == window &&

s/==/===/

Attachment #8788248 - Flags: review?(kmaglione+bmo) → review+

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/11-12/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/11-12/

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/12-13/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/12-13/

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 34

•

8 years ago

mozreview-review-reply

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review110916

> Please use an async function for this.

I've not applied this change yet because there is the following issue when I turn `devtools.panels.create` into an "ES7 async function":

In the current version the returned promise is recognized as an instance of the context.cloneScope.Promise (here: http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/toolkit/components/extensions/ExtensionCommon.jsm#298-300) and runSafeWithoutClone is going to be used (needed to return the panel API object to the caller context).

Once I turn the method into an async function (even without changing the API method implementation), the returned promise is not detected as an instance of the context.cloneScope.Promise anymore, and runSafe is used instead of runSafeWithoutClone (which prevents the panel API object to work correctly when the caller context tries to use its methods).

I took a brief look at it and it seems that it could work, e.g. if we decide to handle the "async functions" differntly and use the runSafeWithoutClone on their resolved value inside the wrapPromise utility method (but I'm not sure if it is the general case and so I decided to defer any change related to this after a discussion with you about this issue), e.g.:

```
  const apiMethod = this.pathObj[this.name];
  const isAsyncFunction = typeof(apiMethod) == "function" &&
    apiMethod.constructor.name == "AsyncFunction";
  
  ...
  return this.context.wrapPromise(promise, callback, isAsyncFunction);
```

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 35

•

8 years ago

mozreview-review-reply

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review110916

> Please just use `context.callOnClose`, or add an array of cleanup functions, for this kind of thing.

This class already uses the context.callOnClose to cleanup itself when the caller context is closed.

This part is related to the "watch for the created proxy context on this browser element and set the associated toolbox to them" 
(which is needed for the top level context as well as for any of its subframes pointed to an extension url), and it currently follows the same
"pattern" we are using in the devtools_page implementation (e.g. here: http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/browser/components/extensions/ext-devtools.js#114,126,169-173)

I added the same inline comments that we have on the devtools_page counterpart (so that it is clear what is the goal of these parts).

Let me know if there is anything unclear or confusing about it 
(but I'd like to find a solution that keeps both the devtools_panel and the devtools_page consistent on handling this)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 36

•

8 years ago

mozreview-review-reply

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

https://reviewboard.mozilla.org/r/76820/#review110918

> Hm... This worries me, but I guess I can live with it for now, as long as it's behind a `!RELEASE_OR_BETA` flag.

yeah, this worries me too.

Anyway, in the updated version is now behind the `!RELEASE_OR_BETA` check, but I'm not yet clearing this issue because it basically means that it is going to be broken on release and beta and "workarounded" on nightly (and aurora), and I'm not sure that  it makes sense. 
(what I mean is that if it is broken for a regular user, probably it doesn't make sense to include a workaround on nightly with a solution that is never going to be enabled for the regular users) 

I'm not sure if the referenced Bug 1075490 summary and comments are explaining what it is going to happen from a user perspective:

- a user installs an webextension that defines a new devtools panel
- it open the developer toolbox, and then it changes the "docking position" of the developer toolbox 
  (e.g. from bottom to right side)
- the new developer toolbox created in the new dock position becomes an empty panel without anything rendered inside it, and the developer toolbox becomes irreversibly broken (as the tab, which is visually broken by the unexpected empty XUL rendered inside it)
- there is no way for the user to fix the XUL rendered in the tab where the developer toolbox is broken, the only solution seems to close the tab completely

Kris Maglione [:kmag]

Comment 37

•

8 years ago

mozreview-review-reply

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review110916

> I've not applied this change yet because there is the following issue when I turn `devtools.panels.create` into an "ES7 async function":
> 
> In the current version the returned promise is recognized as an instance of the context.cloneScope.Promise (here: http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/toolkit/components/extensions/ExtensionCommon.jsm#298-300) and runSafeWithoutClone is going to be used (needed to return the panel API object to the caller context).
> 
> Once I turn the method into an async function (even without changing the API method implementation), the returned promise is not detected as an instance of the context.cloneScope.Promise anymore, and runSafe is used instead of runSafeWithoutClone (which prevents the panel API object to work correctly when the caller context tries to use its methods).
> 
> I took a brief look at it and it seems that it could work, e.g. if we decide to handle the "async functions" differntly and use the runSafeWithoutClone on their resolved value inside the wrapPromise utility method (but I'm not sure if it is the general case and so I decided to defer any change related to this after a discussion with you about this issue), e.g.:
> 
> ```
>   const apiMethod = this.pathObj[this.name];
>   const isAsyncFunction = typeof(apiMethod) == "function" &&
>     apiMethod.constructor.name == "AsyncFunction";
>   
>   ...
>   return this.context.wrapPromise(promise, callback, isAsyncFunction);
> ```

OK. That behavior isn't obvious. Please create the promise for the target scope explicitly and use an async function for the logic. Something like this should do:

    return context.cloneScope.Promise.resolve().then(async () => {
      ...
    });

> This class already uses the context.callOnClose to cleanup itself when the caller context is closed.
> 
> This part is related to the "watch for the created proxy context on this browser element and set the associated toolbox to them" 
> (which is needed for the top level context as well as for any of its subframes pointed to an extension url), and it currently follows the same
> "pattern" we are using in the devtools_page implementation (e.g. here: http://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/browser/components/extensions/ext-devtools.js#114,126,169-173)
> 
> I added the same inline comments that we have on the devtools_page counterpart (so that it is clear what is the goal of these parts).
> 
> Let me know if there is anything unclear or confusing about it 
> (but I'd like to find a solution that keeps both the devtools_panel and the devtools_page consistent on handling this)

It doesn't matter. callOnClose is cheap, and handling this cleanup that way is much simpler than storing a cleanup function in a special-purpose property like this.

Comment hidden (mozreview-request)

- a DevToolsExtensionPageContextParent instance can be missing a devToolsTarget
  property when it is destroyed (because the devToolsTarget property is set lazily
  if the extension page is using any API that actually needs a TabTarget instance)

- the DevToolsExtensionPageContextParent's shutdown method should
  call super.shutdown to complete its cleanup.

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

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/13-14/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/13-14/

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Attachment #8835589 - Flags: review?(kmaglione+bmo)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 41

•

8 years ago

mozreview-review-reply

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review110916

> OK. That behavior isn't obvious. Please create the promise for the target scope explicitly and use an async function for the logic. Something like this should do:
> 
>     return context.cloneScope.Promise.resolve().then(async () => {
>       ...
>     });

> OK. That behavior isn't obvious.

Yep, it is exactly what I meant in my previous comment

> Please create the promise for the target scope explicitly and use an async function for the logic. Something like this should do...

Sure, it sounds great to me, patch updated as suggested.

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 42

•

8 years ago

mozreview-review-reply

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review110916

> It doesn't matter. callOnClose is cheap, and handling this cleanup that way is much simpler than storing a cleanup function in a special-purpose property like this.

> It doesn't matter. callOnClose is cheap, and handling this cleanup that way is much simpler than storing a cleanup function in a special-purpose property like this.

Oh yeah, I see what you mean, and I agree.

I looked it a bit more deeply, and the "unwatching of the created context proxy" should also stop when the panel is disabled from the toolbox preferences (when the user disable the custom panel from there, the built panel is destroyed but its definition is still valid and it is rebuilt if the user re-enable it from there)

And so, the above "unwatchExtensionProxyContextLoad" method is not stored in a property of the class anymore, but it is now part of the destroy method (that is called when the panel is destroyed).

When the creator context is destroyed, the custom panel definition is removed from the toolbox, which will call the destroy method described above.

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 43

•

8 years ago

While I was looking into the "callOnClose" issue described in Comment 42, I also noticed that the shutdown method from the `DevToolsExtensionPageContextParent` class has currently two issues:

- it raises an exception when there is no devToolsTarget on it (which is a valid scenario, the context's devToolsTarget property is set lazily if the extension page uses any devtools API that needs the TabTarget instance, e.g. devtool.inspectedWindow.eval, devtools.inspectedWindow.reload or devtools.network.onNavigated, but it will be missing if the extension page only uses devtools.panel.create, that only needs the toolbox associated to the context that is creating the panel)

- it should also call super.shutdown to complete its cleanup

I've added the changes needed to fix both the above issue in a new patch attached to this issue (https://reviewboard.mozilla.org/r/111278/diff/#index_header)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Iteration: --- → 54.2 - Feb 20

Kris Maglione [:kmag]

Comment 44

•

8 years ago

mozreview-review

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

https://reviewboard.mozilla.org/r/76818/#review114264

::: browser/components/extensions/ext-devtools-panels.js:72
(Diff revision 14)
> +      this.onToolboxReady();
> +    } else {
> +      this.toolbox.once("ready", this.onToolboxReady);
> +    }
> +
> +    this.waitForTopLevelContext = new Promise(resolve => {

s/waitForTopLevelContext/topLevelContextPromise/

`waitFor` implies a function.

::: browser/components/extensions/ext-devtools-panels.js:73
(Diff revision 14)
> +    } else {
> +      this.toolbox.once("ready", this.onToolboxReady);
> +    }
> +
> +    this.waitForTopLevelContext = new Promise(resolve => {
> +      this.resolveTopLevelContext = resolve;

`this._resolve...`

Attachment #8788247 - Flags: review?(kmaglione+bmo) → review+

Kris Maglione [:kmag]

Comment 45

•

8 years ago

mozreview-review

Comment on attachment 8835589 [details]
Bug 1300587 - Fix DevToolsExtensionPageContextParent shutdown method.

https://reviewboard.mozilla.org/r/111278/#review114266

Attachment #8835589 - Flags: review?(kmaglione+bmo) → review+

Comment hidden (mozreview-request)

Comment on attachment 8835589 [details]
Bug 1300587 - Fix DevToolsExtensionPageContextParent shutdown method.

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

Comment hidden (mozreview-request)

Comment on attachment 8788247 [details]
Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76818/diff/14-15/

Comment hidden (mozreview-request)

Comment on attachment 8788248 [details]
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76820/diff/14-15/

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Keywords: checkin-needed

Pulsebot

Comment 49

•

8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/10293eb8e332
Fix DevToolsExtensionPageContextParent shutdown method. r=kmag
https://hg.mozilla.org/integration/autoland/rev/de8052439ef2
Implements devtools_panel context and devtools.panel.create API method. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6d4af96450ed
Workaround issues with dock/undock devtools toolbox with a devtools_panel. r=kmag

Keywords: checkin-needed

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 50

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/10293eb8e332
https://hg.mozilla.org/mozilla-central/rev/de8052439ef2
https://hg.mozilla.org/mozilla-central/rev/6d4af96450ed

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox54: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla54

Wladimir Palant

Updated

•

7 years ago

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1366755

Wladimir Palant

Updated

•

7 years ago

See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1366752

Will Bamberg [:wbamberg]

Comment 51

•

7 years ago

-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels

Keywords: dev-doc-needed → dev-doc-complete

Daniel Veditz [:dveditz]

Updated

•

7 years ago

Depends on: CVE-2018-5112

BMO Automation

Updated

•

6 years ago

Product: Toolkit → WebExtensions

Bug 1300587 - Implements devtools_panel context and devtools.panel.create API method. 8 years ago Luca Greco [:rpl] [:luca] [:lgreco] (deleted), text/x-review-board-request	kmag : review+	Details
Bug 1300587 - Workaround issues with dock/undock devtools toolbox with a devtools_panel. 8 years ago Luca Greco [:rpl] [:luca] [:lgreco] (deleted), text/x-review-board-request	kmag : review+	Details
Bug 1300587 - Fix DevToolsExtensionPageContextParent shutdown method. 8 years ago Luca Greco [:rpl] [:luca] [:lgreco] (deleted), text/x-review-board-request	kmag : review+	Details