Bug 221062 – Web Inspector: implement the basics for showing/hiding grid overlays

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

Created attachment 418587 [details]
Patch v0.1 - WIP (no drawing code)

I've done the boring parts to hook up the frontend and backend. This needs: drawing code, tests for the protocol, and exposed via the UI somewhere (Layout pane?)

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

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

[Joseph Pecoraro]

Comment on attachment 418587 [details]
Patch v0.1 - WIP (no drawing code)

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

Just some drive by comments because I was interested.

> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:87
> +    "DOM.GridOverlayConfig": [],

Oh? What additional properties do you expect?

> Source/WebCore/inspector/InspectorOverlay.h:139
> +        Color gridColor;
> +        bool showLineNames;
> +        bool showLineNumbers;
> +        bool showExtendedGridlines;
> +        bool showTrackSizes;
> +        bool showAreaNames;

Hold a `Config` here instead of duplicating all the properties?

> Source/WebInspectorUI/UserInterface/Models/Color.js:644
> +        let rgba = this.rgba;
> +        return {r: rgba[0], g: rgba[1], b: rgba[2], a: rgba[3]};

Too crazy? =)

```
let [r, g, b, a] = this.rgba;
return {r, g, b, a};
```

If there is a performance difference, I'd want JSC to look into it!

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:557
> +    // Supported options: showLineNames, showLineNumbers, showExtendedGridLines, showTrackSizes, showAreaNames.
> +    // See DOM.GridOverlayConfig and associated protocol commands for more information about these grid overlay options.

Alternative: You could make a WI.GridOverlayConfig Model object with a .toProtocol().

[Patrick Angle]

Created attachment 418597 [details]
Patch v0.1 - WIP (drawing code)

[Patrick Angle]

Comment on attachment 418597 [details]
Patch v0.1 - WIP (drawing code)

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

> Source/WebCore/inspector/InspectorOverlay.cpp:578
>  InspectorOverlay::RulerExclusion InspectorOverlay::drawNodeHighlight(GraphicsContext& context, Node& node)

To be clear, the new code below should not live in this method, it was just a convenient place for prototyping. This drawing code doesn't handle rotation/scaling/translation/etc.

[Devin Rousso]

Comment on attachment 418587 [details]
Patch v0.1 - WIP (no drawing code)

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

Some comments on the protocol.

> Source/JavaScriptCore/inspector/protocol/DOM.json:38
> +            "enum": ["grid", "subgrid", "other"],

Do we really need an `"other"` if this is already being used optionally?  What would `"other"` even be used for?

Also, it might be a good idea to actually enumerate all of the types here right now and instrument them all so that even if you're not able to build UI for all of them (assuming you want to) then if the UI is added in a later release it would still be able to support older remote debuggables.

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

IMO these properties should really just be parameters of `showGridOverlay` rather than be part of an object (which is harder to deal with as a `JSON::Object` instead of individual POD values).

> Source/JavaScriptCore/inspector/protocol/DOM.json:525
> +            "name": "showGridOverlay",

This should also be `"condition": "!(defined(WTF_PLATFORM_IOS_FAMILY) && WTF_PLATFORM_IOS_FAMILY)",` until `InspectorOverlay` is made to work on iOS.

> Source/JavaScriptCore/inspector/protocol/DOM.json:534
> +            "name": "hideGridOverlay",

ditto (:525)

[Devin Rousso]

Comment on attachment 418597 [details]
Patch v0.1 - WIP (drawing code)

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

> Source/JavaScriptCore/inspector/protocol/CSS.json:312
> +            "name": "getGridDataForNode",

I realize this is WIP, but I'm curious as to what direction you're heading in.  Does this exist only for getting data to be shown in the frontend (e.g. the names of the tracks so you can assign colors to them), or is it actually going to be data that you're going to pass back to the backend.  If the former, 👍.  If the latter, I wonder if that's really necessary and would be interested in hearing why you think so.

[Devin Rousso]

Comment on attachment 418587 [details]
Patch v0.1 - WIP (no drawing code)

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

>> Source/JavaScriptCore/inspector/protocol/DOM.json:38
>> +            "enum": ["grid", "subgrid", "other"],
> 
> Do we really need an `"other"` if this is already being used optionally?  What would `"other"` even be used for?
> 
> Also, it might be a good idea to actually enumerate all of the types here right now and instrument them all so that even if you're not able to build UI for all of them (assuming you want to) then if the UI is added in a later release it would still be able to support older remote debuggables.

