The goal of this issue is introducing, in the devtools.inspectedWindow.eval method, support for the $0 and inspect bindings. These two bindings should provide, basically as supported by the webconsole actor, the reference to the DOM element currently selected in the Inspector tool associated to the "$0" identifier, and an `inspect` helper method (that select the DOM element passed as parameter in the Inspector tool and switch to the Inspector panel). The $0 bindings is often used by devtools addons to get informations from the currently selected node in the inspector, the `inspect` helper method to open a DOM element in the Inspector (both these bindings provide a basic but effective integration between the addon panels and the integrated) The $0 and inspect bindings should be implemented taking into account that the connection that the WebExtension devtools contexts (e.g. the devtools_page and the devtools_panel) is not the same connection used by the integrated developer tools (which means that the actor ID that we can retrieve from the devtools target instance used by the integrated developer tools has to be translated into an valid actor ID for the connection associated to that particular context).

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 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. Hi Alex, Like briefly discussed in our last meeting, now that we landed the devtools.inspectedWindow.eval API, this issue has now an higher priority, because the devtools addons often use this two bindings to better integrate the custom panel with the regular developer tools. We have to consider that the devtools addons are connected to the target (the same target of the toolbox they are running into) using a different connection from the one used by the regular developer tools, and this is the first devtools API issue that is affected by this design choice (like we discussed while we were working on the initial subset of the devtools API, we knew that we were entering an uncharted territory). To prototype the feature I initially used the existent DebuggerServer's '_searchAllConnectionsForActor' method, which was ok to explore it, but it definitely can't be a reasonable approach for the final patch (e.g. to retrieve the actor this helper iterates on all the open connection tracked by that DebuggerServer instance), and so I spent some time on identifying and experimenting a better strategy for retrieving an actor from a different connection tracked by the same DebuggerServer instance. This patch contains the approach that I currently chose as the preferred one (alongside with to the changes needed to the devtools.inspectedWindow.eval API method, the related actor and its spec, and a couple of additional tests that I used to guide my search for a better approach). The current approach is based on the following changes to the devtools internals: - a childConnectionPrefix property added to the tab actor (which is used by the API implementation to send the connection prefix of the target actor in the RDP requests sent to the webextension inspected window actor) - a connectionPrefix property in the root initial hello packed, to be used as a fallback (which I think that is not really needed and it can be removed until the devtools addons targets are restricted to the regular webpage tabs) - a new `getActorFromConnection` method in the DebuggerServer (this try to retrieve the actor instance given the connection and actor IDs) Another interesting part of the patch is the way the devtools API implementation and the actor collaborate to achieve the needed integrations on the client side (e.g. switch to the inspector panel and select a node, or opening the split console and open the sidebar to inspect an object that is not a DOM node), which has been modelled on the same kind of interaction between the webconsole actor and the regular developer tools panels (basically an helperResult property is included in the actor result when some help on the client side is needed to complete the request on the client side). Let me know how the approach looks to you (so that I can proceed to create more tests if the approach is ok, or look for a better approach if there are downsides in this approach that I'm not seeing yet). Thanks in advance for your help.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review129386 ::: devtools/server/actors/webextension-inspected-window.js:444 (Diff revision 1) > + Object.defineProperty(bindings, "$0", { > + enumerable: true, > + configurable: true, > + get() { > + return selectedNodeActor ? > + dbgWindow.makeDebuggeeValue(selectedNodeActor.rawNode) : undefined; This toolboxConnectionPrefix, toolboxConsoleActorID and toolboxSelectedNodeActorID dance is way too complex. It would have been much simplier if the currently selected node was stored in the server rather than the client... What about passing the unique selector from the client? options.selectedNodeSelector = yield selectedNode.nodeFront.getUniqueSelector(); front.eval(..., options); And from here: let node = this.window.document.querySelector(selectedNodeSelector); dbgWindow.makeDebuggerValue(node); This won't work with nodes from iframes, but you could surely factorize some code with devtools-browser to support cross-frame selectors: http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#302-307 (i.e. put that notion of cross frame selector in css-logic module and expose a getCrossFrameSelector on NodeActor) You can do that in a followup if that helps. Then, it means that you have to also find something simplier for inspect. Can we assume a toolbox will always be opened? I think that was your conclusion about the devtools as an addon. devtools API are only working if there is a toolbox opened. Is there a case where you would call inspectedWindow.eval without any toolbox up and running? In such case we may send an event to the existing tabactor or console actor in order to ask it to send an inspect event to its toolbox client. Using some new event emitter on DebuggerServer or Services.obs. You could pass a raw reference to the js object to inspect. Then tab.js or webconsole.js would listen for that and send an event to its client. toolbox.js on the client side would listen for that and open the console and the variableview. I'm very open to simpliers alternatives. The current patch is just way too hairy. ::: devtools/server/actors/webextension-inspected-window.js:469 (Diff revision 1) > + toolboxConsoleActor.objectGrip); > + helperResult = { > + type: "inspectObject", > + input: expression, > + object: grip, > + }; It means that only calls to inspect() done during direct script evaluation are going to be considered. if the eval script looks like this, inspect won't do anything: setTimeout(() => inspect(document.body)); window.onclick = () => inspect(document.body);

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review129386 > This toolboxConnectionPrefix, toolboxConsoleActorID and toolboxSelectedNodeActorID dance is way too complex. > It would have been much simplier if the currently selected node was stored in the server rather than the client... > > What about passing the unique selector from the client? > options.selectedNodeSelector = yield selectedNode.nodeFront.getUniqueSelector(); > front.eval(..., options); > And from here: > let node = this.window.document.querySelector(selectedNodeSelector); > dbgWindow.makeDebuggerValue(node); > > This won't work with nodes from iframes, > but you could surely factorize some code with devtools-browser to support cross-frame selectors: > http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools-browser.js#302-307 > (i.e. put that notion of cross frame selector in css-logic module and expose a getCrossFrameSelector on NodeActor) > You can do that in a followup if that helps. > > Then, it means that you have to also find something simplier for inspect. > Can we assume a toolbox will always be opened? > I think that was your conclusion about the devtools as an addon. devtools API are only working if there is a toolbox opened. Is there a case where you would call inspectedWindow.eval without any toolbox up and running? > > In such case we may send an event to the existing tabactor or console actor in order to ask it to send an inspect event to its toolbox client. > Using some new event emitter on DebuggerServer or Services.obs. You could pass a raw reference to the js object to inspect. Then tab.js or webconsole.js would listen for that and send an event to its client. toolbox.js on the client side would listen for that and open the console and the variableview. > > I'm very open to simpliers alternatives. > The current patch is just way too hairy. I definitely agree that the previous approach wasn't great from many point of views, also I wasn't happy with it and your feedback has been very helpful (as usually) to point me other possible strategies. Like briefly discussed over IRC, the main reason why I would like to find a reasonable approach that doesn't use a CSS selector (for the toolbox selected node part of this issue) is that we are likely to need something similar in the near future also for other integrations between the devtools addons and the integrated devtools that cannot probably use this trick (e.g. the current selected request in the network panel, the current selected source code, the current activated breakpoint etc.). The new version makes use of another of the suggestions in the comment above (using the observer service to explicitly allow two actors running in the same process but in different RDP connection to be able to integrate each other), I'm not very happy with the current "naming" (mainly: I don't like the "enableDevToolsExtensionsRequests" / "disableDevToolsExtensionsRequests" method names) but I think that the approach is way better than the previous one: it is more explicit, and it ensure that the toolbox actors involved are explicitly aware of the "integration"s and every actor provides the "integrations" needed only for the actors under their own responsability (e.g. the inspectorActor is the one that resolve the selectedNodeActorID into the DOM raw node, the consoleActor is the one that "inspect an object", by creating the object grip and send the unsolicited event to the client part). As a result, the change on the webextension API implementation is much smaller, and the responsabilities are better splitted between the involved components. The new approach has required more changes in the devtools components, but they seems to be reasonable small and applied to the components that already have other similar responsabilities (unfortunately the webconsole often requires more changes because it doesn't use the protocol.js abstractions, but the changes applied are still pretty small). NOTE: I'm not clearing this issue yet, as a reminder that we are still evaluating our preferred approach. > It means that only calls to inspect() done during direct script evaluation are going to be considered. > > if the eval script looks like this, inspect won't do anything: > > setTimeout(() => inspect(document.body)); > window.onclick = () => inspect(document.body); The new version should also fix this scenario: the new "inspect" binding implementation send a request to the toolbox to inspect an objectActor using an RDP unsolicited event. (the event is sent from the webconsole actor to the client, the Target class subscribes this unsolicited event and routes it to the toolbox, and the toolbox is now the component responsible to decide how it is better to proceed to inspect the objectActor, e.g. if the object actor is a DOM node, it should be selected in the inspector panel, which have to be initialized if it has been never selected before, if it is not a DOM node then the toolbox can open the split console and inspect the object in the split console sidebar).

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. Renewed the feedback request flag, for a preliminary feedback related to the new proposed approach.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review136220 With all these comments It looks like we could end up having to pass only "toolboxSelectedNodeActorID: selectedNode.nodeFront.actorID" as argument to eval() request. ::: browser/components/extensions/ext-devtools-inspectedWindow.js:37 (Diff revision 2) > + if (context.devToolsToolbox.inspector && > + selectedNode && selectedNode.nodeFront) { > + // If there is a selected node in the inspector, we add the > + // inspector and node actor ids to the eval request optons, > + // used to provide the "$0" binding. > + options.toolboxSelectedNodeActorID = selectedNode.nodeFront.actorID; I imagine you could pass selectorNode.nodeFront.actorID even if context.devToolsToolbox.inspector is undefined/null. (so you could move this line out of this condition) ::: browser/components/extensions/ext-devtools-inspectedWindow.js:38 (Diff revision 2) > + selectedNode && selectedNode.nodeFront) { > + // If there is a selected node in the inspector, we add the > + // inspector and node actor ids to the eval request optons, > + // used to provide the "$0" binding. > + options.toolboxSelectedNodeActorID = selectedNode.nodeFront.actorID; > + options.toolboxInspectorActorID = toolbox.target.form.inspectorActor; Aren't actorID unique, even across connections? http://searchfox.org/mozilla-central/source/devtools/server/actors/common.js#243 actor.actorID = this.conn.allocID(prefix || undefined); http://searchfox.org/mozilla-central/source/devtools/server/main.js#1493 allocID(prefix) { return this.prefix + (prefix || "") + this._nextID++; http://searchfox.org/mozilla-central/source/devtools/server/main.js#1215 connID = "server" + loader.id + ".conn" + this._nextConnID++ + "."; ... new DebuggerServerConnection(connID, transport); So it should look like that: server{loaderID}.conn{ConnectionID}{ActorPrefix}{ActorID} ::: browser/components/extensions/ext-devtools-inspectedWindow.js:53 (Diff revision 2) > + options.toolboxConsoleActorID = toolbox.target.form.consoleActor; > + > + // Enable the toolbox webconsole actor to handle requests received > + // from the actors related to the devtools extensions connections. > + // (so that is can help to implement the inspect binding). > + await toolbox.target.activeConsole.enableDevToolsExtensionsRequests(); I'm not sure it is worth explicitely calling {enable,disable}DevToolsExtensionsRequests. It can be considerer as being cheap enough to be listening at each actor instanciation (console and inspector). ::: devtools/client/framework/toolbox.js:2228 (Diff revision 2) > + ); > + return; > + } > + yield this.selection.setNodeFront(nodeFront, inspectFromAnnotation); > + yield this.selectTool("inspector"); > + } It looks like there is some code to share with the console: http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3166 http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3253 But feel free to consider that as a nice to have and/or do it as a followup. ::: devtools/server/actors/inspector.js:2747 (Diff revision 2) > get window() { > return this.tabActor.window; > }, > > + enableDevToolsExtensionsRequests() { > + devToolsExtensionsUtils.subscribe(this, { I'm note convinced making a "per actor ID" API is any useful, at least for this current patch? I was thinking about something simplier like: DebuggerServer.on("get-raw-node", (actorID, callback) => { let actor = this.conn.getActor(actorID); if (actor) { callback(actor.rawNode); } }); DebuggerServer will be shared between actors living in the same loader. I think it is fine for your purpose? (The typical scenario of duplicated loader is when you are debugging a tab, via a regular toolbox. And opening a browser toolbox, for debugging firefox, is going to spawn another loader and set of actors) Note that for this very particular request, we may introduce a method on DebuggerServer to retrieve the actor of any active connection: DebuggerServer.getActor = (actorID) => { for (let conn of this._connections) { let actor = conn.getActor(conn); if (actor) return actor; } } There is surely a faster implementation if we consider the actorID pattern and fetch the connID out of the actorID string. ::: devtools/server/actors/utils/devtools-extensions-utils.js:21 (Diff revision 2) > + * toolbox, and so the actors that are accessible to the integrated tools (e.g. like > + * the current selected node, an object actor to open in the webconsole variables view > + * sidebar, the current selected request in the network panel) have to be able to > + * be explicitly exposed to the other "devtools extensions" connections related to the > + * same target. > + */ I'm not fully convince of the value of such helper. But if you prefer to keep it, simplify the name: * No need to have "devtools" in the name of a devtool's helper, * Even if you are using it just for WebExtension, I don't see this helper as specific to it, so I wouldn't using "extensions" in the name, * "utils" should be banned in any project ;) Ok, so we just have the dashes left. What about CrossActorEvents/ActorEvents/CrossActorRequests/ActorRequests/... But again, weight the value of this helper versus using DebuggerServer EventEmitter API. ::: devtools/server/actors/webconsole.js:31 (Diff revision 2) > loader.lazyRequireGetter(this, "addWebConsoleCommands", "devtools/server/actors/utils/webconsole-utils", true); > loader.lazyRequireGetter(this, "CONSOLE_WORKER_IDS", "devtools/server/actors/utils/webconsole-utils", true); > loader.lazyRequireGetter(this, "WebConsoleUtils", "devtools/server/actors/utils/webconsole-utils", true); > loader.lazyRequireGetter(this, "EnvironmentActor", "devtools/server/actors/environment", true); > > +loader.lazyGetter(this, "devToolsExtensionsUtils", nit: loader.lazyRequireGetter(this, "devToolsExtensionsUtils", "./utils/devtools-extensions-utils"); ::: devtools/server/actors/webextension-inspected-window.js:14 (Diff revision 2) > const {Ci, Cu, Cr} = require("chrome"); > > -const Services = require("Services"); > +loader.lazyGetter(this, "devToolsExtensionsUtils", > + () => require("./utils/devtools-extensions-utils"), true); > +loader.lazyGetter(this, "DevToolsUtils", () => require("devtools/shared/DevToolsUtils"), true); > +loader.lazyGetter(this, "Services", () => require("Services"), true); nit: There is no need to lazy load Services and DevToolsUtils as that's the typical modules that will be loaded anyway. nit2: There is a shorter helper for commonjs modules: loader.lazyRequireGetter(this, "devToolsExtensionsUtils", "./utils/devtools-extensions-utils");

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. We are getting there! I think you can r? your next patch.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review136220 > I imagine you could pass selectorNode.nodeFront.actorID even if context.devToolsToolbox.inspector is undefined/null. > (so you could move this line out of this condition) If there is no inspector actor there will be nothing listening to the "get-raw-dom-node" event, but I guess that it is reasonable to still have the $0 binding defined, it will return undefined when it is not able to resolve the node actor ID into the raw dom node. Moved out of the `if (toolbox.inspector) { ... }` condition in the updated patch. > Aren't actorID unique, even across connections? > > http://searchfox.org/mozilla-central/source/devtools/server/actors/common.js#243 > actor.actorID = this.conn.allocID(prefix || undefined); > > http://searchfox.org/mozilla-central/source/devtools/server/main.js#1493 > allocID(prefix) { > return this.prefix + (prefix || "") + this._nextID++; > > http://searchfox.org/mozilla-central/source/devtools/server/main.js#1215 > connID = "server" + loader.id + ".conn" + this._nextConnID++ + "."; > ... > new DebuggerServerConnection(connID, transport); > > So it should look like that: > server{loaderID}.conn{ConnectionID}{ActorPrefix}{ActorID} Yes, the actor ID is unique, the only thing that we need to ensure is that only one actor is going to try to serve the "get-raw-dom-node" event, the actor that serve the toolbox RDP clients. Anyway, if we still enable the "get-raw-dom-node" explicitly on the actor which serve the toolbox RDP clients, we can definitely avoid to send the inspector actor ID in the eval request (which is great!!!). > I'm not sure it is worth explicitely calling {enable,disable}DevToolsExtensionsRequests. > It can be considerer as being cheap enough to be listening at each actor instanciation (console and inspector). As described above, the main reason why I think that it is reasonable to explicitly enable the actors that will serve this "cross actor requests" is that we want to be absolutely sure that they are only handled by the actors that serve the toolbox RDP clients (in other words: the integrated panels), and not one of the actors coming from one of the other connections related to the same DebuggerServer instance (e.g. every addon is going to get its own connection connected to the same DebuggerServer instance, one for every devtools page/panel top level frame, and one for anyone of their sub iframes if any). To make even more explicit that this method should only be called on the toolbox actors to make them able to serve the cross-actor requests related to "toolbox integrations", I'm proposing to rename it to `enableToolboxActorRequests`. I agree that if we use the DebuggerServer events, listening to this requests is pretty cheap, if we prefer, we can call the `enableToolboxActorRequests` RDP method from the toolbox when we connect the actors for the first time (instead of enabling it on demand from the webextensions API). How that sounds to you? > It looks like there is some code to share with the console: > http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3166 > http://searchfox.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3253 > > But feel free to consider that as a nice to have and/or do it as a followup. Oh yeah, there is definitely some code to share here, I think that I'll file a follow up issue to handle it (I want to look a bit deeper into both the old webconsole and new webconsole clients to be sure that I share this piece correctly) > I'm note convinced making a "per actor ID" API is any useful, at least for this current patch? > > I was thinking about something simplier like: > DebuggerServer.on("get-raw-node", > (actorID, callback) => { > let actor = this.conn.getActor(actorID); > if (actor) { > callback(actor.rawNode); > } > }); > > DebuggerServer will be shared between actors living in the same loader. I think it is fine for your purpose? > (The typical scenario of duplicated loader is when you are debugging a tab, via a regular toolbox. And opening a browser toolbox, for debugging firefox, is going to spawn another loader and set of actors) > > Note that for this very particular request, > we may introduce a method on DebuggerServer to retrieve the actor of any active connection: > DebuggerServer.getActor = (actorID) => { > for (let conn of this._connections) { > let actor = conn.getActor(conn); > if (actor) > return actor; > } > } > There is surely a faster implementation if we consider the actorID pattern and fetch the connID out of the actorID string. +1 for the "DebuggerServer events" strategy! it is definitely cleaner, simpler and cheaper of the observer service approach. I've rewritten this patch with this strategy and remove the devtools-extensions-utils module from the patch. I still prefer if we can keep the toolbox inspector actor the one responsible to resolve the node actor into the raw dom node and the toolbox webconsole actor the one responsible to handle the inspect object actor, mostly because it is more explicit and it is able to handle both the needed integrations with the same strategy, which helps to make us confident that we can reuse the exactly same strategy once we need to integrate the devtools webextensions APIs with other toolbox panels. > I'm not fully convince of the value of such helper. > But if you prefer to keep it, simplify the name: > * No need to have "devtools" in the name of a devtool's helper, > * Even if you are using it just for WebExtension, I don't see this helper as specific to it, so I wouldn't using "extensions" in the name, > * "utils" should be banned in any project ;) > Ok, so we just have the dashes left. > What about CrossActorEvents/ActorEvents/CrossActorRequests/ActorRequests/... > > But again, weight the value of this helper versus using DebuggerServer EventEmitter API. Me neither, the "DebuggerServer events" strategy makes it way simpler, and so I've removed the helper module from the updated patch. (the enable/disable RDP request types are there for the reason described above, but I've renamed them to `enableToolboxActorRequests/disableToolboxActorRequests` to make it explicit that it is about integrating with the toolbox actor, but I'm open to rename them using one of the names suggested above if we prefer to not use `Toolbox` in the method name)

(In reply to Alexandre Poirot [:ochameau] from comment #9) > Comment on attachment 8851377 [details] > Bug 1300590 - Implement support for $0 and inspect bindings in > devtools.inspectedWindow.eval. > > We are getting there! I think you can r? your next patch. Thanks Alex, the feedback has been super-useful, I like the proposed "DebuggerServer event" strategy and how it makes the new version of this patch way simpler. I've added some comments on mozreview (Comment 11) with some rationale related to the reason to subscribe this event listeners explicitly and only on the toolbox actors (and how to name these RDP methods, and where we should call them, eg. the "webextensions devtools API" vs. the "toolbox client itself when it creates the related RDP clients"). Anyway, I totally agree that we are definitely getting near there ;-), and so I'm moving the patch in the review stage as suggested.

(In reply to Luca Greco [:rpl] from comment #11) > Comment on attachment 8851377 [details] > Yes, the actor ID is unique, the only thing that we need to ensure is that > only one actor is going to try to serve the "get-raw-dom-node" event, the > actor that serve the toolbox RDP clients. If the actor ID is unique, only one actor is going to reply something. So got "get-raw-dom-node" I still think we can always listen. You should just ensure not throwing an error if you didn't found any matching actor in the get-raw-dom-node listener. > > I'm not sure it is worth explicitely calling {enable,disable}DevToolsExtensionsRequests. > > It can be considerer as being cheap enough to be listening at each actor instanciation (console and inspector). > > As described above, the main reason why I think that it is reasonable to > explicitly enable the actors that will serve this "cross actor requests" is > that we want to be absolutely sure that they are only handled by the actors > that serve the toolbox RDP clients (in other words: the integrated panels), > and not one of the actors coming from one of the other connections related > to the same DebuggerServer instance (e.g. every addon is going to get its > own connection connected to the same DebuggerServer instance, one for every > devtools page/panel top level frame, and one for anyone of their sub iframes > if any). > > To make even more explicit that this method should only be called on the > toolbox actors to make them able to serve the cross-actor requests related > to "toolbox integrations", I'm proposing to rename it to > `enableToolboxActorRequests`. > > I agree that if we use the DebuggerServer events, listening to this requests > is pretty cheap, if we prefer, we can call the `enableToolboxActorRequests` > RDP method from the toolbox when we connect the actors for the first time > (instead of enabling it on demand from the webextensions API). "from the toolbox when we connect the actors for the first time" How would that be any different than always listening? I get your point about toolbox-actor-request:inspect-object. If you get two tabs, with toolbox opened in both, calling inspect(...) would open the split console and inspect the object in both toolboxes. You were right in your very first patch, by sending the console actor to eval request. I think that is the only way to get that right. But I'm still convinced these enable/disable request provide a false sense of correctness. If you happen to use web-ext eval method from both toolboxes, you would still have inspection running in both toolboxes. I would suggest passing toolbox.target.form.consoleActor to eval request, then pass the consoleActorID to toolbox-actor-request:inspect-object message and from _inspectObjectActor, compare the given consoleActorID with this.actorID. (That would redo what you did in your first patch) > I still prefer if we can keep the toolbox inspector actor the one > responsible to resolve the node actor into the raw dom node and the toolbox > webconsole actor the one responsible to handle the inspect object actor, > mostly because it is more explicit and it is able to handle both the needed > integrations with the same strategy, which helps to make us confident that > we can reuse the exactly same strategy once we need to integrate the > devtools webextensions APIs with other toolbox panels. I would like to see you elaborate as it isn't clear why a message is any better. But may be I miss some future usecase. Again, with this: DebuggerServer.getActor = (actorID) => { for (let conn of this._connections) { let actor = conn.getActor(conn); if (actor) return actor; } } It would be much simplier everywhere: 1) instead of toolbox-actor-request:get-raw-node message if (options.toolboxSelectedNodeActorID) { let actor = DebuggerServer.getActor(options.toolboxSelectedNodeActorID); if (actor && actor instanceof NodeActor) { selectedDOMNode = actor.rawObj; } } 1-bis) if you want to be very explicit, but I don't see the value: if (options.toolboxSelectedNodeActorID && options.toolboxInspetorID) { let actor = DebuggerServer.getActor(options.toolboxInspectorID); selectedDOMNode = actor.getDOMNodeByActorId(options.toolboxSeletedNodeActorID); } 2) instead of toolbox-actor-request:inspect-object message value: dbgWindow.makeDebuggeeValue((object) => { let actor = DebuggerServer.getActor(options.toolboxConsoleActorID); // Or TabActor (see my comment in webconsole.js) // let actor = DebuggerServer.getActor(options.toolboxTabActorID); actor.inspectObject(dbgWindow.makeDebuggeeValue(object)) });

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review140986 Reseting review flag, mostly to close the discussion about DebuggerServer.getActor versus events. Please see my suggestion in comment 14. Consider this as a discussion, do not hesitate to prove me I'm wrong and r? the same patch with the few things addressed from mozreview. ::: browser/components/extensions/ext-devtools-inspectedWindow.js:30 (Diff revision 4) > return new WebExtensionInspectedWindowFront(clonedTarget.client, clonedTarget.form); > } > > + async function prepareToolboxOptions(context, options) { > + const selectedNode = context.devToolsToolbox.selection; > + const toolbox = context.devToolsToolbox; const toolbox = context.devToolsToolbox; const selectedNode = toolbox.selection; ::: browser/components/extensions/ext-devtools-inspectedWindow.js:35 (Diff revision 4) > + const toolbox = context.devToolsToolbox; > + > + if (selectedNode && selectedNode.nodeFront) { > + // If there is a selected node in the inspector, we add the > + // inspector and node actor ids to the eval request optons, > + // used to provide the "$0" binding. It needs to be updated, I would phrase it like this: If there is a selected node in the inspector, we hand over its actor id to the eval request in order to provide the "$0" binding. ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:119 (Diff revision 4) > + yield waitInspectorPanelSelected; > + info("Toolbox has been switched to the inspector as expected"); > + > + info("Test inspectedWindow.eval inspect() binding called for a JS object"); > + > + const waitSpliPanelOpened = new Promise((resolve, reject) => { typo: waitSplitPanelOpened ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:131 (Diff revision 4) > + is(objectType, "object", "The inspected object has the expected type"); > + Assert.deepEqual(Object.keys(objectPreviewProperties), ["testkey"], > + "The inspected object has the expected preview properties"); > + resolve(); > + }); > + }); Could you use async/wait here? const waitSplitPanelOpened = (async function () { await toolbox.once("split-console"); ... })(); ::: devtools/client/framework/toolbox.js:2208 (Diff revision 4) > + _onInspectObject: function (evt, packet) { > + this.inspectObjectActor(packet.objectActor, packet.inspectFromAnnotation); > + }, > + > + inspectObjectActor: function (objectActor, inspectFromAnnotation) { > + return Task.spawn(function* () { It would be easier to read by using async instead of spawn: inspectObjectActor: Task.async(function* (...) { ::: devtools/client/framework/toolbox.js:2212 (Diff revision 4) > + inspectObjectActor: function (objectActor, inspectFromAnnotation) { > + return Task.spawn(function* () { > + if (objectActor.preview && > + objectActor.preview.nodeType === domNodeConstants.ELEMENT_NODE) { > + // Open the inspector and select the DOM Element. > + yield this.loadTool("inspector"); If you happen to followup on this, it may be handy to move the implementation to the inspector with something like this: yield this.loadTool("inspector"); let inspector = yield this.getPanel("inspector"); yield inspector.inspectNodeActor(objectActor.actor); (It would be similar to what we do next for the console) ::: devtools/client/framework/toolbox.js:2217 (Diff revision 4) > + yield this.loadTool("inspector"); > + const nodeFront = yield this._walker > + .getNodeActorFromObjectActor(objectActor.actor); > + if (!nodeFront) { > + Cu.reportErrort(new Error("The object cannot be linked to the inspector, the " + > + "corresponding nodeFront could not be found")); Here and a couple of lines after: Please use console.error unless there is a good reason to use this complex error logging? (note that there is a typo "reportErrort") ::: devtools/server/actors/webconsole.js:569 (Diff revision 4) > + */ > + onEnableToolboxActorRequests: function WCU_enableToolboxActorRequests() > + { > + if (!this._inspectObjectActor) { > + this._inspectObjectActor = (evt, params) => { > + this.conn.sendActorEvent(this.parentActor.actorID, "inspectObject", { This is weird to send an event from webconsole.js for the TabActor(tab.js). You should either send it with this.actorID from here, or move this to tab.js

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review140986 > Could you use async/wait here? > const waitSplitPanelOpened = (async function () { > await toolbox.once("split-console"); > ... > })(); sure, I also rewrote the test from genetors functions to async functions (which prepare it for the upcoming rebase of this patch). > It would be easier to read by using async instead of spawn: > inspectObjectActor: Task.async(function* (...) { I opted to rewrote it as an async function (instead of using Task.async). > If you happen to followup on this, it may be handy to move the implementation to the inspector with something like this: > > yield this.loadTool("inspector"); > let inspector = yield this.getPanel("inspector"); > yield inspector.inspectNodeActor(objectActor.actor); > > (It would be similar to what we do next for the console) Totally agree, I refactored it now (moved that part into a new `inspector.inspectNodeActor` method). > Here and a couple of lines after: > Please use console.error unless there is a good reason to use this complex error logging? (note that there is a typo "reportErrort") sure, I rewrote these into console.error calls (also, thanks for catching the typo) > This is weird to send an event from webconsole.js for the TabActor(tab.js). > You should either send it with this.actorID from here, or move this to tab.js yeah, I agree, I changed this to send it with this.actorID (then the event is received by the webconsole client which re-emit it, so that the toolbox can listen for it and decide how to handle it).

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review146518 ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:120 (Diff revision 5) > + const waitSplitPanelOpened = (async () => { > + await toolbox.once("split-console"); > + let jsterm = toolbox.getPanel("webconsole").hud.jsterm; > + > + const options = await new Promise(resolve => { > + jsterm.once("variablesview-open", (evt, view, options) => resolve(options)); > + }); I feel that I should split this test and move anything that does too much checks on the devtools toolbox into a new test file inside the devtools/ dir. The reason is that with the upcoming changes related to the "devtools as an addon", any test that is outside of the devtools sources itself should be able to run with the DevTools shims, and I feel that it would be better to move this part of this test in the devtools tests now (so that once the devtools are moved out of mozilla-central, these part of the test is going to be moved with the rest of the devtools sources). ::: devtools/server/main.js:1370 (Diff revision 5) > /** > * ⚠ TESTING ONLY! ⚠ Searches all active connections for an actor matching an ID. > * This is helpful for some tests which depend on reaching into the server to check some > * properties of an actor. > */ > _searchAllConnectionsForActor(actorID) { > for (let connID of Object.getOwnPropertyNames(this._connections)) { > let actor = this._connections[connID].getActor(actorID); > if (actor) { > return actor; > } > } > return null; > }, > + > + /** > + * Retrieve an actor by ActorID. > + */ > + getActor(actorID) { > + for (let [, conn] of Object.entries(this._connections)) { > + let actor = conn.getActor(actorID); > + if (actor) { > + return actor; > + } > + } > + return null; > + } I'm pretty convinced that the `DebuggerServer.getActor` strategy makes this change much more simpler than the 'exchange messages' strategy (what I meant previously is that after the previous round of changes, I liked that we can keep the webconsole actor responsible of `inspectObject` and do not duplicate that part of the webconsole behavior elsewhere, and we are able to do it with both the implementation strategies and so I'm happy with both). My only concern related to this new `DebuggerServer.getActor` method is that it is basically doing the same thing that the `DebuggerServer._searchAllConnectionsForActor` method above it already does, which is test only (and it has a "warning" comment on top of it) and I'm not completely sure if we want/should unify them.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review146518 > I'm pretty convinced that the `DebuggerServer.getActor` strategy makes this change much more simpler than the 'exchange messages' strategy (what I meant previously is that after the previous round of changes, I liked that we can keep the webconsole actor responsible of `inspectObject` and do not duplicate that part of the webconsole behavior elsewhere, and we are able to do it with both the implementation strategies and so I'm happy with both). > > My only concern related to this new `DebuggerServer.getActor` method is that it is basically doing the same thing that the `DebuggerServer._searchAllConnectionsForActor` method above it already does, which is test only (and it has a "warning" comment on top of it) and I'm not completely sure if we want/should unify them. I marked it testing only because it's somewhat inefficient, since it loops through all connections. If you now need it from production, I would say remove the "testing only" bit and the "_" prefix. I think the name `search*` is helpful because it emphasizes it could be somewhat slow. (Though I guess we don't have that many connections active at the same time in practice...) Anyway, it does seem a little silly to have both of them!

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review146518 > I marked it testing only because it's somewhat inefficient, since it loops through all connections. If you now need it from production, I would say remove the "testing only" bit and the "_" prefix. I think the name `search*` is helpful because it emphasizes it could be somewhat slow. (Though I guess we don't have that many connections active at the same time in practice...) > > Anyway, it does seem a little silly to have both of them! Thanks for the super fast confirmation! I'm going to make the suggested changes.

The last pushed update contains only the rebase of the patch on a recent mozilla-central tip (while the previous one, https://reviewboard.mozilla.org/r/123690/diff/5-6/, contains the changes needed to unify the new `DebuggerServer.getActor` with the existent `DebuggerServer.searchAllConnectionsForActor` as suggested in Comment 19). Follows a push to try of the rebased patch: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=de145140d59d87de6b5877fac4644d70368a09cc

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review146518 > I feel that I should split this test and move anything that does too much checks on the devtools toolbox into a new test file inside the devtools/ dir. > > The reason is that with the upcoming changes related to the "devtools as an addon", any test that is outside of the devtools sources itself should be able to run with the DevTools shims, and I feel that it would be better to move this part of this test in the devtools tests now (so that once the devtools are moved out of mozilla-central, these part of the test is going to be moved with the rest of the devtools sources). I'm dropping this issue for now, after a brief chat with Julian we agreed that we can defer a re-evaluation of how we want to change this test (and the other tests that are related to both the WebExtensions devtools API implementation and the developer toolbox) to a follow up step.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review136220 > Oh yeah, there is definitely some code to share here, I think that I'll file a follow up issue to handle it (I want to look a bit deeper into both the old webconsole and new webconsole clients to be sure that I share this piece correctly) Filed as a follow up in Bug 1368028 ("Extracting a shared helper for inspecting a DOM node")

Comment on attachment 8871778 [details] Bug 1300590 - Use DevToolsShim in the devtools.inspectedWindow.eval bindings tests. https://reviewboard.mozilla.org/r/143234/#review147438 For tests I think that's what we should do for now. I might add getTargetForTab directly on the shim, but it doesn't make much difference as it would throw id gDevTools is not available.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review148240 You should be able to proceed with this patch, thanks for addressing my feedback since March!! ::: browser/components/extensions/ext-devtools-inspectedWindow.js:38 (Diff revision 8) > + // If there is a selected node in the inspector, we hand over > + // its actor id to the eval request in order to provide the "$0" binding. > + options.toolboxSelectedNodeActorID = selectedNode.nodeFront.actorID; > + } > + > + // Provide the console actor ID to provide the "inspect" binding. nit: I would avoid repeating provide by using `implement the "inspect" binding` ::: browser/components/extensions/ext-devtools-inspectedWindow.js:59 (Diff revision 8) > waitForInspectedWindowFront = getInspectedWindowFront(); > } > > const front = await waitForInspectedWindowFront; > - return front.eval(callerInfo, expression, options || {}).then(evalResult => { > + > + options = options || {}; nit: may be it is better to use default value in function definition? `async eval(expression, options = {}) {` ::: devtools/server/actors/webextension-inspected-window.js:268 (Diff revision 8) > + }); > + > + // The eval is coming from an RDP request and we have receive a > + // toolboxConsoleActor to be used to define the inspect binding, > + // (on the contrary it will not be provided during an > + // inspectedWindow.reload request). I would rephrase this comment to something like: This function is used by 'eval' and 'reload' requests, but only 'eval' pass 'toolboxConsoleActor' from the client side in order to set the 'inspect' binding. ::: devtools/server/actors/webextension-inspected-window.js:409 (Diff revision 8) > * javascript code in every sub-frame of the target window during the tab reload. > * NOTE: this parameter is not part of the RDP protocol exposed by this actor, when > * it is called over the remote debugging protocol the target window is always > * `tabActor.window`. > */ > - eval(callerInfo, expression, options, customTargetWindow) { > + eval(callerInfo, expression, options = {}, customTargetWindow) { super-nit: it looks weird to have an explicit default value on `options` and not on `customTargetWindow`. ::: devtools/server/main.js:1378 (Diff revision 8) > + * > * This is helpful for some tests which depend on reaching into the server to check some > - * properties of an actor. > + * properties of an actor, and it is also used by the actors related to the > + * DevTools WebExtensions API to be able to interact with the actors created by the > + * panels natively provided by the DevTools Toolbox. > */ You may keep the warning about being test only when used from the client. Using it from the client is wrong as it totally breaks client <> server abstraction where everything should go throught RDP requests. This is the typical usage we still would like to avoid. (the one you refactored to use searchAllConnectionsForActor instead of _searchAllConnectionsForActor) Your usage is different as it is being used from the server. `Searches all active connections for an actor matching an ID. ⚠ TO BE USED ONLY FROM SERVER CODE OR TESTING ONLY! ⚠` ::: devtools/server/main.js:1379 (Diff revision 8) > * This is helpful for some tests which depend on reaching into the server to check some > - * properties of an actor. > + * properties of an actor, and it is also used by the actors related to the > + * DevTools WebExtensions API to be able to interact with the actors created by the > + * panels natively provided by the DevTools Toolbox. > */ > - _searchAllConnectionsForActor(actorID) { > + searchAllConnectionsForActor(actorID) { Note that if you are concerned about performances, the actor id strings have a precise format which should look like this: server{loaderID}.conn{ConnectionID}{ActorPrefix}{ActorID} So we can come up with a regexp to query only the right connection via its id. But I'm not concerned about the performances given the current context. So there is not need to do that in this bug.

Comment on attachment 8871778 [details] Bug 1300590 - Use DevToolsShim in the devtools.inspectedWindow.eval bindings tests. https://reviewboard.mozilla.org/r/143234/#review148242

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review148240 Thanks Alex for your invaluable feedback! I applied all the suggested tweaks related to the small nits highlighted in the last review comments (besides one, for the reason described below). > nit: may be it is better to use default value in function definition? > `async eval(expression, options = {}) {` Unfortunately I cannot set the default value options in the parameters because the API schema wrappers (automatically generated from the API json schema files) are explicitly setting the `options` parameter and so it always overrides the default set in the parameters. In the meantime I added a comment to explain why we set the default there instead of using the API method parameters. > You may keep the warning about being test only when used from the client. > Using it from the client is wrong as it totally breaks client <> server abstraction where everything should go throught RDP requests. > This is the typical usage we still would like to avoid. (the one you refactored to use searchAllConnectionsForActor instead of _searchAllConnectionsForActor) Your usage is different as it is being used from the server. > > `Searches all active connections for an actor matching an ID. > ⚠ TO BE USED ONLY FROM SERVER CODE OR TESTING ONLY! ⚠` Yeah, sounds reasonable to leave such warning comment there, I added the new version of the warning comment. > Note that if you are concerned about performances, > the actor id strings have a precise format which should look like this: > server{loaderID}.conn{ConnectionID}{ActorPrefix}{ActorID} > So we can come up with a regexp to query only the right connection via its id. > > But I'm not concerned about the performances given the current context. So there is not need to do that in this bug. I also added a inline comment with a note about the possible optimization described above, as I agree that it the number of connection should not be that hight, but it is a reasonable approach to consider as a follow up, especially if it turns out that the number of the RDP connections is high enough to affect the performace in perceivable ways.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review148526 I haven't finished looking through the test, but here are partial comments. ::: browser/components/extensions/ext-devtools-inspectedWindow.js:28 (Diff revision 10) > // the front instance until it is connected to the remote debugger successfully). > const clonedTarget = await getDevToolsTargetForContext(context); > return new WebExtensionInspectedWindowFront(clonedTarget.client, clonedTarget.form); > } > > + async function prepareToolboxOptions(context, options) { you don't need to pass context here, its already available in the enclosing scope. also why is this async? ::: browser/components/extensions/ext-devtools-inspectedWindow.js:59 (Diff revision 10) > + // NOTE: we have to set the default `options` value to {} here > + // because the API schema wrappers explicitly set `options` to null > + // and it would override the default value if specified on the > + // method parameters. > + options = options || {}; I don't understand this comment, but you should be able to put `"default": {}` into declaration of options in the schema to avoid this... ::: browser/components/extensions/ext-devtools-inspectedWindow.js:65 (Diff revision 10) > + // because the API schema wrappers explicitly set `options` to null > + // and it would override the default value if specified on the > + // method parameters. > + options = options || {}; > + > + await prepareToolboxOptions(context, options); nit: I think this would read more clearly if this just returned the new toolbox options and then you use `Object.assign()` or something here to put everything together. up to you though... ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:68 (Diff revision 10) > - const evalTestCases = [ > - // Successful evaluation results. > + let waitEvalResult; > + let res; why not just declare these in-line below? ::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:95 (Diff revision 10) > - const {evalResult, errorResult} = await extension.awaitMessage(`inspectedWindow-eval-result`); > + // Test that inspect($0) switch the developer toolbox to the inspector. > + await gDevTools.showToolbox(target, "styleeditor"); > > - Assert.deepEqual(evalResult, expectedResults.evalResult, "Got the expected eval result"); > + info("Toolbox switched back to the styleeditor panel"); > > - if (errorResult) { > + const waitInspectorPanelSelected = (async () => { nit: I think this would be clearer if it was named `inspectorPanelSelectedPromise`, this name makes it sound like a callable function.

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review148526 Thanks for this review comments! I applied all of them (more details in the comments below) and re-newer the review request on the updated patches. > you don't need to pass context here, its already available in the enclosing scope. also why is this async? yep, this function has changed (it used to be actually async due to some request to the remote debugging server) and it has been moved during the reviews, but now we can definitely remove context from the parameters and it doesn't need to be async anymore. Thanks for catching it! > I don't understand this comment, but you should be able to put `"default": {}` into declaration of options in the schema to avoid this... yeah, changing the default in the schema would probably work, but it would be hidden in a file very far from here, I think that I like more the approach which uses `Object.assign` like suggested in your other review comment. > nit: I think this would read more clearly if this just returned the new toolbox options and then you use `Object.assign()` or something here to put everything together. up to you though... Yeah, this is definitely the approach that I like most! and it pairs pretty well with the change applied to the `prepareToolboxOptions` helper function suggested above (which I renamed to `getToolboxOptions`, it doesn't take any parameters anymore, it is not async anyore and now it returns an object with the toolbox options properties set. With this approach, the final `options` object is now defined as: ``` const evalOptions = Object.assign({}, options, getToolboxOptions()); ``` > why not just declare these in-line below? The two identifier were reused (re-assigned) by more than one test cases, but I agree that it doesn't help while reading the test. In the last version I turned these variables into `const`, and they are declared in-line in the test case that is going to use them. > nit: I think this would be clearer if it was named `inspectorPanelSelectedPromise`, this name makes it sound like a callable function. sounds good, I renamed this promise (and also the other one that follows which are similar, so that they all use the same naming convention).

Comment on attachment 8851377 [details] Bug 1300590 - Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. https://reviewboard.mozilla.org/r/123690/#review149262 r=me on the bits in browser/components/extensions/, I didn't look at the stuff under devtools/

Patches rebased on a recent mozilla-central tip (and resolved the conflicts in advance) The try build looks good (there are a couple of unrelated known intermittents and an xpcshell test failure on osx that is not related to the changes introduced by these patches, I also tested it locally on osx and ensured it is related to the osx artifact build, a full build doesn't have any issue and the same issue is reproducible on an "artifact build" try push of the mozilla-central tip without these patches applied).

Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/99c6dd76dbc8 Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval. r=aswan,ochameau https://hg.mozilla.org/integration/autoland/rev/0403297b318b Use DevToolsShim in the devtools.inspectedWindow.eval bindings tests. r=jdescottes,ochameau

https://hg.mozilla.org/mozilla-central/rev/99c6dd76dbc8 https://hg.mozilla.org/mozilla-central/rev/0403297b318b

I've updated the eval() docs for this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.inspectedWindow/eval In particular: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.inspectedWindow/eval#Helper_functions https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.inspectedWindow/eval#Helper_function_examples https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.inspectedWindow/eval#Browser_compatibility I've slightly changed the emphasis of the docs: the old docs talks about how the WebExtensions inspectedWindow.eval supports "the same helpers as the [Web Console](/en-US/docs/Tools/Web_Console/)". But I'm not very comfortable referring out to the devtools docs for this, because WE is supposed to be cross-browser, and the devtools docs are clearly not. So instead I'm intending to document the helpers inside the inspectedWindow.eval docs page, and add them as they land in Firefox. Please let me know if we need anything else, or if you see any issues with these docs.

Sorry Will, I (wrongly) thought that I had already cleared this needinfo and marked this issue as dev-doc-complete. I agree completely that it is better to document the supported helpers directly in the devtools.inspectedWindow.eval doc page (also because there are other helpers available in the webconsole that are not available in the devtools.inspectedWindow.eval). I only applied the following small changes: - added an additional note about the unsupported options near to the list of options - added a note related to the behavior of the inspect helper when we pass a JS object instead of a DOM Node (it allows to inspect that object in the webconsole, as well as the split webconsole)