1707884 - Fix grid inspector on target switching

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

Once we enable bug 1698891, the grid inspector no longer updates correctly.
That's because grid codebase store the initial target's front over here and never updates them:
https://searchfox.org/mozilla-central/rev/6cbe34b441f7c7c29cd1e5f0e19c7000142f1423/devtools/client/inspector/grids/grid-inspector.js#116-123

    try {
      // TODO: Call this again whenever targets are added or removed.
      this.layoutFronts = await this.getLayoutFronts();
    } catch (e) {
      // This call might fail if called asynchrously after the toolbox is finished
      // closing.
      return;
    }

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1707884 - Fix grid inspector when the target is switched.

Comment 3

[Julian Descottes [:jdescottes]]

Note: using the patch currently attached here fixes the grid tests with bfcacheInParent

• devtools/client/inspector/grids/test/browser_grids_number-of-css-grids-telemetry.js
• devtools/client/inspector/grids/test/browser_grids_restored-after-reload.js
• devtools/client/inspector/grids/test/browser_grids_restored-multiple-grids-after-reload.js

So once this lands we can remove the following snippet from those tests.

  await SpecialPowers.pushPrefEnv({
    set: [["fission.bfcacheInParent", false]],
  });

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I can probably move the attached patch forward by having target-switching-only testing, and have another bug to focus on grid inspector + remote frames

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)

I can probably move the attached patch forward by having target-switching-only testing, and have another bug to focus on grid inspector + remote frames

The current patch might have performance implications, there's no hurry to land it (getting all layoutFronts every time we call getGrids sounds a bit scary). Also it should already work with remote frames I think (with or without the patch from Alex)? It's only that it's not updated dynamically, so new targets are ignored?

I just added the comment so that we don't forget about fixing the tests whenever we fix this :)

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1707884 - [devtools] Fix grid inspector with Fission cases. r=ochameau.

This patch makes the inspector use the new reflow resource instead of directly
handling a reflow front. This allows the grid inspector to update the list of
grids to include remote documents.
The grid inspector was also keeping references of layout fronts which was causing
issues with target switching.
A new test is added to ensure these different cases are now working properly.

Depends on D117899

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58c3344a4f19
[devtools] Fix grid inspector with Fission cases. r=ochameau.

Comment 8

[Marian-Vasile Laza]

Backed out 2 changesets (Bug 1707884, Bug 1716284) for causing xpshell failures on test_layout-reflows-observer.js
Backout link
Push with failures
Failure Log

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

oops, sorry, looking at it right now

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/353975edfc17
[devtools] Fix grid inspector with Fission cases. r=ochameau.

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/353975edfc17