Also, I wonder if maybe this should be in `CSS` given that layout contexts and grid stuff are all CSS concepts.

[Patrick Angle]

Comment on attachment 418597 [details]
Patch v0.1 - WIP (drawing code)

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

>> Source/JavaScriptCore/inspector/protocol/CSS.json:312
>> +            "name": "getGridDataForNode",
> 
> I realize this is WIP, but I'm curious as to what direction you're heading in.  Does this exist only for getting data to be shown in the frontend (e.g. the names of the tracks so you can assign colors to them), or is it actually going to be data that you're going to pass back to the backend.  If the former, 👍.  If the latter, I wonder if that's really necessary and would be interested in hearing why you think so.

Neither. This was a stub of mine for testing that is superseded by what BJ uploaded and we have sketched out for protocol. The preliminary attempt at drawing code is the only useful thing in this patch. Sorry for the confusion.

[Blaze Burg]

Comment on attachment 418587 [details]
Patch v0.1 - WIP (no drawing code)

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

Thanks for the comments, all. Now time to mash the two together!

>>> Source/JavaScriptCore/inspector/protocol/DOM.json:38
>>> +            "enum": ["grid", "subgrid", "other"],
>> 
>> Do we really need an `"other"` if this is already being used optionally?  What would `"other"` even be used for?
>> 
>> Also, it might be a good idea to actually enumerate all of the types here right now and instrument them all so that even if you're not able to build UI for all of them (assuming you want to) then if the UI is added in a later release it would still be able to support older remote debuggables.
> 
> Also, I wonder if maybe this should be in `CSS` given that layout contexts and grid stuff are all CSS concepts.

As for layout context types, it is fine to stub out all the values, but I don't see the point implementing them on the backend until more important pieces are working.

I'm indifferent about the domains, but other highlighting commands are in DOM and deal with DOMNodes, so I put it here.

>> Source/JavaScriptCore/inspector/protocol/DOM.json:42
>> +            "id": "GridOverlayConfig",
> 
> IMO these properties should really just be parameters of `showGridOverlay` rather than be part of an object (which is harder to deal with as a `JSON::Object` instead of individual POD values).

If it's not possible to feature detect fields on an object, then perhaps it would be better as command parameters.

>> Source/JavaScriptCore/inspector/protocol/DOM.json:525
>> +            "name": "showGridOverlay",
> 
> This should also be `"condition": "!(defined(WTF_PLATFORM_IOS_FAMILY) && WTF_PLATFORM_IOS_FAMILY)",` until `InspectorOverlay` is made to work on iOS.

Is there really a benefit to doing this right now? I know it doesn't work right now, and fixing it is scheduled. If we don't end up doing it, then sure, guard it off.

>> Source/WebCore/inspector/InspectorOverlay.h:139
>> +        bool showAreaNames;
> 
> Hold a `Config` here instead of duplicating all the properties?

OK

>> Source/WebInspectorUI/UserInterface/Models/Color.js:644
>> +        return {r: rgba[0], g: rgba[1], b: rgba[2], a: rgba[3]};
> 
> Too crazy? =)
> 
> ```
> let [r, g, b, a] = this.rgba;
> return {r, g, b, a};
> ```
> 
> If there is a performance difference, I'd want JSC to look into it!

I knew there was a clever way to do it, thanks!

>> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:557
>> +    // See DOM.GridOverlayConfig and associated protocol commands for more information about these grid overlay options.
> 
> Alternative: You could make a WI.GridOverlayConfig Model object with a .toProtocol().

This is a solid idea. I want to hold off on making this more generalized until we've figured out how overlay configs will be serialized to async storage. Ideally we can store and fetch the entire POD JSON object.

[Blaze Burg]

Created attachment 418820 [details]
Patch v1.0

I think this patch is ready to proceed with review.

[Blaze Burg]

Created attachment 418839 [details]
Patch v1.1

[Devin Rousso]

Comment on attachment 418839 [details]
Patch v1.1

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

r=me with some things to address before landing :)

