[webkit-reviews] review denied: [Bug 221228] Web Inspector: Implement backend support for maintaining a list of Grid layout contexts : [Attachment 419205] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Patrick Angle <pangle at apple.com>'s
request for review:
Bug 221228: Web Inspector: Implement backend support for maintaining a list of
Grid layout contexts
https://bugs.webkit.org/show_bug.cgi?id=221228

Attachment 419205: Patch v1.0

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 419205
  --> https://bugs.webkit.org/attachment.cgi?id=419205
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.di
splay = "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"