Bug 228363 – [Win][Uniscribe] Implement Font::applyTransforms

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Text

[Fujii Hironori]

[Win][Uniscribe] Implement Font::applyTransforms

See also:
  Bug 206208 – [Cocoa] Delete the complex text codepath 😮

[Fujii Hironori]

Created attachment 434314 [details]
WIP patch

[Myles C. Maxfield]

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

[Myles C. Maxfield]

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

[Fujii Hironori]

Created attachment 434395 [details]
WIP patch

[Fujii Hironori]

Thank you for the feedback. Will check later.

[Fujii Hironori]

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

[Radar WebKit Bug Importer]

<rdar://problem/81480150>

[Fujii Hironori]

Created attachment 436142 [details]
WIP patch

[Myles C. Maxfield]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

Created attachment 436155 [details]
WIP patch

This code works for selecting a Arabic text.
However, it looks complicated and doesn't look effective code.

[Fujii Hironori]

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

[Myles C. Maxfield]

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

[Myles C. Maxfield]

There are no ports that use Uniscribe any more.

[Fujii Hironori]

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.