> Source/WebInspectorUI/ChangeLog:26
> +        no callback argument was pasased. This is used by a new test.

s/pasased/passed

> Source/JavaScriptCore/inspector/protocol/DOM.json:497
> +            "name": "showGridOverlay",

>> This should also be `"condition": "!(defined(WTF_PLATFORM_IOS_FAMILY) && WTF_PLATFORM_IOS_FAMILY)",` until `InspectorOverlay` is made to work on iOS.
>>
> Is there really a benefit to doing this right now? I know it doesn't work right now, and fixing it is scheduled. If we don't end up doing it, then sure, guard it off.

I think the benefit of doing it right now is that we won't forget to do it later and we can confirm now that it is working as intended (since the iOS overlay work hasn't been done yet).

> Source/WebCore/inspector/InspectorOverlay.cpp:418
> +    for (const GridOverlay& gridOverlay : m_activeGridOverlays)
> +        drawGridOverlay(context, gridOverlay);

NIT: I feel like grid overlay UI should be drawn below paint rects, not above.

> Source/WebCore/inspector/InspectorOverlay.cpp:566
> +    if (!renderer)

NIT: do we really need two checks with two different errors?  `is<RenderGrid>(renderer)` will already check for whether `renderer` is a `nullptr` or not.

> Source/WebCore/inspector/InspectorOverlay.cpp:572
> +        return gridOverlay.gridNode.get() == &node;

Do we need to boolean-check `gridOverlay.gridNode` before we call `.get()`?
```
    return !gridOverlay.gridNode || gridOverlay.gridNode.get() == &node;
```

> Source/WebCore/inspector/InspectorOverlay.cpp:585
> +        return gridOverlay.gridNode.get() == &node;

ditto (:572)

> Source/WebCore/inspector/InspectorOverlay.cpp:1148
> +            return gridOverlay.gridNode.get() == gridOverlay.gridNode;

ditto (:572)

> Source/WebCore/inspector/InspectorOverlay.cpp:1156
> +    if (!is<RenderGrid>(renderer))

Should we remove this from the list if it's no longer a grid context?

> Source/WebCore/inspector/InspectorOverlay.h:49
> +namespace Inspector {
> +using ErrorString = String;
> +
> +template <typename T>
> +using ErrorStringOr = Expected<T, ErrorString>;
> +}

NIT: I'd just `#include <JavaScriptCore/InspectorProtocolTypes.h>`

