Implement support for $0 and inspect bindings in devtools.inspectedWindow.eval

ASSIGNED
Assigned to

Status

()

Toolkit
WebExtensions: Developer Tools
P1
normal
ASSIGNED
8 months ago
4 days ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [devtools.inspectedWindow] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
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).
(Assignee)

Updated

8 months ago
Blocks: 1211859
Depends on: 1300584
Priority: -- → P2

Updated

8 months ago
Whiteboard: [devtools.inspectedWindow] triaged
Keywords: dev-doc-needed

Comment 1

6 months ago
I'm giving a beer to the dev that implements this. 5$ on Bountysource :) I know it's not much but I can't wait for Chrome DevTools extensions to work in Firefox also :) I might increase the amount in the future.

Updated

4 months ago
webextensions: --- → +

Updated

4 months ago
Assignee: nobody → lgreco
Comment hidden (mozreview-request)
(Assignee)

Comment 3

28 days ago
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.
Attachment #8851377 - Flags: feedback?(poirot.alex)
(Assignee)

Updated

28 days ago
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Updated

28 days ago
Blocks: 1341305

Comment 4

19 days ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 6

4 days ago
mozreview-review-reply
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).
(Assignee)

Comment 7

4 days ago
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.
Attachment #8851377 - Flags: feedback?(poirot.alex)
You need to log in before you can comment on or make changes to this bug.