The goal of this issue is: - importing the set devtools API schema files and create the related set of ext-devtools*.js API implementation files. - implementing the first DevTools WebExtension context, the devtools_page - implementing the devtools.inspectedWindow.tabId getter, as the first API method implemented The above is the minimal amount of changes needed to get a new testable step, which ensures that: - the devtools_page context is created correctly when a developer toolbox has been opened in a tab - the devtools API are available only in devtools contexts - the devtools contexts doesn't have access to all the APIs available in the full privileged contexts (e.g. a background page) - the value returned by devtools.inspectedWindow.tabId is the same that we get from the tabs API in other WebExtensions context.

This patch import the DevTools API JSON schema files, creates the related API implementation files and implement the devtools_page context and the devtools.inspectedWindow.tabId getter. Review commit: https://reviewboard.mozilla.org/r/68966/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68966/

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. The patch attached to this bugzilla issue introduces an very minimal initial subset of DevTools APIs (the minimum needed to be able to put together a reasonable test case). The patch introduces the following new schema files (that needs the changes that will be introduced by Bug 1290901): - "devtools.json" - this schema file doesn't exist in the Chrome sources, it defines the "devtools_page" property that can be optionally used in the manifest.json file to specify which extension asset should be loaded in the invisible "devtools_page" context. - "devtools-inspectedWindow.json", "devtools-panels.json", "devtools-network.json" - are the schema files that define all the DevTools API (these are the schema files available in the Chrome sources, with the addition of our custom "unsupported" and "async" properties already applied on them). Besides the JSON schema files, the following API implementation files are added by this patch: - "ext-devtools.js" - this file doesn't implement a DevTools API, but provides helpers used in the other API implementation files, and it manages the lifecycle (and the definition) of the devtools_page context (basically it is responsable of the creation of a new devtools_page context when a new devtools toolbox is opened, and to destroy the devtools_page contexts when their related toolbox has been destroyed). - "ext-devtools-inspectedWindow.js" - this file is responsable of the implementation of the "devtools.inspectedWindow" API, it currently implements only the "devtools.inspectedWindow.tabId", mainly used to fully test this first piece of the DevTools API implementation. - "ext-devtools-panels.js"/"ext-devtools-network.js" - this files are currently skeleton ready for the implementation of their related "devtools API namespaces", mainly created to provide the backbone for the next followups issues that are going to introduce more APIs implementation inside them. Finally, the patch contains a proposed small change to Extension.jsm which is responsible of ensuring that any "DevTools WebExtension context" (e.g. devtools_page and the upcoming devtools_panel) have access only to a subset of the WebExtensions API (e.g. they should not be able to directly use any privileged API even if the addon has the related permission, the provided API should be the same subset available to a content script or to a WebExtension iframe injected in a webpage). The new test case added is: browser/components/extensions/test/browser/browser_ext_devtools_page.js

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review69616 This looks like a nice start. Is it possible to split the new schemas from the devtools_page changes just to keep the individual patches from being too big? Also, it would be great to have the common bits for creating background and devtools pages be factored into something shareable... ::: browser/components/extensions/ext-devtools.js:23 (Diff revision 1) > +const { > + promiseDocumentLoaded, > + promiseObserved, > +} = ExtensionUtils; > + > +const DEVTOOLS_PAGE_XUL_URL = "data:application/vnd.mozilla.xul+xml;charset=utf-8," + encodeURI( xul? yikes. i didn't realize this was in background pages too... ::: browser/components/extensions/ext-devtools.js:34 (Diff revision 1) > + * > + * @param {ExtensionContext} context The context to will be checked. > + * @returns {boolean} returns true if the context is a devtools context, > + * false otherwise. > + */ > +global.isDevToolsContext = function isDevToolsContext(context) { can this just go into BaseContext? ::: browser/components/extensions/ext-devtools.js:42 (Diff revision 1) > + } > + > + return false; > +}; > + > +// Map[extension -> devtools_page] Can you use DevtoolsPage to make it clear that the value in the map is an instance of that class ::: browser/components/extensions/ext-devtools.js:57 (Diff revision 1) > + * 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. > + */ > +function DevtoolsPage(extension, url) { can this just be a class? ::: browser/components/extensions/ext-devtools.js:70 (Diff revision 1) > + let url = this.extension.baseURI.resolve(this.url); > + > + if (!this.extension.isExtensionURL(url)) { > + this.extension.manifestError("DevTools page must be a file within the extension"); > + url = this.extension.baseURI.resolve("_blank.html"); > + } > + > + let system = Services.scriptSecurityManager.getSystemPrincipal(); could this all be moved to the constructor? or if not, an initialize() method that gets called lazily the first time a page is actually created? ::: browser/components/extensions/schemas/devtools.json:14 (Diff revision 1) > + { > + "$extend": "WebExtensionManifest", > + "properties": { > + "devtools_page": { > + "type": "string", > + "format": "relativeUrl", looks like other similar properties use ExtensionURL, any reason not to use that here? ::: browser/components/extensions/schemas/devtools_inspected_window.json:9 (Diff revision 1) > + > +[ > + { > + "namespace": "devtools.inspectedWindow", > + "description": "Use the <code>chrome.devtools.inspectedWindow</code> API to interact with the inspected window: obtain the tab ID for the inspected page, evaluate the code in the context of the inspected window, reload the page, or obtain the list of resources within the page.", > + "nocompile": true, what is this? also, does this (and the other devtools.* schemas) need a permissions property? or will that come later when these apis actually get implemented?

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review69616 Sounds more than reasonable, in the next update I'm going to keep only the minimum schema and implementation files needed by this first step, and defer the other ones to the next steps of the DevTools API implementation. And I'll take a look at the refactoring of the common part of the background and devtools pages into a new helper. > xul? yikes. i didn't realize this was in background pages too... yeah, it was introduced by Bug 1274775 to fix exception on canvas API usage (and it fixes an issue when inspecting our background page from the DevTools inspector) > can this just go into BaseContext? it could be moved there, I'm only a bit unsure if this fits well into BaseContext. > Can you use DevtoolsPage to make it clear that the value in the map is an instance of that class sure (the inline comments are still a bit rough) > can this just be a class? it seems so, I'm trying to remember if I rewrote this without ES6 class for some issue that I've found during the prototyping, but I don't recall and in that case it is better if I rewrite it as an ES6 class and if there were issues I'll put a inline comment near to the constructor function. > could this all be moved to the constructor? > or if not, an initialize() method that gets called lazily the first time a page is actually created? This remember me that I really need to put some inline comments in jsdoc syntax on this DevtoolsPage component. The instances of the DevtoolsPage class doesn't represent the single DevtoolsPage context instances, they represent the DevtoolsPage declared by a single extension (1 instance per extension with a "devtools_page" property in the manifest) The DevtoolsPage is an hidden WebExtension context, as a "Background page" content, but with a different lifecycle: - a new context of type "devtools_page" is created for every opened developer toolbox (and the context is destroyed when the developer toolbox is closed), and so they are 1 instance per toolbox. So basically an instance of DevtoolsPage manages (for its related extension) the creation and shutdown of the contexts of type "devtools_page", and the `buildForToolbox` creates a new context when a new developer toolbox is opened (and from this point of view, it is "lazy" by requirements) > looks like other similar properties use ExtensionURL, any reason not to use that here? I've probably used the same format of the "background.page" manifest property, but I'm going to take a look to ExtensionURL and see if it is a better fit (and which are its differences from the `relativeUrl` format) > what is this? > also, does this (and the other devtools.* schemas) need a permissions property? or will that come later when these apis actually get implemented? it is something used in Chrome (and it was in the original schema file), probably to configure how this file is used during the build of the Chrome sources. We can remove it, it is ignored by our JSON schema processing code.

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review69616 > it is something used in Chrome (and it was in the original schema file), probably to configure how this file is used during the build of the Chrome sources. > > We can remove it, it is ignored by our JSON schema processing code. My apologies Andrew, I just noticed that I forgot to add the details related to the second part of the above review comment :-( The DevTools APIs don't have an explicit permission to be requested, they are only available in the "special" DevTools contexts (e.g. the devtools page that this pages creates, as well as the upcoming the devtools panel context, and once we decide to support them, the devtool sidebar contexts), and they are never available to any other context (and with the changes introduced by Bug 1290901 we can return null ,instead of an API object, to make an API not available in a particular context).

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review69616 > it could be moved there, I'm only a bit unsure if this fits well into BaseContext. After re-looking at the entire patch again, I must say that I tend to agree, I'm going to move this helper into the BaseContext.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. This new attached patch adds to the `Extension` class a new `createWindowlessExtensionPage` helper that contains the shared bits needed to create this kind of invisible extension pages, based on the needs of ext-backgroundPage and ext-devtools. Set f? to Andrew for a feedback.

Comment on attachment 8783664 [details] Bug 1291737 - handle empty API namespaces in schema injection helpers. This new attached patch refactors ext-backgroundPage to use the new `createWindowlessExtensionPage` helper, which makes much more clear how such helper would be used. (the other example usage is in the ext-devtools.js from the updated version of attachment 8777399 [details]) Set f? to Andrew for a feedback.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. Hey Kris, Given that we are going to create a new extension context that is going to need to be initialized with a sequence of code very similar to the "windowless browser"-boilerplate that we are already using in ext-backgroundPage, Andrew is proposing to refactor the common bits into an helper. This patch contains my current proposal (and the next f? contains the patch that show how it would be introduced in the refactored ext-backgroundPage), can you take a look at it for a feedback?

Comment on attachment 8783664 [details] Bug 1291737 - handle empty API namespaces in schema injection helpers. This is the patch that shows how the new proposed helper would be introduced in the refactored ext-backgroundPage.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. I rebased the patches attached to this issue to rewrite (and rethink) the changes on top of the changes related to OOP WebExtensions introduced by Bug 1287010. Re-new the feedback request assigned to both Andrew and Kris. (This change is used in both Attachment 8783664 [details], which refactor ext-backgroundPage.js to use the new proposed helper, and Attachment 8777399 [details], which uses the proposed helper to create the invisible devtools_page)

Comment on attachment 8783664 [details] Bug 1291737 - handle empty API namespaces in schema injection helpers. The last update contains a rebase of this patch and a fix on the previous version. Re-new the feedback request assigned to both Andrew and Kris.

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. After the brief chat with Rob Wu related to the changes introduced by Bug 1287010, I've re-wrote this patch to use a more explicit (and hopefully more compatible with the OOP plans) to achieve the following corner case related to the devtools APIs: - The devtools APIs doesn't have an associated explicit permission to request in the manifest - The devtools APIs are only available in devtools contexts (e.g. the invisible devtools_page, the devtools_panel etc.) - The devtools APIs have the same API level of a content script/injected webextension iframe (plus the "devtools.*" API namespaces) - in Chrome they are probably running in same process of the target window, even if on Firefox we run them in the main process (until we are ready to move it in the content process or in the addon process) we should not provide to the devtools context any API that will not be available once moved (e.g. even if the "runtime" namespace is available, the "runtime.getBackgroundPage" should not be available) The proposed approach (updated based on the changes introduced by Bug 1287010) is composed of: - a special permission specified in the devtools API schema (e.g. similar to the custom "manifest:browser_Action"): > "permissions": ["context:devtools"] - integrate devtools checks into the checks already done in hasPermission and shouldInject: > hasPermission(permission) { > if (permission == "context:devtools") { > return context.isDevTools(); > } > return context.extension.hasPermission(permission); > }, > shouldInject(namespace, name, restrictions) { > // Do not generate content script APIs, unless explicitly allowed. > if (context.envType === "content_parent" && > (!restrictions || !restrictions.includes("content"))) { > return false; > } > // NOTE: until the devtools page are running in the main process, > // apply to the devtools pages the same API restrictions applied > // to content scripts and iframe injected into a content webpage. > if (!namespace.startsWith("devtools.") && context.isDevTools() && > (!restrictions || !restrictions.includes("content"))) { > return false; > } > return findPathInObject(apis, namespace) !== null; > },

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. https://reviewboard.mozilla.org/r/73390/#review72542 ::: toolkit/components/extensions/Extension.jsm:1379 (Diff revision 2) > + * implementations to share the boilerplate code needed to create the > + * windowless browser, prepare it to load the extension page and provide > + * access to the bits needed by the caller (e.g. the reference to the > + * newly created windowlessBrowser, webNav, contentWindow) > + * > + * NOTE: this is a generator function, converted into an async method in the I would vote for just using Task.spawn() until we have real async functions ::: toolkit/components/extensions/Extension.jsm:1443 (Diff revision 2) > + if (context.contentWindow == browser.contentWindow && > + context.type == contextType) { > + Management.off("page-load", listener); > + > + runSafeSyncWithoutClone(onExtensionContextCreated, context); > + resolve(context); I don't think anything actually looks at the resolved value? In any case, it doesn't look like the main function task here does anything asynchronous after this, how about just making the parameter a boolean for whether the context should be included in the return value, if that parameter is false you don't have to wait for context, if its true, you can wait and then just include it in the returned object?

Comment on attachment 8783664 [details] Bug 1291737 - handle empty API namespaces in schema injection helpers. https://reviewboard.mozilla.org/r/73392/#review72544 I would roll this together with the previous patch.

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review72546 ::: browser/components/extensions/ext-devtools.js:22 (Diff revisions 1 - 3) > - return true; > - } > > - return false; > +// Map[context => Devtools Toolbox] > +let devtoolsToolboxForContext = new Map(); > +global.getDeveloperToolboxForContext = (context) => { this can be shortened to `context => devtoolsToolsboxForContent.get(context)` ::: browser/components/extensions/ext-devtools.js:46 (Diff revisions 1 - 3) > - this.extension = extension; > + this.extension = extension; > > - // Map[target -> {browser, chromeShell}] > - this.devtoolsPageContextForTarget = new Map(); > + // Map[Devtools target -> {webNav, windowlessBrowser}] > + this.devtoolsPageChromeForTarget = new Map(); > + > + // Convert the generator function into an async method. I would vote for using Task.spawn() instead of this ::: toolkit/components/extensions/ExtensionUtils.jsm:187 (Diff revision 3) > + * Checks if the context is a devtools context (e.g. "devtools_page", "devtools_panel" etc.) > + * > + * @returns {boolean} Returns true if the context is a devtools context, false otherwise. > + */ > + isDevTools() { > + if (this.type && this.type.startsWith("devtools_")) { nit: this could be shortened to just `return (this.type && ...);`

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review72488 ::: toolkit/components/extensions/Extension.jsm:305 (Diff revision 3) > // are excluded in `shouldInject` by this check, but remote APIs do not have > // this information and will therefore cause the schema API generator to > // create an API that proxies to a non-existing API implementation. > if (!obj || !(elt in obj)) { > return null; > } Can we now remove this TODO and `if` ? It was introduced with an earlier devtools-related commit of yours, and since your comment was deleted I think that it should be safe to remove it.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. https://reviewboard.mozilla.org/r/68964/#review72498 You should create a new envType and BaseContext. I've created a wiki page that explains how API bindings work, so you can make a more informed decision on how to choose the envType and context: https://wiki.mozilla.org/WebExtensions/Implementing_APIs_out-of-process ::: browser/components/extensions/ext-devtools-inspectedWindow.js:11 (Diff revision 3) > + > +/** > + * This module provides the implementation of the devtools.inspectedWindow namespace. > + */ > + > +extensions.registerSchemaAPI("devtools.inspectedWindow", "addon_parent", (context) => { With `addon_parent` here, the API would unnecessarily be included in the `apis` object in `GlobalManager.injectInObject`. See http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/toolkit/components/extensions/ExtensionUtils.jsm#1822 for documentation on the current types (and add the new one there). The devtools does not necessarily run in an addon process, and neither in a content script process by design, so it should get its own type. When you add a new type, don't forget to add it to the `_schemaApis` in the constructor and adjust its JSDoc. ::: browser/components/extensions/schemas/devtools_inspected_window.json:8 (Diff revision 3) > +// found in the LICENSE file. > + > +[ > + { > + "namespace": "devtools.inspectedWindow", > + "permissions": ["context:devtools"], Remove this permission and make it ``` "restrictions": ["devtools"], "defaultRestrictions": ["devtools"], ``` and then in `shouldInject` a check similar to the current `content` check (https://hg.mozilla.org/mozilla-central/annotate/c78a0b3e1152/toolkit/components/extensions/Extension.jsm#l591). With the suggested check, devtools contexts get no APIs by default, unless opted-in via the schema JSON via the "restrictions" key. `restrictions` should explicitly be set. If the full API of a namespace should be visible to the context, the `defaultRestrictions` key can be set on the namespace. ::: toolkit/components/extensions/Extension.jsm:190 (Diff revision 3) > // |docShell| is the docshell the content runs in (optional). > ExtensionContext = class extends BaseContext { > constructor(extension, params) { > // TODO(robwu): This should be addon_child once all ext- files are split. > // There should be a new ProxyContext instance with the "addon_parent" type. > super("addon_parent", extension); Add a new class that derives from `BaseContext` and use that for devtools_page instead of an ExtensionContext. Is there anything in ExtensionContext that you're relying on? I guess not? If yes and you can't figure out how to use BaseContext, do this at the very least: ``` class DevtoolsPageContext extends ExtensionContext { constructor(...args) { super(...args); this.envType = "devtools"; } get externallyVisible() { return false; } } ::: toolkit/components/extensions/Extension.jsm:305 (Diff revision 3) > // are excluded in `shouldInject` by this check, but remote APIs do not have > // this information and will therefore cause the schema API generator to > // create an API that proxies to a non-existing API implementation. > if (!obj || !(elt in obj)) { > return null; > } Can you delete the whole TODO and if here? This conditional was added for an earlier devtools feature, and since you've deleted your comment above it should probably be fine to also delete the conditional. ::: toolkit/components/extensions/Extension.jsm:589 (Diff revision 3) > }, > > hasPermission(permission) { > + if (permission == "context:devtools") { > + return context.isDevTools(); > + } Remove this, use `shouldInject` for API access control (see below for more details). ::: toolkit/components/extensions/Extension.jsm:605 (Diff revision 3) > + > + // NOTE: until the devtools page are running in the main process, > + // apply to the devtools pages the same API restrictions applied > + // to content scripts and iframe injected into a content webpage. > + if (!namespace.startsWith("devtools.") && context.isDevTools() && > + (!restrictions || !restrictions.includes("content"))) { Together with the schema change suggested below, ``` if (context.envType === "devtools" && (!restrictions || (!restrictions.includes("devtools") && !restrictions.includes("content")))) { return false; } ``` ... with a comment explaining why the devtools gets the same APIs as content scripts. If there is ever an API available to content scripts that is not available to devtools, then you have to modify the few schema files and explicitly insert "devtools" in the "restrictions"/"defaultRestrictions" field. And also, if you intentionally want to support content script APIs, add some basic tests, like runtime.sendMessage from devtools to background page and using the storage API. Note that the other direction is not as trivial. For content scripts the API is tabs.sendMessage, but devtools are not a tabs. Devtools are neither part of the addon process, so runtime.sendMessage won't reach it either. FYI, Chrome does not even implement it and need a work-around (https://stackoverflow.com/q/11661613), so it is OK if you leave communicating to the devtools page open for now.

(In reply to Rob Wu [:robwu] from comment #23) > Comment on attachment 8777399 [details] > Bug 1291737 - [webext] Implement devtools_page and > devtools.inspectedWindow.tabId. > > https://reviewboard.mozilla.org/r/68966/#review72488 > > ::: toolkit/components/extensions/Extension.jsm:305 > (Diff revision 3) > > // are excluded in `shouldInject` by this check, but remote APIs do not have > > // this information and will therefore cause the schema API generator to > > // create an API that proxies to a non-existing API implementation. > > if (!obj || !(elt in obj)) { > > return null; > > } > > Can we now remove this TODO and `if` ? It was introduced with an earlier > devtools-related commit of yours, and since your comment was deleted I think > that it should be safe to remove it. Not yet unfortunately, my change was applied over a similar change applied by a previous patch (https://hg.mozilla.org/mozilla-central/rev/7cf25e2c60be#l4.31, my change was just the only one which added a comment that explains the dependency, and so I needed to run a try build to be sure of what parts of our internals still need the check). On friday I launched a try build to be sure of it, and as I suspected there is a real consumer of that `if` statement, it is basically used to filter out the following namespaces: 'manifest', 'events', 'contextMenusInternal', that cannot be found in the API object because they are not real API namespaces. And so, to prevent our internals, and our tests, to fail, (at least currently) we have to filter them , e.g.: ``` function findPathInObject(obj, path) { for (let elt of path.split(".")) { obj = obj[elt] || undefined; } return obj; } ```

Comment on attachment 8783664 [details] Bug 1291737 - handle empty API namespaces in schema injection helpers. https://reviewboard.mozilla.org/r/73392/#review74936 ::: toolkit/components/extensions/Extension.jsm:296 (Diff revision 3) > function findPathInObject(obj, path) { > for (let elt of path.split(".")) { > - // If we get a null object before reaching the requested path > - // (e.g. the API object is returned only on particular kind of contexts instead > - // of based on WebExtensions permissions, like it happens for the devtools APIs), > - // stop searching and return undefined. > + // NOTE: some of the defined namespaces in the schema (e.g. manifest, > + // events, extensionTypes) doesn't have an API implementation (and > + // none of them is currently a nested namespace), if any of these > + // namespace is nested, then this check will not be enough. This comment is not a solution. To really fix the problem, the manifest should somehow specify that no bindings should be generated for the given schema. Otherwise, any child context will -through ChildAPIManager- generate an API for thee non-existent APIs (and end up with generating an error in the parent). I have written two changes related to this test, which conflicts with the change you're doing here. Show a useful error message instead of an obscure message: https://hg.mozilla.org/try/rev/bd1b0615ed3e97d633f720fcd74590e21ed2e248 Refactor your unit test so that it tests what it is supposed to test without depending too much on the above details. https://hg.mozilla.org/try/rev/81adbea2ead983afd0f55337503c1ad72936804e (I made these changes because they blocked my patch by causing test failures)

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. https://reviewboard.mozilla.org/r/73390/#review72542 > I would vote for just using Task.spawn() until we have real async functions Sounds good to me (I applied this change to the updated version of this patch) > I don't think anything actually looks at the resolved value? > In any case, it doesn't look like the main function task here does anything asynchronous after this, how about just making the parameter a boolean for whether the context should be included in the return value, if that parameter is false you don't have to wait for context, if its true, you can wait and then just include it in the returned object? Based on the feedback, I've reworked this helper to do just what is needed to both the background page and the devtools page, e.g. - the `context` is only needed by the devtools page internals, and I've changed the approach used to get it, so that it doesn't impact anymore on this common helper. - a `waitForBrowserLoad` promise is returned by this common helper, because the background page implementation needs to wait the "load" event on the `browser` XUL element that wraps the actual extension page, on the contrary if the common async helper waits the browser "load" event before resolving its final results then the devtools page fails to be able to associate the toolbox to the devtools contexts before the created devtools context is able to execute the devtools API methods (which will fail if we cannot retrieve the debugging target/devtools toolbox related to the calling context) the proposed usage of this `createWindowlessExtensionPage` helper [in the ext-backgroundPage.js file is included in this patch](https://reviewboard.mozilla.org/r/73390/diff/5#1), and its usage [in the devtools page implementation is included in the other patch from this same mozreview](https://reviewboard.mozilla.org/r/68966/diff/6#1)

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. Hi Rob, do you mind taking another look to the last version of this patch for a new round of feedback from a "webext-oop" point of view? (e.g. it now defines a new "devtools" API restriction, and the devtools API are now registered to their own endTypes, the devtools contexts are created from a DevtoolsContext which inherits from BaseContext)

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review76410 ::: browser/components/extensions/ext-devtools-inspectedWindow.js:11 (Diff revision 6) > + > +/** > + * This module provides the implementation of the devtools.inspectedWindow namespace. > + */ > + > +extensions.registerSchemaAPI("devtools.inspectedWindow", "devtools_parent", (context) => { The `inspectedWindow.tabId` property must live in `devtools_child` because the `tabId` getter offers synchronous access to the API. I made the tabId available in the child process, you can probably use it. The `tabId` is mapped from the frame script global, here is the patch: https://reviewboard.mozilla.org/r/77082/diff/3#index_header Starting from that patch there are two ways to get a tabId for a given (proxy) extension context: Extension.jsm, ExtensionProxyContext has a tabId getter for use in the parent process. ExtensionChild.jsm, ExtensionContext has a tabId property when `viewType` is tab. ::: browser/components/extensions/ext-devtools.js:134 (Diff revision 6) > +/* eslint-disable mozilla/balanced-listeners */ > + > +// Create a devtools page context for a new opened toolbox. > +gDevTools.on("toolbox-created", (evt, toolbox) => { > + if (!toolbox.target.isLocalTab) { > + // Only local tabs are currently (e.g. because we cannot currently This sentence is incomplete and missed a period. Is there any bug link that you can refer to from here? ::: browser/components/extensions/schemas/devtools_inspected_window.json:9 (Diff revision 6) > + > +[ > + { > + "namespace": "devtools.inspectedWindow", > + "restrictions": ["devtools"], > + "defaultRestrictions": ["devtools"], Add `devtools_only` to these two. ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:14 (Diff revision 6) > + > +/** > + * This test file ensures that: > + * > + * - the devtools_page property creates a new WebExtensions context > + * - the devtools API object is not available in a background page In https://bugzil.la/1296900 I added a test that looks for all available APIs and fails if there is an unexpected API (this patch is still under review so the details may change. I will let you know). You can re-use this test by appending the following to the next file. browser/components/extensions/test/mochitest/test_ext_all_apis.html ``` <script> /* globals sendAllApis, expectedBackgroundApis, generateExpectations */ add_task(function* test_enumerate_devtools_script_apis() { let extensionData = { manifest: { devtools_page: "devtools_page.html" }, background: sendAllApis, files: { "devtools.html": `<script src="devtools.js"></script>`, "devtools.js": sendAllApis, }, }; let extension = ExtensionTestUtils.loadExtension(extensionData); yield extension.startup(); let actualApis = yield extension.awaitMessage("allApis"); let expectedApis = generateExpectations(expectedBackgroundApis); isDeeply(actualApis, expectedApis, "background script APIs with devtools_page"); // TODO: Open devtools page. The script `sendAllApis` will now send the allApis message and you can check whether the API matches, similarly to the code above. yield extension.unload(); }); </script> ``` ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:70 (Diff revision 6) > + </head> > + <body> > + <script src="devtools_page.js"></script> > + </body> > + </html>`, > + "devtools_page.js": `new ${devtools_page}();`, You can simply use `"devtools_page.js": devtools_page,` The landed patch from https://bugzil.la/1298716 ensures that it works as expected. ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:106 (Diff revision 6) > + > +add_task(function* test_devtools_page_runtime_api_messaging() { > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/"); > + > + function background_page() { > + browser.runtime.onConnect.addListener((port) => { What is the expected value of `port.sender`? Can you add assertions for its properties ? In particular, I expect `port.sender.url` to be set because the message came from a frame. `port.sender.tab` may or may not be set, depending on whether you think that the devtools is part of a tab. ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:123 (Diff revision 6) > + const port = browser.runtime.connect(); > + port.postMessage("devtools -> background port message"); > + } > + > + let extension = ExtensionTestUtils.loadExtension({ > + background: `new ${background_page}();`, Rename the `background_page` function to `bsckground` and use `background,` here. ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:127 (Diff revision 6) > + let extension = ExtensionTestUtils.loadExtension({ > + background: `new ${background_page}();`, > + manifest: { > + manifest_version: 2, > + name: "test-devtools-page-addon", > + version: "0.1.0", The boilerplate manifest_version, name and version keys can be removed. The test extension generator automatically adds it: http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/toolkit/components/extensions/Extension.jsm#1219 `Extension.generateXPI` is called by `Extension.generate`, which in turn is called when you call `Extensionutils.loadExtension` (both in mochitests and XPCShell tests). This and the last three comments also apply to your other test. ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:140 (Diff revision 6) > + </head> > + <body> > + <script src="devtools_page.js"></script> > + </body> > + </html>`, > + "devtools_page.js": `new ${devtools_page}();`, You can simply use `"devtools_page.js": devtools_page,` The landed patch from https://bugzil.la/1298716 ensures that it works as expected. ::: toolkit/components/extensions/Extension.jsm:176 (Diff revision 6) > } > + if (envType === "devtools_child") { > + // TODO(rpl): Remove this. Similarly to the above, this should be a temporary hack, > + // that should make it easier to transition the devtools api in a child process > + // in follow ups. > + super.registerSchemaAPI(namespace, "devtools_parent", getAPI); Please do not use this hack for new code. I removed the hack for addon code, and properly created a new SchemaAPIManager in the child process @ https://reviewboard.mozilla.org/r/77072/diff/2#index_header Note that the WannabeChildAPIManager over there is meant as a temporary hack to help with migrating addon code. Since you are building the devtools API from the grounds up, it should be possible for you to design the implementation in a way that is webext-oop-compatible. I.e. devtools_parent for Chrome code, and devtools_child for code running in the same process as the devtools_page scripts. ::: toolkit/components/extensions/Extension.jsm:403 (Diff revision 6) > + let sender = {id: extension.uuid}; > + if (uri) { > + sender.url = uri.spec; > + } > + let filter = {extensionId: extension.id}; > + this.messenger = new Messenger(this, [Services.cpmm], sender, filter); Please comment on why you are using `Services.cpmm`. ::: toolkit/components/extensions/Extension.jsm:420 (Diff revision 6) > + return this.contentWindow.document.nodePrincipal; > + } > + > + get externallyVisible() { > + return false; > + } Remove this `externallyVisible`. It is not used by BaseContext. ::: toolkit/components/extensions/Extension.jsm:757 (Diff revision 6) > if (context.envType === "content_parent" && > (!restrictions || !restrictions.includes("content"))) { > return false; > } > + > + // Do not generate content script APIs, unless explicitly allowed. s/content script/devtools page/ ::: toolkit/components/extensions/Extension.jsm:768 (Diff revision 6) > + // Skip devtools API in any non devtools context. > + if (context.envType !== "devtools_parent" && restrictions > + && restrictions.includes("devtools") > + && !restrictions.includes("addon")) { > + return false; > + } Please remove the `"addon"` restriction and use `"devtools_only"`. This communicates the intent more clearly. ``` if (context.envType !== "devtools_parent" && restrictions && restrictions.includes("devtools_only")) { return false; } ``` ::: toolkit/components/extensions/Extension.jsm:843 (Diff revision 6) > + if (["devtools_page", "devtools_panel"].includes(type)) { > + context = new DevtoolsContext(extension, {type, contentWindow, uri, docShell}); > + } else { > + context = new ExtensionContext(extension, {type, contentWindow, uri, docShell}); > + } > + The initialization of `ExtensionContext` moved. Here are the relevant commits: - Step 1: Moving the code (and preserving the existing ExtensionContext initialization logic as much as possible even though the logic would violate process boundaries). - Step 2: Determine viewType, tabId, windowId in the "child process" without directly accessing XUL objects from the main process: https://reviewboard.mozilla.org/r/77082/diff/3#index_header - Step 3: Use the child process' Extension instead of the one in Extension.jsm: https://reviewboard.mozilla.org/r/77084/diff/3#index_header Once the patch passes review I will try to land it. I expect it to land at the start of next week. This is the squashed diff, look at `ExtensionChild.jsm`, specifically `createExtensionContext`: https://reviewboard.mozilla.org/r/77052/diff/3#index_header ::: toolkit/components/extensions/schemas/extension.json:8 (Diff revision 6) > // found in the LICENSE file. > > [ > { > "namespace": "extension", > - "restrictions": ["content"], > + "restrictions": ["content", "devtools", "addon"], As mentioned before, remove `addon` and add `devtools_only` to the devtools schema file. The whole point of a free-form `"restrictions"` field in the schemas is to allow conditions without too much boilerplate.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. https://reviewboard.mozilla.org/r/73390/#review76452 ::: toolkit/components/extensions/Extension.jsm:1559 (Diff revision 5) > + browser.removeEventListener("load", onLoad, true); > + // resolve to the loaded window > + resolve(browser.contentWindow); > + } > + }, true); > + }); This onload stuff is not webext-oop compatible. To fix it (including viewType initialization), see https://reviewboard.mozilla.org/r/77082/diff/3#5

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. In addition to all comments above, please add a new parameter to runtime.connect and runtime.sendMessage to explicitly signal where a message should be delivered. For example, the following API seems good: chrome.runtime.sendMessage(message, { toDevtools: { tabId: tabId, }, }, responseCallback); I proposed tabId because it fits well in the extension API. Choose makes sense for identication of devtools pages. Matt introduced toProxyScriptSandbox in https://reviewboard.mozilla.org/r/72514/diff/7#5, you can look at it if you need some inspiration.

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review76776 ::: browser/components/extensions/ext-devtools.js:106 (Diff revision 6) > + const { > + webNav, > + windowlessBrowser, > + } = this.devtoolsPageChromeForTarget.get(target); > + > + // Navigate away from the background page to invalidate any Should the comment say "devtools page", rather than "background page"? I know they serve the same purpose, but we're working with devtools pages here, correct? ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:53 (Diff revision 6) > + browser.test.notifyFail("devtools_page.done"); > + } > + } > + > + let extension = ExtensionTestUtils.loadExtension({ > + background: `new ${background_page}();`, Why is this required, rather than simply `background: background_page,`?

Comment on attachment 8777399 [details] Bug 1291737 - [webext] Implement devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/68966/#review76776 > Should the comment say "devtools page", rather than "background page"? I know they serve the same purpose, but we're working with devtools pages here, correct? yep, you are right, it should be "devtools page" here. Thanks for noticing! I'm going to fix it in the next updates on these patches. > Why is this required, rather than simply `background: background_page,`? it is required anymore, after some of the recent changes in the test utils, this can be definitely be rewritten like you are suggesting. I'm going to fix it in the next update.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. I'm still coming up to speed on all the oop work but it looks like you've already worked closely with Rob on that so I didn't read it in depth. Just one higher-level comment that perhaps this could be broken down into a few smaller patches?

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 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review90796 ::: browser/components/extensions/ext-devtools.js:123 (Diff revision 3) > + }, > +}; > + > +global.DevtoolsContextsManager = DevtoolsContextsManager; > + > +global.getTabIdForToolbox = (toolbox) => { This is the tabId for the toolbox _target_, correct? Should the function be renamed to reflect that, or is it just assumed that we are talking about the toolbox target?

Comment on attachment 8807892 [details] Bug 1291737 - implements devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/90882/#review90800 ::: browser/components/extensions/ext-c-devtools-inspectedWindow.js:8 (Diff revision 1) > +"use strict"; > + > +extensions.registerSchemaAPI("devtools.inspectedWindow", "devtools_child", context => { > + // `devtoolsToolboxInfo` is received from the child process when the root devtools view > + // has been created, and every sub-frame of that top level devtools frame will > + // receive the same informations when the context has been created from the s/informations/information ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:12 (Diff revision 1) > +XPCOMUtils.defineLazyModuleGetter(this, "devtools", > + "resource://devtools/shared/Loader.jsm"); > + > +/** > + * This test file tests the devtools.inspectedWindow.tabId and > + * devtools.inspectedWindow.eval behavior. This file does not include tests for `devtools.inspectedWindow.eval` as of this patch. ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:36 (Diff revision 1) > + "The `runtime.getBackgroundPage` API method should be missing in a devtools_page context" > + ); > + > + if (browser.devtools) { > + // If the devtools API object is defined, get the inspectedWindow tabId > + // to check that it is the same of the tabId that we retrieve from the s/of/as Also, please add periods to the end of the comments, here and on line 43. ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:44 (Diff revision 1) > + browser.test.sendMessage("inspectedWindow-tab-id", tabId); > + > + browser.test.notifyPass("devtools_page.done"); > + } else { > + // Just fail if the devtools API object is not defined > + browser.test.notifyFail("devtools_page.done"); Would it be better to provide a more explicit failure message if the devtools API is not defined? In this case all we'll see is that the test failed, but not know why.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review90804 ::: browser/components/extensions/ext-devtools.js:200 (Diff revision 3) > + > + this.extension.on("extension-proxy-context-load", listener); > + }); > + > + extensions.emit("extension-browser-inserted", browser); > + browser.messageManager.sendAsyncMessage("Extension:InitExtensionView", { It looks like this implements the `devtools.inspectedWindow.tabId` functionality, so should it be moved into the next patch? It is neither used nor tested in this patch.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review90804 > It looks like this implements the `devtools.inspectedWindow.tabId` functionality, so should it be moved into the next patch? It is neither used nor tested in this patch. After brief chat, we agreed that it is probably not worth to move it into the third patch (which is part of the same mozreview request and Bugzilla issue). But I'm open to move it if it will come out again during the review.

Comment on attachment 8807892 [details] Bug 1291737 - implements devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/90882/#review90800 > Would it be better to provide a more explicit failure message if the devtools API is not defined? In this case all we'll see is that the test failed, but not know why. I reworked the test a bit, so that they fail fast when there are failures (instead of waiting the test timeout) and log detailed errors (e.g. on exceptions raised if the devtools namespace is missing).

Comment on attachment 8809266 [details] Bug 1291737 - Export TabTarget from 'devtools/client/framework/target' module. https://reviewboard.mozilla.org/r/91850/#review91950 ::: devtools/client/framework/target.js:146 (Diff revision 1) > +exports.TabTarget = TabTarget; > + This patch exports the `TabTarget` class from the `"devtools/client/framework/target"` module to be able to create a separate target instance, it is used in the other patch from this queue: - https://reviewboard.mozilla.org/r/80716/diff/5#index_header in the file ext-devtools.js at line 114-117 I choose to create a new target instance for the tab instead of reusing the existent target from the tab, because when we worked on exposing a similar feature through the SDK in the past we had a discussion about it and the result was that using a shared RDP connection for the devtools addons can potentially introduce issues in the integrated devtools when one of the devtools addons doesn't behave correctly (e.g. flood the RDP connection of their RDP requests) The new target instance points to the same tab of the original toolbox target, but it can be destroyed without affecting the integrated devtools (on the contrary if the target is shared, the toolbox will auto-close itself when the target.destroy() is called).

Yes, you have to do that. Mostly because of what you said about toolbox destroying the target. But you have to acknowledge you are entering a new world. Our actors are used to be run solo. We typically have only one instance at a time. By having multiple targets, you now open ways to have multiple instances for each actors. But given that you are mostly going to use new actor you are designing, it should be fine. So just keep that in mind if you see unexpected things happening.

Hey Kris, I've just completed a rebase of this set of patches (and the other patches related to the devtools API that have not been landed yet) on top of your changes from Bug 1317101. The part of Bug 1317101 which introduced the refactoring of the BackgroundPage (to make it possible to run the background page in webext-oop mode) didn't take into account the first patch from this issue (Attachment 8783663 [details]) which was going to refactoring out the common bits needed by both the background page and the devtools page (they are both invisible extension pages and they both need the same workaround implemented for the background page to run the tests in webext-oop mode). I've now totally reworked Attachment 8783663 [details] to better fit to the changes in the remote browser initialization introduced by Bug 1317101, and changed the BackgroundPage class accordingly. It would be awesome if you can take a look at it asap, even just Attachment 8783663 [details] for now, so that we can start to its review and land it as soon as possible, because it is definitely the part of the remaining devtools API patches on which it is more likely to conflict with any further cleanup/refactoring activities related to the webext-oop changes.

I was actually planning on reviewing these today, anyway. I've only been putting it off because I wanted the OOP tests landed first.

(In reply to Kris Maglione [:kmag] from comment #73) > I was actually planning on reviewing these today, anyway. I've only been > putting it off because I wanted the OOP tests landed first. That's great, thanks! Totally agree, being able to run our tests in an "out of process" mode was definitely something pretty important to land as soon as possible, even if the webext-oop mode is not going to be enabled yet, running the test in both modes will give us much more confidence that we are not going to break any "webext-oop assumptions" in the meantime. And exactly for this reason, I was already keeping an eye on the patches from 1317101 and preparing to adapt my patches as fast as possible, so that I could be absolutely sure that the devtools API patches are "webext-oop compatible", and "not just on the paper" but also by successfully passing the tests with the out of process mode enabled.

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. https://reviewboard.mozilla.org/r/73390/#review95298 ::: toolkit/components/extensions/ExtensionUtils.jsm:1193 (Diff revision 11) > + * @param {Extension} extension > + * the Extension which owns the hidden extension page created (used to decide > + * if the hidden extension page parent doc is going to be a windowlessBrowser or > + * a visible UL window) > + */ > +class HiddenExtensionPageUtils { s/Utils// ::: toolkit/components/extensions/ExtensionUtils.jsm:1206 (Diff revision 11) > + // A background page loaded into a visible dialog window. Only to be > + // used for debugging, and in temporary, test-only use for > + // out-of-process extensions. > + return Task.spawn(function* () { Please move these to separate `createWindowlessBrowser` and `createWindowedBrowser` methods. ::: toolkit/components/extensions/ExtensionUtils.jsm:1216 (Diff revision 11) > + .getInterface(Ci.nsIDocShell) > + .QueryInterface(Ci.nsIWebNavigation); Please align dots with the first dot of the previous line. ::: toolkit/components/extensions/ext-backgroundPage.js:21 (Diff revision 11) > - > // WeakMap[Extension -> BackgroundPage] > var backgroundPagesMap = new WeakMap(); > > // Responsible for the background_page section of the manifest. > -class BackgroundPageBase { > +class BackgroundPage { This should be a subclass of `HiddenExtensionPage` ::: toolkit/components/extensions/ext-backgroundPage.js:49 (Diff revision 11) > let chromeDoc = yield this.getParentDocument(); > > let browser = chromeDoc.createElement("browser"); > browser.setAttribute("type", "content"); > browser.setAttribute("disableglobalhistory", "true"); > browser.setAttribute("webextension-view-type", "background"); > > let awaitFrameLoader; > if (this.extension.remote) { > browser.setAttribute("remote", "true"); > awaitFrameLoader = promiseEvent(browser, "XULFrameLoaderCreated"); > } > > chromeDoc.documentElement.appendChild(browser); > yield awaitFrameLoader; > > this.browser = browser; > > extensions.emit("extension-browser-inserted", browser); > > browser.loadURI(url); > > yield new Promise(resolve => { > browser.messageManager.addMessageListener("Extension:ExtensionViewLoaded", function onLoad() { > browser.messageManager.removeMessageListener("Extension:ExtensionViewLoaded", onLoad); > resolve(); > }); > }); All of this can be moved to the `HiddenExtensionPage` base class.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review95304 ::: browser/components/extensions/ext-devtools.js:19 (Diff revision 7) > + promiseEvent, > + HiddenExtensionPageUtils, Please keep sorted. ::: browser/components/extensions/ext-devtools.js:47 (Diff revision 7) > + this.contextDataMap.set(context, { > + toolbox, target: toolbox.target, > + }); It seems like this data belongs in the context. ::: browser/components/extensions/ext-devtools.js:67 (Diff revision 7) > + if (contextData.contextToolboxTarget) { > + contextData.contextToolboxTarget.destroy(); > + } It seems like this belongs in the context's unload code. ::: browser/components/extensions/ext-devtools.js:102 (Diff revision 7) > + * A devtools extension proxy context. > + * > + * @returns {Promise<TabTarget>} > + * The devtools target associated to the context. > + */ > + getLocalTabTargetForContext(context) { Also probably belongs in the context. ::: browser/components/extensions/ext-devtools.js:154 (Diff revision 7) > + * @param {Extension} extension > + * The extension that owns the devtools_page. > + * @param {string} url > + * The path to the devtools page html page relative to the extension base URL. > + */ > +class DevtoolsPage { This should be a subclass of `HiddenExtensionPage` ::: browser/components/extensions/ext-devtools.js:196 (Diff revision 7) > + return; > + } > + > + let url = this.extension.baseURI.resolve(this.url); > + > + if (!this.extension.isExtensionURL(url)) { This probably isn't necessary anymore. The schema checks should handle it. ::: browser/components/extensions/ext-devtools.js:210 (Diff revision 7) > + if (context.viewType == "devtools_page" && context.xulBrowser == browser) { > + // Keep track of the toolbox and target associated to the context, which is > + // needed by the API methods implementation. > + DevtoolsParentContextsManager.addContext(context, toolbox); > + if (!topLevelContextInitialized) { > + topLevelContextInitialized = true; > + > + // Remove listeners when the top level context has been destroyed. > + context.callOnClose({ > + close: () => { > + this.extension.off("extension-proxy-context-load", listener); > + }, > + }); > + > + // Resolve the promise when the root devtools_page context has been created. > + resolve(context); > + } > + } This should all be done from the context. ::: browser/components/extensions/ext-devtools.js:243 (Diff revision 7) > + let webNav; > + > + // This will run only if the browser element is not `remote=true`. > + if (browser.docShell) { > + webNav = browser.docShell.QueryInterface(Ci.nsIWebNavigation); > + } This shouldn't be necessary. ::: browser/components/extensions/ext-devtools.js:259 (Diff revision 7) > + webNav, > + hiddenExtensionPageUtils, > + }); > + } > + > + return Task.spawn(asyncBuildForToolbox.bind(this)); Please just `return Task.spawn(function* ...)` at the start. It makes things much easier to follow, and can just go away when we switch to native async functions. ::: browser/components/extensions/ext-devtools.js:275 (Diff revision 7) > + if (hiddenExtensionPageUtils) { > + hiddenExtensionPageUtils.shutdown(); > + } This isn't right. You're using the parent hidden window for multiple toolbox <browser>s, but destroying the whole thing when one of them is destroyed. We're also probably creating multiple parent windows and dropping the references to previous ones when a new toolbox opens, too. There should probably just be one DevtoolsPage instance per toolbox. ::: browser/components/extensions/test/browser/browser_ext_devtools_page.js:23 (Diff revision 7) > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/"); > + > + function background() { > + browser.runtime.onConnect.addListener((port) => { > + port.onDisconnect.addListener(() => { > + browser.test.notifyPass("devtools_page_connect.done"); This will still pass if we get onDisconnect without ever having received a message.

Comment on attachment 8807892 [details] Bug 1291737 - implements devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/90882/#review95308 ::: browser/components/extensions/ext-c-devtools-inspectedWindow.js:10 (Diff revision 5) > + let tabId = context.devtoolsToolboxInfo && > + context.devtoolsToolboxInfo.inspectedWindowTabId; Please either move to the previous line, or add parens and align this after the opening paren. ::: browser/components/extensions/schemas/devtools_inspected_window.json:28 (Diff revision 5) > + "functions": [ > + { > + "name": "getContent", > + "unsupported": true, > + "type": "function", > + "description": "Gets the content of the resource.", `"async": "callback"` ::: browser/components/extensions/schemas/devtools_inspected_window.json:53 (Diff revision 5) > + }, > + { > + "name": "setContent", > + "unsupported": true, > + "type": "function", > + "description": "Sets the content of the resource.", `"async": "callback"` ::: browser/components/extensions/schemas/devtools_inspected_window.json:72 (Diff revision 5) > + "name": "error", > + "type": "object", > + "additionalProperties": {"type": "any"}, > + "optional": true, > + "description": "Set to undefined if the resource content was set successfully; describes error otherwise." :/ Why doesn't this just use the error handling style of every other async API? :\ ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:23 (Diff revision 5) > + */ > +add_task(function* test_devtools_inspectedWindow_tabId() { > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/"); > + > + async function background() { > + browser.test.sendMessage("is-devtools-in-backgroundpage", !!browser.devtools); `browser.test.assertEq(undefined, ...)` ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:25 (Diff revision 5) > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://mochi.test:8888/"); > + > + async function background() { > + browser.test.sendMessage("is-devtools-in-backgroundpage", !!browser.devtools); > + > + const tabs = await browser.tabs.query({active: true}); `, lastFocusedWindow: true` ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:49 (Diff revision 5) > + try { > + let tabId = browser.devtools.inspectedWindow.tabId; > + browser.test.sendMessage("devtools_page_iframe.inspectedWindow-tab-id", tabId); > + } catch (err) { > + browser.test.sendMessage("devtools_page_iframe.inspectedWindow-tab-id", undefined); > + throw err; browser.test.fail(`Error: ${err} :: ${err.stack}`); browser.test.notifyFail(...)

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review95304 > It seems like this data belongs in the context. I would really like to move these helpers into the context, but the context here is an `ExtensionChildProxyContext`, which is defined at toolkit level, while the devtools API is defined and available only at browser level and so I tried to keep inside the ext-devtools.js module the tracking of the additional properties needed by the devtools APIs (which is used by both the devtools_page and devtools_panel contexts). > This should all be done from the context. In the updated patch I've refactored this code, but it is still part of the DevtoolsPage (which is a new class, it has been extracted by the previous DevtoolsPage class, now renamed DevtoolsPageDefinition), because the purpose of this code is: - detect and wait for the top level devtools_page context to be loaded - associate all the newly created devtools_page context proxy (the top level and its sub frames if any) with its related toolbox it seems that this code found a better fit here, because it is now strictly related to a single context, but to all the context created in the same devtools_page (top level devtools_page and its sub frames). > This shouldn't be necessary. I was assuming that it was still necessary (and the background page still uses it, e.g. https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#79-80) May I ask why is not needed anymore? > This isn't right. You're using the parent hidden window for multiple toolbox <browser>s, but destroying the whole thing when one of them is destroyed. We're also probably creating multiple parent windows and dropping the references to previous ones when a new toolbox opens, too. > > There should probably just be one DevtoolsPage instance per toolbox. This is probably a misunderstanding related to my poor choice for the class name. In the refactored patch, this class is now named DevtoolsPageDefinition (1 instance per extension), and the actual devtools page (1 instance per toolbox) is now named DevtoolsPage.

Comment on attachment 8807892 [details] Bug 1291737 - implements devtools.inspectedWindow.tabId. https://reviewboard.mozilla.org/r/90882/#review95308 > :/ Why doesn't this just use the error handling style of every other async API? :\ It is probably because it is not considered a real error, but a successful "eval" result, which may contain an error.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review96092 ::: browser/components/extensions/ext-devtools.js:155 (Diff revision 8) > + * @param {Toolbox} options.toolbox > + * The developer toolbox instance related to this devtools_page. > + * @param {string} options.url > + * The path to the devtools page html page relative to the extension base URL. > + */ > +class DevtoolsPage extends HiddenExtensionPage { On the DevTools team, "DevTools" is the more preferred casing choice instead of "Devtools". If it doesn't go against your own casing policies, it would be nice to use this.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #84) > Comment on attachment 8794136 [details] > Bug 1291737 - Implements the devtools_page context. > > https://reviewboard.mozilla.org/r/80716/#review96092 > > ::: browser/components/extensions/ext-devtools.js:155 > (Diff revision 8) > > + * @param {Toolbox} options.toolbox > > + * The developer toolbox instance related to this devtools_page. > > + * @param {string} options.url > > + * The path to the devtools page html page relative to the extension base URL. > > + */ > > +class DevtoolsPage extends HiddenExtensionPage { > > On the DevTools team, "DevTools" is the more preferred casing choice instead > of "Devtools". If it doesn't go against your own casing policies, it would > be nice to use this. Sure, I'm going to update these patches (and the other patches that are going to be applied on top of these ones) to use the above casing convention.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review95304 > I would really like to move these helpers into the context, but the context here is an `ExtensionChildProxyContext`, which is defined at toolkit level, while the devtools API is defined and available only at browser level > and so I tried to keep inside the ext-devtools.js module the tracking of the additional properties needed by the devtools APIs (which is used by both the devtools_page and devtools_panel contexts). Just create a context class that's a subclass of ExtensionChildProxyContext. > I was assuming that it was still necessary (and the background page still uses it, e.g. https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#79-80) > > May I ask why is not needed anymore? If it comes to it, it was never really needed.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review95304 > Also probably belongs in the context. In the refactoring from the last pushed updates on these patches, all the other methods of this helper class are now methods of the new DevToolsExtensionPageContextParent class defined in ExtensionParent.jsm, besides this one, which I still think that it better fits into this ext-devtools.js, or at least it doesn't fit into the "ExtensionParent.jsm" (e.g. because, as I was trying to explain in my previous comment this helper needs to use the devtools loader to retrieve a devtools module, ""devtools/client/framework/target").

Comment on attachment 8809266 [details] Bug 1291737 - Export TabTarget from 'devtools/client/framework/target' module. https://reviewboard.mozilla.org/r/91850/#review97734

Hi Kris, is there anything that I can do to make it easier for you to complete the next round of review comments of these patches? This bug is a blocker for every one of the other patches related to the WebExtensions DevTools APIs (in particular Bug 1300584, Bug 1300587 and Bug 1300588, which already have patches attached)

Comment on attachment 8783663 [details] Bug 1291737 - Added a new helper to create windowless extension pages. https://reviewboard.mozilla.org/r/73390/#review104986 r=me with comments addressed. ::: toolkit/components/extensions/ExtensionUtils.jsm:1200 (Diff revision 13) > + * @param {Extension} extension > + * the Extension which owns the hidden extension page created (used to decide > + * if the hidden extension page parent doc is going to be a windowlessBrowser or > + * a visible XUL window) > + */ > +class HiddenExtensionPage { This should be in ExtensionParent. ::: toolkit/components/extensions/ExtensionUtils.jsm:1202 (Diff revision 13) > + * if the hidden extension page parent doc is going to be a windowlessBrowser or > + * a visible XUL window) > + */ > +class HiddenExtensionPage { > + constructor(extension) { > + this.extension = extension; Please set `parentWindow` and `windowlessBrowser` to null here. ::: toolkit/components/extensions/ExtensionUtils.jsm:1231 (Diff revision 13) > + * in the created browser element (e.g. "background" or "devtools_page"). > + * > + * @returns {Promise<XULElement>} > + * a Promise which resolves to the newly created browser XUL element. > + */ > + createBrowserElement(viewType) { Please set the view type in the constructor. And make sure that this can only be called once. ::: toolkit/components/extensions/ExtensionUtils.jsm:1242 (Diff revision 13) > + } else { > + awaitFrameLoader = Promise.resolve(); > + } No `else` branch, please. `let awaitFrameLoader = Promise.resolve();` ::: toolkit/components/extensions/ExtensionUtils.jsm:1319 (Diff revision 13) > + /** > + * Private helper that creates and initializes the parent document for an hidden extension page. > + * > + * @returns {Promise<nsIXULDocument>} > + * the created parent chrome document. > + */ > + createParentDocument() { > + if (this.extension.remote) { > + return this.createWindowedBrowser(); > + } > + > + return this.createWindowlessBrowser(); > + } Please fold this into `createBrowserElement`. ::: toolkit/components/extensions/ExtensionUtils.jsm:1359 (Diff revision 13) > + return promiseObserved("chrome-document-global-created", > + win => win.document == chromeShell.document); > + } > +} > + > +function promiseExtensionViewLoaded(browser) { Also belongs in ExtensionParent. ::: toolkit/components/extensions/ext-backgroundPage.js:45 (Diff revision 13) > - } > + } > > - chromeDoc.documentElement.appendChild(browser); > - yield awaitFrameLoader; > - > + build() { > + return Task.spawn(function* () { > + const browser = yield this.createBrowserElement("background"); > this.browser = browser; Please set this from `createBrowserElement`, and initially set it to null in the constructor. ::: toolkit/components/extensions/ext-backgroundPage.js:76 (Diff revision 13) > if (this.browser) { > this.browser.remove(); > this.browser = null; > } > > // Navigate away from the background page to invalidate any > // setTimeouts or other callbacks. > if (this.webNav) { > this.webNav.loadURI("about:blank", 0, null, null, null); > this.webNav = null; > } Move to the super class.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review105004 r=me with comments addressed. ::: browser/components/extensions/ext-devtools.js:19 (Diff revision 11) > +XPCOMUtils.defineLazyModuleGetter(this, "Task", > + "resource://gre/modules/Task.jsm"); > + > +Cu.import("resource://gre/modules/ExtensionUtils.jsm"); > + > +const {HiddenExtensionPage} = ExtensionUtils; Please use the same multi-line formatting we use elsewhere. ::: browser/components/extensions/ext-devtools.js:109 (Diff revision 11) > + this.toolbox = options.toolbox; > + > + this.onExtensionProxyContextLoad = this.onExtensionProxyContextLoad.bind(this); > + this.waitForTopLevelContext = new Promise((resolve, reject) => { > + this.resolveTopLevelContext = resolve; > + this.rejectTopLevelContext = reject; This is unused. ::: browser/components/extensions/ext-devtools.js:116 (Diff revision 11) > + } > + > + build() { > + return Task.spawn(function* () { > + const browser = yield this.createBrowserElement("devtools_page"); > + this.browser = browser; Move to parent class. ::: browser/components/extensions/ext-devtools.js:135 (Diff revision 11) > + }.bind(this)); > + } > + > + shutdown() { > + if (this.destroyed) { > + return; Please throw instead. ::: browser/components/extensions/ext-devtools.js:143 (Diff revision 11) > + if (this.browser) { > + this.browser.remove(); > + this.browser = null; > + } Move to parent class. ::: browser/components/extensions/ext-devtools.js:151 (Diff revision 11) > + } > + > + super.shutdown(); > + } > + > + onExtensionProxyContextLoad(event, context) { Can we make this a helper in ExtensionParent instead? ::: browser/components/extensions/ext-devtools.js:157 (Diff revision 11) > + if (context.viewType == "devtools_page" && context.xulBrowser == this.browser) { > + // Keep track of the toolbox and target associated to the context, which is > + // needed by the API methods implementation. > + context.setDevToolsToolbox(this.toolbox); > + > + if (!this.topLevelContext) { Please throw in this case. ::: browser/components/extensions/ext-devtools.js:169 (Diff revision 11) > + close() { > + this.shutdown(); > + } Please remove this alias and just rename `shutdown()` ::: browser/components/extensions/ext-devtools.js:213 (Diff revision 11) > + > + return devtoolsPage.build(); > + } > + > + shutdownForTarget(target) { > + if (this.devtoolsPageForTarget.has(target)) { Please report an error if this is not the case. ::: browser/components/extensions/ext-devtools.js:224 (Diff revision 11) > + > + shutdown() { > + for (let target of this.devtoolsPageForTarget.keys()) { > + this.shutdownForTarget(target); > + } > + this.devtoolsPageForTarget.clear(); This shouldn't be necessary. Please assert that it's empty, instead. ::: browser/components/extensions/ext-devtools.js:234 (Diff revision 11) > + // Only local tabs are currently supported (See Bug 1304378 for additional details > + // related to remote targets support). Please report a warning when this happens. ::: browser/components/extensions/schemas/devtools.json:1 (Diff revision 11) > +// Copyright (c) 2012 The Chromium Authors. All rights reserved. > +// Use of this source code is governed by a BSD-style license that can be > +// found in the LICENSE file. > + Please remove.

Comment on attachment 8816575 [details] Bug 1291737 - Renamed DevtoolsContextChild to DevToolsContextChild. https://reviewboard.mozilla.org/r/97254/#review105008

Comment on attachment 8816576 [details] Bug 1291737 - Changes to ExtensionParent JSM for the DevTools APIs. https://reviewboard.mozilla.org/r/97256/#review105010 r=me with comments addressed. ::: toolkit/components/extensions/ExtensionParent.jsm:379 (Diff revision 1) > +/** > + * The parent side of proxied API context for devtools extension page, such as a > + * devtools pages and panels running in ExtensionChild.jsm. > + */ > +class DevToolsExtensionPageContextParent extends ExtensionPageContextParent { > + setDevToolsToolbox(toolbox) { Please use ordinary setters rather than setter methods. ::: toolkit/components/extensions/ExtensionParent.jsm:408 (Diff revision 1) > + get devToolsTarget() { > + return this._devToolsTarget; > + } > + > + shutdown() { > + if (this._devToolsTarget) { Please report an error if this is not defined.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review105004 > Can we make this a helper in ExtensionParent instead? turned into an helper defined in ExtensionParent.jsm (`watchExtensionProxyContextLoaded`). > Please throw in this case. I cannot throw here because it is supposed to receive the contexts that are related to iframe descendants of the top level context (and associate the toolbox to the devtools context, so that in the mozextensions iframes the devtools API works correctly). > Please report a warning when this happens. This is a global devtools listener, and it will receive this event for the toolboxes that we don't support yet, e.g. the remote targets, and they are filtered out here by the if statements that follows)

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review105004 > I cannot throw here because it is supposed to receive the contexts that are related to iframe descendants of the top level context (and associate the toolbox to the devtools context, so that in the mozextensions iframes the devtools API works correctly). OK. > This is a global devtools listener, and it will receive this event for the toolboxes that we don't support yet, e.g. the remote targets, and they are filtered out here by the if statements that follows) The problem is that the lack of support will confuse developers, so they should at least be made aware of it with a console warning.

Comment on attachment 8794136 [details] Bug 1291737 - Implements the devtools_page context. https://reviewboard.mozilla.org/r/80716/#review105004 > The problem is that the lack of support will confuse developers, so they should at least be made aware of it with a console warning. oh yeah, that sounds reasonable to me, thanks for clarifying it!

In the last push to mozreview: - integrated the last minor changes based on the last feedback - patches rebased one more time on a recent mozilla-central tip One last push to try: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=055a5b7a4c1066f53f8f9d9c610776aebdfa94b0 (I'm also clearing the pending needinfo, because it is not needed anymore)

None of the tests in the push to try linked in Comment 120 seems to contain any failures, but I looked one more time into these patches for any potential source of leaking (or class instances that risk to be closed multiple times) and I found some additional change needed in this set of patches: - prevent the HiddenExtensionPage's shutdown method to be called twice (and raises an exception if it happens) - ensure that a DevToolsPage instance is always removed from the map of "DevToolsPage per target" that is managed by their related DevToolsPageDefinition - ensure that a closed DevToolsPage is not closed again once its related topLevelContext has been closed

Tomcat autolanded this, but pulsebot bug marking is currently broken. https://hg.mozilla.org/integration/autoland/rev/702eed7d62fa https://hg.mozilla.org/integration/autoland/rev/889eb9b46665 https://hg.mozilla.org/integration/autoland/rev/00f26cb7131e https://hg.mozilla.org/integration/autoland/rev/235ba91834d5 https://hg.mozilla.org/integration/autoland/rev/71414e9dc3ee https://hg.mozilla.org/integration/autoland/rev/6ff304ff8518

sorry had to back this out, this caused merge regressions with mozilla-central in toolkit/components/extensions/ExtensionParent.jsm toolkit/components/extensions/ext-backgroundPage.js

(In reply to Carsten Book [:Tomcat] from comment #131) > sorry had to back this out, this caused merge regressions with > mozilla-central in > toolkit/components/extensions/ExtensionParent.jsm > toolkit/components/extensions/ext-backgroundPage.js Yep, my guess is that the conflicts are coming from Bug 1320395 (which landed recently on mozilla-central and it was coming from inbound). I'm updating mozilla-central right now and working on resolving the conflicts.

Hi Carsten and Ryan, I reopened the mozreview related to this issue and pushed on it the updated patches (rebased on a recent mozilla-central tip and conflicts resolved). I also pushed them on try (there weren't many changes needed to solve conflicts and so there shouldn't be any new issue): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca7dbe40005938d4b39566600b85123743ec29e7 Thanks again for your help!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/52c940b9aff6 Added a new helper to create windowless extension pages. r=kmag https://hg.mozilla.org/integration/autoland/rev/dc42f50d06e8 Renamed DevtoolsContextChild to DevToolsContextChild. r=kmag https://hg.mozilla.org/integration/autoland/rev/0d88c3c271d2 Changes to ExtensionParent JSM for the DevTools APIs. r=kmag https://hg.mozilla.org/integration/autoland/rev/6b78221fc2ab Export TabTarget from 'devtools/client/framework/target' module. r=ochameau https://hg.mozilla.org/integration/autoland/rev/72d47e72ed1a Implements the devtools_page context. r=kmag https://hg.mozilla.org/integration/autoland/rev/bec155412573 implements devtools.inspectedWindow.tabId. r=kmag

https://hg.mozilla.org/mozilla-central/rev/52c940b9aff6 https://hg.mozilla.org/mozilla-central/rev/dc42f50d06e8 https://hg.mozilla.org/mozilla-central/rev/0d88c3c271d2 https://hg.mozilla.org/mozilla-central/rev/6b78221fc2ab https://hg.mozilla.org/mozilla-central/rev/72d47e72ed1a https://hg.mozilla.org/mozilla-central/rev/bec155412573

Backout by philringnalda@gmail.com: https://hg.mozilla.org/mozilla-central/rev/06749abe0260 Backed out changeset 6ff304ff8518 https://hg.mozilla.org/mozilla-central/rev/eee33eb6c7fd Backed out changeset 71414e9dc3ee https://hg.mozilla.org/mozilla-central/rev/4aaca43c6884 Backed out changeset 235ba91834d5 https://hg.mozilla.org/mozilla-central/rev/b80cc52e2058 Backed out changeset 00f26cb7131e https://hg.mozilla.org/mozilla-central/rev/2d8e2075c434 Backed out changeset 889eb9b46665 https://hg.mozilla.org/mozilla-central/rev/7a14625f2b87 Backed out changeset 702eed7d62fa for causing merge regression with m-c

It looks like the Pulsebot Comment 142 is referred to the backout that was already mentioned in Comment 130 and Comment 131. The updated patches (rebased on top of the changes that introduced the conflicts) have been landed successfully as mentioned by Comment 141 (and how it can be verified by looking at the related push: https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=52c940b9aff6)

I donwload this and tried in the dev edition loading from about:debugging but i don't get a new panel or in the list of default dev tools in the settings they are not available. I am missing something?

Sorry i forgot the link: https://github.com/GoogleChrome/devtools-extension-boilerplate As I can see in the patches we use browser.devtools but in that example they use chrome.devtools. Also after adding the 'devtools' in the permission now they are loaded and i can debug but i got error about that both the api are not available on Firefox dev edition 53.0a2 of 2017-01-29

(In reply to Daniele "Mte90" Scasciafratte from comment #144) > I donwload this and tried in the dev edition loading from about:debugging > but i don't get a new panel or in the list of default dev tools in the > settings they are not available. > I am missing something? Yep, the devtools APIs necessary to be able to define a new devtools panel will be provided by Bug 1300587 which has not landed yet (e.g. the first two examples from the link in Comment 145 are going to define a devtools panel). >As I can see in the patches we use browser.devtools but in that example they use chrome.devtools. The DevTools API are going to be provided through both the "browser.*" and "chrome.*" API namespaces, with the "browser.*" that provides the Promise-based API as usually (and "chrome.*" that provides the chrome-compatible "callbacks"-based API). > Also after adding the 'devtools' in the permission... There is no "devtools" permission needed for the "devtools" API, the "devtools" API namespace is only available in DevTools-specific extension contexts (in particular in the "devtools_page", and in the "devtools_panel" once Bug 1300587 has been landed).

Perfect thank you for the clarification :-) So I will wait for Bug 1300587 to try again that API.