Closed Bug 1707884 Opened 2 months ago Closed 7 days ago

Fix grid inspector on target switching

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Fission Milestone:M8, firefox91 fixed)

RESOLVED FIXED
91 Branch
Fission Milestone M8
Tracking Status
firefox91 --- fixed

People

(Reporter: ochameau, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission-m3-mvp)

Attachments

(2 files)

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;
    }
Duplicate of this bug: 1582732
Whiteboard: dt-fission-m3-triage
Fission Milestone: --- → M8
Whiteboard: dt-fission-m3-triage → dt-fission-m3-mvp
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Type: enhancement → defect

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]],
  });

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

(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 :)

Severity: -- → S3
Priority: -- → P2
Depends on: 1716284

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

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

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

Flags: needinfo?(nchevobbe)

oops, sorry, looking at it right now

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/353975edfc17
[devtools] Fix grid inspector with Fission cases. r=ochameau.
Status: ASSIGNED → RESOLVED
Closed: 7 days ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in before you can comment on or make changes to this bug.