1409968 - Implement FlexboxActor for fetching all flexbox containers

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

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

Attachment: Bug 1409968 - Implement FlexboxActor for fetching all flexbox containers.

Review commit: https://reviewboard.mozilla.org/r/190984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190984/

Comment 2

[Patrick Brosset <:pbro>]

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb66fc11489e
Implement FlexboxActor for fetching all flexbox containers. r=pbro

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2cac6e43a2
Follow up: Add a semicolon to fix eslint error. r=me

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/eb66fc11489e
https://hg.mozilla.org/mozilla-central/rev/4b2cac6e43a2