Bug 221449 – Web Inspector: Add `CSS.setLayoutContextTypeChangedMode` for getting information about all layout contexts

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

From Devin's Review Comment on attachment 419315 [details]:
```
As a potential optimization, maybe you could add a `CSS.setGlobalLayoutContextTypeInstrumentationEnabled` that's toggled on inside `WI.LayoutDetailsSidebarPanel.prototype.attached` and off inside `WI.LayoutDetailsSidebarPanel.prototype.detached` which controls whether the `Node` is always instrumented (when toggled on) or if only previously instrumented `Node` are allowed (when toggled off).  This means that until the developer shows the Layout panel, we'd only be getting `CSS.nodeLayoutContextTypeChanged` events for nodes that have been shown in the Elements Tab, which is likely _far_ more performant than if we'd instrumented every potentially viable node.
```

[Patrick Angle]

Created attachment 419615 [details]
Patch v1.0

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Comment on attachment 419615 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=419615&action=review

r=me, nice tests :)

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:940
> +    for (auto* child = node.firstChild(); child; child = child->nextSibling())

Are we sure that there's not a more efficient or less intense way to find all the grid contexts other than to walk the entire DOM tree? :(

Maybe we could use `childrenOfType<Element>(node)` instead so that we only even attempt to instrument elements (since `Node` cannot be a grid context)?

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:959
> +        for (auto document : domAgent->documents())

NIT: `auto*`

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:86
> +            WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.All);

While it is a valid assumption (for now) that only the main target has a `CSS` agent, in order to make this code more future-friendly we should iterate `WI.targets`.  Also, I'd move this to a `WI.CSSManager.prototype.set layoutContextTypeChangedMode` so that protocol stuff is kept outside of view classes :)

CSSManager.js
```
    set layoutContextTypeChangedMode(layoutContextTypeChangedMode)
    {
        for (let target of WI.targets) {
            // COMPATIBILITY (iOS 14.5): CSS.setLayoutContextTypeChangedMode did not exist
            if (target.hasCommand("CSS.setLayoutContextTypeChangedMode"))
                target.CSSagent.setLayoutContextTypeChangedMode(layoutContextTypeChangedMode);
        }
    }
```

LayoutDetailsSidebarPanel.js
```
    attached()
    {
        super.attached();

        WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._mainResourceDidChange, this);

        WI.cssManager.layoutContextTypeChangedMode = WI.CSSManager.LayoutContextTypeChangedMode.All);
    }
```

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:93
> +            WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.Observed);

ditto (:86)

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode-expected.txt:11
> +PASS: 0 grid node should be instrumented.
> +PASS: 1 grid nodes should be instrumented.
> +PASS: 0 grid nodes should be instrumented.
> +PASS: 1 grid nodes should be instrumented.
> +PASS: 0 grid nodes should be instrumented.
> +PASS: 1 grid nodes should be instrumented.

these should be "0 grid nodes" (plural) and "1 grid node" (singular) and "2 grid nodes" (plural) 😅

(I feel bad for being this pedantic, but it just sticks out so much to my eyes 🤣)

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:22
> +    // TODO: [PCA] Use Razvan's impl in WI.DOMManager instead of redeclaring it.

oops?

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:37
> +                documentNode.querySelector("#queryGrid"),

Just to be extra sure/careful, can we pull this into a `let queryGridNode = ...` and then do `queryGridNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged)` so that we know for a fact it's the right node being instrumented/updated?

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:43
> +                changeElementDisplayValue("queryGrid", "block"),

NIT: to assist someone trying to read the output of the test, I'd suggest putting some `InspectorTest.log("Changing '#queryGrid' to 'display: block;'...");` (we usually use "..." to represent something that's about to happen whereas "." represents something that has happened) in between each of these checks :)

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:75
> +                WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.All),

ditto (LayoutDetailsSidebarPanel.js:86)

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:81
> +            await WI.assumingMainTarget().CSSAgent.setLayoutContextTypeChangedMode(WI.CSSManager.LayoutContextTypeChangedMode.Observed);

ditto (LayoutDetailsSidebarPanel.js:86)

> LayoutTests/inspector/css/setLayoutContextTypeChangedMode.html:130
> +    WI.domManager.requestDocument().then((doc) => {

NIT: now that `WI.DOMManager.prototype.requestDocument` supports returning (and caching) a `Promise`, we should move away from this pattern and instead have each test that needs the `documentNode` fetch it itself

[Radar WebKit Bug Importer]

<rdar://problem/74115701>

[Patrick Angle]

Created attachment 419658 [details]
Patch v1.1 - Review Notes, More Verbose Test

[Patrick Angle]

Created attachment 419666 [details]
Patch v1.2 - Added reviewer to changelogs

[EWS]

Committed r272566: <https://commits.webkit.org/r272566>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419666 [details].