[webkit-reviews] review granted: [Bug 222319] Web Inspector: Persist CSS Grid overlay colors : [Attachment 422220] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 222319: Web Inspector: Persist CSS Grid overlay colors
https://bugs.webkit.org/show_bug.cgi?id=222319

Attachment 422220: Patch

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




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

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

r=me, nice work :)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:154
> +	   colorSetting.value = color.hsl;

I think at the very least we should `console.assert(colorSetting, "There should
already be a setting created when the grid color is first fetched");` or
something like that (feel free to adjust the message as you see fit).

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:125
> +		   if (swatch.value !== gridColor)

I think you'll also want to `gridColor = swatch.value;` so that if the swatch
is reactivated later it wouldn't trigger a re-save if no changes are made.

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:127
> +	       }, swatch);

>> I think you should use `swatch` instead of `this` (and then change `swatch`
inside the arrow function to `this` or `event.target`) so that the event
listener is kept alive so long as `swatch` is kept alive (which is either the
same or shorter than `this`).
> 
> I'm confused about this. It's the reverse of your previous comment:
>
>> By using `swatch` here, you're implicitly capturing `swatch` as a strong
reference, meaning that this event listener could keep `swatch` alive.	Since
you're already providing `swatch` as the `thisObject` (3rd argument to
`WI.Object.prototype.addEventListener`), you should change these `swatch` to
`this` to avoid the implicit capture.
>
> Now that `swatch` is no longer stored in a `Map` it would be orphaned once
the DOM of the node list is replaced. Wouldn't this be enough to garbage
collect it?

I see now that what I wrote in my first comment could be interpreted in a few
ways, so let me try to be clearer now.	The event listener system provided by
`WI.Object` is awesome for a few reasons:
 1) it allows us to avoid having to `Function.prototype.bind` functions (or use
an arrow function) by passing a 3rd argument `thisObject` to `addEventListener`
which is used with `Function.prototype.call` when the listener is to be invoked
 2) by using `WeakRef` to store the `thisObject` we can avoid situations where
this custom event listener system would normally keep objects alive (but this
only works so long as we're _very_ careful to make sure that we don't
accidentally capture something we don't want to keep alive in the listener)
 3) we can listen for things on the prototype/constructor too, which works just
like the bubbling phase of events in the DOM, but instead walks the prototype
chain of the dispatching `WI.Object`
My point in the past reviews was specifically about #2, in that if you use
`swatch` both as the `thisObject` _and_ inside the listener function then the
benefits provided by `WeakRef` are possibly nullified because you're capturing
a strong reference to the same value you're trying to have a `WeakRef` for
(which means that the `WeakRef` is useless since there's also a strong
reference somewhere else).  With the majority of uses of
`WI.Object.prototype.addEventListener`, there's no need to be concerned about
this because the listener is a member function of a `class` (and therefore
doesn't have any local scope captured values), but if the listener is an inline
function (like what you're doing) then there is a possibility for error.  To be
clear, I'm not saying that we need to make this into a member function; there
are plenty of situations where a local inline function is preferred (like this
one).  All I am asking is that we make sure we don't accidentally create strong
references to objects where there don't need to be.
```
    swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, (event) => {
	if (event.target.value === gridColor)
	    return;

	gridColor = event.target.value;
	WI.overlayManager.setGridColorForNode(domNode, gridColor);
    }, swatch);
```
(you could also replace `event.target` with `this`, but that's perhaps more
confusing to read than using `event.target`)

It's very possible (I'm pretty sure that it does actually) that the garbage
collector in JSC is able to handle isolated "islands"/cycles of object keeping
each other alive, but I'd rather just be safe/correct and do the right thing
without even worrying.