Bug 209591 - [HarfBuzz] Not all CSS font features are applied
Summary: [HarfBuzz] Not all CSS font features are applied
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar
Depends on:
Blocks: 209593
  Show dependency treegraph
 
Reported: 2020-03-26 03:47 PDT by Carlos Garcia Campos
Modified: 2020-03-30 02:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (27.09 KB, patch)
2020-03-26 03:53 PDT, Carlos Garcia Campos
aperez: review-
aperez: commit-queue-
Details | Formatted Diff | Diff
Patch (26.86 KB, patch)
2020-03-26 05:40 PDT, Carlos Garcia Campos
aperez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2020-03-26 03:47:39 PDT
We need to implement  https://www.w3.org/TR/css-fonts-3/#feature-precedence
Comment 1 Carlos Garcia Campos 2020-03-26 03:53:46 PDT
Created attachment 394590 [details]
Patch
Comment 2 Adrian Perez 2020-03-26 04:54:33 PDT
Comment on attachment 394590 [details]
Patch

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

Logic looks good. There is one thing that I find hard to believe that it would
be working, please check below ⬇️

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:570
> +                char buffer[5];

If it was me, I would probably have done instead:

  const auto& tag = fontFaceFeature.tag();
  const char buffer[] = { tag[0], tag[1], tag[2], tag[3], '\0' };
  FcPatternAddString(resultPattern.get(), FC_FONT_FEATURES, reinterpret_cast<const FcChar*>(buffer));

But in the end both are equivalent, so no big deal 😉️

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:577
> +                FcPatternAddString(resultPattern.get(), FC_FONT_FEATURES, reinterpret_cast<const FcChar8*>(&buffer));

Due to array-to-pointer decay with “&buffer” jere you are passing a “char**”
so I do not know how this can possibly work correctly. We need to plainly
pass “buffer” here.

Using “FcPatternAddString(..., buffer)” might render the cast as unneeded,
too (I don't remember whether “FcChar8” is exactly the same as “char”, though).

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:94
> +            FcPatternAddString(pattern.get(), FC_FONT_FEATURES, reinterpret_cast<const FcChar8*>(&buffer));

Same here.
Comment 3 Carlos Garcia Campos 2020-03-26 05:40:11 PDT
Created attachment 394596 [details]
Patch
Comment 4 Carlos Garcia Campos 2020-03-30 02:11:22 PDT
Committed r259188: <https://trac.webkit.org/changeset/259188>
Comment 5 Radar WebKit Bug Importer 2020-03-30 02:12:16 PDT
<rdar://problem/61046669>