Bug 185933 – Improve the performance of Font::canRenderCombiningCharacterSequence()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Improve the performance of Font::canRenderCombiningCharacterSequence()

[Myles C. Maxfield]

Created attachment 341162 [details]
Patch

[Myles C. Maxfield]

The patch has a 2% progression on the attached performance test.

[Myles C. Maxfield]

Created attachment 341167 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/40509552>

[EWS Watchlist]

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

[EWS Watchlist]

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

[Myles C. Maxfield]

Created attachment 341323 [details]
Patch

[Myles C. Maxfield]

Created attachment 341335 [details]
Patch

[Myles C. Maxfield]

Created attachment 341337 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Myles C. Maxfield]

Created attachment 341372 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 341372 [details]
Patch

Clearing flags on attachment: 341372

Committed r232221: <https://trac.webkit.org/changeset/232221>

[WebKit Commit Bot]

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.