[webkit-reviews] review granted: [Bug 167956] [Harfbuzz] Implement ComplexTextController on top of HarfBuzz : [Attachment 322296] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 3 11:56:54 PDT 2017


Myles C. Maxfield <mmaxfield at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 167956: [Harfbuzz] Implement ComplexTextController on top of HarfBuzz
https://bugs.webkit.org/show_bug.cgi?id=167956

Attachment 322296: Patch

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




--- Comment #14 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 322296
  --> https://bugs.webkit.org/attachment.cgi?id=322296
Patch

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

This is really great, and is way less code than before!

> Source/WebCore/platform/graphics/ComplexTextController.h:82
> +	   static Ref<ComplexTextRun> create(hb_buffer_t* buffer, const Font&
font, const UChar* characters, unsigned stringLocation, unsigned stringLength,
unsigned indexBegin, unsigned indexEnd, bool ltr)

This is fine for now, but at some point we should probably make the header file
not know anything about CT* types or hb_* types. We could do this by migrating
to the last create() below. I can probably do this task.

> Source/WebCore/platform/graphics/FontCascade.cpp:1333
> +#if PLATFORM(COCOA) || USE(HARFBUZZ)

Can we change these to !PLATFORM(WIN)? I think Windows is the only one who
doesn't want ComplexTextController.

>
Source/WebCore/platform/graphics/harfbuzz/ComplexTextControllerHarfBuzz.cpp:102
> +    for (auto& feature : font.fontDescription().featureSettings()) {

What about font-variant-* settings?

> Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.h:-47
> -class HarfBuzzShaper {

\o/

>> Source/WebCore/platform/graphics/harfbuzz/HbUniquePtr.h:37
>> +	    RELEASE_ASSERT_NOT_REACHED();
> 
> I have no clue if it would work or not, but you could try
static_assert(false) here?

Why don't you " = delete" the function?


More information about the webkit-reviews mailing list