[webkit-reviews] review denied: [Bug 221145] Web Inspector: Display all CSS grids on page in Layout panel : [Attachment 419507] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 8 15:41:05 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 419507: Patch
https://bugs.webkit.org/attachment.cgi?id=419507&action=review
--- Comment #19 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 419507
--> https://bugs.webkit.org/attachment.cgi?id=419507
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=419507&action=review
r- due to performance concerns in LayoutDetailsSidebarPanel.js, but otherwise
mostly just Style and naming :)
Making good progress!
> Source/WebInspectorUI/ChangeLog:9
> + Show a list of CSS Grid containers in the Layout sidebar panel of
the Elements tab.
NIT: we capitalize `"Tab"` when it's used alongside the name of the tab (e.g.
"tabs" vs "Elements Tab")
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:707
> +
no newline
> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:709
> +localizedStrings["Grid overlays heading @ Layout Sidebar Section"] = "Grid
overlays";
NIT: we usually only put the string to be shown in the UI before the "@" (e.g.
"Grid Overlays @ Layout Sidebar Section Header")
Also, I think this is missing a comment based on the `WI.UIString` call with
this key. Rather than edit this file manually, you can just run
`./Tools/Scripts/extract-localizable-js-strings --utf8
Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js
Source/WebInspectorUI/UserInterface' and have it all done for you :)
> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:186
> + getNodes() {
Style: `{` on next line
Why not make this an actual JS getter (i.e. `get nodes()`)?
> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:703
> - .then(({eventNames}) => new Set(eventNames));
> + .then(({eventNames}) => new Set(eventNames));
while I appreciate these style fixes, we usually don't do these kind of
drive-by fixes as it makes it harder to `git blame` a line and find out when it
was added
> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:778
> - if (!WI.debuggerManager.breakpointsDisabledTemporarily)
> - WI.debuggerManager.breakpointsEnabled = true;
> + if (!WI.debuggerManager.breakpointsDisabledTemporarily)
> + WI.debuggerManager.breakpointsEnabled = true;
ditto (:+703)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:32
> + this._gridOverlayForNodeMap = new Map;
It's really odd to have a `Map` saved to a variable but also be present inside
another `Map` (which is also a member variable). Can we just replace
`this._overlayMapForOverlayTypeMap` with a `switch`?
```
_overlayForOverlayType(overlayType, domNode)
{
switch (overlayType) {
case WI.OverlayManager.OverlayType.Grid:
return this._gridOverlayForNodeMap.get(domNode);
}
console.assert(false); // not reached
return null;
}
```
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:38
> + // Private
Style: `// Private` should go below `// Public`
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:45
> + let overlayMap = this._overlayMapForOverlayTypeMap.get(overlayType);
ditto (:+32)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:46
> + let overlay = {overlayType, domNode, ...options};
I feel like we should create a `WI.OverlayManager.OverlayConfiguration` class
that has this logic so that we don't need to pass around plain objects and do
(potentially colliding) object spreads.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:56
> + let overlayMap = this._overlayMapForOverlayTypeMap.get(overlayType);
ditto (:+32)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:57
> + let overlay = overlayMap.get(domNode);
We have a utility for `Map.prototype.take` which does a `get` and `delete` in
one call :)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:65
> + // Grid Overlay
Style: this should just be `'// Public`
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:73
> + console.assert(color instanceof WI.Color, color);
Since `color` is part of the implicit `options` object, it's not required, so
this should be accepting of that.
```
console.assert(!color || color instanceof WI.Color, color);
```
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:76
> + if (!color)
> + color = WI.Color.fromString("magenta"); // fallback color
```
color ||= WI.Color.fromString("magenta");
```
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:79
> + let options = {
NIT: a more accurate name for this would be `arguments`
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:90
> + target.DOMAgent.showGridOverlay.invoke(options);
> +
> + this._show(WI.OverlayManager.OverlayType.Grid, domNode, options);
Does this need an `await` somewhere? I ask because you're returning a rejected
`Promise `above and we prefer to have our functions be consistent in their
return values.
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:103
> + target.DOMAgent.hideGridOverlay(domNode.id);
> +
> + this._hide(WI.OverlayManager.OverlayType.Grid, domNode);
ditto (:+90)
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:106
> + toggleGridOverlay(domNode, options = {})
This doesn't appear to be called anywhere. Do we need it?
Generally we try to reflect the state of things based on what's shown in the
UI, so for example if somehow a checkbox becomes out of sync with the backend
we'd rather send the same message twice than have the backend not show an
overlay when the checkbox is checked (or vice versa), so we usually avoid
"toggle" kinds of things and instead prefer explicit "enable" and "disable".
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:114
> + getNodesForGridOverlay()
Why not make this an actual JS getter (i.e. `get nodesWithGridOverlay()`)?
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:116
> + return this._gridOverlayForNodeMap.keys();
NIT: this will return an interator, not an array/set. I notice that you wrap
it in `new Set` at the callsite, but that shouldn't be necessary since `Map`
keys cannot be repeated. Perhaps we should just `Array.from` here so that
callers can `filter`/`map`/etc.?
> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:125
> +WI.OverlayManager.OverlayType = {
based on my comment on CSSGridSection.js:+103, this will not be used outside of
`WI.OverlayManager`, so you could make this "private"
(`WI.OverlayManager._OverlayType`) as it really is a bookkeeping detail of
`WI.OverlayManager` and perhaps not anything anyone outside of
`WI.OverlayManager` would ever need to use
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.css:33
> + margin-block-start: 1em;
> + margin-block-end: 0.75em;
we usually don't use `em` units
also, please use `margin-top` and `margin-bottom` since Web Inspector only
supports RTL not vertical writing modes
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.
2021
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:34
> + this._gridNodeList = new Set;
`List` should not be used when the value is a `Set`
Either make it `_gridNodeSet` or just drop it altogether as `_gridNodes` (this
implies some form of flat collection, which is usually good enough)
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:38
> +
WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayShown,
this._handleOverlayStateChanged, this);
> +
WI.overlayManager.addEventListener(WI.OverlayManager.Event.OverlayHidden,
this._handleOverlayStateChanged, this);
please move these to `initialLayout`
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:43
> + set gridNodeList(nodeList) {
Style: `{` on new line
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:48
> + get gridNodeList() {
Style: `{` on new line
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:68
> + this._listElement.removeChildren();
Style: newline after
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:69
> + let shownOverlayNodes = new
Set(WI.overlayManager.getNodesForGridOverlay());
see comments on OverlayManager.js:+116
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:72
> +
Style: unnecessary newline
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:90
> + const readOnly = false;
> + let swatch = new WI.InlineSwatch(WI.InlineSwatch.Type.Color,
WI.Color.fromString("magenta"), readOnly);
it's not necessary to provide falsy arguments to functions/constructors if they
are trailing arguments as falsy (specifically `undefined`) is implicitly used
if the argument is not provided
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:102
> + _handleOverlayStateChanged(event) {
Style: `{` on new line
> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:104
> + // Only care about events for Grid overlays, ignore all else.
> + if (event.data.overlayType !== WI.OverlayManager.OverlayType.Grid)
Do we ever think there will be a scenario where a listener will care about more
than one type of overlay at the same time? I would think not.
Based on this, I'd suggest making the `WI.OverlayManager.Event` more specific
to the type of overlay (e.g. `WI.OverlayManager.Event.GridOverlayShown`) so
that this can be avoided (not to mention it'll be more performant if more
overlays are introduced because this won't have to listen for changes to
those).
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.css:29
> + margin-block-end: 4px;
please use `margin-bottom` since Web Inspector only supports RTL not vertical
writing modes
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:33
> + this._gridNodeList = new Set;
ditto (CSSGridSection.js:+34)
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:86
> + WI.domManager.addEventListener(WI.DOMManager.Event.NodeInserted,
this._refreshGridNodeList, this);
> + WI.domManager.addEventListener(WI.DOMManager.Event.NodeRemoved,
this._refreshGridNodeList, this);
> + WI.domManager.addEventListener(WI.DOMManager.Event.DocumentUpdated,
this._refreshGridNodeList, this);
This is a really inefficient system. Not only are you listening to _every_
node addition/removal, but you're also iterating over the _entire_ list of
instrumented nodes each time this happens. This should be optimized.
I think you should modify `WI.DOMNode` to call `this.layoutContextType = null;`
whenever it's removed from the DOM tree (meaning that it'll no longer be
rendered and therefore can be assumed to not have a layout context), which for
most nodes would be a no-op since their `layoutContextType` was already `null`
(i.e. not a grid context), meaning that all you'd need is the single
`WI.DOMNode.Event.LayoutContextTypeChanged` listener added to `WI.DOMNode` to
handle all modifications (but more importantly the nature of
`WI.DOMNode.prototype.set layoutContextType` would effectively filter out most
of the no-op operations). Also, this would make it so that you'd only have to
`add`/`delete` from `_gridNodes` inside `_handleLayoutContextTypeChanged` since
you're specifically listening for changes from `WI.DOMNode.prototype.set
layoutContextType`. Note that you may need to make some minor modifications to
`WI.DOMNode` to make this happen (e.g. instead of `console.assert` inside
`WI.DOMNode.prototype.set layoutContextType` you'd want to actually `return`
and the `_layoutContextType = ...` in the constructor should also invoke the
setter so that `WI.DOMNode.Event.LayoutContextTypeChanged` is fired). Also you
would likely need to regenerate the `Set` inside `attached()` since this isn't
listening to `WI.DOMNode.Event.LayoutContextTypeChanged` when not shown, but
that's _far_ less often than what you have now so it's more OK :)
>>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:94
>>> + WI.domManager.removeEventListener(WI.DOMManager.Event.NodeRemoved,
this._refreshGridNodeList, this);
>>
>> Do these really need to refresh the entire list, or could they work
similarly to `_handleLayoutContextTypeChanged` where you check the layout
context type and add/remove only if necessary?
>
> We should support the case where the insertion of a sibling or an ancestor
causes grid context to become available on a node that's already present in the
DOM.
>
> Perhaps a rare case, but not impossible: adding an h2 causes an already
present div to become a grid.
>
> ```
> h2 + div {
> display: grid;
> }
> ```
>
> Or maybe `_handleLayoutContextTypeChanged()` supports this class of use cases
already?
+1 to @Patrick Angle's comment (also see my comment above)
BTW if a sibling is added that causes the node to become a grid context, I
would think that that would be handled by `CSS.layoutContextTypeChanged`. All
you're really handling here is the case where a brand-new node is added or when
the node (or ancestor) is fully removed from the DOM tree.
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:117
> + this.addSubview(this._cssGridSection);
> + cssGridRow.element.appendChild(this._cssGridSection.element);
you'll actually want to flip the order of these, as
`WI.View.prototype.addSubview` has a special case for if the `WI.View` being
added is already attached (see `WI.View.prototype.insertSubviewBefore`)
> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.js:159
> + this._cssGridSection.gridNodeList = this._gridNodeList;
Why can't the `WI.CSSGridSection` handle the logic of maintaining `_gridNodes`?
Are you expecting to have a `WI.CSSGridSection` that only shows a subset of
grid nodes? To be fair, if we expect to have special instrumentation for other
layout contexts (e.g. `display: flex;`) it's possibly more efficient to have
the listener here and have it provide a `Set` to the relevant
`WI.CSSGridSection`/`WI.CSSFlexSection`/etc. based on the old and new context
types, but I kinda doubt it. :/
More information about the webkit-reviews
mailing list