Bug 218515 – Web Inspector: Underline active font-face(s) in Elements sidebar Styles panel

• Component: Web Inspector
• Status: NEW

[Patrick Angle]

Currently it is not possible to determine which font from a `font-family` declaration is actually being used. The active face(s) should be underlined.

<rdar://69795515>

[Patrick Angle]

Created attachment 413062 [details]
WIP

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Patrick Angle]

Created attachment 413075 [details]
Active Family Underlined

[Patrick Angle]

Created attachment 413076 [details]
Implicit Active Family Underlined

[Patrick Angle]

Created attachment 413077 [details]
Multiple Active Families Underlined

[Devin Rousso]

Comment on attachment 413062 [details]
WIP

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

I just now realized that you marked this as a WIP, so my apologies if some of my comments are about things you were planning on changing 😅

> Source/JavaScriptCore/inspector/protocol/Font.json:5
> +    "debuggableTypes": ["itml", "page", "web-page"],
> +    "targetTypes": ["itml", "page"],

this domain does not exist for ITML

> Source/JavaScriptCore/inspector/protocol/Font.json:15
> +                { "name": "type", "$ref": "Type", "description": "-" },
> +                { "name": "availableFeatures", "type": "array", "items": { "$ref": "Feature" }, "description": "-" },
> +                { "name": "availableVariations", "type": "array", "items": { "$ref": "Variation" }, "description": "-" }

Are these going to be added to this patch or are they planned for later patches?  If the latter, I think it'd be better for you to add them at that time, as it makes each individual commit more focused and cleaner :)

> Source/JavaScriptCore/inspector/protocol/Font.json:64
> +        {
> +            "name": "enable",
> +            "description": "Enables the Font agent for the given page. Clients should not assume that the Font agent has been enabled until the result of this command is received."
> +        },
> +        {
> +            "name": "disable",
> +            "description": "Disables the Font agent for the given page."
> +        },

The primary purpose of `enable`/`disable` is to control whether events are able to be dispatched from the backend.  Since there are no events for this domain, there's no reason to have `enable`/`disable`.

> Source/WebCore/inspector/InstrumentingAgents.h:129
> +    DEFINE_ENABLED_INSPECTOR_AGENT(macro, Font) \

Is this (i.e. `enabledInspectorFontAgent`) actually used anywhere?  If not, why do we need it?

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:81
> +Inspector::Protocol::ErrorStringOr<Ref<JSON::ArrayOf<Protocol::Font::ComputedFontRange>>> InspectorFontAgent::getComputedFontRangesForNode(int nodeId)

NIT: `Inspector::Protocol::DOM::NodeId`

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:104
> +Ref<JSON::ArrayOf<Protocol::Font::ComputedFontRange>> InspectorFontAgent::inspectorComputedFontRangesForNode(Node* node)

