Bug 228363 - [Win][Uniscribe] Implement Font::applyTransforms
Summary: [Win][Uniscribe] Implement Font::applyTransforms
Status: NEW
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: 228536 229421
Blocks: 206208
  Show dependency treegraph
 
Reported: 2021-07-27 14:19 PDT by Fujii Hironori
Modified: 2021-09-01 18:47 PDT (History)
2 users (show)

See Also:


Attachments
WIP patch (5.45 KB, patch)
2021-07-27 14:19 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (8.33 KB, patch)
2021-07-27 19:52 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (10.54 KB, patch)
2021-08-22 17:37 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (10.95 KB, patch)
2021-08-22 22:36 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (11.17 KB, patch)
2021-08-23 00:24 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2021-07-27 14:19:26 PDT
[Win][Uniscribe] Implement Font::applyTransforms

See also:
  Bug 206208 – [Cocoa] Delete the complex text codepath 😮
Comment 1 Fujii Hironori 2021-07-27 14:19:46 PDT
Created attachment 434314 [details]
WIP patch
Comment 2 Myles C. Maxfield 2021-07-27 17:17:54 PDT
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).
Comment 3 Myles C. Maxfield 2021-07-27 17:18:59 PDT
Does Uniscribe produce an initial advance? See also: https://bugs.webkit.org/show_bug.cgi?id=227979
Comment 4 Fujii Hironori 2021-07-27 19:52:16 PDT
Created attachment 434395 [details]
WIP patch
Comment 5 Fujii Hironori 2021-07-27 19:52:46 PDT
Thank you for the feedback. Will check later.
Comment 6 Fujii Hironori 2021-07-27 23:58:25 PDT
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
Comment 7 Radar WebKit Bug Importer 2021-08-03 14:20:53 PDT
<rdar://problem/81480150>
Comment 8 Fujii Hironori 2021-08-22 17:37:55 PDT
Created attachment 436142 [details]
WIP patch
Comment 9 Myles C. Maxfield 2021-08-22 17:42:57 PDT
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 10 Fujii Hironori 2021-08-22 18:28:09 PDT
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.
Comment 11 Fujii Hironori 2021-08-22 22:36:10 PDT
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.
Comment 12 Fujii Hironori 2021-08-23 00:24:01 PDT
Created attachment 436155 [details]
WIP patch

This code works for selecting a Arabic text.
However, it looks complicated and doesn't look effective code.
Comment 13 Fujii Hironori 2021-08-23 00:28:44 PDT
This WIP patch is shaping whole 'text' every time, and picking up only glyphs corresponding the text range of (beginningStringIndex, stringLength).
Comment 14 Myles C. Maxfield 2021-08-23 01:46:45 PDT
(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.