[WIN] Fix text with initial advance tests
Created attachment 345931 [details] Test patch
Attachment 345931 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 345931 [details] Test patch View in context: https://bugs.webkit.org/attachment.cgi?id=345931&action=review > Source/WebCore/platform/graphics/FontCascade.cpp:1437 > + float initialYPosition = point.y(); Does the Windows Glyph concept not track or deal with initial advance? I wonder if it could just always be set to a height of zero so there is no difference in the code here?
Comment on attachment 345931 [details] Test patch View in context: https://bugs.webkit.org/attachment.cgi?id=345931&action=review >> Source/WebCore/platform/graphics/FontCascade.cpp:1437 >> + float initialYPosition = point.y(); > > Does the Windows Glyph concept not track or deal with initial advance? I wonder if it could just always be set to a height of zero so there is no difference in the code here? Windows does deal with initial advances.
Comment on attachment 345931 [details] Test patch Attachment 345931 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8677106 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 345961 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 345983 [details] Patch
<rdar://problem/42682699>
Created attachment 345988 [details] Patch
Created attachment 345995 [details] Patch
Created attachment 346007 [details] Patch
Comment on attachment 346007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346007&action=review > Source/WebCore/platform/graphics/ComplexTextController.cpp:49 > + WTF_MAKE_FAST_ALLOCATED; Seems like this is unneeded, since we won‘t ever allocate one. > Source/WebCore/platform/graphics/FontCascade.cpp:1436 > + FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height()); This can be written like this: FloatPoint startPoint { point + glyphBuffer.initialAdvance() }; Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately.
Comment on attachment 346007 [details] Patch Attachment 346007 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8690617 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 346027 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
*** Bug 188168 has been marked as a duplicate of this bug. ***
Created attachment 346184 [details] Patch
Comment on attachment 346007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346007&action=review >> Source/WebCore/platform/graphics/FontCascade.cpp:1436 >> + FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height()); > > This can be written like this: > > FloatPoint startPoint { point + glyphBuffer.initialAdvance() }; > > Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately. Looks like Windows can't handle that :(
Committed r234448: <https://trac.webkit.org/changeset/234448>
Comment on attachment 346007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346007&action=review >>> Source/WebCore/platform/graphics/FontCascade.cpp:1436 >>> + FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height()); >> >> This can be written like this: >> >> FloatPoint startPoint { point + glyphBuffer.initialAdvance() }; >> >> Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately. > > Looks like Windows can't handle that :( I’d like to know more. Can you show me which version you tried and how Windows failed?
(In reply to Darin Adler from comment #19) > Comment on attachment 346007 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346007&action=review > > >>> Source/WebCore/platform/graphics/FontCascade.cpp:1436 > >>> + FloatPoint startPoint(point.x() + glyphBuffer.initialAdvance().width(), point.y() + glyphBuffer.initialAdvance().height()); > >> > >> This can be written like this: > >> > >> FloatPoint startPoint { point + glyphBuffer.initialAdvance() }; > >> > >> Or any variant using { }, (), or =, and either FloatPoint or auto. But no reason to do the x and y separately. > > > > Looks like Windows can't handle that :( > > I’d like to know more. Can you show me which version you tried and how > Windows failed? FloatPoint startPoint { point + glyphBuffer.initialAdvance() }; https://webkit-queues.webkit.org/patch/346184/win-ews c:\cygwin\home\buildbot\webkit\source\webcore\platform\graphics\fontcascade.cpp(1436): error C2678: binary '+': no operator found which takes a left-hand operand of type 'WebCore::FloatPoint' (or there is no acceptable conversion) (compiling source file C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource330.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
Yes, I see what the issue is. I thought that initialAdvance() returned a FloatSize. But even though this is Windows, we have USE(CG) true, so GlyphBufferAdvance is not a FloatSize, it's a struct derived from CGSize instead. The comment says: // CG uses CGSize instead of FloatSize so that the result of advances() // can be passed directly to CGContextShowGlyphsWithAdvances in FontMac.mm That comment points to some things that are not quite right. For one thing, there is no call to CGContextShowGlyphsWithAdvances any more so the comment is not correct. I am not at all sure that using CGSize instead of FloatSize here is a good idea. And I’m even more unsure that using a struct derived from CGSize with added functions is a good style choice. So I think this is something worth cleaning up eventually.
And of course FontMac.mm is not used on Windows.
I see the code taking advantage of this now. It’s Font::applyTransforms, and it’s inside #if PLATFORM(COCOA), not #if USE(CG).