[webkit-reviews] review denied: [Bug 221145] Web Inspector: Display all CSS grids on page in Layout panel : [Attachment 419051] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Feb 2 14:12:18 PST 2021
Devin Rousso <drousso at apple.com> has denied Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 221145: Web Inspector: Display all CSS grids on page in Layout panel
https://bugs.webkit.org/show_bug.cgi?id=221145
Attachment 419051: Patch
https://bugs.webkit.org/attachment.cgi?id=419051&action=review
--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 419051
--> https://bugs.webkit.org/attachment.cgi?id=419051
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=419051&action=review
r-, this is a good start! I have some code layout/organization comments, as
well as lots of style comments (I'd suggest familiarizing with
<https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide>), but I think the
core logic/UI is solid. Please let me know if anything is confusing :)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:37
> + async show(type, config)
`DOM.showGridOverlay` requires a `DOM.NodeId`, so we should match that here as
well
```
async show (overlayType, domNode, options = {})
{
console.assert(Object.values(WI.OverlayManager.OverlayType).includes(overlayTyp
e), overlayType);
console.assert(domNode instanceof WI.DOMNode, domNode);
```
where `options` would contain the remaining optional parameters listed in the
protocol
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:41
> + let overlayIndex = this._shownOverlays.findIndex((overlay) => {
return overlay.type === type && overlay.nodeId === config.nodeId; });
Style: either drop the `{` and `}` and `return` or put the function body onto
separate lines
```
let overlayIndex = this._shownOverlays.findIndex((overlay) => overlay.type
=== overlayType && overlay.domNode === domNode);
```
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:45
> + if (Object.shallowEqual({type, ...config}, overlay))
While I appreciate this check, is there ever a situation right now where we
pass the exact same values? If it is, assuming the backend doesn't error if
the exact same values are passed, I'd just make this into a `console.assert` as
it wouldn't cause problems but is perhaps something that we should avoid/fix.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:51
> + let domNode = WI.domManager.nodeForId(config.nodeId);
> + if (!domNode)
> + return Promise.reject(`Cannot show overlay, node is missing for
id ${config.nodeId}`);
I would strongly suggest that you store the actual `WI.DOMNode` instead of the
ID wherever possible as it'll give you more flexibility (and avoid having to
repeatedly call `WI.domManager.nodeForId`).
Also, by passing a `WI.DOMNode` you know for a fact that you have something
valid (you'd also want to check `domNode.destroyed` though) and wouldn't need
these extra checks.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:58
> + // Overwrite the existing overlay object if already defined or
create a new one.
> + overlay = {type, ...config};
Rather than have one giant list that needs to be searched for both `WI.DOMNode`
and `WI.OverlayManager.OverlayType`, can we create a separate `Map` for each
`WI.OverlayManager.OverlayType`? As an example, you could have
`_gridOverlayForNode` which you'd use if the given `overlayType ===
WI.OverlayManager.OverlayType.Grid`. That way you wouldn't have to do as much
searching when showing/hiding non-grid overlays and could avoid having to do
things based off index (the key would be the `WI.DOMNode`).
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:64
> + // FIXME (rcaliman): Set pending promise and wait for it to settle
before calling another to prevent race conditions.
oops?
Also, the Web Inspector protocol is guaranteed to resolve in the order in which
things are called, so calling two `DOM.showGridOverlay` where the second is
invoked before the first resolves is not a problem.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:70
> + async hide(type, config)
ditto (:37)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:111
> + _getShowOverlayMethod(type, domNode) {
This type of indirection is not usually something we do as it makes it harder
for code to be followed and searched. Since this is only used once, I'd move
the `switch` into `async show`.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
> + // FIXME (rcaliman): Move contents of DOMNode.showGridOverlay()
here and obsolete that.
oops?
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:124
> + _getHideOverlayMethod(type, domNode) {
ditto (:111) with `async hide`
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:143
> +WI.OverlayManager.Type = {
NIT: I'd call this `OverlayType` so that it's clear that the type is referring
to the overlay and not the overlay manager. My initial thought when i saw this
was that you'd have a different `WI.OverlayManager` per type instead of one
global `WI.OverlayManager`.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:144
> + Grid: "Grid",
We usually lowercase enum values (unless they match an uppercase value in the
protocol, in which case this should have a comment indicating that).
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:26
> +.grid-style-section > .content {
These classes do not correspond to anything created in
`GridDetailsSectionRow.js`. We should only be using styles present there.
This rule should probably be moved to `LayoutDetailsSidebarPanel.css` instead.
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:27
> + font-size: 11px;
Is the inherited `font-size` too large? Can we put this in `.details-section >
.row.css-grid` instead so that if other things are added to
`.grid-style-section > .content` then they are not also affected?
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:30
> +.grid-style-section .css-grid {
I'd suggest using a more specific selector like `.details-section >
.row.css-grid` so that we avoid conflicts (especially with such a general class
like `.css-grid`).
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.css:31
> + margin: 0 4px 4px 6px;
Please use LTR/RTL agnostic properties if the left and right values differ
```
margin-inline-start: 6px;
margin-inline-end: 4px;
margin-bottom: 4px;
```
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:26
> +WI.GridDetailsSectionRow = class GridDetailsSectionRow extends
WI.DetailsSectionRow {
Style: braces on new lines
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:29
> + super("No CSS grids on page.");
Style: when passing non-obvious constants to a function as an argument, we
usually prefer to create a `const` local variable with the name of the
parameter as the name of the variable so that it's clear what it's used for
```
const emptyMessage = WI.UIString("No CSS grid contexts.");
super(emptyMessage);
```
Also, please use `WI.UIString` for text that is shown in the UI.
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:32
> + this._linkListElement = document.createElement("ul");
NIT: I think just `_listElement` is fine
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:34
> + this.element.appendChild(this._linkListElement);
NIT: we often cut down on line numbers by combining this with the same line as
`document.createElement`
```
this._listElement = this.element.appendChild(document.createElement("ul"));
```
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:36
> + this.element.addEventListener("change", this);
Please don't use the "legacy" event listener object "system" in favor of
explicitly adding event listeners for that event onto the elements that
dispatch them.
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:38
> + this._gridNodeList = [];
NIT: while there's nothing wrong with this, we usually avoid words like "List"
instead preferring just to use the plural form of the items expected to be held
(e.g. `_gridNodes` or `_nodesWithGridContext`)
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:42
> + this._onOverlayStateChanged =
this._onOverlayStateChanged.bind(this);
> +
> +
WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayShown,
this._onOverlayStateChanged);
> +
WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden,
this._onOverlayStateChanged);
Web Inspector's custom event listener system doesn't require (and actively
discourages) from using `.bind`. In fact, this should have triggered some
`console.assert` in inspector^2 since you didn't supply a 3rd argument
(`thisObject`).
```
WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayShown,
this._handleOverlayStateChanged, this);
WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden,
this._handleOverlayStateChanged, this);
```
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:45
> + set gridNodeList(nodeList) {
ditto (:26)
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:46
> + // FIXME (rcaliman): check for different node ids and skip relayout
if not required
oops?
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:51
> + get gridNodeList() {
ditto (:26)
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:55
> + handleEvent(event)
ditto (:36)
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:63
> + _onOverlayStateChanged(event) {
NIT: we prefer to prefix event handlers with `_handle` instead of `_on`
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:68
> + let checkboxElement =
this._linkListElement.querySelector(`input[value="${event.data.nodeId}"]`);
We try to avoid using any DOM querying in Web Inspector code as much as
possible. I'd suggest having a `this._checkboxElementForNode = new Map` in the
constructor that's populated by `this._checkboxElementForNode.set(domNode.id,
checkboxElement)` and can then be queried later by
`this._checkboxElementForNode.get(event.data.nodeId)`.
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:76
> + if (event.type === WI.OverlayManager.Event.OverlayShown)
> + checkboxElement.checked = true;
> +
> + if (event.type === WI.OverlayManager.Event.OverlayHidden)
> + checkboxElement.checked = false;
Can we simplify this as just
```
checkboxElement.checked = event.type ===
WI.OverlayManager.Event.OverlayShown;
```
or are you expecting this handler to deal with more events in the future (if
that's the case then I'd suggest creating specific handlers for each so that
the logic is more straightfoward).
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:86
> + let idAttribute = `toggle-grid-overlay-${domNode.id}`;
Instead of polluting the `id` map, one trick we often use is to nest the form
element inside the `<label>`, so I'd do this instead
```
let itemElement =
this._listElement.appendChild(document.createElement("li"));
let labelElement =
itemElement.appendChild(document.createElement("label"));
let checkboxElement =
labelElement.appendChild(document.createElement("input"));
checkboxElement.type = "checkbox";
checkboxElement.checked = shownOverlayNodes.has(domNode);
labelElement.appendChild(WI.linkifyNodeReference(domNode));
...
checkboxElement.addEventListener("change", (event) => {
if (checkboxElement.checked)
WI.overlayManager.show(WI.OverlayManager.Type.Grid, domNode,
{color: swatch.value});
else
WI.overlayManager.hide(WI.OverlayManager.Type.Grid, domNode);
});
swatch.addEventListener(WI.InlineSwatch.Event.ValueChanged, function(event)
{
if (checkboxElement.checked)
WI.overlayManager.show(WI.OverlayManager.Type.Grid, domNode,
{color: this.value});
}, swatch);
```
> Source/WebInspectorUI/UserInterface/Views/GridDetailsSectionRow.js:101
> + let readOnly = true;
Style: `const`
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:35
> + this._gridListRefresh = this._gridListRefresh.bind(this);
ditto (GridDetailsSectionRow.js:39)
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:104
> + let gridRow = new WI.GridDetailsSectionRow();
Style: you can drop `()` when constructing if there are no arguments
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:106
> + let gridSection = new WI.DetailsSection("grid-style-section",
WI.UIString("Grid", "Grid @ Styles sidebar", "CSS Grid layout section name"),
[gridGroup]);
NIT: the key really should say `Grid @ Elements details sidebar` since this has
nothing to do with the Styles sidebar/panel
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:109
> + this.gridDetailsSectionRow = gridRow;
Should this be "private"? `this>_gridDetailsSectionRow`
Also, please just assign/initialize it directly instead of creating a local
variable.
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:110
> + this._addEventListenersForGridListRefresh();
This should be moved to `attached` so that the event listeners are re-added
after the Layout is re-shown.
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:144
> + _addEventListenersForGridListRefresh()
IMO this could just be inlined in `attached` instead of being a separate
function
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:153
> + _removeEventListenersForGridListRefresh()
ditto (:144) with `detached`
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:162
> + _gridListRefresh()
NIT: `_refreshGridList`?
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:166
> + this._getAllGridDOMNodes().then(gridNodeList => {
NIT: you could make this `async _refreshGridList` so that you can use `await`
```
let gridNodes = await this._getAllGridDOMNodes();
this._gridDetailsSectionRow.gridNodeList = gridNodes;
```
Style: we always wrap the parameters list of arrow functions in `(` `)` even if
there's only one parameter
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:171
> + // Returns an array of WI.DOMNode instances that have `display: grid` or
`display: inline-grid`.
These kinds of comments are kinda redundant when you look at the name of the
function. I think you can remove it.
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:178
> + let ids = await doc.querySelectorAll("*");
I know you said that this isn't expected to be here forever, but please put a
FIXME comment with a bug URL so that we know that this needs to be changed.
Also, I'd use a more descriptive name like `nodeIDs`.
More information about the webkit-reviews
mailing list