[webkit-reviews] review granted: [Bug 168487] REGRESSION(r212513): LastResort is platform-dependent, so its semantics should not be required to perform font loading correctly. : [Attachment 310094] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 168487: REGRESSION(r212513): LastResort is platform-dependent, so its
semantics should not be required to perform font loading correctly.
https://bugs.webkit.org/show_bug.cgi?id=168487

Attachment 310094: Patch

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




--- Comment #42 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 310094
  --> https://bugs.webkit.org/attachment.cgi?id=310094
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:532
> +	       m_timeoutTimer.startOneShot(3_s);

Please use an informatively named constants for numeric values like this. It
can be inline, just above the using line.

> Source/WebCore/platform/graphics/Font.h:90
> +    enum class ShouldDisplay {
> +	   Visible,
> +	   Invisible
> +    };

"Display" and "Visible" seems like two different words for the same thing. How
about calling this enum "Visibility"?

> Source/WebCore/platform/graphics/Font.h:94
> +    enum class OrientationFallback {
> +	   Fallback,
> +	   NoFallback
> +    };

"Fallback" seems redundant in the value. Just Yes/No maybe like Interstitial
has?

> Source/WebCore/platform/graphics/Font.h:189
> +    Interstitial isInterstitial() const { return m_interstitial ?
Interstitial::Yes : Interstitial::No; }

is* functions should return bools. Either rename this to interstitial() or make
it return a bool. Currently the call sites read awkwardly.

> Source/WebCore/platform/graphics/Font.h:190
> +    ShouldDisplay shouldDisplay() const { return m_shouldDisplay ?
ShouldDisplay::Visible : ShouldDisplay::Invisible; }

As should should* functions. "Visibility visibility() const" would work.

> Source/WebCore/platform/graphics/Font.h:318
>      unsigned m_treatAsFixedPitch : 1;
> -    unsigned m_isCustomFont : 1; // Whether or not we are custom font loaded
via @font-face
> -    unsigned m_isLoading : 1; // Whether or not this custom font is still in
the act of loading.
> +    unsigned m_origin : 1; // Whether or not we are custom font loaded via
@font-face
> +    unsigned m_interstitial : 1; // Whether or not this custom font is the
last resort placeholder for a loading font
> +    unsigned m_shouldDisplay : 1; // @font-face's internal timer can cause
us to show fonts even when a font is being downloaded.
>  
> -    unsigned m_isTextOrientationFallback : 1;
> +    unsigned m_orientationFallback : 1;
>      unsigned m_isBrokenIdeographFallback : 1;
>      unsigned m_hasVerticalGlyphs : 1;

There are not enough Fonts for it to be worth to optimize this with bitfields
(and size of associated actual font data is many orders of magnitude greater).
It would be better to just use properly typed enum values.

> Source/WebCore/platform/graphics/FontCascade.cpp:1402
> +    // Don't draw anything while we are using custom fonts that are in the
process of loading,
> +    // except if the 'force' argument is set to true (in which case it will
use a fallback
> +    // font).
> +    return font.isInterstitial() == Font::Interstitial::No ||
font.shouldDisplay() == Font::ShouldDisplay::Visible ||
customFontNotReadyAction ==
FontCascade::CustomFontNotReadyAction::UseFallbackIfFontNotReady;

I'm not sure I understand how the comment and the code matches. The comment
mentions 'force' argument and loading. The code has nothing called 'force' and
nothing obviously about loading. It also tests three conditions while the text
implies two.

> Source/WebCore/platform/graphics/FontRanges.h:40
> +enum class ForbidDownloadingExternalResource {
> +    Forbid,
> +    Allow
> +};

ExternalResourceDownloadPolicy? You even already name the variable 'policy' in
many place.