[webkit-reviews] review granted: [Bug 221062] Web Inspector: implement the basics for showing/hiding grid overlays : [Attachment 418839] Patch v1.1

Mon Feb 1 14:56:08 PST 2021

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 221062: Web Inspector: implement the basics for showing/hiding grid
overlays
https://bugs.webkit.org/show_bug.cgi?id=221062

Attachment 418839: Patch v1.1

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

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