Bug 221228 – Web Inspector: Implement backend support for maintaining a list of Grid layout contexts

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

- a command to fetch nodes that have layout contexts of a certain type (for now, just Grid/Subgrid)
- an event to maintain additions and removals from the list as web content changes.

[Patrick Angle]

<rdar://73504532>

[Patrick Angle]

Created attachment 419205 [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 419205 [details]
Patch v1.0

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

r- due to the comments on in InspectorDOMAgent.cpp.

> Source/JavaScriptCore/inspector/protocol/DOM.json:42
> +            "id": "LayoutContextType",

NIT: this should really be in `CSS` :/

> Source/JavaScriptCore/inspector/protocol/DOM.json:688
> +            "name": "nodeLayoutContextChanged",

NIT: I think this should include "Type" to match the enum name

> Source/JavaScriptCore/inspector/protocol/DOM.json:691
> +                { "name": "nodeId", "$ref": "NodeId", "description": "Id of the node whose layout context changed." },

NIT: "ID" or "Identifier"

> Source/WebCore/dom/Node.cpp:2134
> +    if (auto document = ownerDocument())

I think you can drop `document` altogether and have that be fetched inside `InspectorInstrumentation::nodeLayoutContextChanged` instead so that we will bail before calling `ownerDocument()` if Web Inspector is not open.

Also, why not just `document()`?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2478
> +    m_frontendDispatcher->nodeLayoutContextChanged(m_documentNodeToIdMap.get(&node), is<RenderGrid>(node.renderer()) ? Protocol::DOM::LayoutContextType::Grid : Protocol::DOM::LayoutContextType::Other);

AFAIK there's no guarantee that `&node` will be in `m_documentNodeToIdMap` by this point.  I think you need to `identifierForNode(node)` (and check the result) so that the node is pushed to the frontend before you dispatch this event (if you do this you'll also want to add a FIXME comment for <https://webkit.org/b/189687>).

Also, if a node changed from `display: block;` to `display: flex;` would this still fire even though both are categorized as `Other`?  If so, for now we may want to include the old renderer in the `InspectorInstrumentation` call so that we don't dispatch this event if neither the previous nor the new renderer is `display: grid;` or `display: inline-grid;`.

> Source/WebCore/rendering/RenderObject.h:1151
> +    didChangeRenderer();

rather than have another function `Node::didChangeRenderer` you could put the `InspectorInstrumentation` call here

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:449
> +    _nodeLayoutContextChanged(nodeId, newLayoutContextType)

Style: I realize that the other `DOM` events follow this convention, but I personally would prefer breaking from that and having the name of the function (and the parameters) in `WI.DOMManager` exactly match the name in the protocol (so remove the leading `_` and just have `layoutContextType`)

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:452
> +        if (!node || node.layoutContextType === newLayoutContextType)

If we ever have a situation where `node.layoutContextType === layoutContextType` (which I think will happen a lot based on my comment on InspectorDOMAgent.cpp:2478) then we really shouldn't be dispatching an event in the first place.

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:455
> +        this.dispatchEventToListeners(WI.DOMManager.Event.NodeLayoutContextChanged, {node});

we should have the `WI.DOMNode` dispatch a `WI.DOMNode.Event.LayoutContextTypeChanged` not the manager

> LayoutTests/inspector/dom/nodeLayoutContextChanged.html:33
> +            return new Promise((resolve) => {

If you follow my suggestion on DOMManager.js:455 you could rewrite this to use `WI.Object.prototype.awaitEvent` so that there's less nesting
```
    domNodeHandler(domNode) {
        InspectorTest.expectEqual(domNode.layoutContextType, WI.DOMNode.LayoutContextType.Grid, "Layout context should be `grid`.");

        await Promise.all([
            domNode.awaitEvent(WI.DOMNode.Event.LayoutContextTypeChanged),
            InspectorTest.evaluateInPage(`document.getElementById("gridToNonGrid").style.display = "block"`),
        ]);

        InspectorTest.expectEqual(domNode.layoutContextType, WI.DOMNode.LayoutContextType.Other, "Layout context should now be `other`.");
    }
```

> LayoutTests/inspector/dom/nodeLayoutContextChanged.html:42
> +                InspectorTest.evaluateInPage("document.getElementById(\"gridToNonGrid\").style.display = \"block\"");

Style: we normally use backticks (`) in tests to signify "this string will be evaluated in the context of the page"

[Patrick Angle]

Created attachment 419315 [details]
Patch v1.1 - Review Notes

[Blaze Burg]

Comment on attachment 419315 [details]
Patch v1.1 - Review Notes

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

r=me

> LayoutTests/inspector/dom/nodeLayoutContextTypeChanged.html:51
> +                InspectorTest.evaluateInPage(`document.getElementById("nonGridToGrid").style.display = "grid"`),

Personally, I like to put these triggers into their own named local function for clarity. e.g., changeStyleToDisplayGrid() => ...

> LayoutTests/inspector/dom/nodeLayoutContextTypeChanged.html:58
> +    WI.domManager.requestDocument((node) => {

Nit: requestDocument now returns a promise if no callback. So this could be WI.domManager.requestDocument().then((doc) => ...)

[Devin Rousso]

Comment on attachment 419315 [details]
Patch v1.1 - Review Notes

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

r=me as well, nice work!

> Source/WebCore/dom/Element.h:731
> +    void didChangeRenderer(RenderObject*) override;

NIT: should this be `final`?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1768
> +    value->setLayoutContextType(layoutContextTypeForRenderer(node->renderer()));

Hmm, this means that for detached nodes we may send `Other` even though it doesn't even have a renderer (and therefore a layout context) yet.  Perhaps we should check that `node->renderer()` exists?

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:226
> +        this._idToDOMNode[nodeId].layoutContextType = layoutContextType;

NIT: just in case, it'd probably be a good idea to check that the `WI.DOMNode` exists (especially given Bug 189687).
```
    let domNode = this._idToDOMNode[nodeId];
    console.assert(domNode instanceof WI.DOMNode, domNode, nodeId);
    if (!domNode)
        return;

    domNode.layoutContextType = layoutContextType;
```

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:253
> +        this._layoutContextType = layoutContextType;

NIT: I'd also add a check/assert here to make sure that `layoutContextType` is actually different

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1137
> +    LayouytContextTypeChanged: "dom-node-layout-context-type-changed",

`Layouyt` oops :P

[Devin Rousso]

Comment on attachment 419315 [details]
Patch v1.1 - Review Notes

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

I talked a bit more with @Patrick Angle offline and had some other comments.

> Source/JavaScriptCore/inspector/protocol/CSS.json:259
> +            "enum": ["grid", "other"],

IMO the concept of `Other` doesn't really make much sense to me.  I think it may be easier to just make usages of `CSS.LayoutContextType` always optional and to have the "not provided" case be equivalent to `Other`.  As it is, we're not instrumenting all the layout context types (e.g. `block`, `inline`, etc.) so having `Other` is equivalent to "layout context we don't care about" which is IMO basically the same thing as not having a `CSS.LayoutContextType` at all (e.g. `null`).

> Source/JavaScriptCore/inspector/protocol/DOM.json:682
> +            "name": "nodeLayoutContextTypeChanged",

NIT: thinking about this more, this should probably be in `CSS` 😅

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1768
>> +    value->setLayoutContextType(layoutContextTypeForRenderer(node->renderer()));
> 
> Hmm, this means that for detached nodes we may send `Other` even though it doesn't even have a renderer (and therefore a layout context) yet.  Perhaps we should check that `node->renderer()` exists?

Also, if a `Node` starts out as non-`Grid`, do we really want to send the `CSS.LayoutContextType` to the frontend?  Couldn't we have the frontend just assume non-`grid` and only send the event if it's something we care about (see comment on CSS.json:259)?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2483
> +    auto nodeId = identifierForNode(node);

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.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2488
> +    auto newLayoutContextType = layoutContextTypeForRenderer(node.renderer());
> +    if (newLayoutContextType != layoutContextTypeForRenderer(oldRenderer))

We should check this before we `identifierForNode` so that if the `CSS.LayoutContextType` doesn't change to something we care about (e.g. `block` to `flex`) we don't instrument.

[Patrick Angle]

Created attachment 419358 [details]
Patch v1.2 - Review Notes

[Devin Rousso]

Comment on attachment 419358 [details]
Patch v1.2 - Review Notes

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

r=me (after EWS is happy), nice fixes!  Thanks for iterating with me :)

> Source/JavaScriptCore/inspector/protocol/CSS.json:456
> +                { "name": "layoutContextType", "$ref": "LayoutContextType", "optional": true, "description": "The new layout context type of the node." }

NIT: I'd suggest expanding this description a bit to indicate what not providing `layoutContextType` means (e.g. "When not provided the `LayoutContextType` of the node is something that Web Inspector doesn't have specific functionality for." or something).

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:942
> +    if (newLayoutContextType != layoutContextTypeForRenderer(oldRenderer)) {

NIT: you could make this into an early-return instead of nesting :P

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:1159
> +WI.DOMNode.LayoutContextType = {

NIT: since the name of this doesn't directly match any `WI.CSS*` model object, perhaps a comment like
```
    // Corresponds to `CSS.LayoutContextType`.
```
would be helpful (and allow for searching) :)

> LayoutTests/inspector/css/nodeLayoutContextTypeChanged.html:24
> +                let domNode = await WI.domManager.nodeForId(nodeId);

NIT: no need for `await` since `WI.DOMManager.prototype.nodeForId` is not `async` :P

[Patrick Angle]

Created attachment 419415 [details]
Patch v1.3 - Review Notes

[EWS]

Committed r272433: <https://trac.webkit.org/changeset/272433>

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

[Ryosuke Niwa]

Comment on attachment 419415 [details]
Patch v1.3 - Review Notes

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

> Source/WebCore/dom/Node.h:710
> +    virtual void didChangeRenderer(RenderObject*) { };

Please don't introduce a virtual function call like this in the middle of a perf critical path.

> Source/WebCore/rendering/RenderObject.h:1152
> +    auto oldRenderer = this->renderer();
>      m_rendererWithStyleFlags.setPointer(renderer);
> +    didChangeRenderer(oldRenderer);

This is not okay. We can't introduce an extra load to renderer() and a dependent write and then a virtual function call.

[Patrick Angle]

Comment on attachment 419415 [details]
Patch v1.3 - Review Notes

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

>> Source/WebCore/rendering/RenderObject.h:1152
>> +    didChangeRenderer(oldRenderer);
> 
> This is not okay. We can't introduce an extra load to renderer() and a dependent write and then a virtual function call.

From a performance standpoint, would it be acceptable to gate this behavior behind a boolean check for having an inspector attached? I can also refactor to not be a virtual function.

[Ryosuke Niwa]

(In reply to Patrick Angle from comment #14)
> Comment on attachment 419415 [details]
> Patch v1.3 - Review Notes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419415&action=review
> 
> >> Source/WebCore/rendering/RenderObject.h:1152
> >> +    didChangeRenderer(oldRenderer);
> > 
> > This is not okay. We can't introduce an extra load to renderer() and a dependent write and then a virtual function call.
> 
> From a performance standpoint, would it be acceptable to gate this behavior
> behind a boolean check for having an inspector attached? I can also refactor
> to not be a virtual function.

Not really. This function is inlined everywhere. Adding an extra function call is going to be really expensive. Given we're only interested in RenderGrid, why isn't this done inside RenderGrid::RenderGrid and RenderGrid::~RenderGrid instead? Why slow down everything else?