Created attachment 385483 [details] WIP patch

Created attachment 385485 [details] WIP patch

Created attachment 385582 [details] WIP patch

Created attachment 385587 [details] WIP patch

Created attachment 385746 [details] WIP patch

Created attachment 385870 [details] WIP patch

Created attachment 385932 [details] WIP patch

Created attachment 385944 [details] WIP patch

Created attachment 386175 [details] WIP patch

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.

Created attachment 386184 [details] Patch

Created attachment 386944 [details] Patch

Created attachment 387087 [details] Patch

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

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

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

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

Created attachment 387303 [details] Patch for landing

Comment on attachment 387303 [details] Patch for landing Clearing flags on attachment: 387303 Committed r254323: <https://trac.webkit.org/changeset/254323>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58468325>

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

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

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

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

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.