this can be a `Node&` as it's only caller does a null-check before

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:111
> +    for (unsigned cascadeIndex = 0; cascadeIndex < fontCascade.fontDescription().effectiveFamilyCount(); cascadeIndex++) {

`++cascadeIndex`

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:114
> +        for (unsigned rangeIndex = 0; rangeIndex < fontRanges.size(); rangeIndex++) {

`++rangeIndex`

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:158
> +Vector<const Font*> InspectorFontAgent::fontsUsedToLayoutTextInNode(Node* node)

this can be a `Node&` as it's only caller does a null-check before

> Source/WebCore/inspector/agents/InspectorFontAgent.h:55
> +    Inspector::Protocol::ErrorStringOr<Ref<JSON::ArrayOf<Inspector::Protocol::Font::ComputedFontRange>>> getComputedFontRangesForNode(int);

NIT: `Inspector::Protocol::DOM::NodeId`

> Source/WebCore/platform/graphics/Font.h:330
> +    CSSFontFace* m_cssFontFace;

Can we make `CSSFontFace` inherit from `CanMakeWeakPtr` so we can avoid using a raw pointer here?  This patch doesn't appear to ever `setCSSFontFace(nullptr)` meaning that this is potentially a UAF.

Alternatively, if every creator then immediately calls `setCSSFontFace`, could we just add a `CSSFontFace&` parameter to the constructor?

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:143
> +    return WTF::visit(WTF::makeVisitor([&](const AtomString& family) -> const AtomString {

you can use `switchOn` to make this easier
```
    WTF::switchOn(family,
        [&](const AtomString& family) {
            return family;
        },
        [&](const FontFamilyPlatformSpecification& fontFamilySpecification) {
            return fontFamilySpecification.fontFamily();
        }
    );

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:40
> +    get domains() { return ["Font"]; }

this is only needed for the "extra domains" concept, which doesn't apply to the `Font` domain since it's brand new (also, extra domains were effectively removed going forward in r266072)

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:46
> +        if (!this._enabled)

When does `_enabled` get set?

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:58
> +    getComputedFontRangesForNode(node)
> +    {
> +        return this._mainTargetFontAgent.getComputedFontRangesForNode(node.id).then((result) => { return result.fontRanges; });
> +    }

Style: why not use an inline arrow function? `(result) => result.fontRanges`

NIT: I'd personally even just make this entire function `async`
```
    async getComputedFontRangesForNode(node)
    {
        console.assert(node instanceof WI.DOMNode, node);

        let target = WI.assumingMainTarget();
        let {fontRanges} = await target.getComputedFontRangesForNode(node.id):
        return fontRanges;
```

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:60
> +    static filterComputedFontRangesMatchingFontFamily(family, ranges)

`static` functions should go in a `// Static` section before `// Public`

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:62
> +        // If the family in inherit, then all ranges are from the parent element. Inherit becomes the effective value,

typo

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:79
> +        return [...new Set(ranges.map(range => range.font.name))];

Style: we prefer `Array.from(foo)` over `[...foo]`
Style: we always wrap parameters of arrow functions in parenthesis `(range) => range.font.name`

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:89
> +        else

Style: no need for `else` when related `if` has a `return`

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:97
> +        return WI.assumingMainTarget().FontAgent;

Please don't do this.  `WI.assumingMainTarget()` is a signpost/signal of sorts for when process-per-origin is completed/enabled as to what places in the Web Inspector frontend need to be updated to support multiple targets.  We should put `WI.assumingMainTarget` every place where the target is assumed to be the main target (hence the name).

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:100
> +    _reset()

Why have this if it does nothing?

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:106
> +        console.assert(event.target instanceof WI.Frame);

this isn't necessary since you've added the event listener to `WI.Frame` (i.e. it's guaranteed to be a `WI.Frame` by convention)

> Source/WebInspectorUI/UserInterface/Views/DetailsSection.js:82
> +        this._titleElement.hidden = title === null;

We rarely directly compare to `null`.  We prefer to just use `!` to check for falsy.

More importantly tho, why is this necessary/desired?  I don't think we ever want a situation where there's a `WI.DetailsSection` with no title (is there even anything to click on to collapse it then?).

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:250
> +.spreadsheet-style-declaration-editor > .property:not(.disabled, .invalid-name, .invalid-value, .other-vendor, .overridden) .content .value:not(.editing) .token-font-family-active {
> +    text-decoration: underline;
> +}

IMO this is perhaps a bit too subtle of a distinction, especially when you consider that underlining already has other assumed meanings (e.g. hovering a link underlines it).

What about making non-active fonts more transparent (and maybe even making the active font(s) bold)?  I think that if a developer puts multiple fonts into their `font-family`, it's likely for compatibility purposes for various OSes/devices/browsers/etc., meaning that it's expected for some of them to not match.  As such, making those that don't match slightly less visible in favor of emphasizing those that do match seems like a benefit for the developer.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:254
> +.spreadsheet-style-declaration-editor > .property:not(.disabled, .invalid-name, .invalid-value, .other-vendor, .overridden) .content .value:not(.editing) .token-font-family-implicit {
> +    color: var(--error-text-color);
> +}

Why are we using an error color?

Also, what use is it to the user/developer to know that this font is implicit?

EDIT: Oh ok so an "implicit" font is the sorta "ultimate fallback" if there are ranges used by text in the selected node that don't match any value in `font-family`?  Is it always the same `-webkit-standard`?  I'm skeptical that this something we want to show inline in the `font-family` property, as it's (likely) not written by the developer in their CSS source (assuming that's even possible).  I would imagine this as something shown in a swatch popover or Fonts panel that says "the following ranges were used but did not exist in any of the above/other fonts".

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:568
> +        if (this._property.name === "font-family")

What about the `font` shorthand?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:789
> +        // FIXME/TODO: [PCA] This is the reason there is a chain of async methods in rendering. The font ranges should instead be part of the node's style representation, and loaded at the same time as other properties.

Yeah I feel like we should merge `Font` with the `CSS` domain (since it's really entirely controlled by CSS anyways) and have it either be part of `CSS.getComputedStyleForNode` or as a new command `CSS.getComputedFontForNode` which we'd call in `WI.DOMNodeStyles.prototype.refresh`.  Is there a reason why we don't want to do this, or is this your goal/plan and (like you mention) this approach was just for prototyping?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:801
> +                tokenElement.setAttribute("title", titleFormat.format(WI.FontManager.uniqueFontNamesInRanges(fontRangesForToken).join(", ")));

`.title = `?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:807
> +        }.bind(this);

you can use an arrow function to avoid having to explicitly `.bind(this)`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:811
> +            if (currentToken)
> +                currentToken = currentToken + tokens[i].value;

Why is this necessary?  Is CodeMirror somehow splitting `font-family` tokens?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:821
> +            if (i === tokens.length - 1 || tokens[i + 1].value === "," || (currentToken.type && (currentToken.type.includes("string") || currentToken.type.includes("link") || currentToken.type.includes("comment")))) {

NIT: I think you can simplify this a bit
```
    if (i === tokens.length - 1 || tokens[i + 1].value === "," || /\b(?:string|link|comment)\b/.test(currentToken.type || ""))
```
we normally use a regex when dealing with CodeMirror token types :)

> LayoutTests/inspector/font/computed-font-for-node.html:18
> +            test(resolve, reject) {

Can we make this `async`?  `WI.DOMNode.prototype.querySelector` should support it.
```
    async test() {
        let {nodeId} = await documentNode.querySelector(selector);
        let domNode = WI.domManager.nodeforId(nodeId);
        InspectorTest.assert(domNode, `Should find DOM node for selector '${selector}'.");

        let computedFontRanges = await WI.fontManager.getComputedFontRangesForNode(domNode);
        computedFontRangeHandler(computedFontRanges);
    },
```

> LayoutTests/inspector/font/computed-font-for-node.html:28
> +        })

missing `;`

[Patrick Angle]

Created attachment 413104 [details]
WIP

[Patrick Angle]

Comment on attachment 413062 [details]
WIP

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

Thanks for the notes! I know I just uploaded a new patch, which we can ignore for now. It's mostly about the FIXME that was in this patch and trying to get GTK/Win to build. I'll get started on these new notes now.

>> Source/WebCore/platform/graphics/Font.h:330
>> +    CSSFontFace* m_cssFontFace;
> 
> Can we make `CSSFontFace` inherit from `CanMakeWeakPtr` so we can avoid using a raw pointer here?  This patch doesn't appear to ever `setCSSFontFace(nullptr)` meaning that this is potentially a UAF.
> 
> Alternatively, if every creator then immediately calls `setCSSFontFace`, could we just add a `CSSFontFace&` parameter to the constructor?

Only some `Font`s are created from a `CSSFontFace`. I'll look at making CSSFontFace inherit from `CanMakeWeakPtr` though to avoid the raw pointer.

>> Source/WebInspectorUI/UserInterface/Views/DetailsSection.js:82
>> +        this._titleElement.hidden = title === null;
> 
> We rarely directly compare to `null`.  We prefer to just use `!` to check for falsy.
> 
> More importantly tho, why is this necessary/desired?  I don't think we ever want a situation where there's a `WI.DetailsSection` with no title (is there even anything to click on to collapse it then?).

I couldn't even tell you why I made this change... It certainly is unrelated to this patch.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:250
>> +}
> 
> IMO this is perhaps a bit too subtle of a distinction, especially when you consider that underlining already has other assumed meanings (e.g. hovering a link underlines it).
> 
> What about making non-active fonts more transparent (and maybe even making the active font(s) bold)?  I think that if a developer puts multiple fonts into their `font-family`, it's likely for compatibility purposes for various OSes/devices/browsers/etc., meaning that it's expected for some of them to not match.  As such, making those that don't match slightly less visible in favor of emphasizing those that do match seems like a benefit for the developer.

I like the idea of bold instead of underlined as well. I actually ran into an issue where I tried to take a screenshot, which involved holding the Cmd key, at which point the entire property became underlined because it became a link, so avoiding looking like a link is probably a good thing.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:254
>> +}
> 
> Why are we using an error color?
> 
> Also, what use is it to the user/developer to know that this font is implicit?
> 
> EDIT: Oh ok so an "implicit" font is the sorta "ultimate fallback" if there are ranges used by text in the selected node that don't match any value in `font-family`?  Is it always the same `-webkit-standard`?  I'm skeptical that this something we want to show inline in the `font-family` property, as it's (likely) not written by the developer in their CSS source (assuming that's even possible).  I would imagine this as something shown in a swatch popover or Fonts panel that says "the following ranges were used but did not exist in any of the above/other fonts".

This is indeed the ultimate fallback. The consideration here is that it should be considered an error by the developer if none of the fonts in their cascade were usable for some text, and that displaying the font in red would communicate that (the tooltip for the family name also indicates this). Unless something goes horribly wrong or I misunderstand the fallback mechanism internally, it should always be `-webkit-standard`. I'm going to try and get more opinions on this tomorrow when I do a small demo internally. I agree this is also information we can and should show in the Font panel once it exists. I think some kind of communication in the styles panel is helpful, however. Perhaps a warning flag similar to that used for property names that aren't valid with an explainer tooltip?

If the families defined were enough to lay out the text, we don't show this implicit family.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:789
>> +        // FIXME/TODO: [PCA] This is the reason there is a chain of async methods in rendering. The font ranges should instead be part of the node's style representation, and loaded at the same time as other properties.
> 
> Yeah I feel like we should merge `Font` with the `CSS` domain (since it's really entirely controlled by CSS anyways) and have it either be part of `CSS.getComputedStyleForNode` or as a new command `CSS.getComputedFontForNode` which we'd call in `WI.DOMNodeStyles.prototype.refresh`.  Is there a reason why we don't want to do this, or is this your goal/plan and (like you mention) this approach was just for prototyping?

I think you are probably correct. At the outset of this patch I was thinking of CSS and Font as two separate things, but as I've worked on it it has become clearer that this is better done as an addition to the CSS domain instead.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:811
>> +                currentToken = currentToken + tokens[i].value;
> 
> Why is this necessary?  Is CodeMirror somehow splitting `font-family` tokens?

Some tokens will be split, unfortunately. For example in `font-family: 'MyFont', Helvetica Neue, sans-serif;` the value tokens we get would be [`'MyFont'`, `,`, ` `, `Helvetica`, `Neue`, `,`, ` `, `sans-serif`]. While I wouldn't necessarily call it good practice to not put `Helvetica Neue` inside quotes, it is valid CSS that we need to handle.

[Devin Rousso]

Comment on attachment 413062 [details]
WIP

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:254
>>> +}
>> 
>> Why are we using an error color?
>> 
>> Also, what use is it to the user/developer to know that this font is implicit?
>> 
>> EDIT: Oh ok so an "implicit" font is the sorta "ultimate fallback" if there are ranges used by text in the selected node that don't match any value in `font-family`?  Is it always the same `-webkit-standard`?  I'm skeptical that this something we want to show inline in the `font-family` property, as it's (likely) not written by the developer in their CSS source (assuming that's even possible).  I would imagine this as something shown in a swatch popover or Fonts panel that says "the following ranges were used but did not exist in any of the above/other fonts".
> 
> This is indeed the ultimate fallback. The consideration here is that it should be considered an error by the developer if none of the fonts in their cascade were usable for some text, and that displaying the font in red would communicate that (the tooltip for the family name also indicates this). Unless something goes horribly wrong or I misunderstand the fallback mechanism internally, it should always be `-webkit-standard`. I'm going to try and get more opinions on this tomorrow when I do a small demo internally. I agree this is also information we can and should show in the Font panel once it exists. I think some kind of communication in the styles panel is helpful, however. Perhaps a warning flag similar to that used for property names that aren't valid with an explainer tooltip?
> 
> If the families defined were enough to lay out the text, we don't show this implicit family.

I dunno if I'd agree with "it should be considered an error by the developer".  A warning perhaps, maybe with UI similar to what Web Inspector has for repeated properties where there's a yellow area with a warning icon on the right.  I believe there are parts of unicode that are reserved for private use (not to mention other languages, emoji, etc.), so expecting fonts to have characters for all of those (especially when you consider that any given website might display content input by other users) is not realistic.

Most importantly tho, I do not think Web Inspector should be "mis-representing" the actual CSS content.  As an example, if the developer clicks on the value of a `font-family` property, would the `-webkit-standard` disappear?

[Joseph Pecoraro]

Comment on attachment 413104 [details]
WIP

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

Very Neat! Nice tests too.

> Source/JavaScriptCore/inspector/protocol/Font.json:5
> +    "debuggableTypes": ["itml", "page", "web-page"],
> +    "targetTypes": ["itml", "page"],

Should

> Source/WebCore/inspector/InstrumentingAgents.h:54
> +class InspectorFontAgent;

Style: Sorted

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:147
> +                .setFrom(makeString(hex(range.from())))
> +                .setTo(makeString(hex(range.to())))

Any particular reason we send these numbers as hex?

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:154
> +            if (!range.from() && range.to() == 0x7fffffff)

This seems like a magical value, only otherwise known internally to FontRanges.cpp? It might be better to add a descriptive field or name to FontRanges::Range such as `isTerminalRange` or `spansFullRange`.

> Source/WebCore/inspector/agents/InspectorFontAgent.cpp:162
> +Vector<const Font*> InspectorFontAgent::fontsUsedToLayoutTextInNode(Node* node)

Could use a `HashSet` instead of a `Vector`, which avoids the awkward `fonts.appendIfNotContains`? Or is order significant?

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:32
> +    constructor()
> +    {
> +        super();
> +    }

Style: Normally we'd drop this, assuming nothing is going to be added to it.

> Source/WebInspectorUI/UserInterface/Controllers/FontManager.js:59
> +        // Names may be within single or double quotes in CSS

Style: Comments are sentences that should end in a period.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:320
> +            this._computedFontRanges = fontRangesPayload || [];

I think it is normally rare for us to use the raw payload object as a member. We would normally create a `WI.FontRange` model type with something like `WI.FontRange.fromPayload(...)`. The idea being if we change the backend year to year we can just update this Model object creation and the rest of the Web Inspector frontend code is fine.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:789
> +

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:808
> +        }.bind(this);

Style: Given we don't use `arguments` in this function an arrow function might be better than the bind.

> LayoutTests/inspector/font/computed-font-for-node.html:18
> +            test(resolve, reject) {

Style: I think the more modern way would be to make this `async test`...

> LayoutTests/inspector/font/computed-font-for-node.html:26
> +                    cssStyles.refreshIfNeeded()

And then just make these `await ...` statements instead of a chain.

> LayoutTests/inspector/font/computed-font-for-node.html:143
> +            InspectorTest.expectEqual(computedFontRanges[0].from, "A5", "First range should start at 'A5'.");
> +            InspectorTest.expectEqual(computedFontRanges[0].to, "A5", "First range should end at 'A5'.");

If this is a hex number I'd probably expect a string like `0xA5`.

[Patrick Angle]

Created attachment 413358 [details]
Patch

[Patrick Angle]

Created attachment 413360 [details]
Screenshots of attachment 413358 [details]

[Devin Rousso]

Comment on attachment 413358 [details]
Patch

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

this is looking awesome! 🤩

some more style/NIT and a couple design questions

> Source/JavaScriptCore/inspector/protocol/CSS.json:249
> +                { "name": "family", "type": "string", "description": "The family name used to request the font."},

Style: missing space `" }`
(a few other places below too)

> Source/JavaScriptCore/inspector/protocol/CSS.json:309
> +            "targetTypes": ["page", "web-page"],

this should just be `["page"]`

`"web-page"` targets only have `Target` and `Browser` domains (see `WebPageInspectorController`)

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1073
> +    auto fontCascade = node.computedStyle()->fontCascade();

Can this return `nullptr`?

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1120
> +    Vector<Node*> textNodes;

Is there any reason to actually have this vector?  You could have a lambda that captures `fonts` and call it wherever `textNodes.append` is called.

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1130
> +    auto fontCascade = node.computedStyle()->fontCascade();

ditto (:1073)

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1134
> +        TextRun textRun = TextRun(emptyString());

NIT: i'd either `auto textRun = TextRun(emptyString());` or `TextRun textRun(emptyString());` to avoid having to write `TextRun` twice :P

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1135
> +        if (auto renderer = downcast<RenderText>(textNode->renderer()))

This will crash if `textNode->renderer()` is not actually a `RenderText`.  The type casting system defined in WTF expects callers to `is<Type>` before calling `downcast<Type>`.
```
    if (is<RenderText>(textNode->renderer())
        textRun = TextRun(String(downcast<RenderText>(*textNode->renderer()).text()));
    else if (is<CharacterData>(textNode))
        textRun = TextRun(downcast< CharacterData >(*textNode).data());

    if (!textRun.length())
        continue;
```

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1137
> +        else if (auto characterData = downcast<CharacterData>(textNode))

Is this actually necessary?  If it is, can we add tests for it?

I wonder if instead of examining the immediate children of the given DOM node (or itself if it's `Text`) we should be looking at the immediate children of the `node.renderer()` instead?  This is similar to what you're already doing, but perhaps it could catch more edge cases.

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1140
> +        auto glyphBuffer = fontCascade.layoutTextForInspector(textRun);

How expensive is it to do this?  Is there any way to cache this data during normal layout if Web Inspector is open so that we don't have to do it here (assuming it's less expensive than doing it on-demand)?

> Source/WebCore/platform/graphics/Font.cpp:720
> +const CSSFontFace* Font::cssFontFace() const { return m_cssFontFace.get(); }
> +void Font::setCSSFontFace(CSSFontFace& cssFontFace) { m_cssFontFace = makeWeakPtr(cssFontFace); }

Style: I don't think we usually inline things in `.cpp` files

> Source/WebCore/platform/graphics/Font.h:229
> +    const CSSFontFace* cssFontFace() const;

I dunno what the "right" way is, but I wonder if you should just return `const WeakPtr<CSSFontFace>` instead of a raw pointer 🤔

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:142
> +    const auto& family = description.effectiveFamilyAt(index);

NIT: you could inline this and eliminate the `family` variable

> Source/WebCore/platform/graphics/FontFamilySpecificationNull.cpp:43
> +    return "";

`emptyAtom()`?

> Source/WebCore/platform/graphics/FontFamilySpecificationNull.h:40
> +    const AtomString fontFamily() const;

`const AtomString&`?

> Source/WebCore/platform/graphics/FontRanges.h:92
> +    const AtomString fontFamily() const { return m_fontFamily; }
> +    void setFontFamily(const AtomString fontFamily) { m_fontFamily = fontFamily; }

`const AtomString&`?

> Source/WebCore/platform/graphics/FontRanges.h:97
> +    AtomString m_fontFamily = "";

Is it actually necessary to initialize this?

if so, our usual style is to use `{ ... }` instead of `= ...`

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.h:44
> +    const AtomString fontFamily() const { return m_fontFamily; };

`const AtomString&`?

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:325
> +            else {

Style: no need for `{`  and `}` for single line control flow

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:339
> +        // COMPATIBILITY (iOS 14.?): `getComputedFontRangesForNode` did not exist yet.

the compatibility comments serve as an indicator of "the last iOS version that did NOT have this", so this should be 14.0

also, please use the fully qualified name `CSS.getComputedFontRangesForNode`

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:342
> +            pendingRefreshTaskPromises.push(fetchedComputedFontFamiliesPromise.promise);

NIT: rather than pull out `pendingRefreshTaskPromises` into a variable, you could just have an `else \n fetchedComputedFontFamiliesPromise.resolve();` and include `fetchedComputedFontFamiliesPromise.promise` in the `Promise.all` that was there before

> Source/WebInspectorUI/UserInterface/Models/Font.js:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

2020

> Source/WebInspectorUI/UserInterface/Models/Font.js:41
> +            case "web-font": fontType = WI.Font.Type.WebFont; break;
> +            case "system": fontType = WI.Font.Type.System; break;

Style: we don't indent `case`
Style: we don't inline `case` blocks

```
    switch (payload.type) {
    case InspectorBackend.Enum.CSS.ComputedFontType.WebFont:
        fontType = WI.Font.Type.WebFont;
        break;

    case InspectorBackend.Enum.CSS.ComputedFontType.System:
        fontType = WI.Font.Type.System;
        break;
    }
    console.assert(fontType);
```

> Source/WebInspectorUI/UserInterface/Models/Font.js:52
> +    // Private

no need for this since there are no private methods

> Source/WebInspectorUI/UserInterface/Models/Font.js:59
> +// Keep these in sync with the "ComputedFontType" enum defined by the "CSS" domain.
> +WI.Font.Type = {
> +    WebFont: "font-type-web-font",
> +    System: "font-type-system",
> +};

NIT: we normally keep the value identical to the backend too.
```
    WI.Font.Type = {
        WebFont: "web-font",
        System: "system",
    };
```

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:50
> +    get equivilentStandardFamily()

`get canonicalFontFamily`?

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:52
> +        // A handful of `-webkit-` prefixed font families are defined in WebCore\css\WebKitFontFamilyName.in. Most names

NIT: do you mean `WebCore/css/WebKitFontFamilyName.in`? :P

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:55
> +        if (this.family === "-webkit-standard")

NIT: we normally use the actual variable `_family` instead of `get family` when inside the same class
(ditto below and in other files)

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:57
> +        else

Style: no `else` if the related `if` has a `return`

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:61
> +    get fromHex()

`get fromAsHexValue`?  This makes me thing of our other `static fromJSON` methods :(

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:71
> +    // Private

no need for this since there are no private methods

> Source/WebInspectorUI/UserInterface/Models/FontRanges.js:54
> +    keepingFamily(family)

NIT: `rangesForFontFamily`?  The word "keeping" is a bit odd here IMO, in that I'm not really sure what this is doing (I had to come read the definition).

> Source/WebInspectorUI/UserInterface/Models/FontRanges.js:74
> +    removingRanges(ranges)

`excludingRanges`?

> Source/WebInspectorUI/UserInterface/Models/FontRanges.js:76
> +        if (ranges instanceof WI.FontRanges)

should we assert this?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:268
> +        if (this._property.name === "font" || this._property.nane === "font-family") {

oops `nane`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:272
> +            if (fontRanges.hasFamily("-webkit-standard") && !families.includes("-webkit-standard")) {

it'd probably be safer/easier to do a regex check for `-webkit-standard` instead of splitting based on "," (e.g. what if the font name has a comma in it)
```
    if (fontRanges.hasFamily("-webkit-standard") && !/\b-webkit-standard\b/.test(this._property.value)) {
```

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:274
> +                elementTitle = WI.UIString("No explicitly defined family was available for some characters. Using additional fonts: %s").format(fontRanges.keepingFamily("-webkit-standard").fontNames.join(", "));

Should we list the characters that don't match any other fonts?

Also, isn't `fontRanges.keepingFamily("-webkit-standard").fontNames.join(", ")` just a fancy way of saying `"-webkit-standard"`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:579
> +        else if (this._property.name === "font-family")

Style: no `else` if the related `if` has a `return`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:798
> +        // If this is a system font declaration (e.g. `menu` or `-apple-system-headline`), attribute all ranges to the declaration.
> +        if (this._matchesSystemFont(tokens)) {

Would this still show the font as active even if the selected element doesn't have any text in it?  Why is this necessary/desired?  Does the logic below not handle this?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:810
> +

NIT: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:833
> +        const systemFontValues = [

Should we hardcode this value somewhere else (or get it from the backend), like `WI.CSSKeywordCompletions.SystemFonts`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:866
> +        if(tokens[0]?.value === "inherit")

Style: missing space `if (`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:871
> +        if (fontRanges.isEmpty)

it seems like all other places just use `.ranges.length` to check

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:883
> +                tokenElement.title = titleFormat.format(fontRangesForToken.fontNames.join(", "));

Is it possible for `fontRangesForToken.fontNames` to be more than one value?  Each `familyToken` represents a single item in `font-family`, right?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:907
> +                newTokens.push(createTokenElement(currentToken, WI.UIString("Using fonts: %s", "Tooltip for CSS `font-family` value that is being used to draw characters in an element.")));

I think we should avoid using `WI.UIString` without all three values (format, key, comment) as it makes it easier for repeated strings to conflict

> LayoutTests/inspector/css/computed-font-for-node.html:10
> +    let suite = InspectorTest.createAsyncSuite("CSS.ComputedFonts");

NIT: I personally like to have the suite (and file) name match the name of the command/event so that it makes searching the codebase for it _really_ easy and consistent, so I'd rename this test `LayoutTests/inspector/css/getComputedFontRangesForNode.html` and rename this suite `CSS.getComputedFontRangesForNode` (adding to it like you did to each test case)

> LayoutTests/inspector/css/computed-font-for-node.html:93
> +        setup(resolve) {
> +            InspectorTest.evaluateInPage(loadFontFaceSourceText("normal 300 12px WebFont", " ", "TestPage-Webfont1Loaded"));
> +            InspectorTest.singleFireEventListener("TestPage-Webfont1Loaded", (event) => {
> +                resolve();
> +            });
> +        },

you could rework this a bit to avoid the `resolve` hassle
```
    async setup() {
        const eventName = "TestPage-Webfont1Loaded";
        return Promise.all([
            InspectorTest.awaitEvent(eventName),
            loadFontFaceSourceText("normal 300 12px WebFont", " ", eventName);
        ]);
    },
```
assuming you have `loadFontFaceSourceText` call `InspectorTest.evaluateInPage` instead :)

[Patrick Angle]

Comment on attachment 413358 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1140
>> +        auto glyphBuffer = fontCascade.layoutTextForInspector(textRun);
> 
> How expensive is it to do this?  Is there any way to cache this data during normal layout if Web Inspector is open so that we don't have to do it here (assuming it's less expensive than doing it on-demand)?

There isn't a clear path to caching this during a normal layout. A previous version of this work was caching used fonts in the FontCascade, but the FontCascade itself is not persistent (the font cascade at layout time was not the same FontCascade when the inspector went to check. Additionally, the FontCascade doesn't have insight into the element/node for which it exists.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:274
>> +                elementTitle = WI.UIString("No explicitly defined family was available for some characters. Using additional fonts: %s").format(fontRanges.keepingFamily("-webkit-standard").fontNames.join(", "));
> 
> Should we list the characters that don't match any other fonts?
> 
> Also, isn't `fontRanges.keepingFamily("-webkit-standard").fontNames.join(", ")` just a fancy way of saying `"-webkit-standard"`?

The font name will be (on macOS at least) `Times` under normal circumstances, but that could vary by platform and is not guaranteed.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:798
>> +        if (this._matchesSystemFont(tokens)) {
> 
> Would this still show the font as active even if the selected element doesn't have any text in it?  Why is this necessary/desired?  Does the logic below not handle this?

That's a good catch. We should make sure there are font ranges before we mark the system font as being used.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:883
>> +                tokenElement.title = titleFormat.format(fontRangesForToken.fontNames.join(", "));
> 
> Is it possible for `fontRangesForToken.fontNames` to be more than one value?  Each `familyToken` represents a single item in `font-family`, right?

A font family that matches a web font could in turn refer to two different fonts that cover different ranges of characters. One of the tests actually covers this case (`#webfont-cjk-test-2`) where the family is the same for both ranges, but two different fonts are used for that family.

>> LayoutTests/inspector/css/computed-font-for-node.html:10
>> +    let suite = InspectorTest.createAsyncSuite("CSS.ComputedFonts");
> 
> NIT: I personally like to have the suite (and file) name match the name of the command/event so that it makes searching the codebase for it _really_ easy and consistent, so I'd rename this test `LayoutTests/inspector/css/getComputedFontRangesForNode.html` and rename this suite `CSS.getComputedFontRangesForNode` (adding to it like you did to each test case)

For test naming, is there any rhyme or reason two the casing of names? Some files are in snake-case and some are in camel case. Happy to conform to either, but is there one we prefer. Camel case seems simpler because then everything (file name, suite name, and protocol command name) can match case.

>> LayoutTests/inspector/css/computed-font-for-node.html:93
>> +        },
> 
> you could rework this a bit to avoid the `resolve` hassle
> ```
>     async setup() {
>         const eventName = "TestPage-Webfont1Loaded";
>         return Promise.all([
>             InspectorTest.awaitEvent(eventName),
>             loadFontFaceSourceText("normal 300 12px WebFont", " ", eventName);
>         ]);
>     },
> ```
> assuming you have `loadFontFaceSourceText` call `InspectorTest.evaluateInPage` instead :)

Beyond just the resolve hassle, I like that this removes the double declaration of the event name.

[Joseph Pecoraro]

Comment on attachment 413358 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/FontRange.js:61
>> +    get fromHex()
> 
> `get fromAsHexValue`?  This makes me thing of our other `static fromJSON` methods :(

This is a getter, not a method. So usage would be `font.fromHex` which I think is fine. But potentially we could return ranges `font.range` => `[from, to]` and `font.rangeHex` => `[fromHex, toHex]`.

That said, converting to hex seems like a display time thing not so much a model time thing to me. Shrug.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:268
>> +        if (this._property.name === "font" || this._property.nane === "font-family") {
> 
> oops `nane`

Nice catch =)

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:833
>> +        const systemFontValues = [
> 
> Should we hardcode this value somewhere else (or get it from the backend), like `WI.CSSKeywordCompletions.SystemFonts`?

Getting this list from the backend is a great idea!

>>> LayoutTests/inspector/css/computed-font-for-node.html:10
>>> +    let suite = InspectorTest.createAsyncSuite("CSS.ComputedFonts");
>> 
>> NIT: I personally like to have the suite (and file) name match the name of the command/event so that it makes searching the codebase for it _really_ easy and consistent, so I'd rename this test `LayoutTests/inspector/css/getComputedFontRangesForNode.html` and rename this suite `CSS.getComputedFontRangesForNode` (adding to it like you did to each test case)
> 
> For test naming, is there any rhyme or reason two the casing of names? Some files are in snake-case and some are in camel case. Happy to conform to either, but is there one we prefer. Camel case seems simpler because then everything (file name, suite name, and protocol command name) can match case.

No rhyme or reason. I think I started the trend of `LayoutTests/inspector/domain/commandName.html` where possible so that it would be obvious that this test corresponds to a basic test of `Domain.CommandName`. For example the debugger tests. But this naming scheme starts to breaks down as soon as there is a test that isn't focusing on a single command...

The `InspectorTest` Suite Names we've tried to make pretty consistently useful to uniquely identify a test and provide good hierarchical categorization and subcategorization. It also allows for more flexible naming which can correspond to Inspector Domain / Command / Events or Frontend Unit Tests.

[Patrick Angle]

Created attachment 413488 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 413488 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1081
> +    for (unsigned cascadeIndex = 0; cascadeIndex < fontCascade.fontDescription().effectiveFamilyCount(); ++cascadeIndex) {
> +        auto& fontRanges = fontCascade.fallbackRangesAt(cascadeIndex);

I don't think this is what you want. I believe calling fallbackRangesAt() can trigger downloads. E.g.

font-family: WebFont1, WebFont2;

if WebFont1 supports all the characters in the page, we should never need to download WebFont2. But this code will cause WebFont2 to be downloaded.

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1126
> +        auto glyphBuffer = fontCascade.layoutTextForInspector(TextRun(String(renderer.text())));

How sure are you that the FontCascade has all the necessary information in it? Conceptually, the input to text layout is a RenderStyle, not just a FontCascade.

> Source/WebCore/platform/graphics/Font.h:331
> +    WeakPtr<CSSFontFace> m_cssFontFace;

I think this is fundamentally wrong. Multiple CSSFontFaces may reference the same Font object. For example:

@font-face {
    font-family: Font1;
    src: local("Helvetica");
}
@font-face {
    font-family: Font2;
    src: local("Helvetica");
}

Also, Fonts are per-process (they are owned by the FontCache singleton) but CSSFontFaces are per-document.

> Source/WebCore/platform/graphics/FontCascade.cpp:288
> +GlyphBuffer FontCascade::layoutTextForInspector(const TextRun& run) const

We usually don't use names like "ForInspector". We usually name functions for what they do, not who calls them.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:209
> +            fontRanges.setFontFamily(standardFamily);

I think this is the wrong level to be modifying the FontRanges. Instead of a setter, it should be an extra argument to the constructor.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:129
> -                return FontFamilySpecification(cascadeList[index].get());
> +                return FontFamilySpecificationCoreText(cascadeList[index].get(), cssFamily);

Why rename FontFamilySpecification to FontFamilySpecificationCoreText?

[Patrick Angle]

Comment on attachment 413488 [details]
Patch

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

Thank you for your insight!

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1081
>> +        auto& fontRanges = fontCascade.fallbackRangesAt(cascadeIndex);
> 
> I don't think this is what you want. I believe calling fallbackRangesAt() can trigger downloads. E.g.
> 
> font-family: WebFont1, WebFont2;
> 
> if WebFont1 supports all the characters in the page, we should never need to download WebFont2. But this code will cause WebFont2 to be downloaded.

Looking at this again, I don't see where a download would be triggered until the call below to `font(...)` on a range, where I'm explicitly passing `ExternalResourceDownloadPolicy::Forbid` to prevent a download from being triggered. I'll add a test case to confirm this and prevent us from making this mistake in the future if it is a problem.

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1126
>> +        auto glyphBuffer = fontCascade.layoutTextForInspector(TextRun(String(renderer.text())));
> 
> How sure are you that the FontCascade has all the necessary information in it? Conceptually, the input to text layout is a RenderStyle, not just a FontCascade.

My understanding of how this works is that FontCascade is responsible for actually drawing the text `FontCascade::drawText(...)` which takes the graphics context, the text run, a point at which to draw, a position for the start and stop of the run to be drawn, and an action if drawing can not be completed. Other properties like weight/variation are held by the FontCasecade's `FontCascadeDescription`. This description is passed when resolving which glyphs to use to ensure that an appropriate font is used based on weight and other variations, so I believe FontCascade to hold all the information needed to resolve which fonts were used to lay out the node. The FontCascade may not know about the actual position for the text, but that information seems inconsequential to the layout, unless I misunderstand how the FontCascade works (which is completely possible).

>> Source/WebCore/platform/graphics/Font.h:331
>> +    WeakPtr<CSSFontFace> m_cssFontFace;
> 
> I think this is fundamentally wrong. Multiple CSSFontFaces may reference the same Font object. For example:
> 
> @font-face {
>     font-family: Font1;
>     src: local("Helvetica");
> }
> @font-face {
>     font-family: Font2;
>     src: local("Helvetica");
> }
> 
> Also, Fonts are per-process (they are owned by the FontCache singleton) but CSSFontFaces are per-document.

Darn. Unsurprisingly, you are right. But in looking at this it should be possible to point to the font face from the range the range in created in `CSSSegmentedFontFace`, which caches the ranges, but each range should only exist from a single font-family.

>> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:209
>> +            fontRanges.setFontFamily(standardFamily);
> 
> I think this is the wrong level to be modifying the FontRanges. Instead of a setter, it should be an extra argument to the constructor.

Agreed. Based on your other notes, that should be relatively straightforward.

[Patrick Angle]

Created attachment 413771 [details]
WIP

[Patrick Angle]

Comment on attachment 413771 [details]
WIP

This is a major WIP based on Myles' review notes. It is mostly functional, but suffers from an issue where a `FontRanges::Range` can end up being destroyed and recreated at a different address when another `FontRanges::Range` is appended to a `FontRanges`. This complicates the storage of extra data we need as part of a GlyphData because GlyphData is not necessarily the authority of which font gets used for a glyph. During layout in `ComplexTextController` (actually `ComplexTextControllerCoreText.mm`) the entire character can be thrown out and recalculated as part of the fallback mechanism (even though those mechanisms should not be necessary since earlier font resolution should already have chosen a fallback).

[Patrick Angle]

Created attachment 413995 [details]
WIP

[Patrick Angle]

Comment on attachment 413995 [details]
WIP

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

> Source/WebCore/ChangeLog:1
> [phone]  Patrick Angle  <[e-mail]>

There remains quite a bit to clean up in WebCore, but this patch is once again functional, incorporating Myles' feedback. Feel free to comment on any larger issues you see.

[Patrick Angle]

Created attachment 414059 [details]
Patch

[Patrick Angle]

Created attachment 414063 [details]
Speculative fix for Win/GTK AtomString availability

[Patrick Angle]

Created attachment 414066 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414068 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414074 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414084 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414090 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414094 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414107 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414112 [details]
WIP - Win/GTK

[Patrick Angle]

Created attachment 414124 [details]
Patch

[Blaze Burg]

Comment on attachment 414124 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Backend support for determining the font family/families used for laying out a node. This patch provides some

Nit: not a sentence

> Source/WebCore/ChangeLog:12
> +        font inspection features we want to support.

I would leave out the second sentence.

> Source/WebCore/ChangeLog:14
> +        The primary changes in Webcore are:

Nit: WebCore

> Source/WebCore/ChangeLog:19
> +        - Text layout now propegates not only the Font for text rendering but also the GlyphRange.

Nit: propagates

> Source/WebCore/ChangeLog:30
> +        the pointer to no longer be viable).

This paragaph should go into the bug, not the changelog. Just describe what the change does :)

> Source/WebCore/ChangeLog:53
> +        - Families are determined by simulating a layout.

"Re-running the font cascade."

> Source/WebCore/ChangeLog:62
> +        - Support for ComplexTextRun holding a RefPtr<GlyphRange>

Reword: Teach ComplexTextRun how to figure out what glyphs it uses.

> Source/WebCore/ChangeLog:73
> +        - fontForCombiningCharacterSequence is now fontAndRangeForCombiningCharacterSequence and provides a GlyphRange

You should put comments for a function under the function name above.

> Source/WebInspectorUI/ChangeLog:8
> +        Adds support for underlining the active font face(s) used by a node in the Styles sidebar from either a `font` or `font-family` property. For nodes that used a font other than one defined in their `font`/`font-family` property we show a warning for the property indicating that the cascade was not able to provide a font for one or more characters.

Please wrap the text at about 100 characters so it fits with the rest.

> Source/JavaScriptCore/inspector/protocol/CSS.json:236
> +            "id": "ComputedFont",

What about calling it a UsedFont?

Used and Computed have meanings in the context of CSS. In this case, we are querying about what was actually used, rather than what the font family property is according to the CSS cascade. (Correct me if I'm wrong!)

> Source/JavaScriptCore/inspector/protocol/CSS.json:256
> +            "id": "ComputedFontType",

Please put types for enums above objects.

> Source/WebCore/css/CSSFontSelector.cpp:331
> +    return FontRanges(WTFMove(font), familyName);

Probably best to keep brace initializers and add familyName inside it.

[Devin Rousso]

Comment on attachment 414124 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1077
> +    auto fontCascade = computedStyle->fontCascade();

NIT: can we `auto&`?

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1080
> +    for (auto range : rangesUsedDuringLayout) {

ditto (:1077)

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1105
> +        for (unsigned i = 0; i < glyphBuffer.size(); i++)

`++i`

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1111
> +            layoutRenderer(*downcast<RenderText>(renderer));

NIT: I'd move the `*` inside the `downcast` to avoid another pointer check (`downcast<RenderText>(*renderer)`)

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1117
> +                    for (auto* innerChild = child->childAt(0); innerChild; innerChild = innerChild->nextSibling()) {

Why are we going two levels deep?  I would think that we'd only care about the direct text descendants of `node`?

> Source/WebCore/platform/graphics/ComplexTextController.cpp:382
> +    tie(nextFont, nextRanges) = m_font.fontAndRangesForCombiningCharacterSequence(cp, curr - cp);

`std::tie`

> Source/WebCore/platform/graphics/ComplexTextController.cpp:428
> +            tie(nextFont, nextRanges) = m_font.fontAndRangesForCombiningCharacterSequence(cp + index, curr - cp - index);

ditto (:382)

> Source/WebCore/platform/graphics/ComplexTextController.cpp:824
> +ComplexTextController::ComplexTextRun::ComplexTextRun(const Font& font, const UChar* characters, unsigned stringLocation, unsigned stringLength, unsigned indexBegin, unsigned indexEnd, bool ltr, GlyphRange* fontRange)

Can we use `RefPtr` instead of a raw pointer?  This would avoid all of the `.get()` too.  Also, it eventually ends up in a `RefPtr` anyways so there's no reason to not have it be a `RefPtr` here and then just `WTFMove`.

Ditto in other files as well (although some other places might want to use `WeakPtr` instead).  We should avoid using raw pointers.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:73
> +    RefPtr<GlyphRange> m_ranges[GlyphPage::size] { };

the `{ }` isn't necessary since `RefPtr` has a default constructor

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:144
> +    const auto& family = description.effectiveFamilyAt(index);

NIT: this name is shadowed by the first lambda, so I'd either inline this or rename one of the two `family` to avoid shadowing

> Source/WebCore/platform/graphics/GlyphPage.h:51
> +        static unsigned hash(const RefPtr<GlyphRange> key) { return key->hash(); }

Are you sure that `key` is never `nullptr`?

> Source/WebCore/platform/graphics/GlyphPage.h:52
> +        static unsigned hash(const RefPtr<GlyphRange> key) { return key->hash(); }
> +        static bool equal(const RefPtr<GlyphRange> a, const RefPtr<GlyphRange> b)

`const RefPtr<GlyphRange>&`

> Source/WebCore/platform/graphics/GlyphPage.h:57
> +            return a.get() == b.get()
> +                || !a.get()
> +                || !b.get()
> +                || *a.get() == *b.get();

This seems wrong.  Why would this return `true` if `!a.get()`?  Isn't that equivalent to "if one of them is `nullptr` then they're equal"?
```
    return a == b || (a && b && *a == *b);
```

> Source/WebCore/platform/graphics/GlyphPage.h:59
> +        static constexpr bool safeToCompareToEmptyOrDeleted = false;

Really?  Wouldn't the empty/deleted value be `nullptr`?  That should/could be handled by `equal`.

> Source/WebCore/platform/graphics/GlyphPage.h:86
> +        return m_from
> +            + m_to
> +            + WTF::PtrHash<const Font*>::hash(m_font)
> +            + StringHash::hash(m_family.string());

This seems very fragile.  I can imagine a case where two `GlyphRange` have the same `m_font` and `m_family` with different `m_from` and `m_to` that add to the same sum (e.g. `0` and `10` vs `4` and `6`).  Is it not possible to have a better hash?

> Source/WebCore/platform/graphics/GlyphPage.h:119
> +        , m_range(range ? range : glyphData.range())

`range ?: glyphData.range()`

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:169
> +WI.CSSKeywordCompletions.SystemFontKeywords = [

Is this something we should get from the backend instead of hardcoding in the frontend?  Maybe as another return value of `CSS.getSupportedSystemFontFamilyNames`?

I ask because it's very easy for this list to become out-of-sync with the backend implementation (as an example, just look at the rest of this file).  We should generally try to get as much data from the inspected page as possible.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:328
> +            this.dispatchEventToListeners(WI.DOMNodeStyles.Event.Refreshed, {significantChange: true});

We don't want to dispatch this more than once per `refresh` call, but we also only want to dispatch this once all the results of all of the backend commands have been received.  I'd either:
 - move `significantChange` outside of `fetchComputedStyle` to be a captured variable and then include it here
 - remove both calls in `fetchComputedStyle` and `fetchComputedFontFamilies` and have `fetchComputedStyle` resolve its promise with it (`fetchedComputedStylesPromise.resolve(significantChange);`) and then do the dispatch inside the `Promise.all([...]).then`
We know that the order of dispatch/result for all of these commands is linear because none of them are marked `async`, so all that really matters is that the dispatch occurs after all of the commands finish.  Since `CSS.getComputedStyleForNode` was the last one to be invoked previously, that was the one that did the dispatch.

> Source/WebInspectorUI/UserInterface/Models/Font.js:26
> +WI.Font = class Font

NIT: I think this should be called `WI.ComputedFont` (or `WI.UsedFont` based on @Brian Burg's suggestion) so that it exactly matches what's in the protocol

> Source/WebInspectorUI/UserInterface/Models/Font.js:57
> +// Keep these in sync with the "ComputedFontType" enum defined by the "CSS" domain.

NIT: this is implicit/expected (especially if the name of the object matches the protocol type), so you could drop it

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:26
> +WI.FontRange = class FontRange

NIT: I think this should be called `WI.ComputedFontRange` (or `WI.UsedFontRange` based on @Brian Burg's suggestion) so that it exactly matches what's in the protocol

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:55
> +        if (this._family === "-webkit-standard")

Can we make this into a constant somewhere so we don't have to keep retyping it?  e.g. `WI.CSSKeywordCompletions.GlobalFallbackFontName = "-webkit-standard";`

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:63
> +        return this.from.toString(16).toUpperCase();

NIT: you can use the direct member variable `this._from` when inside the same class and avoid an unnecessary getter call

> Source/WebInspectorUI/UserInterface/Models/FontRange.js:68
> +        return this.to.toString(16).toUpperCase();

ditto (:63) for `this._to`

> Source/WebInspectorUI/UserInterface/Models/FontRanges.js:30
> +        this._ranges = ranges || [];

Should we add a `console.assert(!ranges || Array.isArray(ranges), ranges);`?

> Source/WebInspectorUI/UserInterface/Models/FontRanges.js:46
> +        return Array.from(new Set(this.ranges.map((range) => range.font.name)));

ditto (FontRange.js:63) for `this._ranges`

> Source/WebInspectorUI/UserInterface/Models/FontRanges.js:65
> +        return this._ranges.some((range) => family === range.family || family === range.equivilentStandardFamily);

`canonicalFamily`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:269
> +            let families = this._property.value.split(",").map((family) => family.trim());

this appears unused

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:271
> +

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:274
> +                elementTitle = WI.UIString("No explicitly defined family was available for some characters. Using additional fonts: %s", "No explicitly defined family was available for some characters. Using additional fonts: %s @ Style Sidebar Font Warning Tooltip", "Tooltip warning that one or more fonts had to be used that were not defined by the style.").format(fontRanges.filterByFamily("-webkit-standard").fontNames.join(", "));

`WI.UIString(", ")`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:486
> +        const maxValueLength = 150;

NIT: I'd move this to right above where its used

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:801
> +        let composedToken = tokens.map((token) => token.value).join("");

NIT: I think `valueString` or `tokensString` is a better name for this

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:807
> +            tokenElement.title = WI.UIString("Using fonts: %s", "Using fonts: %s @ `font` Style Property", "Tooltip for CSS `font-family` value that is being used to draw characters in an element.").format(fontRanges.fontNames.join(", "));

`WI.UIString(", ")`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:811
> +        let firstFamilyIndex = tokens.findIndex((token) => token.value === ",") - 1;

I'd add a comment here explaining why the `- 1` is necessary.

Also, I'm guessing that CodeMirror handles this, but what about if the family name contains a comma (e.g. `font: 42 "Font, 42";`)?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:816
> +        if (!tokens[firstFamilyIndex].value.trim())

Is it guaranteed that there can only be one whitespace token, or should this be a `while`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:830
> +

Style: unnecessary whitespace

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:843
> +                tokenElement.title = titleFormat.format(fontRangesForToken.fontNames.join(", "));

`WI.UIString(", ")`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:854
> +        for (let i = 0; i < tokens.length; i++) {

`++i`

[Patrick Angle]

Comment on attachment 414124 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1117
>> +                    for (auto* innerChild = child->childAt(0); innerChild; innerChild = innerChild->nextSibling()) {
> 
> Why are we going two levels deep?  I would think that we'd only care about the direct text descendants of `node`?

In most cases we do only care about direct descendants of `node`, however there are cases where what seems like a direct descendant can actually be nested one level deeper. I discovered this by inspecting the hero text on webkit.org that reads "A fast, open source web browser engine.". That text (roughly) is defined as:

```
<h1 id="inspect-me" style="font-family: serif; display: block;"><a name="emptylink" style="display: block;"></a>A fast, open source<br>web browser engine.</h1>
```

Which in turn gives a tree of renders like so:

```
RenderBlockFlow             h1        <- This is the node that is inspected.
 |- RenderBlockFlow         a
 |- RenderBlockFlow
     |- RenderText          "A fast, open source"
     |- RenderLineBreak     br
     |- RenderText          "web browser engine."
```

I have added a test case for this which will be part of the next patch for review.

[Patrick Angle]

A note on the decision to copy data from FontRanges::Range into our own GlyphRange:

We specifically copy values from FontRanges::Range into our own GlyphRange in order to facilitate reference counting and lifetime management of the range information. Other options investigated included copying the FontRanges::Range to the GlyphData (which was not chosen because there will be a GlyphData for every single character on the page) or holding a pointer to the FontRanges::Range (which in cases where the first range did not satisfy all characters, the range would be copied and then deleted as part of FontRanges' storage, causing the pointer to no longer be viable).

[Patrick Angle]

Created attachment 414408 [details]
Patch

[Patrick Angle]

Created attachment 414409 [details]
Patch

[Devin Rousso]

Comment on attachment 414124 [details]
Patch

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

>>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1117
>>> +                    for (auto* innerChild = child->childAt(0); innerChild; innerChild = innerChild->nextSibling()) {
>> 
>> Why are we going two levels deep?  I would think that we'd only care about the direct text descendants of `node`?
> 
> In most cases we do only care about direct descendants of `node`, however there are cases where what seems like a direct descendant can actually be nested one level deeper. I discovered this by inspecting the hero text on webkit.org that reads "A fast, open source web browser engine.". That text (roughly) is defined as:
> 
> ```
> <h1 id="inspect-me" style="font-family: serif; display: block;"><a name="emptylink" style="display: block;"></a>A fast, open source<br>web browser engine.</h1>
> ```
> 
> Which in turn gives a tree of renders like so:
> 
> ```
> RenderBlockFlow             h1        <- This is the node that is inspected.
>  |- RenderBlockFlow         a
>  |- RenderBlockFlow
>      |- RenderText          "A fast, open source"
>      |- RenderLineBreak     br
>      |- RenderText          "web browser engine."
> ```
> 
> I have added a test case for this which will be part of the next patch for review.

I see.  This mainly concerns me because it's somewhat arbitrary and is possibly very fragile (i.e. the implementation could change one day where there's three levels instead of two in this case).  It feels like you're relying on an implementation detail.

Looking at some of the related classes, perhaps we should keep going one level deeper so long as `RenderObject::isAnonymous` (i.e. when the render object doesn't have a node)?  I wonder if there's a situation right now that has more than one level of anonymous render objects (e.g. using CSS `columns`).  It might be worth asking someone about this.

[Devin Rousso]

Comment on attachment 414409 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Added `getUsedFontRangesForNode` and associated types `UsedFont`, `CUsedFontRange`, and `UsedFontType` to CSS

oops `CUsedFontRange`

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:860
> +    auto fontFamilyKeywords = JSON::ArrayOf<String>::create();
> +    // The family keywords are intentionally layed out so that `caption` comes first and `status-bar` follows all platform-dependent family keywords.
> +    for (unsigned i = CSSValueCaption; i <= CSSValueStatusBar; ++i)
> +        fontFamilyKeywords->addItem(getValueNameString(convertToCSSValueID(i)));

awesome awesome awesome

One minor suggestion: could we create something like
```
    using FirstCSSSystemFont = CSSValueCaption;
    using LastCSSSystemFont = CSSValueStatusBar;
```
that's defined somewhere so that they can be shared between both this usage and `CSSPropertyParser::consumeSystemFont`, meaning that if a new system font is added after `status-bar` then it's more likely that both places get updated and this isn't forgotten?

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1118
> +            for (auto* child = renderer->childAt(0); child; child = child->nextSibling()) {

please see comment #41

> Source/WebCore/platform/graphics/ComplexTextController.cpp:446
> +                        collectComplexTextRunsForCharacters(m_smallCapsBuffer.data() + itemStart, itemLength, itemStart, smallSynthesizedFont, WTFMove(nextRanges));

I realize I asked you to do this (and I thank you for it), but we should treat the `nextRanges` as invalid after `WTFMove`, meaning that we should either `nextRanges.copyRef()` or `std::exchange(nextRanges, nullptr)`.

Unless you know (and can guarantee) that `nextRanges` is not used after the `WTFMove`, in which case you can ignore me (although I'd suggest adding some sort of `ASSERT`).

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:73
> +    RefPtr<GlyphRange> m_ranges[GlyphPage::size];

OH wait oops this is a static array not just a single `RefPtr`.  I actually dunno if the `{ }` is required/desired or not in this case.  Perhaps it's best to include it to match the others.

> Source/WebCore/platform/graphics/GlyphPage.h:51
> +        static unsigned hash(const RefPtr<GlyphRange> key) { return key->hash(); }

`const RefPtr<GlpyhRange>&`?

> Source/WebCore/platform/graphics/GlyphPage.h:54
> +            return a == b || (a && b && !a.isHashTableDeletedValue() && !b.isHashTableDeletedValue() && *a == *b);

Why the explicit checks for `isHashTableDeletedValue`?

> Source/WebCore/platform/graphics/GlyphPage.h:99
> +    const Font* m_font;

`RefPtr`/`WeakPtr`?

> Source/WebInspectorUI/UserInterface/Main.html:411
> +    <script src="Models/UsedFont.js"></script>
> +    <script src="Models/UsedFontRange.js"></script>
> +    <script src="Models/UsedFontRanges.js"></script>

Style: these should be moved below so that this is list is alphabetical

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:165
> +            WI.CSSKeywordCompletions.SystemFontKeywords = fontFamilyKeywords;

you should only do this if `fontFamilyKeywords` exists, which it won't when inspecting an older device
```
    // COMPATIBILITY (iOS 14.0): `fontFamilyKeywords` return value for `CSS.getSupportedSystemFontFamilyNames` did not exist yet.
    if (fontFamilyKeywords)
        WI.CSSKeywordCompletions.SystemFontKeywords = fontFamilyKeywords;
```

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:170
> +WI.CSSKeywordCompletions.SystemFontKeywords = new Array;

just use `[]`

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:161
> +        let fetchedComputedFontFamiliesPromise = new WI.WrappedPromise;

NIT: this should also be `Used`

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:341
> +        .then((significantChange) => {

When using `Promise.all`, the value given to the resulting promise is an array of the resolved values of all the promises provided as an input.  As such, I'd restructure this slightly to have
```
    fetchedComputedStylesPromise.resolve({significantChange});
```
and
```
    .then(([fetchMatchedStylesResult, fetchInlineStylesResult, fetchComputedStylesResult, fetchUsedFontFamiliesResult]) => {
        this._pendingRefreshTask = null;
        this.dispatchEventToListeners(WI.DOMNodeStyles.Event.Refreshed, {
            significantChange: fetchComputedStylesResult.significantChange,
        });
        return this;
    });
```

I probably wasn't clear enough in my last comment.  My apologies.

> Source/WebInspectorUI/UserInterface/Models/UsedFontRanges.js:52
> +        return !this.ranges.length;

NIT: `this._ranges`

> Source/WebInspectorUI/UserInterface/Models/UsedFontRanges.js:71
> +        console.assert(ranges instanceof WI.UsedFontRanges);

NIT: I'd add `ranges` as an extra argument so that it's logged if this assertion fails
```
    console.assert(ranges instanceof WI.UsedFontRanges, ranges);
```

> Source/WebInspectorUI/UserInterface/Test.html:160
> +    <script src="Models/UsedFont.js"></script>
> +    <script src="Models/UsedFontRange.js"></script>
> +    <script src="Models/UsedFontRanges.js"></script>

ditto (Main.html:409)

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:270
> +            if (fontRanges.hasFamily("-webkit-standard") && !/\b-webkit-standard\b/.test(this._property.value)) {

you should use `WI.CSSKeywordCompletions.GlobalFallbackFontFamilyName`

Also, using `/\b-webkit-standard\b/` to test for whether `-webkit-standard` was used isn't a perfect solution because it's possible that it's inside a CSS variable (e.g. `--foo: -webkit-standard; font-family: var(--foo);`).  I wonder if it's even really necessary to check for this since it's likely to be extremely rare.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:272
> +                elementTitle = WI.UIString("No explicitly defined family was available for some characters. Using additional fonts: %s", "No explicitly defined family was available for some characters. Using additional fonts: %s @ Style Sidebar Font Warning Tooltip", "Tooltip warning that one or more fonts had to be used that were not defined by the style.").format(fontRanges.filterByFamily("-webkit-standard").fontNames.join(WI.UIString(", ")));

ditto (:270)

[Patrick Angle]

Comment on attachment 414409 [details]
Patch

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

>> Source/WebCore/platform/graphics/GlyphPage.h:54
>> +            return a == b || (a && b && !a.isHashTableDeletedValue() && !b.isHashTableDeletedValue() && *a == *b);
> 
> Why the explicit checks for `isHashTableDeletedValue`?

The deleted value is different from `nullptr` (the underlying pointer points to `PtrTraits::hashTableDeletedValue()` instead of `nullptr`). This state is not taken into account when treating the `RefPtr` as a boolean. Without this check, we could end up comparing the deleted value to a real value. Am I missing a piece of the puzzle that would allow this to not be the case?

[Patrick Angle]

Created attachment 414472 [details]
Patch

[Patrick Angle]

Comment on attachment 414472 [details]
Patch

Looks like I need to rebase this patch again.

[Devin Rousso]

Comment on attachment 414409 [details]
Patch

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

>>> Source/WebCore/platform/graphics/GlyphPage.h:54
>>> +            return a == b || (a && b && !a.isHashTableDeletedValue() && !b.isHashTableDeletedValue() && *a == *b);
>> 
>> Why the explicit checks for `isHashTableDeletedValue`?
> 
> The deleted value is different from `nullptr` (the underlying pointer points to `PtrTraits::hashTableDeletedValue()` instead of `nullptr`). This state is not taken into account when treating the `RefPtr` as a boolean. Without this check, we could end up comparing the deleted value to a real value. Am I missing a piece of the puzzle that would allow this to not be the case?

Oh wow yeah oops.  Leave it to 2am me to forget that "empty" is not the same as "deleted" 🤦‍♂️

[Patrick Angle]

Created attachment 414476 [details]
Patch

[Patrick Angle]

Created attachment 414485 [details]
Patch

[Devin Rousso]

Comment on attachment 414485 [details]
Patch

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

Nice work!  I think the inspector bits of this are good.  I'd suggest talking with Myles again (or anyone else familiar with fonts) to double check that this is right (and/or if there's a better way to achieve what you want).

A general piece of advice/feedback (which I commented on in a few specific spots) is that we should use `RefPtr`/`WeakPtr` instead of raw pointers wherever possible, as that dramatically lessens the likelihood of UAFs.  Some related info: <https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html>.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:99
> +static void appendFont(FontRanges& ranges, Ref<FontAccessor>&& fontAccessor, const Vector<CSSFontFace::UnicodeRange>& unicodeRanges, CSSFontFace* fontFace)

It looks like the only caller of this has a `Ref<CSSFontFace>`, so can we just `CSSFontFace&` instead?

Also, can we change `FontRanges::Range::Range` to take a `WeakPtr<CSSFontFace>` instead of a raw pointer and do the `makeWeakPtr` here?

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1088
> +            .setName(range->font()->platformData().familyName())

This is a `WeakPtr` now, so it should be null-checked before being used.

Along the lines of what I wrote below, would it be possible/easier to just save the `font.platformData().familyName()` when the `GlpyhRange` is constructed?  Although since it's a string, perhaps that would be worse for memory.

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1089
> +            .setType(range->cssFontFace() ? Protocol::CSS::UsedFontType::WebFont : Protocol::CSS::UsedFontType::System)

Thinking about this a bit more, if the only usage of `GlyphRange::cssFontFace` is to null-check it, then perhaps we should just have an `enum class CSSFontFaceType { WebFont, System };` that's set in the constructor of `GlyphRange` based on the existence of the `CSSFontFace` (i.e. instead of `WeakPtr<CSSFontFace> m_cssFontFace;` you could just have `CSSFontFaceType m_cssFontFaceType;`).  This way we don't need to deal with `WeakPtr` and the value is constant once the object is created (I dunno if it'd actually be possible, but using a `WeakPtr` theoretically means that the `CSSFontFace` could be valid when the `GlyphRange` is created but then disappears before this call, meaning that Web Inspector would report the wrong type).

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1126
> +        if (is<RenderText>(renderer))

NIT: I think you can rework `layoutRendererTextChildren` to avoid some repeated logic
```
    static void layoutRendererTextChildren(const RenderObject& renderer, Function<void(const RenderObject&)> layoutRendererCallback)
    {
        if (is<RenderText>(renderer)) {
            layoutRendererCallback(downcast<RenderText>(renderer));
            return;
        }

        for (auto* child = renderer.childAt(0); child; child = child->nextSibling())
            layoutRendererTextChildren(*child, layoutRendererCallback);
    }
```
and then just call it by
```
    if (auto* renderer = node.renderer()) {
        layoutRendererTextChildren(*renderer, [&] (const RenderText& textRenderer) {
            auto glyphBuffer = fontCascade.layoutText(TextRun(String(textRenderer.text())));
            for (unsigned i = 0; i < glyphBuffer.size(); ++i)
                activeRanges.add(glyphBuffer.fontRangeAt(i));
        });
    }
```

> Source/WebCore/inspector/agents/InspectorCSSAgent.h:161
> +    void layoutRendererTextChildren(const RenderObject&, std::function<void(const RenderText&)>);

NIT: this could just be a `static` function in the `.cpp`
NIT: I think we prefer `Function` (from WTF)

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:186
> +            auto ranges = FontRanges(WTFMove(font), family);
> +            return ranges;

NIT: you could inline this like it was before

> Source/WebCore/platform/graphics/GlyphPage.h:87
> +    GlyphData(Glyph glyph = 0, const Font* font = nullptr, GlyphRange* range = nullptr)

`RefPtr<GlyphRange>&& range = nullptr`

> Source/WebCore/platform/graphics/GlyphPage.h:97
> +        , m_range(range ? range : glyphData.range())

NIT: `range ?: glyphData.range()`

> Source/WebCore/platform/graphics/GlyphPage.h:190
> +        , m_range(range ? GlyphRange::create(*range) : RefPtr<GlyphRange>())

just use `nullptr`

[Patrick Angle]

Comment on attachment 414485 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1088
>> +            .setName(range->font()->platformData().familyName())
> 
> This is a `WeakPtr` now, so it should be null-checked before being used.
> 
> Along the lines of what I wrote below, would it be possible/easier to just save the `font.platformData().familyName()` when the `GlpyhRange` is constructed?  Although since it's a string, perhaps that would be worse for memory.

While this patch is only concerned with the family name of the font, another upcoming patch will need to get the available variation axes from the same font, which is why I decided to keep a pointer to the font instead of just storing the family name.

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1089
>> +            .setType(range->cssFontFace() ? Protocol::CSS::UsedFontType::WebFont : Protocol::CSS::UsedFontType::System)
> 
> Thinking about this a bit more, if the only usage of `GlyphRange::cssFontFace` is to null-check it, then perhaps we should just have an `enum class CSSFontFaceType { WebFont, System };` that's set in the constructor of `GlyphRange` based on the existence of the `CSSFontFace` (i.e. instead of `WeakPtr<CSSFontFace> m_cssFontFace;` you could just have `CSSFontFaceType m_cssFontFaceType;`).  This way we don't need to deal with `WeakPtr` and the value is constant once the object is created (I dunno if it'd actually be possible, but using a `WeakPtr` theoretically means that the `CSSFontFace` could be valid when the `GlyphRange` is created but then disappears before this call, meaning that Web Inspector would report the wrong type).

Along the same line of thinking as :1088, the ability to jump to the font-face declaration in source is desired in the future. We could do that by keeping the WeakPtr here, or we can add new members to GlyphRange for each new property we care about from the CSSFontFace.

>> Source/WebCore/platform/graphics/GlyphPage.h:97
>> +        , m_range(range ? range : glyphData.range())
> 
> NIT: `range ?: glyphData.range()`

I actually did make this change earlier and had to revert back as it failed to compile on Windows. It seems it doesn't know how to handle this specific piece of syntax: (Attachment 414409 [details]) https://ews-build.webkit.org/#/builders/10/builds/41801

[Patrick Angle]

Comment on attachment 414485 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1126
>> +        if (is<RenderText>(renderer))
> 
> NIT: I think you can rework `layoutRendererTextChildren` to avoid some repeated logic
> ```
>     static void layoutRendererTextChildren(const RenderObject& renderer, Function<void(const RenderObject&)> layoutRendererCallback)
>     {
>         if (is<RenderText>(renderer)) {
>             layoutRendererCallback(downcast<RenderText>(renderer));
>             return;
>         }
> 
>         for (auto* child = renderer.childAt(0); child; child = child->nextSibling())
>             layoutRendererTextChildren(*child, layoutRendererCallback);
>     }
> ```
> and then just call it by
> ```
>     if (auto* renderer = node.renderer()) {
>         layoutRendererTextChildren(*renderer, [&] (const RenderText& textRenderer) {
>             auto glyphBuffer = fontCascade.layoutText(TextRun(String(textRenderer.text())));
>             for (unsigned i = 0; i < glyphBuffer.size(); ++i)
>                 activeRanges.add(glyphBuffer.fontRangeAt(i));
>         });
>     }
> ```

I think it's still a bit more complicated than this because we need to consider every child of the provided `Node`, but those children we only recurse deeper on children for which `isAnonymous()` is true.

[Devin Rousso]

Comment on attachment 414485 [details]
Patch

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

>>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1126
>>> +        if (is<RenderText>(renderer))
>> 
>> NIT: I think you can rework `layoutRendererTextChildren` to avoid some repeated logic
>> ```
>>     static void layoutRendererTextChildren(const RenderObject& renderer, Function<void(const RenderObject&)> layoutRendererCallback)
>>     {
>>         if (is<RenderText>(renderer)) {
>>             layoutRendererCallback(downcast<RenderText>(renderer));
>>             return;
>>         }
>> 
>>         for (auto* child = renderer.childAt(0); child; child = child->nextSibling())
>>             layoutRendererTextChildren(*child, layoutRendererCallback);
>>     }
>> ```
>> and then just call it by
>> ```
>>     if (auto* renderer = node.renderer()) {
>>         layoutRendererTextChildren(*renderer, [&] (const RenderText& textRenderer) {
>>             auto glyphBuffer = fontCascade.layoutText(TextRun(String(textRenderer.text())));
>>             for (unsigned i = 0; i < glyphBuffer.size(); ++i)
>>                 activeRanges.add(glyphBuffer.fontRangeAt(i));
>>         });
>>     }
>> ```
> 
> I think it's still a bit more complicated than this because we need to consider every child of the provided `Node`, but those children we only recurse deeper on children for which `isAnonymous()` is true.

Ah oops yes I forgot that bit.  I think you can add a
```
    if (child->isAnonymous() || is<RenderText>(child))
```
inside the `for` to fix that :)

That does make it slightly more complex tho, so up to you how you want to write this.

[Antti Koivisto]

Comment on attachment 414485 [details]
Patch

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

> Source/WebCore/platform/graphics/GlyphPage.h:110
>      Glyph glyph;
>      const Font* font;
> +
> +private:
> +    RefPtr<GlyphRange> m_range;

I don't really understand what this patch is doing but adding a refcounted member to a performance critical low level type like GlyphData to support an Inspector feature seems wrong. Please consider some other approaches. 

This shoudn't be landed as-is without review by Myles.

[alan]

Comment on attachment 414485 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1122
> +    auto layoutRenderer = [&activeRanges, &fontCascade](const RenderText& renderer) {
> +        auto glyphBuffer = fontCascade.layoutText(TextRun(String(renderer.text())));
> +        for (unsigned i = 0; i < glyphBuffer.size(); ++i)
> +            activeRanges.add(glyphBuffer.fontRangeAt(i));

calling this lambda 'layoutRenderer' is rather confusing. Laying out a renderer in WebKit means that we compute the CSS box geometries for the associated render object. Please find a more appropriate name for this function and for its callers (layoutRendererTextChildren etc).

[Antti Koivisto]

Not how applicable it is here but an approach taken by some other Inspector features is to lazily build HashMaps for reverse lookups. This allows the core data structures to stay compact while only taking performance hit when Inspector is attached. See InspectorCSSOMWrappers for example.

[Blaze Burg]

Comment on attachment 414485 [details]
Patch

Patrick and I discussed an alternate instrumentation approach that copies GlyphData and GlyphRanges into a side buffer if any inspector frontends are open. The side buffer could be threaded through as an out-parameter to the various places where we can associate GlyphData with a GlyphRange (mostly, implementations of glyphDataForCharacter()).

Instead of passing around a pointer to a side table, it may be easier to use RAII to make a singleton side buffer that can later be filled with instrumentation data (GlyphData x GlyphRange) in the fonts code if any inspector frontends are open.