This patch is the first step in switching away from the GDI-based Uniscribe controller to DirectWrite. This provides sufficient features to support web fonts handling and rendering text. Subsequent patches will build out full support for the font layout tests.
<rdar://problem/56695130>
Created attachment 382153 [details] Patch
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review > Source/WebCore/platform/graphics/Font.cpp:130 > + m_zeroWidthSpaceGlyph = zeroWidthSpaceGlyph; You don't need to restore m_zeroWidthSpaceGlyph because there is a hack to clear m_zeroWidthSpaceGlyph if m_zeroWidthSpaceGlyph == m_spaceGlyph. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/Font.cpp?rev=248846#L133 I think these logic should be reordered to be more comprehensible: if (glyphPageSpace) m_spaceGlyph = glyphPageSpace->glyphDataForCharacter(space).glyph; if (glyphPageZeroWidthSpace) m_zeroWidthSpaceGlyph = glyphPageZeroWidthSpace->glyphDataForCharacter(zeroWidthSpaceCharacter).glyph; if (m_zeroWidthSpaceGlyph == m_spaceGlyph) m_zeroWidthSpaceGlyph = 0; float width = widthForGlyph(m_spaceGlyph); m_spaceWidth = width; > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80 > + COMPtr<IDWriteTextFormat> textFormat; Where is 'textFormat' used?
Can you implement Direct2D integration without using HFONT and LOGFONT at all?
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:76 > + int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH); GetUserDefaultLocaleName. Should it be "user default locale" rather than the locale of the text? > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:78 > + localeName[localeLength] = 0; According to the spec, you don't need to zero-terminate explicitly. https://docs.microsoft.com/en-us/windows/win32/api/winnls/nf-winnls-getuserdefaultlocalename wchar_t localeName[LOCALE_NAME_MAX_LENGTH]; int localeLength = ::GetUserDefaultLocaleName(localeName, LOCALE_NAME_MAX_LENGTH);
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review > Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:53 > + int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH); I believe this unnecessary '::' prefix doesn't conform to WebKit Code Style Guidelines even though it doesn't disallow it explicitly because nobody prepend a unnecessary :: prefix to other global names, WTF, WebCore, WCHAR, UChar. https://webkit.org/code-style-guideline/
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review > Source/WebCore/platform/graphics/FontPlatformData.h:62 > +#include <wtf/text/StringImpl.h> Do you need to include StringImpl.h here?
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review > Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:80 > + hr = analyzer->GetGlyphs(text, run.length, fontPlatformData.dwFontFace(), fontPlatformData.orientation() == FontOrientation::Vertical, false, Should the first argument be text + run.startPosition? > Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:87 > + text += returnedCount; Is this right? I think returnedCount is the number of glyphs, not characters. Spec says "Note that the mapping from characters to glyphs is, in general, many-to-many. " https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review > Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:46 > + auto platformFontPair = DirectWrite::createWithPlatformFont(logfont); you want to use C++17 here? auto [font, collection] = DirectWrite::createWithPlatformFont(logfont);
Comment on attachment 382153 [details] Patch win and wincairo EWS can't compile.
Comment on attachment 382153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382153&action=review >> Source/WebCore/platform/graphics/Font.cpp:130 >> + m_zeroWidthSpaceGlyph = zeroWidthSpaceGlyph; > > You don't need to restore m_zeroWidthSpaceGlyph because there is a hack to clear m_zeroWidthSpaceGlyph if m_zeroWidthSpaceGlyph == m_spaceGlyph. > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/Font.cpp?rev=248846#L133 > > I think these logic should be reordered to be more comprehensible: > > if (glyphPageSpace) > m_spaceGlyph = glyphPageSpace->glyphDataForCharacter(space).glyph; > if (glyphPageZeroWidthSpace) > m_zeroWidthSpaceGlyph = glyphPageZeroWidthSpace->glyphDataForCharacter(zeroWidthSpaceCharacter).glyph; > if (m_zeroWidthSpaceGlyph == m_spaceGlyph) > m_zeroWidthSpaceGlyph = 0; > float width = widthForGlyph(m_spaceGlyph); > m_spaceWidth = width; I was afraid of modifying the code too much, but this seems reasonable. I'll revise the patch. >> Source/WebCore/platform/graphics/FontPlatformData.h:62 >> +#include <wtf/text/StringImpl.h> > > Do you need to include StringImpl.h here? No -- I'll remove it. >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:76 >> + int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH); > > GetUserDefaultLocaleName. Should it be "user default locale" rather than the locale of the text? I'm not sure. I don't see a text-specific locale in the call stack here, or on our other ports. Is there a way to get this information? >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80 >> + COMPtr<IDWriteTextFormat> textFormat; > > Where is 'textFormat' used? In an older version of the patch. This is no longer needed -- I will remove it. >> Source/WebCore/platform/graphics/win/FontPlatformDataDirect2D.cpp:46 >> + auto platformFontPair = DirectWrite::createWithPlatformFont(logfont); > > you want to use C++17 here? > auto [font, collection] = DirectWrite::createWithPlatformFont(logfont); Oh! Good idea. >> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:53 >> + int localeLength = ::GetUserDefaultLocaleName(reinterpret_cast<LPWSTR>(&localeName), LOCALE_NAME_MAX_LENGTH); > > I believe this unnecessary '::' prefix doesn't conform to WebKit Code Style Guidelines even though it doesn't disallow it explicitly because nobody prepend a unnecessary :: prefix to other global names, WTF, WebCore, WCHAR, UChar. > https://webkit.org/code-style-guideline/ Okay. We used to do this for MS API calls, but I guess it's not really done anymore. >> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:80 >> + hr = analyzer->GetGlyphs(text, run.length, fontPlatformData.dwFontFace(), fontPlatformData.orientation() == FontOrientation::Vertical, false, > > Should the first argument be text + run.startPosition? Yes! >> Source/WebCore/platform/graphics/win/GlyphPageTreeNodeDirect2D.cpp:87 >> + text += returnedCount; > > Is this right? I think returnedCount is the number of glyphs, not characters. Spec says "Note that the mapping from characters to glyphs is, in general, many-to-many. " https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs I think you are right. The other glyph-related counts are correct (even 'textPropertiesData', which seems to be per-glyph) but the text position is not. Your suggestion above fixes that problem.
(In reply to Fujii Hironori from comment #4) > Can you implement Direct2D integration without using HFONT and LOGFONT at > all? Yes, but not quite yet. There are still a number of code paths that expect HFONT/LOGFONT access. I intend to remove them in a subsequent patch once the initial logic is in place.
Created attachment 382204 [details] Patch v2 Update with Fujii's suggestions.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review > Source/WebCore/platform/graphics/ComplexTextController.cpp:154 > +#if PLATFORM(WIN) && !USE(DIRECT2D) This line can be removed because this is in #else of line #46 condition. > Source/WebCore/platform/graphics/FontPlatformData.h:242 > + COMPtr<IDWriteFontCollection1> m_dwFontCollection; You added 'm_dwFontCollection', but not used in this patch. Remove it until it is really needed? > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80 > + localeName[localeLength] = 0; I think this line is not needed. See Comment 5.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:115 > + const UChar* currentCP = cp + run.startPosition; This variable 'currentCP' hides outer 'currentCP'. Either should be renamed.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review >> Source/WebCore/platform/graphics/ComplexTextController.cpp:154 >> +#if PLATFORM(WIN) && !USE(DIRECT2D) > > This line can be removed because this is in #else of line #46 condition. Done. >> Source/WebCore/platform/graphics/FontPlatformData.h:242 >> + COMPtr<IDWriteFontCollection1> m_dwFontCollection; > > You added 'm_dwFontCollection', but not used in this patch. Remove it until it is really needed? For that matter, I don't really use faceName either, so I should remove both. >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:80 >> + localeName[localeLength] = 0; > > I think this line is not needed. See Comment 5. Fixed.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113 > + Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount); These Vector is allocating and deallocating for every runs because you put them in this loop. I think these Vector should put outside of the loop. Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16).
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113 >> + Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount); > > These Vector is allocating and deallocating for every runs because you put them in this loop. > I think these Vector should put outside of the loop. > Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16). Ah, you call shrink in shape. I think these vectors should not shrink for every runs.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148 > + glyphs[clusterMap[k]] = font->spaceGlyph(); clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:151 > + spaceCharacters[clusterMap[k]] = m_currentCharacter + k + run.startPosition; Is 'spaceCharacters' used anywhere?
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review >>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113 >>> + Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount); >> >> These Vector is allocating and deallocating for every runs because you put them in this loop. >> I think these Vector should put outside of the loop. >> Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16). > > Ah, you call shrink in shape. I think these vectors should not shrink for every runs. I'm not sure. The length of each text run varies at each step of the loop, so these buffers need adjust size too match or else the call to 'ComplexTextRun::create' will fail because it uses the 'size' of the glyphs vector inside its constructor. I suppose I could adjust ONLY the glyphs vector, but I was trying to be consistent. >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:115 >> + const UChar* currentCP = cp + run.startPosition; > > This variable 'currentCP' hides outer 'currentCP'. Either should be renamed. Only the inner one is needed. And the old 'textStart' variable is unnecessary, too. Fixed. >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148 >> + glyphs[clusterMap[k]] = font->spaceGlyph(); > > clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index. This comes form the existing Uniscribe logic, so I believe it is correct. The documentation in https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs says that clusterMap is a mapping from character positions to glyphs, so I think that is correct. > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:166 > + currentCP += run.length; And this isn't needed anymore.
Created attachment 382269 [details] Patch
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review >>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148 >>> + glyphs[clusterMap[k]] = font->spaceGlyph(); >> >> clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index. > > This comes form the existing Uniscribe logic, so I believe it is correct. The documentation in https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs says that clusterMap is a mapping from character positions to glyphs, so I think that is correct. Oh. the length of clusterMap should be the length of text.
Did you see comment 20?
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review >>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:148 >>>> + glyphs[clusterMap[k]] = font->spaceGlyph(); >>> >>> clusterMap[k] looks wrong. clusterMap takes glyph index, but k is a char index. >> >> This comes form the existing Uniscribe logic, so I believe it is correct. The documentation in https://docs.microsoft.com/en-us/windows/win32/api/dwrite/nf-dwrite-idwritetextanalyzer-getglyphs says that clusterMap is a mapping from character positions to glyphs, so I think that is correct. > > Oh. the length of clusterMap should be the length of text. Ah! Good point. I will fix. >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:151 >> + spaceCharacters[clusterMap[k]] = m_currentCharacter + k + run.startPosition; > > Is 'spaceCharacters' used anywhere? No. I have removed it in my local build and will upload a new patch. This was from the UniscribeController, so I might need to bring it back if I learn that it is needed once I complete other work on DirectWrite support.
Created attachment 382282 [details] Patch
Comment on attachment 382282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:71 > + wchar_t localeName[LOCALE_NAME_MAX_LENGTH + 1] = { }; Remove '+ 1'. > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:118 > + stringIndices.reserveCapacity(totalSuggestedCount); You resize these Vectors for every runs. I think you don't need to put these outside of the loop. And, you don't need 'totalSuggestedCount'.
Comment on attachment 382204 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=382204&action=review >>>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:113 >>>> + Vector<DWRITE_SHAPING_GLYPH_PROPERTIES> glyphProperties(suggestedCount); >>> >>> These Vector is allocating and deallocating for every runs because you put them in this loop. >>> I think these Vector should put outside of the loop. >>> Or, if you want to put them in the loop, suggestedCount should be (3 * run.length / 2 + 16) instead of (3 * length / 2 + 16). >> >> Ah, you call shrink in shape. I think these vectors should not shrink for every runs. > > I'm not sure. The length of each text run varies at each step of the loop, so these buffers need adjust size too match or else the call to 'ComplexTextRun::create' will fail because it uses the 'size' of the glyphs vector inside its constructor. > > I suppose I could adjust ONLY the glyphs vector, but I was trying to be consistent. Sorry. you are right. My comment 17 is wrong. Those Vectors can not be reused.
Comment on attachment 382282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:71 >> + wchar_t localeName[LOCALE_NAME_MAX_LENGTH + 1] = { }; > > Remove '+ 1'. Will do. >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:118 >> + stringIndices.reserveCapacity(totalSuggestedCount); > > You resize these Vectors for every runs. I think you don't need to put these outside of the loop. And, you don't need 'totalSuggestedCount'. This reserves sufficient storage for the largest possible run inside the loop. The 'resize' and 'shrink' operations just adjust the size of the vector, without changing the actual memory allocation. So I think this is correct as-is.
Comment on attachment 382282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review > Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:175 > + m_complexTextRuns.append(ComplexTextRun::create(advances, origins, glyphs, stringIndices, FloatSize(), *font, currentCP, 0, run.length, 0, run.length, m_run.ltr())); If you reuse Vectors, all ComplexTextRun shares same Vectors for advances, origins, glyphs, stringIndices.
Comment on attachment 382282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review >> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:175 >> + m_complexTextRuns.append(ComplexTextRun::create(advances, origins, glyphs, stringIndices, FloatSize(), *font, currentCP, 0, run.length, 0, run.length, m_run.ltr())); > > If you reuse Vectors, all ComplexTextRun shares same Vectors for advances, origins, glyphs, stringIndices. I believe the constructor for ComplextTextRun makes a copy of the vectors it receives, so I think each will be distinct.
Comment on attachment 382282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382282&action=review >>> Source/WebCore/platform/graphics/win/ComplexTextControllerDirectWrite.cpp:175 >>> + m_complexTextRuns.append(ComplexTextRun::create(advances, origins, glyphs, stringIndices, FloatSize(), *font, currentCP, 0, run.length, 0, run.length, m_run.ltr())); >> >> If you reuse Vectors, all ComplexTextRun shares same Vectors for advances, origins, glyphs, stringIndices. > > I believe the constructor for ComplextTextRun makes a copy of the vectors it receives, so I think each will be distinct. Ah. You are right. Sorry for the noise.
Created attachment 382523 [details] Patch for landing
Comment on attachment 382523 [details] Patch for landing Clearing flags on attachment: 382523 Committed r251900: <https://trac.webkit.org/changeset/251900>
All reviewed patches have been landed. Closing bug.