[webkit-reviews] review granted: [Bug 177755] [GTK] Support the "system" CSS font family : [Attachment 322423] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 3 00:19:46 PDT 2017


Carlos Garcia Campos <cgarcia at igalia.com> has granted Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 177755: [GTK] Support the "system" CSS font family
https://bugs.webkit.org/show_bug.cgi?id=177755

Attachment 322423: Patch

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




--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 322423
  --> https://bugs.webkit.org/attachment.cgi?id=322423
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:333
> +#if PLATFORM(GTK)
> +	   || equalLettersIgnoringASCIICase(familyNameString,
"-webkit-system-font")
> +	   || equalLettersIgnoringASCIICase(familyNameString,
"-webkit-system-ui")
> +#endif

You can mode the ifdefed code to the beginning, since the order doesn't matter,
right?

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:61
> +String getDefaultGtkSystemUiFont()

I would call this just defaultGtkSystemFont().

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:67
> +    auto spaceChar = strrchr(fontString.get(), ' ');

auto*

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:68
> +    return String::fromUTF8(fontString.get(), spaceChar - fontString.get());

I know we are always expecting a valid font name - space - size string, but I
would either assert if that's not the case or handling that case, I don't know
what happens if the user sets an an empty string in the dconf setting, for
example. Since we have a duplicated string, maybe it's easier to set '\0' in
the place of the last space (if found), and then just use String::fromUTF8().


More information about the webkit-reviews mailing list