[webkit-reviews] review granted: [Bug 227811] Web Inspector: Regression (r278607) Jump to CSS variable declaration from Computed panel not working : [Attachment 433499] Patch 1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 227811: Web Inspector: Regression (r278607) Jump to CSS variable
declaration from Computed panel not working
https://bugs.webkit.org/show_bug.cgi?id=227811

Attachment 433499: Patch 1.0

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




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

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

r=me

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:314
> +	   if (!property.overridden &&
this._hiddenUnusedVariables.find(propertiesMatch)) {

Is it actually desirable to have `_hiddenUnusedVariables` be a `Set` anymore? 
Is it only for deduplication?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:316
>	       this.propertyVisibilityMode =
WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.ShowAll;

It's a little unfortunate that we show _all_ variables when trying to find one,
but I suppose that's not the end of the world :/

>>>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:321
>>> +	     {
>> 
>> Did you change this from a `let ...` to a `function ...` to enable its use
before the declaration here? If so, instead of changing these signature, I'd
move the let propertiesMatch to be above its first use on :314.
> 
> Yes, I changed it to an explicitly named function declaration so it benefits
from hoisting and the code block above can call it without erroring.
> 
> Two reasons:
> 1) seemed like the least invasive change to make and preserve a clean blame
history (moving it around muddies that);
> 2) I don't see the need for these _not_ to be explicitly named function
declaration. The indirection is unnecessary in my opinion. 
> 
> Open to discussion if there is a reason for the keeping the functions as
expression assigned to variables/consts.

Our typical style is to only save a function to a `let` if it's using an arrow
function, as there's no way of creating a named arrow function.

What you have here is fine, but we do usually prefer to declare things before
they're used.  In this case it's also not possible to completely do that as
both functions refer to each other, so one of them will have to use and be
declared before the other ��

Style: we put the `{` on the same line for inline functions