1568126 - Fetch the inspector and its related fronts (walker, highlighter, selection) via the nodeFront rather than from toolbox or inspector panel singleton

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

For now, the whole codebase assumes that the inspector front is unique.
But with Fission, we will start having more than one. We will still have the one related to the top level tab document, but we will then have one per iframe running in a distinct process.

This change makes it so that we can't be using a static reference to just one inspector front.
We should rather dynamically retrieve the inspector front which relates to the current context we are currently inspecting.

For example, this markup view code:
https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/devtools/client/inspector/markup/markup.js#2172-2185

  /**
   * Return a list of the children to display for this container.
   */
  _getVisibleChildren: function(container, centered) {
    let maxChildren = container.maxChildren || this.maxChildren;
    if (maxChildren == -1) {
      maxChildren = undefined;
    }

    return this.walker.children(container.node, {
      maxNodes: maxChildren,
      center: centered,
    });
  },

this.walker.children will break with fission as we have to call the WalkerFront related to the current node we are expanding. Not just one hardcoded WalkerFront, which today, will always be the WalkerFront of the top level tab's document. If container.node relates to a DOM Element from an iframe running in a distinct process, we have to call children on the WalkerFront related to this distinct process, and not the one related to the top level document's process.

This work depends on bug 1539764, which is meant to expose the related target object on all the fronts. Then, from the target object, you can derivate almost all other fronts.
For example:
const inspectorFront = target.getInspector();
const walkerFront = target.getInspector().walker;
const consoleFront = target.getFront("console");
const threadFront = target.getFront("thread");
Note that the inspector is special and still requires a special getInspector() method. Some work is still required in order to replace this by getFront("inspector"). But this work is independant from this one.

Once bug 1539764 is completed we should be able to replace
this.walker.children(container.node, ...)
by:

const inspectorFront = await container.node.getTargetToBeImplementedInBug1539764().getInspector();
inspectorFront.walker.children(container.node, ...);

This is just an example, this may be different. For example we may try to expose a synchronous API rather than the async getFront/getInspector. We can have an attribute instead of a function on Front...

Comment 1

[Razvan Caliman [:rcaliman]]

Selection was extracted from InspectorFront to Toolbox in bug 1572460

Comment 2

[Patrick Brosset <:pbro>]

I think we are going to fix these problems in multiple different bugs. Some have been fixed already, and this will continue.
I think we should be able to close this one.

Comment 3

[Patrick Brosset <:pbro>]

So, I went over the codebase to try and find occurrences of code that still need to be changed. A bunch of them are related to features we will fix in M2, so I ignored these ones. From what I can see, the following pieces of code do need to be fixed in this bug:

devtools/client/inspector/shared/style-change-tracker.js
27 this.walker.on("mutations", this.onMutations);
34 this.walker.off("mutations", this.onMutations);
devtools/client/inspector/shared/highlighters-overlay.js
1107 const isInTree = await this.walker.isInDOMTree(node);
devtools/client/inspector/markup/markup.js
316 this.walker.on("display-change", this._onWalkerNodeStatesChanged);
317 this.walker.on("scrollable-change", this._onWalkerNodeStatesChanged);
318 this.walker.on("mutations", this._mutationObserver);
2298 this.walker.off("display-change", this._onWalkerNodeStatesChanged);
2299 this.walker.off("scrollable-change", this._onWalkerNodeStatesChanged);
2300 this.walker.off("mutations", this._mutationObserver);
devtools/client/inspector/inspector.js
460 return walker.querySelector(rootNode, this.selectionCssSelector);
474 return walker.querySelector(rootNode, "body");
486 return this.walker.documentElement();

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1568126 - Part 1: Use the contextual WalkerFront in _hideHighlighterIfDeadNode. r=pbro

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Patrick Brosset <:pbro> from comment #3)

devtools/client/inspector/inspector.js
460 return walker.querySelector(rootNode, this.selectionCssSelector);
474 return walker.querySelector(rootNode, "body");
486 return this.walker.documentElement();

I think these ones are fine since they are related to getting the default selection on a new root. Unless the current target is updated on each new root/navigation and therefore we need to get a new WalkerFront, I don't think we need to do anything here. @ochameau can you confirm if this thinking is correct?

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1568126 - Part 2: Make InspectorStyleChangeTracker work with fission. r=ochameau

Depends on D48579

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #5)

(In reply to Patrick Brosset <:pbro> from comment #3)

devtools/client/inspector/inspector.js
460 return walker.querySelector(rootNode, this.selectionCssSelector);
474 return walker.querySelector(rootNode, "body");
486 return this.walker.documentElement();

I think these ones are fine since they are related to getting the default selection on a new root. Unless the current target is updated on each new root/navigation and therefore we need to get a new WalkerFront, I don't think we need to do anything here. @ochameau can you confirm if this thinking is correct?

I think the 2 last lines are indeed fine, you are right.
However the first one worries me, because I don't even remember how it works today across normal iframes. I'll do some research and report back.

Comment 8

[Patrick Brosset <:pbro>]

Ok, the feature that re-selects the last selected node in the inspector does not work across iframes today. So there's no point trying to make it work with fission. We just want parity now. We should file a bug to add iframe support in the future, but for the sake of the fission project in devtools, let's not worry about this. Which means, you're right, these 3 lines you mentioned are not anything we need to fix.
It would, however, be nice to add a comment near them that says that we use the top level walkerFront here on purpose, to always do the default selection on the top level document only.

Comment 9

[Alexandre Poirot [:ochameau]]

+1
Note that if you happen to fix return walker.querySelector(rootNode, this.selectionCssSelector); and make it return a NodeFront from another iframe, you would have to change the two following callsites mentioned:

474 return walker.querySelector(rootNode, "body");
486 return this.walker.documentElement();
As this will modify rootNode and we should then use rootNode.walkerFront.querySelector as well as rootNode.walkerFront.documentElement.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/906c06cac59d
Part 1: Use the contextual WalkerFront in _hideHighlighterIfDeadNode. r=pbro

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b9df1cffc79
Backed out changeset 906c06cac59d for devtools tests timing out e.g browser_rules_flexbox-highlighter-on-mutation.js on a CLOSED TREE.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1568126 - Part 1: Use the contextual WalkerFront in _hideHighlighterIfDeadNode. r=pbro

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc0dd7454893
Part 1: Use the contextual WalkerFront in _hideHighlighterIfDeadNode. r=pbro

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/dc0dd7454893

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc53e489f541
Part 2: Make InspectorStyleChangeTracker work with fission. r=ochameau

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1568126 - Part 3: Use the contextual WalkerFront in the markup view event handlers. r=ochameau

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dc53e489f541

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6470ba2c5a50
Part 3: Use the contextual WalkerFront in the markup view event handlers. r=ochameau

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6470ba2c5a50

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1568126 - Part 2: Make getAllFonts fission compatible. r=ochameau