[webkit-reviews] review granted: [Bug 218964] Web Inspector: Show current properties for font in new Elements sidebar Font panel : [Attachment 415797] Patch v2.5 - Added tests for WI.Font._calculateProperties

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 218964: Web Inspector: Show current properties for font in new Elements
sidebar Font panel
https://bugs.webkit.org/show_bug.cgi?id=218964

Attachment 415797: Patch v2.5 - Added tests for WI.Font._calculateProperties

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




--- Comment #16 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 415797
  --> https://bugs.webkit.org/attachment.cgi?id=415797
Patch v2.5 - Added tests for WI.Font._calculateProperties

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

r=mews with a few more fixes/comments.	Also, thanks for adding the model
object test!  Really great work :)

> Source/WebInspectorUI/ChangeLog:91
> +	   * Localizations/en.lproj/localizedStrings.js:

i think this a repeat of the above

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:577
> +	   resultVariationAxes->addItem(axis);

`WTFMove(axis)`

> Source/WebInspectorUI/UserInterface/Models/Font.js:140
> +		   let [tag, value] = axis.match(/[^\s"']+|["']([^"']*)["']/g);

This regex is repeated multiple times.	Should we save it to a static like
`WI.Font.SettingPattern = /[^\s"']+|["']([^"']*)["']/g`?

> Source/WebInspectorUI/UserInterface/Models/Font.js:190
> +	       // Only truly null for undefined values should be ignored. `0`
is a valid value.

Did you mean s/for/or?

> Source/WebInspectorUI/UserInterface/Models/Font.js:191
> +	       if (featureSettingValue != null) {

If you want to include `0` I'd suggest one of these instead of using abstract
equality:
 - if valid `featureSettingValue` are always a number, you could use
`!isNaN(featureSettingValue)`
 - if valid `featureSettingValue` can be anything other than
`null`/`undefined`, then I'd `featureSettingValue || featureSettingValue === 0`
We should avoid abstract equality wherever possible.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:42
> +    get supportsToggleClass()

NIT: I realize I suggested the name (so, my apologies), but I'd probably put
"CSS" in there somewhere (e.g. `supportsToggleCSSClass`) so it's clear it's not
talking about JavaScript `class` ��

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:171
> +	   let value = variation.value ?? variation.defaultValue;

Using `??` over `""` means that if `variation.value` is an empty string (`""`)
it will be used instead of the `variation.defaultValue`.  Is that what you
want?  (btw along those lines using `??` prevents `variation.defaultValue` from
being used if `variation.value` is `0`, so if the answer to my question is "no"
then maybe this needs some special logic instead of a logical operator)

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:208
> +	       {tag: WI.unlocalizedString("dlig"), name:
WI.unlocalizedString("discretionary-ligatures"), result:
WI.UIString("Discretionary", "Discretionary @ Font Details Sidebar Property
Value", "Property value for `font-variant-ligatures:
discretionary-ligatures`.")},

Style: These lines are getting a bit long, so maybe we should split the
properties onto multiple lines for readability?
```
    let otherResults = this._formatSimpleFeatureValues(property, [
	{
	    tag: WI.unlocalizedString("dlig"),
	    name: WI.unlocalizedString("discretionary-ligatures"),
	    result: WI.UIString("Discretionary", "Discretionary @ Font Details
Sidebar Property Value", "Property value for `font-variant-ligatures:
discretionary-ligatures`."),
	},
	{
	    tag: WI.unlocalizedString("hlig"),
	    name: WI.unlocalizedString("historical-ligatures"),
	    result: WI.UIString("Historical", "Historical @ Font Details
Sidebar Property Value", "Property value for `font-variant-ligatures:
historical-ligatures`."),
	},
	{
	    tag: WI.unlocalizedString("calt"),
	    name: WI.unlocalizedString("contextual"),
	    result: WI.UIString("Contextual Alternates", "Contextual Alternates
@ Font Details Sidebar Property Value", "Property value for
`font-variant-ligatures: contextual`."),
	},
    ]);
```

ditto below

> Source/WebInspectorUI/UserInterface/Views/FontDetailsPanel.js:277
> +	   if (property.features?.has(featureTag))
> +	       return !!property.features.get(featureTag);

NIT: Out of curiosity, is the value of `property.features.get(featureTag)` ever
possible to be `undefined`?  If not, you could just use that as a replacement
for `property.features?.has(featureTag)` to avoid the double-lookup.  That may
be more prone to accidental breakage tho (although we try to avoid using
`undefined` wherever possible), so what you have is probably fine.

> Source/WebInspectorUI/UserInterface/Views/FontDetailsSidebarPanel.js:30
> +	   const allowFiltering = false;

This doesn't exist anymore in `WI.GeneralStyleDetailsSidebarPanel`.

> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:-109
> -    filterDidChange(filterBar)

Why is this being removed?