Bug 167956

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.

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.

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

Created attachment 322296 [details] Patch

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.

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

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

Does this fix bug #146270?

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

(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

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

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

(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

(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 :-)

Created attachment 322656 [details] Patch for landing Including the GTK rebaseline.

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.

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

<rdar://problem/34810917>