1580463 - Make the reflowtracker multi-target aware

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P1)

Description

[Patrick Brosset <:pbro>]

https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/reflow-tracker.js

The ReflowTracker is used by multiple things in the inspector that need to update their information based on whether a reflow occurred in the page.

One example is the Grid Inspector which relies on this to update the list of grids found in the page, and the aspect ratio of the grid mini-map displayed in the sidebar.

It will become necessary for this utility to be aware of all of the current targets where inspector actors are currently running.

We need to consider performance here however. This mechanism is already not great as it causes multiple tools to refresh themselves for potentially no reason.
Making this multi-target means receiving even more reflow events. Maybe it's a good time to think about all the callsites and see if there isn't a more targeted event we could use instead.

Comment 1

[Patrick Brosset <:pbro>]

There are 3 users of this helper: https://searchfox.org/mozilla-central/search?q=reflowtracker.track&path=devtools

• box-model
• flexbox inspector
• grid inspector

They all need to listen to reflows in order to update themselves when the geometry of the selected node changes (if the grid container size or tracks change, the grid mini-map needs to change, or if the flex item sizing changes, the flex item diagram needs to change, or if the padding/margin/border of an element changes, the box-model diagram needs to change).

So it would seem reasonable to change the helper so it listens to reflows on the selected node target only, on demand.
I think the helper and its users are already done with this in mind, it's just that the target is set once and not passed when the tracking starts.
So this shouldn't be too hard.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug 1580463 - Remove ReflowTracker and listen for reflows on selected element

The ReflowTracker was based on the assumption that there was only ever going to
be one target to be observed.
With Fission, this is no longer true.
Turning the ReflowTracker into something that is multi-target aware seemed more
complex than really worth it. After all, all it was doing is getting a ReflowFront
and listening for events on it.
The only 3 things that needed it are the grid inspector, flex inspector and box
model widget. They all needed it for the same reason: updating the data displayed
in the UI when the size/geometry/box-model regions of the selected node changed.
So, it seems simpler to let the inspector instantiate the right ReflowFront when
it needs it (upon a new node selection).
There's only one node selected at any given time in the inspector, so it's simple
to just listen for reflow in that node's target, and dispatch events to the grid,
flex and box-model tools so they can update themselves.

Note that once a new node is selected, we do the getFront("Reflow") again
since that node can be in a different target than the previous one. If it is,
however, in the same target, then getFront will return the same instance which
is nice.

Comment 3

[Chris Peterson [:cpeterson]]

Tracking Fission DevTools bugs for Fission Nightly (M6)

Comment 4

[Julian Descottes [:jdescottes]]

needinfo on myself to check the remaining intermittent.

For the record I can't reproduce locally (even after pulling from a known "bad" try revision: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=279839728&revision=61900024fdba8b162747d0dea6b18bb5084192fa).

Also logs and build artifacts have expired on this try push so pushed to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27bcb6bc8368e75ab60328143b0f44bea8530384

Comment 5

[Julian Descottes [:jdescottes]]

I thinkI spotted two potential issues here:

• the test is selecting an invalid node: "#grid" does not exist in the test page
• the onNewSelection callback in inspector.js calls trackReflowsInSelection which is now async

But I don't see how this could lead to a reset of the store. I have a few try pushes with fixes for those issues to see if it helps.

Comment 6

[Julian Descottes [:jdescottes]]

the test is selecting an invalid node: "#grid" does not exist in the test page

It seems that fixing the tests to select a valid node (eg .container) instead of #grid fixes the intermittent. Not clear to me why this fixes the issue. Selecting an invalid node means we will "untrack" reflows. However I don't think this should trigger any update of the grid highlighters. I don't have any other good lead at the moment to understand the root cause, so I suppose we can move forward with this fix (already applied by :pbro on phabricator).

Clearing the ni.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e410cfefc08b
Remove ReflowTracker and listen for reflows on selected element r=jdescottes

Comment 9

[Cristian Brindusan [:cbrindusan]]

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