Bug 221449 - Web Inspector: Add `CSS.setLayoutContextTypeChangedMode` for getting information about all layout contexts
Summary: Web Inspector: Add `CSS.setLayoutContextTypeChangedMode` for getting informat...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
Keywords: InRadar
Depends on:
Blocks: 226661
  Show dependency treegraph
Reported: 2021-02-04 20:03 PST by Patrick Angle
Modified: 2021-06-04 13:36 PDT (History)
11 users (show)

See Also:

Patch v1.0 (21.88 KB, patch)
2021-02-08 13:04 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - Review Notes, More Verbose Test (24.86 KB, patch)
2021-02-08 17:30 PST, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v1.2 - Added reviewer to changelogs (24.85 KB, patch)
2021-02-08 18:52 PST, Patrick Angle
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-02-04 20:03:23 PST
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.
Comment 1 Patrick Angle 2021-02-08 13:04:56 PST
Created attachment 419615 [details]
Patch v1.0
Comment 2 EWS Watchlist 2021-02-08 13:05:45 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 3 Devin Rousso 2021-02-08 14:09:21 PST
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 :)

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


        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.


> 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
Comment 4 Radar WebKit Bug Importer 2021-02-08 14:55:56 PST
Comment 5 Patrick Angle 2021-02-08 17:30:29 PST
Created attachment 419658 [details]
Patch v1.1 - Review Notes, More Verbose Test
Comment 6 Patrick Angle 2021-02-08 18:52:07 PST
Created attachment 419666 [details]
Patch v1.2 - Added reviewer to changelogs
Comment 7 EWS 2021-02-08 19:30:23 PST
Committed r272566: <https://commits.webkit.org/r272566>

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