Bug 204884 - [Win] Use ComplexTextController instead of UniscribeController
Summary: [Win] Use ComplexTextController instead of UniscribeController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks: 167566 172565
  Show dependency treegraph
 
Reported: 2019-12-05 03:31 PST by Fujii Hironori
Modified: 2020-01-22 22:49 PST (History)
13 users (show)

See Also:


Attachments
WIP patch (47.02 KB, patch)
2019-12-05 03:31 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (46.38 KB, patch)
2019-12-12 00:28 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (46.39 KB, patch)
2019-12-12 01:06 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (51.16 KB, patch)
2019-12-12 23:11 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (45.10 KB, patch)
2019-12-13 01:58 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (44.36 KB, patch)
2019-12-16 02:46 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (43.80 KB, patch)
2019-12-17 02:18 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (45.16 KB, patch)
2019-12-17 17:59 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (42.91 KB, patch)
2019-12-17 23:45 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
WIP patch (42.97 KB, patch)
2019-12-19 20:04 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (55.26 KB, patch)
2019-12-19 23:12 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (54.90 KB, patch)
2020-01-07 00:05 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (55.10 KB, patch)
2020-01-08 02:03 PST, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch for landing (55.06 KB, patch)
2020-01-09 18:28 PST, 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 2019-12-05 03:31:30 PST
Created attachment 384895 [details]
WIP patch

[Win] Use ComplexTextController instead of UniscribeController
Comment 1 Fujii Hironori 2019-12-12 00:28:50 PST
Created attachment 385483 [details]
WIP patch
Comment 2 Fujii Hironori 2019-12-12 01:06:27 PST
Created attachment 385485 [details]
WIP patch
Comment 3 Fujii Hironori 2019-12-12 23:11:26 PST
Created attachment 385582 [details]
WIP patch
Comment 4 Fujii Hironori 2019-12-13 01:58:15 PST
Created attachment 385587 [details]
WIP patch
Comment 5 Fujii Hironori 2019-12-16 02:46:22 PST
Created attachment 385746 [details]
WIP patch
Comment 6 Fujii Hironori 2019-12-17 02:18:08 PST
Created attachment 385870 [details]
WIP patch
Comment 7 Fujii Hironori 2019-12-17 17:59:53 PST
Created attachment 385932 [details]
WIP patch
Comment 8 Fujii Hironori 2019-12-17 23:45:46 PST
Created attachment 385944 [details]
WIP patch
Comment 9 Fujii Hironori 2019-12-19 20:04:30 PST
Created attachment 386175 [details]
WIP patch
Comment 10 Fujii Hironori 2019-12-19 20:50:21 PST
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.
Comment 11 Fujii Hironori 2019-12-19 23:12:16 PST
Created attachment 386184 [details]
Patch
Comment 12 Fujii Hironori 2020-01-07 00:05:30 PST
Created attachment 386944 [details]
Patch
Comment 13 Fujii Hironori 2020-01-08 02:03:06 PST
Created attachment 387087 [details]
Patch
Comment 14 Fujii Hironori 2020-01-08 03:02:08 PST
Can anyone review this patch? Does AppleWin developer object to this change?
Comment 15 Per Arne Vollan 2020-01-08 08:23:14 PST
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?
Comment 16 Fujii Hironori 2020-01-08 17:46:16 PST
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.
Comment 17 Fujii Hironori 2020-01-08 17:56:56 PST
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
Comment 18 Fujii Hironori 2020-01-08 23:53:26 PST
I found another existing ComplexTextController bug. I'm going to fix it in Bug 205990.
Comment 19 Brent Fulgham 2020-01-09 11:31:40 PST
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
Comment 20 Fujii Hironori 2020-01-09 18:01:28 PST
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.
Comment 21 Fujii Hironori 2020-01-09 18:28:19 PST
Created attachment 387303 [details]
Patch for landing
Comment 22 Fujii Hironori 2020-01-09 19:52:50 PST
Comment on attachment 387303 [details]
Patch for landing

Clearing flags on attachment: 387303

Committed r254323: <https://trac.webkit.org/changeset/254323>
Comment 23 Fujii Hironori 2020-01-09 19:52:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2020-01-09 19:53:21 PST
<rdar://problem/58468325>
Comment 25 Fujii Hironori 2020-01-10 03:39:48 PST
I found one more bug.
Bug 206058 – [Win] elements of Arabic texts are over-wrapping since r254323
Comment 26 Myles C. Maxfield 2020-01-13 19:44:19 PST
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
Comment 27 Fujii Hironori 2020-01-14 01:26:58 PST
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.
Comment 28 Fujii Hironori 2020-01-14 07:09:39 PST
(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 😅
Comment 29 Fujii Hironori 2020-01-16 17:25:24 PST
Committed r254729: <https://trac.webkit.org/changeset/254729>
Comment 30 Fujii Hironori 2020-01-16 17:26:49 PST
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.