Bug 167956 - [Harfbuzz] Implement ComplexTextController on top of HarfBuzz
Summary: [Harfbuzz] Implement ComplexTextController on top of HarfBuzz
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 167566 177921
  Show dependency treegraph
 
Reported: 2017-02-07 13:42 PST by Myles C. Maxfield
Modified: 2017-10-05 00:33 PDT (History)
7 users (show)

See Also:


Attachments
WIP patch (68.40 KB, patch)
2017-09-27 06:47 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (71.06 KB, patch)
2017-09-30 07:15 PDT, Carlos Garcia Campos
mmaxfield: review+
Details | Formatted Diff | Diff
Patch for landing (3.48 MB, patch)
2017-10-04 04:54 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-02-07 13:42:58 PST
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.
Comment 1 Carlos Garcia Campos 2017-09-27 06:47:04 PDT
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.
Comment 2 Carlos Garcia Campos 2017-09-27 06:50:21 PDT
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.
Comment 3 Myles C. Maxfield 2017-09-27 08:09:08 PDT
Thanks for working on this! I’m very excited to see it when we’re finished 😊
Comment 4 Carlos Garcia Campos 2017-09-30 07:15:21 PDT
Created attachment 322296 [details]
Patch
Comment 5 Build Bot 2017-09-30 07:16:38 PDT
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.
Comment 6 Sam Weinig 2017-09-30 17:36:06 PDT
For all the places where we now have PLATFORM(COCOA) || USE(HARFBUZZ), is there more explicit predicate we could use? HAVE(...) for instance?
Comment 7 Carlos Garcia Campos 2017-10-01 01:17:33 PDT
(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.
Comment 8 Michael Catanzaro 2017-10-01 03:52:29 PDT
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?
Comment 9 Michael Catanzaro 2017-10-01 11:10:50 PDT
Does this fix bug #146270?
Comment 10 Sam Weinig 2017-10-01 12:36:28 PDT
(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?
Comment 11 Carlos Garcia Campos 2017-10-02 01:25:26 PDT
(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
Comment 12 Carlos Garcia Campos 2017-10-02 01:29:29 PDT
(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?
Comment 13 Sam Weinig 2017-10-02 09:27:17 PDT
(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.
Comment 14 Myles C. Maxfield 2017-10-03 11:56:54 PDT
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?
Comment 15 Myles C. Maxfield 2017-10-03 11:57:52 PDT
(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
Comment 16 Carlos Garcia Campos 2017-10-04 03:29:30 PDT
(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 :-)
Comment 17 Carlos Garcia Campos 2017-10-04 04:54:51 PDT
Created attachment 322656 [details]
Patch for landing

Including the GTK rebaseline.
Comment 18 Build Bot 2017-10-04 04:58:04 PDT
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.
Comment 19 Carlos Garcia Campos 2017-10-04 06:52:00 PDT
Committed r222843: <http://trac.webkit.org/changeset/222843>
Comment 20 Radar WebKit Bug Importer 2017-10-04 06:53:02 PDT
<rdar://problem/34810917>