Bug 228363

Created attachment 434314 [details] WIP patch

Comment on attachment 434314 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=434314&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:293 > + // state.fOverrideDirection = ? We may need to pass this information inside the TextRun? > Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:323 > + if (!shapeByUniscribe(str, length, item, this, glyphs, clusters, visualAttributes)) I suppose we'll need to create a ComplexTextControllerUniscribe.h to include the signature of this function? > Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:353 > + const float cLogicalScale = platformData().useGDI() ? 1 : 32; Is this really correct? > Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:362 > + advance = roundf(advance); > + offsetX = roundf(offsetX); > + offsetY = roundf(offsetY); This is almost certainly not necessary (any more).

Does Uniscribe produce an initial advance? See also: https://bugs.webkit.org/show_bug.cgi?id=227979

Created attachment 434395 [details] WIP patch

Thank you for the feedback. Will check later.

Comment on attachment 434314 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=434314&action=review >> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:353 >> + const float cLogicalScale = platformData().useGDI() ? 1 : 32; > > Is this really correct? The code was added by r28867. It should be correct. However, I want to remove the code Bug 206273 – [Win] Remove obsolete useGDI code path and FontRenderingMode enum >> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:362 >> + offsetY = roundf(offsetY); > > This is almost certainly not necessary (any more). I copied this code from UniscribeController::shapeAndPlaceItem. OK, I will remove the code. Bug 228536 – [Win][Uniscribe] Remove the code rounding off glyph advances and offsets for system fonts

<rdar://problem/81480150>

Created attachment 436142 [details] WIP patch

Comment on attachment 436142 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=436142&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:277 > +GlyphBufferAdvance Font::applyTransforms(GlyphBuffer& glyphBuffer, unsigned beginningGlyphIndex, unsigned beginningStringIndex, unsigned stringLength, bool enableKerning, bool requiresShaping, const AtomString& locale, StringView text, TextDirection textDirection) const This should probably move to a different file. Is there a FontUniscribe.cpp or FontWin.cpp? If not, we should probably make one.

Comment on attachment 436142 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=436142&action=review >> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:277 >> +GlyphBufferAdvance Font::applyTransforms(GlyphBuffer& glyphBuffer, unsigned beginningGlyphIndex, unsigned beginningStringIndex, unsigned stringLength, bool enableKerning, bool requiresShaping, const AtomString& locale, StringView text, TextDirection textDirection) const > > This should probably move to a different file. Is there a FontUniscribe.cpp or FontWin.cpp? If not, we should probably make one. Will we deprecate ComplexTextController soon? If so, it will be easy to move Font::applyTransforms elsewhere after that. Because Font::applyTransforms is a almost copy of ComplexTextController::collectComplexTextRunsForCharacters, I want to put them in a same file. One idea is adding a new function applyTransformsByUniscribe in ComplexTextControllerUniscribe.cpp, moving Font::applyTransforms to FontWin.cpp. > GlyphBufferAdvance Font::applyTransforms(GlyphBuffer& glyphBuffer, unsigned beginningGlyphIndex, unsigned beginningStringIndex, unsigned stringLength, bool enableKerning, bool requiresShaping, const AtomString& locale, StringView text, TextDirection textDirection) const > { > return applyTransformsByUniscribe(glyphBuffer, beginningGlyphIndex, beginningStringIndex, stringLength, enableKerning, requiresShaping, locale, tesxt, textDirection); > } However, this doesn't look a nice idea.

Created attachment 436149 [details] WIP patch This code doesn't work well for selecting a Arabic text. If I select a part of a Arabic text, combining characters are broken apart.

Created attachment 436155 [details] WIP patch This code works for selecting a Arabic text. However, it looks complicated and doesn't look effective code.

This WIP patch is shaping whole 'text' every time, and picking up only glyphs corresponding the text range of (beginningStringIndex, stringLength).

(In reply to Fujii Hironori from comment #10) > Comment on attachment 436142 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=436142&action=review > > >> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:277 > >> +GlyphBufferAdvance Font::applyTransforms(GlyphBuffer& glyphBuffer, unsigned beginningGlyphIndex, unsigned beginningStringIndex, unsigned stringLength, bool enableKerning, bool requiresShaping, const AtomString& locale, StringView text, TextDirection textDirection) const > > > > This should probably move to a different file. Is there a FontUniscribe.cpp or FontWin.cpp? If not, we should probably make one. > > Will we deprecate ComplexTextController soon? No, it will be a while before we deprecate it. It's fine to leave this function here for now.

There are no ports that use Uniscribe any more.

WinCairo is still using Uniscribe now. https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp I have to a plan to use DirectWrite for it to support color fonts (bug#215259). The first milestone is still using Uniscribe for shaping. I'd like to replace Uniscribe with DirectWrite or HarfBuzz DirectWrite backend in the future. Anyway, I think it's not easy to implement Font::applyTransforms for HarfBuzz, DirectWrite and Uniscribe. I think ComplexTextController is better for the backends.