[webkit-reviews] review granted: [Bug 221228] Web Inspector: Implement backend support for maintaining a list of Grid layout contexts : [Attachment 419358] Patch v1.2 - Review Notes

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted 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 419358: Patch v1.2 - Review Notes

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




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