[webkit-reviews] review granted: [Bug 202793] Implement text rendering on OffscreenCanvas in a Worker : [Attachment 424942] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 202793: Implement text rendering on OffscreenCanvas in a Worker
https://bugs.webkit.org/show_bug.cgi?id=202793

Attachment 424942: Patch

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




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 424942
  --> https://bugs.webkit.org/attachment.cgi?id=424942
Patch

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

> Source/WebCore/css/CSSFontSelector.h:127
> +    RefPtr<FontCache> m_fontCache;

Can this be a Ref instead of a RefPtr? Does it ever have to be null?

> Source/WebCore/html/canvas/OffscreenCanvasRenderingContext2D.cpp:92
> +    auto fontCascade = Style::resolveForFontRaw(*fontRaw,
WTFMove(fontDescription), context);
> +    if (fontCascade) {

Slightly better style to define inside the if:

    if (auto fontCascade = ...

> Source/WebCore/platform/graphics/FontCache.cpp:121
> +struct FontDataCacheKeyHash {

So frustrating! Another patch I need to merge with (since I am redoing some of
this hashing code a bit).

Anyway, I can handle it.

> Source/WebCore/platform/graphics/FontCache.cpp:132
> +    static const bool safeToCompareToEmptyOrDeleted = true;

When touching code like this we can switch from const to constexpr.

> Source/WebCore/platform/graphics/FontCache.cpp:160
> +    WTF_MAKE_FAST_ALLOCATED;
> +public:

Should be WTF_MAKE_STRUCT_FAST_ALLOCATED and then no need for public: here.

> Source/WebCore/platform/graphics/FontCache.h:71
> +struct FontDataCaches;

Could consider making this struct be a member of FontCache instead of at the
file level. Would have little effect, but I think be a bit clearer.

> Source/WebCore/platform/graphics/FontCache.h:222
> +    static Ref<FontCache> create();

Should have commented when we added reference counting. I don’t fully
understand why we need multiple owners for a single FontCache, or ref/deref to
help keep the cache alive during the destruction process. If neither is
required, then this class could just be a normal one that we create and destroy
at will, and store in a UniqueRef or unique_ptr if we want it on the heap.
Don’t need reference counting just to have multiple instances of a class!

> Source/WebCore/platform/graphics/FontCache.h:225
> +    FontCache();

This constructor should probably be private if possible. If it’s reference
counted then we don’t want someone accidentally creating one not on the heap
outside the create function.

> Source/WebCore/platform/graphics/FontCascade.cpp:171
> +    auto& fontCache = fontSelector ? fontSelector->fontCache() :
FontCache::singleton();

Seems like we might want a helper function that does this operation. It’s a
repeated idiom. Just something that takes a possibly-null FontSelector* and
returns the FontCache&. Inline if necessary.

Having the helper function could help rid us of the urge to put it in a local
variable, too.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:105
> +    , m_generation(m_fontSelector ? m_fontSelector->fontCache().generation()
: FontCache::singleton().generation())

Here.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:147
> +    auto& fontCache = fontSelector ? fontSelector->fontCache() :
FontCache::singleton();

Here.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:185
> +    auto& fontCache = m_fontSelector ? m_fontSelector->fontCache() :
FontCache::singleton();

Here.

> Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:141
> +    auto& fontCache = fontSelector() ? fontSelector()->fontCache() :
FontCache::singleton();

Here.

> Source/WebCore/style/StyleResolveForFontRaw.cpp:55
> +    return fontDescription.familyCount() == 1 &&
fontDescription.firstFamily() ==
familyNamesData->at(static_cast<unsigned>(FamilyNamesIndex::MonospaceFamily));

We typically compare family names with a special equality operator, not by
actually doing == on two strings. (In my patch I was toying with the idea of a
class, FontFamilyName, instead of special == and hash functions.)

> Source/WebCore/style/StyleResolveForFontRaw.cpp:78
> +		   family =
familyNamesData->at(static_cast<unsigned>(CSSPropertyParserHelpers::genericFont
FamilyIndex(ident)));

This index type thing is not great. Is there some way we can change
familyNamesData so it’s easier to use the enum values with it without a
static_cast at the call site each time? Maybe just add an overload for at?

> Source/WebCore/style/StyleResolveForFontRaw.cpp:205
> +	       return
static_cast<float>(CSSPrimitiveValue::computeUnzoomedNonCalcLengthDouble(length
.type, length.value, CSSPropertyFontSize, &fontCascade.fontMetrics(),
&fontCascade.fontDescription(), &fontCascade.fontDescription(),
is<Document>(context) ? downcast<Document>(context).renderView() : nullptr));

Is the nullptr behavior OK for offscreen canvas here? I’d like to understand
more why this is OK.