Bug 204884 – [Win] Use ComplexTextController instead of UniscribeController

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

Created attachment 384895 [details]
WIP patch

[Win] Use ComplexTextController instead of UniscribeController

[Fujii Hironori]

Created attachment 385483 [details]
WIP patch

[Fujii Hironori]

Created attachment 385485 [details]
WIP patch

[Fujii Hironori]

Created attachment 385582 [details]
WIP patch

[Fujii Hironori]

Created attachment 385587 [details]
WIP patch

[Fujii Hironori]

Created attachment 385746 [details]
WIP patch

[Fujii Hironori]

Created attachment 385870 [details]
WIP patch

[Fujii Hironori]

Created attachment 385932 [details]
WIP patch

[Fujii Hironori]

Created attachment 385944 [details]
WIP patch

[Fujii Hironori]

Created attachment 386175 [details]
WIP patch

[Fujii Hironori]

I checked the seven failing tests in AppleWin EWS.

fast/text/arabic-zwj-and-zwnj.html
  The arabic text hasn't been shown properly with Times font even before this change.
  I will mark it as Failure.
fast/text/atsui-rtl-override-selection.html
  New bug. Filed an Bug 205486.
fast/text/emoji-with-joiner.html
  Emoji isn't supported. It was false negative. I will mark it as Failure.
fast/text/justify-ideograph-complex.html
  Needs rebaseline.
fast/text/selection-in-initial-advance-region.html
  New bug. Filed in Bug 205487.
fast/text/stale-TextLayout-from-first-line.html
  New bug. Filed in Bug 205485.
imported/blink/editing/selection/offset-from-point-complex-scripts.html
  New bug. Filed an Bug 205486.

[Fujii Hironori]

Created attachment 386184 [details]
Patch

[Fujii Hironori]

Created attachment 386944 [details]
Patch

[Fujii Hironori]

Created attachment 387087 [details]
Patch

[Fujii Hironori]

Can anyone review this patch? Does AppleWin developer object to this change?

[Per Arne Vollan]

Comment on attachment 387087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387087&action=review

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:41
> +    HWndDC hdc;

Should this be initialized?

[Fujii Hironori]

Comment on attachment 387087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387087&action=review

>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:41
>> +    HWndDC hdc;
> 
> Should this be initialized?

I don't think so. HWndDC has ctor.
And, I didn't change the code.

[Fujii Hironori]

Comment on attachment 387087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387087&action=review

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:41
>>> +    HWndDC hdc;
>> 
>> Should this be initialized?
> 
> I don't think so. HWndDC has ctor.
> And, I didn't change the code.

hdc was 0 since the beginning.

https://trac.webkit.org/changeset/23128/webkit
https://trac.webkit.org/browser/webkit/branches/WindowsMerge/WebCore/platform/win/UniscribeController.cpp?rev=23128#L183

[Fujii Hironori]

I found another existing ComplexTextController bug. I'm going to fix it in Bug 205990.

[Brent Fulgham]

Comment on attachment 387087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387087&action=review

Looks good, though I wish we could get rid of all of the GDI-based code.

>>>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:41
>>>> +    HWndDC hdc;
>>> 
>>> Should this be initialized?
>> 
>> I don't think so. HWndDC has ctor.
>> And, I didn't change the code.
> 
> hdc was 0 since the beginning.
> 
> https://trac.webkit.org/changeset/23128/webkit
> https://trac.webkit.org/browser/webkit/branches/WindowsMerge/WebCore/platform/win/UniscribeController.cpp?rev=23128#L183

