[webkit-reviews] review granted: [Bug 222161] Web Inspector: CSS Grid Inspector: use a color palette for default grid overlay colors : [Attachment 420932] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 222161: Web Inspector: CSS Grid Inspector: use a color palette for default
grid overlay colors
https://bugs.webkit.org/show_bug.cgi?id=222161

Attachment 420932: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 420932
  --> https://bugs.webkit.org/attachment.cgi?id=420932
Patch

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

r=me with some comments

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:35
> +	   this._colorForNodeMap = new Map;

I'd use a `WeakMap` so that we don't accidentally keep things alive

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:122
> +	       [40, 97, 57]

Style: missing trailing comma

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:124
> +	   let colorIndex = this._colorForNodeMap.size % hslColors.length;

This means that if a developer enables the grid overlay for a node and then
that node (or any other node) is removed, the next node they show an overlay
for will have the same default color.  Do we want that?  I feel like we should
keep a separate count instead (e.g. `_nextDOMNodeColorIndex` that we
`this._nextDOMNodeColorIndex = (this._nextDOMNodeColorIndex + 1) %
hslColors.length;` and then reset to `0` inside
`_handleMainResourceDidChange`).

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:125
> +	   let hslComponents = hslColors[colorIndex];

NIT: I'd just inline this

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:142
> +    _mainResourceDidChange(event)

NIT: I recommend prefixing event listener callbacks with `_handle*` so they're
immediately identifiable