> Source/WebCore/inspector/InspectorOverlay.h:108
> +    struct GridOverlay {

NIT: I'd just call this `Grid` since it's fully qualified name is `InspectorOverlay::Grid` anyways (the second "Overlay" is kinda redundant IMO)

> Source/WebCore/inspector/InspectorOverlay.h:122
> +        WeakPtr<Node> gridNode;

`#include <wtf/WeakPtr>` (or replace the existing stuff with `#include <wtf/Forward.h>`)

> Source/WebCore/inspector/InspectorOverlay.h:148
> +    unsigned gridOverlayCount() const { return m_activeGridOverlays.size(); }

NIT: rather than return the `size()`, do you want to return a list of `Node` instead?  That way the test can confirm that not only are there the right number of grid overlay UIs shown but that they're also for the right nodes.

> Source/WebCore/inspector/InspectorOverlay.h:156
> +    Inspector::ErrorStringOr<void> setGridOverlayForNode(Node&, const GridOverlay::Config&);

NIT: should we make the `Grid::Config&&` instead?  I doubt that callers would want to keep the `Grid::Config`.

> Source/WebCore/inspector/InspectorOverlay.h:193
> +    Vector<GridOverlay> m_activeGridOverlays;

Since there can only be one `Grid` per `Node`, could we have this be a `HashMap` (or `WeakHashMap` not sure if that exists) instead?  Along these lines, if a `Node` with an active `DOM.showGridOverlay` is removed from the DOM and GCd, what will remove it from this list?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1484
> +    config.gridColor = parseColor(WTFMove(gridColor));

Does this not have a `Protocol::ErrorString`?  What do we output if `gridColor` is invalid?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1499
> +    Protocol::ErrorString errorString;
> +    Node* node = nullptr;

I'd restructure this slightly to have less stuff at a higher scope than necessary
```
    if (nodeId) {
        Protocol::ErrorString errorString;
        auto* node = assertNode(errorString, *nodeId);
        if (!node)
            return makeUnexpected(errorString);

        return m_overlay->clearGridOverlayForNode(*node);
    }

    m_overlay->clearAllGridOverlays();

    return { };
```

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:166
> +        if (this._documentPromise)

If you want to do this then you'll also need to clear `_documentPromise` whenever `_document` changes.

Also, these `_requestDocumentWith*` functions should really be moved to a `// Private` section below (although I admit this file needs a bit of cleanup).

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:173
> +            this._requestDocumentWithCallback((doc) => this._documentPromise.resolve(doc));

Style: We only use single-line arrow functions if we expect to use the implicitly returned value.  Please make this into a multi-line arrow function with `{` `}`.

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:556
> +    showGridOverlay(color, options = {showLineNames: false, showLineNumbers: false, showExtendedGridLines: false, showTrackSizes: false, showAreaNames: false})

Style: We normally don't provide a default value unless it's truthy.

Also, we normally destruct with the values and default-initialize them in the body rather than doing it as part of an optional parameter declaration (we tend to avoid providing default values for optional object parameters because the default value is only used if the key is entirely missing, meaning that we'd likely want to check inside the function body anyways, so why bother having logic to have a default if we're gonna check it later).
```
    showGridOverlay(color, {showLineNames, showLineNumbers, showExtendedGridLines, showTrackSizes, showAreaNames} = {})
```
This way you don't have to `options.*` in the function.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:390
> +        // FIXME: remove these menu items when removing the feature flag. They exist to test the backend commands easily.

Please include a bug with this FIXME so that we have something to keep track of.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:391
> +        if (WI.settings.experimentalEnableLayoutPanel.value && WI.isEngineeringBuild) {

NIT: I'd put the `WI.isEngineeringBuild` first so that we don't even have to look at `localStorage` when in non-engineering builds.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:396
> +                    domNode.showGridOverlay(color).catch((error) => console.error(error));

NIT: you should be able to just `.catch(console.error)` instead of wrapping it in an arrow function

> LayoutTests/inspector/dom/showGridOverlay.html:12
> +        let result = await DOMAgent.querySelector(doc.id, ".grid-container");

Why not `doc.querySelectorAll(".grid-container")`?

> LayoutTests/inspector/dom/showGridOverlay.html:18
> +        let result = await DOMAgent.querySelectorAll(doc.id, ".grid-container");

ditto (:12)

> LayoutTests/inspector/dom/showGridOverlay.html:27
> +        name: "GridOverlay.ShowOneGrid",

Style: We usually prefix the name of each test-case with the name of the overall suite (e.g. "DOM.showGridOverlay.OneGrid").

> LayoutTests/inspector/dom/showGridOverlay.html:29
> +        async test(resolve, reject) {

remove the `resolve, reject` since this is an `async` function

> LayoutTests/inspector/dom/showGridOverlay.html:30
> +            InspectorTest.expectEqual(await gridOverlayCount(), 0, "Expect no grids to be displayed.");

NIT: We normally phrase our messages with verbs like "Should", so something like "Should have 0 grids displayed.".

> LayoutTests/inspector/dom/showGridOverlay.html:35
> +            InspectorTest.expectEqual(await gridOverlayCount(), 1, "Expect one grid to be displayed.");

I personally don't like using `await` inside unless as part of a statement (i.e. not as part of a sub-expression) as it makes it slightly harder to grok the order of things.  I'd rather create closures for each sub-test and have a variable like `let count = await gridOverlayCount()` (or even have something like `async checkGridOverlayCount` that takes the expected value as an argument) so that it's explicit what lines are async and what lines are sync.

> LayoutTests/inspector/dom/showGridOverlay.html:147
> +            margin: 100px;

Are any of the styles other than `display` and `grid-*` really needed?

> LayoutTests/inspector/dom/showGridOverlay.html:153
> +            background-color: #fff;

`white`

> LayoutTests/inspector/dom/showGridOverlay.html:154
> +            color: #555;

Why this color?

> LayoutTests/inspector/dom/showGridOverlay.html:168
> +        <div class="box a">A</div>

Do we actually need content inside these boxes for the test to work?  I ask because it adds to the output and could be confusing to someone trying to diagnose a possible future failure with this test.

[Blaze Burg]

Comment on attachment 418839 [details]
Patch v1.1

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:566
>> +    if (!renderer)
> 
> NIT: do we really need two checks with two different errors?  `is<RenderGrid>(renderer)` will already check for whether `renderer` is a `nullptr` or not.

OK

>> Source/WebCore/inspector/InspectorOverlay.cpp:572
>> +        return gridOverlay.gridNode.get() == &node;
> 
> Do we need to boolean-check `gridOverlay.gridNode` before we call `.get()`?
> ```
>     return !gridOverlay.gridNode || gridOverlay.gridNode.get() == &node;
> ```

OK

>> Source/WebCore/inspector/InspectorOverlay.cpp:585
>> +        return gridOverlay.gridNode.get() == &node;
> 
> ditto (:572)

OK

>> Source/WebCore/inspector/InspectorOverlay.cpp:1156
>> +    if (!is<RenderGrid>(renderer))
> 
> Should we remove this from the list if it's no longer a grid context?

Oh, duh.

>> Source/WebCore/inspector/InspectorOverlay.h:49
>> +}
> 
> NIT: I'd just `#include <JavaScriptCore/InspectorProtocolTypes.h>`

I don't want to qualify Inspector::Protocol::ErrorStringOr all over, since this has nothing to do with protocol code.

>> Source/WebCore/inspector/InspectorOverlay.h:108
>> +    struct GridOverlay {
> 
> NIT: I'd just call this `Grid` since it's fully qualified name is `InspectorOverlay::Grid` anyways (the second "Overlay" is kinda redundant IMO)

OK. Still keeping it in variable names otherwise it would be impossible to grep.

>> Source/WebCore/inspector/InspectorOverlay.h:122
>> +        WeakPtr<Node> gridNode;
> 
> `#include <wtf/WeakPtr>` (or replace the existing stuff with `#include <wtf/Forward.h>`)

OK

>> Source/WebCore/inspector/InspectorOverlay.h:148
>> +    unsigned gridOverlayCount() const { return m_activeGridOverlays.size(); }
> 
> NIT: rather than return the `size()`, do you want to return a list of `Node` instead?  That way the test can confirm that not only are there the right number of grid overlay UIs shown but that they're also for the right nodes.

I considered it, but just went with number since I didn't think it was worth the trouble to figure out how to hook up NodeList. I don't anticipate adding to these tests, they are just to cover the very basics.

>> Source/WebCore/inspector/InspectorOverlay.h:156
>> +    Inspector::ErrorStringOr<void> setGridOverlayForNode(Node&, const GridOverlay::Config&);
> 
> NIT: should we make the `Grid::Config&&` instead?  I doubt that callers would want to keep the `Grid::Config`.

OK

>> Source/WebCore/inspector/InspectorOverlay.h:193
>> +    Vector<GridOverlay> m_activeGridOverlays;
> 
> Since there can only be one `Grid` per `Node`, could we have this be a `HashMap` (or `WeakHashMap` not sure if that exists) instead?  Along these lines, if a `Node` with an active `DOM.showGridOverlay` is removed from the DOM and GCd, what will remove it from this list?

As for why a Vector, I don't want to deal weak keys. Also, I want stable iteration/paint order without depending on HashMap/HastListSet invariants. Also, it's unclear right now whether we want to draw >1 grid per node, which may be relevant for subgrid.

Overlays with an empty WeakPtr<Node> reference get cleared when we try to draw it and discover its empty, or similarly when adding/removing overlays. I extracted this code to removeGridOverlayForNode(Node&).

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1484
>> +    config.gridColor = parseColor(WTFMove(gridColor));
> 
> Does this not have a `Protocol::ErrorString`?  What do we output if `gridColor` is invalid?

Umm.. this is so dumb.:

    auto r = colorObject->getInteger(Protocol::DOM::RGBAColor::rKey);
    auto g = colorObject->getInteger(Protocol::DOM::RGBAColor::gKey);
    auto b = colorObject->getInteger(Protocol::DOM::RGBAColor::bKey);
    if (!r || !g || !b)
        return Color::transparentBlack;


I'll make it return Optional<Color>, but in the interest of preserving behavior, all other callsites will get the "error" behavior tacked on: parseColor(c).valueOr(Color::transparentBlack)

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:1499
>> +    Node* node = nullptr;
> 
> I'd restructure this slightly to have less stuff at a higher scope than necessary
> ```
>     if (nodeId) {
>         Protocol::ErrorString errorString;
>         auto* node = assertNode(errorString, *nodeId);
>         if (!node)
>             return makeUnexpected(errorString);
> 
>         return m_overlay->clearGridOverlayForNode(*node);
>     }
> 
>     m_overlay->clearAllGridOverlays();
> 
>     return { };
> ```

Good catch.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:166
>> +        if (this._documentPromise)
> 
> If you want to do this then you'll also need to clear `_documentPromise` whenever `_document` changes.
> 
> Also, these `_requestDocumentWith*` functions should really be moved to a `// Private` section below (although I admit this file needs a bit of cleanup).

OK

>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:173
>> +            this._requestDocumentWithCallback((doc) => this._documentPromise.resolve(doc));
> 
> Style: We only use single-line arrow functions if we expect to use the implicitly returned value.  Please make this into a multi-line arrow function with `{` `}`.

OK

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:390
>> +        // FIXME: remove these menu items when removing the feature flag. They exist to test the backend commands easily.
> 
> Please include a bug with this FIXME so that we have something to keep track of.

OK

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:391
>> +        if (WI.settings.experimentalEnableLayoutPanel.value && WI.isEngineeringBuild) {
> 
> NIT: I'd put the `WI.isEngineeringBuild` first so that we don't even have to look at `localStorage` when in non-engineering builds.

OK

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:396
>> +                    domNode.showGridOverlay(color).catch((error) => console.error(error));
> 
> NIT: you should be able to just `.catch(console.error)` instead of wrapping it in an arrow function

OK

>> LayoutTests/inspector/dom/showGridOverlay.html:12
>> +        let result = await DOMAgent.querySelector(doc.id, ".grid-container");
> 
> Why not `doc.querySelectorAll(".grid-container")`?

Oh, yeah.

>> LayoutTests/inspector/dom/showGridOverlay.html:18
>> +        let result = await DOMAgent.querySelectorAll(doc.id, ".grid-container");
> 
> ditto (:12)

OK

>> LayoutTests/inspector/dom/showGridOverlay.html:27
>> +        name: "GridOverlay.ShowOneGrid",
> 
> Style: We usually prefix the name of each test-case with the name of the overall suite (e.g. "DOM.showGridOverlay.OneGrid").

OK

>> LayoutTests/inspector/dom/showGridOverlay.html:29
>> +        async test(resolve, reject) {
> 
> remove the `resolve, reject` since this is an `async` function

OK (I feel old)

>> LayoutTests/inspector/dom/showGridOverlay.html:35
>> +            InspectorTest.expectEqual(await gridOverlayCount(), 1, "Expect one grid to be displayed.");
> 
> I personally don't like using `await` inside unless as part of a statement (i.e. not as part of a sub-expression) as it makes it slightly harder to grok the order of things.  I'd rather create closures for each sub-test and have a variable like `let count = await gridOverlayCount()` (or even have something like `async checkGridOverlayCount` that takes the expected value as an argument) so that it's explicit what lines are async and what lines are sync.

OK!

>> LayoutTests/inspector/dom/showGridOverlay.html:147
>> +            margin: 100px;
> 
> Are any of the styles other than `display` and `grid-*` really needed?

It's rather hard to sanity check the grid lines unless the elements have a background or border (harder to see).

>> LayoutTests/inspector/dom/showGridOverlay.html:153
>> +            background-color: #fff;
> 
> `white`

OK

>> LayoutTests/inspector/dom/showGridOverlay.html:154
>> +            color: #555;
> 
> Why this color?

¯\_(ツ)_/¯

>> LayoutTests/inspector/dom/showGridOverlay.html:168
>> +        <div class="box a">A</div>
> 
> Do we actually need content inside these boxes for the test to work?  I ask because it adds to the output and could be confusing to someone trying to diagnose a possible future failure with this test.

I'm not worried about it.

[Blaze Burg]

Created attachment 418966 [details]
Patch v1.2

[EWS]

Committed r272197: <https://trac.webkit.org/changeset/272197>

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

[Blaze Burg]

<rdar://73504216>