Improve the performance of Font::canRenderCombiningCharacterSequence()
Created attachment 341162 [details] Patch
The patch has a 2% progression on the attached performance test.
Created attachment 341167 [details] Patch
<rdar://problem/40509552>
Comment on attachment 341167 [details] Patch Attachment 341167 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7785152 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html
Created attachment 341177 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 341323 [details] Patch
Created attachment 341335 [details] Patch
Created attachment 341337 [details] Patch
Comment on attachment 341337 [details] Patch Attachment 341337 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7805023 New failing tests: fast/text/arabic-zwj-and-zwnj.html fast/text/format-control.html
Created attachment 341353 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341337 [details] Patch Attachment 341337 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7804981 New failing tests: imported/blink/fast/text/international/zerowidthjoiner-should-not-render.html
Created attachment 341358 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 341337 [details] Patch Attachment 341337 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7805225 New failing tests: fast/text/arabic-zwj-and-zwnj.html fast/text/format-control.html
Created attachment 341360 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 341337 [details] Patch Attachment 341337 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7805687 New failing tests: fast/text/arabic-zwj-and-zwnj.html fast/text/format-control.html
Created attachment 341364 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 341372 [details] Patch
Comment on attachment 341372 [details] Patch Clearing flags on attachment: 341372 Committed r232221: <https://trac.webkit.org/changeset/232221>
All reviewed patches have been landed. Closing bug.
Comment on attachment 341372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341372&action=review > Source/WebCore/ChangeLog:9 > + We don't need to create a whole CTLine just to determine whether or not a font supports rendering a grapheme cluster. > + Instead, the right way to do it is just see if the font's cmap table supports every code point in the cluster. How does this “right way” work for cases like bug 68287 and <rdar://problem/9394430> where the font doesn’t have a glyph for each individual code point in the cluster, but has one for the cluster?
(In reply to mitz from comment #21) > Comment on attachment 341372 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341372&action=review > > > Source/WebCore/ChangeLog:9 > > + We don't need to create a whole CTLine just to determine whether or not a font supports rendering a grapheme cluster. > > + Instead, the right way to do it is just see if the font's cmap table supports every code point in the cluster. > > How does this “right way” work for cases like bug 68287 and > <rdar://problem/9394430> where the font doesn’t have a glyph for each > individual code point in the cluster, but has one for the cluster? In looking into this, so far I found the following things: - this patch didn’t break the layout test from bug 68287, platform/mac/fast/text/combining-character-sequence-fallback.html - with this patch, canRenderCombiningCharacterSequence() returns false for Verdana and the “i with U+0302” sequence, because Verdana doesn’t have a glyph for U+0302, even though it has a glyph for the cluster as a whole - when canRenderCombiningCharacterSequence() returns false, we end up taking a “system fallback” path ends up correctly using Verdana - however, if the test is changed to specify “font-family: verdana, times”, then we fall back to Times even though we should be able to use Verdana - however, the modified test fails similarly (using Times) even prior to this patch - that’s because canRenderCombiningCharacterSequence() is returning false for Verdana and the “i with U+0302” sequence even before this patch - but that hasn’t always been the case; the modified test does pass (i.e. renders using Verdana) in OS X 10.8.5 Mountain Lion, which is the first release that included the fix for bug 68287 So in conclusion: - the fix for bug 68287 didn’t include a good test - at some point between OS X 10.8.5 and now a regression occurred and went undetected - it’s possible that this patch didn’t break anything that wasn’t already broken, but this patch will need to be amended in order to fix what got broken
I filed bug 186010.
(In reply to mitz from comment #22) > - when canRenderCombiningCharacterSequence() returns false, we end up taking > a “system fallback” path ends up correctly using Verdana I was wrong: the “system fallback” path picks Lucida Grande for i-circumflex, but it should stick with Verdana, as TextEdit does > So in conclusion: > - the fix for bug 68287 didn’t include a good test I was wrong: the test from bug 68287 was sufficient to catch the regression, if we had bothered to look at the pixels.
(In reply to mitz from comment #24) > (In reply to mitz from comment #22) > > - when canRenderCombiningCharacterSequence() returns false, we end up taking > > a “system fallback” path ends up correctly using Verdana > > I was wrong: the “system fallback” path picks Lucida Grande for > i-circumflex, but it should stick with Verdana, as TextEdit does Did, as of Mountain Lion. No longer does as of Mavericks. > > > So in conclusion: > > - the fix for bug 68287 didn’t include a good test > > I was wrong: the test from bug 68287 was sufficient to catch the regression, > if we had bothered to look at the pixels. I’ll attach screenshots to bug 186010.