I sure wish we could just adopt DirectWrite, rather than creating a Uniscribe implementation. :-(

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:42
> +    HFONT oldFont = 0;

nullptr?

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:56
> +            hdc.setHWnd(0);

nullptr?

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:58
> +            oldFont = (HFONT)SelectObject(hdc, hfont);

This should be a C++ cast, probably reinterpret_cast

[Fujii Hironori]

Comment on attachment 387087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387087&action=review

Thank you for the review. Will fix.

>>>>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:41
>>>>> +    HWndDC hdc;
>>>> 
>>>> Should this be initialized?
>>> 
>>> I don't think so. HWndDC has ctor.
>>> And, I didn't change the code.
>> 
>> hdc was 0 since the beginning.
>> 
>> https://trac.webkit.org/changeset/23128/webkit
>> https://trac.webkit.org/browser/webkit/branches/WindowsMerge/WebCore/platform/win/UniscribeController.cpp?rev=23128#L183
> 
> I sure wish we could just adopt DirectWrite, rather than creating a Uniscribe implementation. :-(

Yeah, WinCairo wants colorful emoji.
But, it's the easiest way to fix some complex text bugs at the moment to use ComplexTextController.

[Fujii Hironori]

Created attachment 387303 [details]
Patch for landing

[Fujii Hironori]

Comment on attachment 387303 [details]
Patch for landing

Clearing flags on attachment: 387303

Committed r254323: <https://trac.webkit.org/changeset/254323>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58468325>

[Fujii Hironori]

I found one more bug.
Bug 206058 – [Win] elements of Arabic texts are over-wrapping since r254323

[Myles C. Maxfield]

Comment on attachment 387303 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=387303&action=review

This patch is surprisingly simple. I thought using Uniscribe was more complicated than this...

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:99
> +        if (hr != E_OUTOFMEMORY) {
> +            ASSERT(SUCCEEDED(hr));

This ASSERT seems bogus.

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:162
> +            const float cLogicalScale = font->platformData().useGDI() ? 1 : 32;

I know you didn't write this originally, but I'd appreciate a link to the docs where these numbers come from.

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:172
> +            // Match AppKit's rules for the integer vs. non-integer rendering modes.
> +            if (!font->platformData().isSystemFont()) {
> +                advance = roundf(advance);
> +                offsetX = roundf(offsetX);
> +                offsetY = roundf(offsetY);
> +            }

Is this really needed?

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:176
> +            // FIXME: We need to take the GOFFSETS for combining glyphs and store them in the glyph buffer
> +            // as well, so that when the time comes to draw those glyphs, we can apply the appropriate
> +            // translation.

I'm not sure this comment is still valid.

> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:183
> +        m_complexTextRuns.append(ComplexTextRun::create(baseAdvances, origins, glyphs, stringIndices, initialAdvance, *font, cp, stringLocation, stringLength, item.iCharPos, items[i+1].iCharPos, ltr));

Cocoa should be using this constructor, too, so that we can keep CoreText stuff out of ComplexTextController.h

[Fujii Hironori]

Comment on attachment 387303 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=387303&action=review

>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:99
>> +            ASSERT(SUCCEEDED(hr));
> 
> This ASSERT seems bogus.

According to the spec, ScriptItemize can return E_INVALIDARG.

ScriptItemize function | Microsoft Docs
https://docs.microsoft.com/en-us/windows/desktop/api/usp10/nf-usp10-scriptitemize

> The function returns E_INVALIDARG if pwcInChars is set to NULL, cInChars is 0, pItems is set to NULL, or cMaxItems < 2.

So, I added the assert ensuring the item buffer has enough size.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:162
>> +            const float cLogicalScale = font->platformData().useGDI() ? 1 : 32;
> 
> I know you didn't write this originally, but I'd appreciate a link to the docs where these numbers come from.

The code was added by r28867.
It draws texts using ExtTextOut with ETO_GLYPH_INDEX for useGDI code path.
  
But, r126666 removes drawGDIGlyphs, the most important part of useGDI code path.

I will remove remaining code for useGDI.

>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:172
>> +            }
> 
> Is this really needed?

This code was added by r23128 and r23199.
I don't know why.
Should I remove the code?

>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:176
>> +            // translation.
> 
> I'm not sure this comment is still valid.

This comment was added by r23154.
r23199 seems fixed the issue. I will remove the comment.

[Fujii Hironori]

(In reply to Myles C. Maxfield from comment #26)
> 
> This patch is surprisingly simple. I thought using Uniscribe was more
> complicated than this...

Because I split a complicated part into Bug 205485 😅

[Fujii Hironori]

Committed r254729: <https://trac.webkit.org/changeset/254729>

[Fujii Hironori]

Comment on attachment 387303 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=387303&action=review

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:162
>>> +            const float cLogicalScale = font->platformData().useGDI() ? 1 : 32;
>> 
>> I know you didn't write this originally, but I'd appreciate a link to the docs where these numbers come from.
> 
> The code was added by r28867.
> It draws texts using ExtTextOut with ETO_GLYPH_INDEX for useGDI code path.
>   
> But, r126666 removes drawGDIGlyphs, the most important part of useGDI code path.
> 
> I will remove remaining code for useGDI.

Filed Bug 206273.

>>> Source/WebCore/platform/graphics/win/ComplexTextControllerUniscribe.cpp:176
>>> +            // translation.
>> 
>> I'm not sure this comment is still valid.
> 
> This comment was added by r23154.
> r23199 seems fixed the issue. I will remove the comment.

Landed r254729.