[webkit-reviews] review granted: [Bug 224178] Implement FontFace in Workers for OffscreenCanvas : [Attachment 426718] 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 224178: Implement FontFace in Workers for OffscreenCanvas
https://bugs.webkit.org/show_bug.cgi?id=224178

Attachment 426718: Patch

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




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

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

> Source/WebCore/ChangeLog:38
> +	     Use FontLoadRequest::isPending instead of making assumptions about
> +	     its state on construction. Also pass the FontCache singleton where
> +	     appropriate in font creation.

Does using isPending have any observable effect? Your comment makes this change
sound sensible, but it’s oblique enough that I can’t tell if you did this to
solve a problem, avoid what would be a new OffscreenCanvas-only problem, or
just did it to make the code more logical with no observable effect.

> Source/WebCore/css/CSSFontFace.h:176
> +    FontCache& fontCacheFallbackToSingleton();

I think in this function name "fall back" is the two word verb, not the single
word noun, so should be spelled "FallBack". But also maybe "FallingBack".

Apparently we have used this name elsewhere, same comment applies there.

> Source/WebCore/css/FontFace.cpp:67
> +    auto result = adoptRef(*new FontFace(*context.cssFontSelector()));

What guarantees cssFontSelector is non-null?

> Source/WebCore/css/FontFace.cpp:80
> +	       auto value =
CSSPropertyParserWorkerSafe::parseFontFaceSrc(string, is<Document>(context) ?
CSSParserContext(downcast<Document>(context)) : HTMLStandardMode);

Can we make a helper so we don’t need to write it with ?: here? Or is this a
unique situation?

> Source/WebCore/platform/graphics/Font.h:97
>      static Ref<Font> create(const FontPlatformData& platformData, Origin
origin = Origin::Local, Interstitial interstitial = Interstitial::No,
>	   Visibility visibility = Visibility::Visible, OrientationFallback
orientationFallback = OrientationFallback::No,
Optional<RenderingResourceIdentifier> identifier = WTF::nullopt)

These create functions shouldn’t be inlined in the header. We should inline
constructors into create functions rather than inlining create functions into
call sites, and keep the create functions out of header files.

> Source/WebCore/platform/graphics/Font.h:101
> +    static Ref<Font> create(const FontPlatformData& platformData, Origin
origin, FontCache* fontCacheForVerticalData, Interstitial interstitial =
Interstitial::No,

Ditto.

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:44
> +    : m_url(url)

Missing WTFMove here.

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:52
> +    ASSERT(is<WorkerGlobalScope>(context));
> +    auto& workerGlobalScope = downcast<WorkerGlobalScope>(context);

Why doesn’t this take a WorkerGlobalScope& instead of a
ScriptExecutionContext&?

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:79
> +bool WorkerFontLoadRequest::ensureCustomFontData(const AtomString&)

Why does this ignore the remote URI?

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:89
> +#if !PLATFORM(COCOA)
> +    if (!m_fontCustomPlatformData && !m_errorOccurred && !m_isLoading &&
m_data && isWOFF(*m_data)) {
> +	   Vector<char> convertedFont;
> +	   if (convertWOFFToSfnt(*m_data, convertedFont))
> +	       m_data = SharedBuffer::create(WTFMove(convertedFont));
> +	   else
> +	       m_errorOccurred = true;
> +    }
> +#endif

Can this be in shared code? How is this worker-specific?

> Source/WebCore/workers/WorkerFontLoadRequest.cpp:121
> +	   return;

I don’t think we need this return statement..

> Source/WebCore/workers/WorkerFontLoadRequest.h:29
> +#include "ResourceLoaderOptions.h"

I don’t see the use of this header; maybe it’s LoadedFromOpaqueSource?

> Source/WebCore/workers/WorkerFontLoadRequest.h:31
> +#include <wtf/RefPtr.h>

URL.h includes RefPtr.h, so you don’t need to include this here.

> Source/WebCore/workers/WorkerFontLoadRequest.h:39
> +class WorkerGlobalScope;

Don’t need this; it’s not used. Although maybe it would be used if you changed
the type of the argument to load as I suggest.

> Source/WebCore/workers/WorkerFontLoadRequest.h:40
> +struct FontCustomPlatformData;

Should leave a blank line before this.

> Source/WebCore/workers/WorkerGlobalScope.cpp:528
> +    return cssFontSelector()->fontFaceSet();

What guarantees cssFontSelector() is non-null?

> Source/WebCore/workers/WorkerGlobalScope.h:55
> +class FontLoadRequest;

This should not be needed. We are overriding a function that takes a parameter
of this type, so if we were able to compile the base class declaration of the
function, we should be able to compile the override without additional forward
declarations.

> Source/WebCore/workers/WorkerGlobalScope.h:130
> +    std::unique_ptr<FontLoadRequest> fontLoadRequest(String&, bool, bool,
LoadedFromOpaqueSource) final;

Why does this take a String&? Even though this is just an override, I think it
should mention the argument names when the type does not make clear what they
are. Debatable, I suppose.