Closed
Bug 1300587
Opened 8 years ago
Closed 8 years ago
Implements devtools_panel context and devtools.panel.create API method
Categories
(WebExtensions :: Developer Tools, defect, P1)
WebExtensions
Developer Tools
Tracking
(firefox54 fixed)
People
(Reporter: rpl, Assigned: rpl)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [devtools][devtools.panels] triaged)
Attachments
(3 files)
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.
|
Assignee | |
Updated•8 years ago
|
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
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 hidden (mozreview-request) |
|
|
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
|
||
Updated•8 years ago
|
Keywords: dev-doc-needed
|
||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•8 years ago
|
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Updated•8 years ago
|
webextensions: --- → +
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•8 years ago
|
Attachment #8788247 -
Flags: review?(kmaglione+bmo)
Attachment #8788248 -
Flags: review?(kmaglione+bmo)
|
|
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`.
|
|
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 hidden (mozreview-request) |
|
||
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)
|
|
||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
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);
```
|
|
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)
|
|
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
|
|
||
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) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•8 years ago
|
Attachment #8835589 -
Flags: review?(kmaglione+bmo)
|
|
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.
|
|
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.
|
|
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)
|
|
Assignee | |
Updated•8 years ago
|
Iteration: --- → 54.2 - Feb 20
|
|
||
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+
|
|
||
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 hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•8 years ago
|
Keywords: checkin-needed
|
||
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
|
||
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
|
||
Comment 51•7 years ago
|
||
Keywords: dev-doc-needed → dev-doc-complete
|
||
Updated•7 years ago
|
Depends on: CVE-2018-5112
|
||
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•