Bug 307002 – GlyphOverflow review: Use RectEdges and/or clarify computeBounds

• Status: NEW
• Component: Layout and Rendering

[Gerald Squelart]

(Spawned from https://github.com/WebKit/WebKit/pull/57225#discussion_r2762657650 and https://github.com/WebKit/WebKit/pull/57225#discussion_r2729602999 .)

GlyphOverflow currently contains 4 LayoutUnit's (left, right, top, bottom), and a bool "computeBounds".

The 4 LayoutUnits might be better represented with a RectEdges<LayoutUnit>.

Regarding ComputeBounds:
At a glance through some of the code using it, it changes the meaning of top and bottom when computed in FontCascade::width(), and then when used after that.

The name "computeBounds" is ambiguous. Is it imperative, or shouldComputedBounds or didComputeBounds?

Presumably the code that requests a GlyphOverflow with a certain computeBounds will then only use it in further code that expects that value, but there is no check around that, so in the worst case top/bottom could be computed one way and then sent to other code that interprets it the other way!
Ideally this could be solved through C++ typing, to ensure it can't be misused; but we may still want both options to be computed in the same FontCascade::width(). Some ideas:
- Compute both values of top&bottom with and without bounds (or store the font ascent&descent), no more need for a computeBounds flag. A bit more wasteful in space and work, but it would clarify which type of "top"/"bottom" is used everywhere. It might not be possible to use RectEdges in this scenario.
- Use different C++ "GlyphOverflow" types gathered in a variant or similar helper type. It adds a bit more work around testing&unwrapping the variant.
- Use different C++ types in separate/duplicated functions, i.e. we'd end up with something like FontCascade::widthWithBounds() and FontCascade::widthNoBounds(). Obviously some duplicated code, which could be hidden in templates.
- Other options?

[Gerald Squelart]

There's also an existing FIXME that could be looked at here:
```
    bool operator!=(const GlyphOverflow& other)
    {
        // FIXME: Probably should name this rather than making it the != operator since it ignores the value of computeBounds.
        return left != other.left || right != other.right || top != other.top || bottom != other.bottom;
    }
```

[Gerald Squelart]

Ignore my last comment, `operator!=()` was removed in https://bugs.webkit.org/show_bug.cgi?id=307281