Closed Bug 1409968 Opened 5 years ago Closed 5 years ago

Implement FlexboxActor for fetching all flexbox containers

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [designer-tools])

Attachments

(1 file)

No description provided.
Priority: -- → P3
Comment on attachment 8920010 [details]
Bug 1409968 - Implement FlexboxActor for fetching all flexbox containers.

https://reviewboard.mozilla.org/r/190984/#review196376

::: devtools/server/actors/layout.js:20
(Diff revision 1)
>   * Set of actors the expose the CSS layout information to the devtools protocol clients.
>   *
>   * The |Layout| actor is the main entry point. It is used to get various CSS
>   * layout-related information from the document.
>   *
> + * The |FlexBox| actor provides the container node information to inspector the flexbox

s/inspector/inspect

::: devtools/server/actors/layout.js:137
(Diff revision 1)
>      this.tabActor = null;
>      this.walker = null;
>    },
>  
>    /**
> +   * Returns an array of FlexboxdActor objects for all the flexbox containers found by 

nit: trailing whitespace.

::: devtools/server/actors/layout.js:151
(Diff revision 1)
> +
> +    if (!rootNode) {
> +      return flexboxes;
> +    }
> +
> +    let treeWalker = this.walker.getDocumentWalker(rootNode);

I see we're not passing any value for `whatToShow` here. And we're not doing it for the css grid either.
We should do that I think.
The default value is `nodeFilterConstants.SHOW_ALL` but really, we only care about element nodes here. So we should pass `SHOW_ELEMENT` instead, in both places.

::: devtools/server/actors/layout.js:152
(Diff revision 1)
> +    while (treeWalker.nextNode()) {
> +      let currentNode = treeWalker.currentNode;
> +      let computedStyle = CssLogic.getComputedStyle(currentNode);
> +
> +      if (!computedStyle) {
> +        continue;
> +      }
> +
> +      if (computedStyle.display == "inline-flex" || computedStyle.display == "flex") {
> +        let flexboxActor = new FlexboxActor(this, currentNode);
> +        flexboxes.push(flexboxActor);
> +      }
> +    }

We know such a loop can be slow. We have this problem for css grids already, even though we use a platform API for that.
As discussed in a chat today, our plan to mitigate this is to land this disabled/behind a flag until we know we have good perf on this.
Attachment #8920010 - Flags: review?(pbrosset) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb66fc11489e
Implement FlexboxActor for fetching all flexbox containers. r=pbro
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2cac6e43a2
Follow up: Add a semicolon to fix eslint error. r=me
https://hg.mozilla.org/mozilla-central/rev/eb66fc11489e
https://hg.mozilla.org/mozilla-central/rev/4b2cac6e43a2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1419462
Product: Firefox → DevTools
Blocks: 1470379
You need to log in before you can comment on or make changes to this bug.