Bug 167956 – [Harfbuzz] Implement ComplexTextController on top of HarfBuzz

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

Now that the shared pieces of ComplexTextController are shared among all the ports, we should implement the required platform-specific part on top of HarfBuzz. Once this is done, we can remove HarfBuzzController.

[Carlos Garcia Campos]

Created attachment 321956 [details]
WIP patch

This is a WIP patch. I'm getting several tests failures, many of them look like they could be just rebaselined, but a few others look like real failures (there are progressions too). I'll investigate the failures, but in the meantime I would appreciate review/feedback of that I have so far, since I'm not a text expert.

[Carlos Garcia Campos]

Btw, some of the failures only fail when run with WTR, and work with MiniBrowser or using WEBKIT_SKIP_WEBKITTESTRUNNER_FONTCONFIG_INITIALIZATION, but I have no idea why. I guess it's because of the fontconfig configuration file we use for the tests, but I don't understand what that config does.

[Myles C. Maxfield]

Thanks for working on this! I’m very excited to see it when we’re finished 😊

[Carlos Garcia Campos]

Created attachment 322296 [details]
Patch

[Build Bot]

Attachment 322296 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ComplexTextController.h:40:  hb_buffer_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

For all the places where we now have PLATFORM(COCOA) || USE(HARFBUZZ), is there more explicit predicate we could use? HAVE(...) for instance?

[Carlos Garcia Campos]

(In reply to Sam Weinig from comment #6)
> For all the places where we now have PLATFORM(COCOA) || USE(HARFBUZZ), is
> there more explicit predicate we could use? HAVE(...) for instance?

I don't think so, those cases are the implementations of ComplexTextController. After this change there are 2 implementations ComplexTextControllerCoreText.mm and ComplexTextControllerHarfBuzz.cpp, only ComplexTextControllerDirectWrite.cpp would be missing. So, we could use something like if !PLATFORM(WIN) maybe, but that would be more confusing, I think.

[Michael Catanzaro]

Comment on attachment 322296 [details]
Patch

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

> 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?

[Michael Catanzaro]

Does this fix bug #146270?

[Sam Weinig]

(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Sam Weinig from comment #6)
> > For all the places where we now have PLATFORM(COCOA) || USE(HARFBUZZ), is
> > there more explicit predicate we could use? HAVE(...) for instance?
> 
> I don't think so, those cases are the implementations of
> ComplexTextController. After this change there are 2 implementations
> ComplexTextControllerCoreText.mm and ComplexTextControllerHarfBuzz.cpp, only
> ComplexTextControllerDirectWrite.cpp would be missing. So, we could use
> something like if !PLATFORM(WIN) maybe, but that would be more confusing, I
> think.

So it USE(COMPLEX_TEXT_CONTROLLER) + NOT WINDOWS. Any idea what makes the DirectWrite implementation different that it shouldn't use these functions?

[Carlos Garcia Campos]

(In reply to Sam Weinig from comment #10)
> (In reply to Carlos Garcia Campos from comment #7)
> > (In reply to Sam Weinig from comment #6)
> > > For all the places where we now have PLATFORM(COCOA) || USE(HARFBUZZ), is
> > > there more explicit predicate we could use? HAVE(...) for instance?
> > 
> > I don't think so, those cases are the implementations of
> > ComplexTextController. After this change there are 2 implementations
> > ComplexTextControllerCoreText.mm and ComplexTextControllerHarfBuzz.cpp, only
> > ComplexTextControllerDirectWrite.cpp would be missing. So, we could use
> > something like if !PLATFORM(WIN) maybe, but that would be more confusing, I
> > think.
> 
> So it USE(COMPLEX_TEXT_CONTROLLER) + NOT WINDOWS. Any idea what makes the
> DirectWrite implementation different that it shouldn't use these functions?

There's a WIP patch in bug #167954

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> Does this fix bug #146270?

I guess, the shaper is removed. Is there any visual difference? or just the log complaining about the shaper failure?

[Sam Weinig]

(In reply to Carlos Garcia Campos from comment #11)
> (In reply to Sam Weinig from comment #10)
> > (In reply to Carlos Garcia Campos from comment #7)
> > > (In reply to Sam Weinig from comment #6)
> > > > For all the places where we now have PLATFORM(COCOA) || USE(HARFBUZZ), is
> > > > there more explicit predicate we could use? HAVE(...) for instance?
> > > 
> > > I don't think so, those cases are the implementations of
> > > ComplexTextController. After this change there are 2 implementations
> > > ComplexTextControllerCoreText.mm and ComplexTextControllerHarfBuzz.cpp, only
> > > ComplexTextControllerDirectWrite.cpp would be missing. So, we could use
> > > something like if !PLATFORM(WIN) maybe, but that would be more confusing, I
> > > think.
> > 
> > So it USE(COMPLEX_TEXT_CONTROLLER) + NOT WINDOWS. Any idea what makes the
> > DirectWrite implementation different that it shouldn't use these functions?
> 
> There's a WIP patch in bug #167954

Ok.

[Myles C. Maxfield]

Comment on attachment 322296 [details]
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?

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #14)
> > 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.
> 

For now! Eventually everyone will use it :D

[Carlos Garcia Campos]

(In reply to Myles C. Maxfield from comment #14)
> Comment on attachment 322296 [details]
> 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!

Indeed, and hopefully less buggy too :-)

> > 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.

Ok, let me know if I can help.

> > 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?

I don't know I copied what we had, if there's anything missing it can be added in a separate patch, I don't even know what font-variant-* settings are :-P

> > 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?

Good question :-)

[Carlos Garcia Campos]

Created attachment 322656 [details]
Patch for landing

Including the GTK rebaseline.

[Build Bot]

Attachment 322656 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/ComplexTextController.h:40:  hb_buffer_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r222843: <http://trac.webkit.org/changeset/222843>

[Radar WebKit Bug Importer]

<rdar://problem/34810917>