[webkit-changes] [188263] trunk

Tue, 11 Aug 2015 11:06:04 -0700

Title: [188263] trunk
Revision
188263
Author
[email protected]
Date
2015-08-11 11:05:40 -0700 (Tue, 11 Aug 2015)

Log Message

[iOS] Arabic letter Yeh is drawn in LastResort
https://bugs.webkit.org/show_bug.cgi?id=147862
<rdar://problem/22202935>

Reviewed by Darin Adler.

Source/WebCore:

In order to perform font fallback, we must know which fonts support which characters. We
perform this check by asking each font to map a sequence of codepoints to glyphs, and
any glyphs which end up with a 0 value are unsupported by the font.

One of the mechanisms that we use to do this is to combine the code points into a string,
and tell Core Text to lay out the string. However, this is fundamentally a different
operation than the one we are trying to perform. Strings combine adjacent codepoints into
grapheme clusters, and CoreText operates on these. However, we are trying to gain
information regarding codepoints, not grapheme clusters.

Instead of taking this string-based approach, we should try harder to use Core Text
functions which operate on ordered collections of characters, rather than strings. In
particular, CTFontGetGlyphsForCharacters() and CTFontGetVerticalGlyphsForCharacters()
have the behavior we want where any unmapped characters end up with a 0 value glyph.

Previously, we were only using the result of those functions if they were successfully
able to map their entire input. However, given the fact that we can degrade gracefully
in the case of a partial mapping, we shouldn't need to bail completely to the
string-based approach should a partial mapping occur.

At some point we should delete the string-based approach entirely. However, this path
is still explicitly used for composite fonts. Fixing that use case is out of scope
for this patch.

Test: fast/text/arabic-glyph-cache-fill-combine.html

* platform/graphics/mac/GlyphPageMac.cpp:
(WebCore::GlyphPage::fill):

LayoutTests:

* fast/text/arabic-glyph-cache-fill-combine-expected.html: Added.
* fast/text/arabic-glyph-cache-fill-combine.html: Added.
* platform/mac/TestExpectations: Mark test as iOS-specific
* platform/gtk/TestExpectations: Mark test as iOS-specific
* platform/efl/TestExpectations: Mark test as iOS-specific
* platform/efl/TestExpectations: Mark test as iOS-specific

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (188262 => 188263)


--- trunk/LayoutTests/ChangeLog	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/LayoutTests/ChangeLog	2015-08-11 18:05:40 UTC (rev 188263)
@@ -1,3 +1,18 @@
+2015-08-11  Myles C. Maxfield  <[email protected]>
+
+        [iOS] Arabic letter Yeh is drawn in LastResort
+        https://bugs.webkit.org/show_bug.cgi?id=147862
+        <rdar://problem/22202935>
+
+        Reviewed by Darin Adler.
+
+        * fast/text/arabic-glyph-cache-fill-combine-expected.html: Added.
+        * fast/text/arabic-glyph-cache-fill-combine.html: Added.
+        * platform/mac/TestExpectations: Mark test as iOS-specific
+        * platform/gtk/TestExpectations: Mark test as iOS-specific
+        * platform/efl/TestExpectations: Mark test as iOS-specific
+        * platform/efl/TestExpectations: Mark test as iOS-specific
+
 2015-08-11  Chris Dumez  <[email protected]>
 
         The 'length' property on interface objects should be configurable

Added: trunk/LayoutTests/fast/text/arabic-glyph-cache-fill-combine-expected.html (0 => 188263)


--- trunk/LayoutTests/fast/text/arabic-glyph-cache-fill-combine-expected.html	                        (rev 0)
+++ trunk/LayoutTests/fast/text/arabic-glyph-cache-fill-combine-expected.html	2015-08-11 18:05:40 UTC (rev 188263)
@@ -0,0 +1,10 @@
+<!DOCTYPE>
+<html>
+<body>
+This test hits a codepath where codepoints were being combined before they were being input into our glyph cache.
+This character occurs just before combining marks in Unicode, and therefore was being erroneously combined. The test
+passes if the glyph is drawn in a font other than LastResort (if the test fails, the character looks like a box or like
+three horizontal lines).
+<div style="font: 50px 'Geeza Pro';">&#x64a;</div>
+</body>
+</html>

Added: trunk/LayoutTests/fast/text/arabic-glyph-cache-fill-combine.html (0 => 188263)


--- trunk/LayoutTests/fast/text/arabic-glyph-cache-fill-combine.html	                        (rev 0)
+++ trunk/LayoutTests/fast/text/arabic-glyph-cache-fill-combine.html	2015-08-11 18:05:40 UTC (rev 188263)
@@ -0,0 +1,10 @@
+<!DOCTYPE>
+<html>
+<body>
+This test hits a codepath where codepoints were being combined before they were being input into our glyph cache.
+This character occurs just before combining marks in Unicode, and therefore was being erroneously combined. The test
+passes if the glyph is drawn in a font other than LastResort (if the test fails, the character looks like a box or like
+three horizontal lines).
+<div style="font: 50px UICTFontTextStyleBody;">&#x64a;</div>
+</body>
+</html>

Modified: trunk/LayoutTests/platform/efl/TestExpectations (188262 => 188263)


--- trunk/LayoutTests/platform/efl/TestExpectations	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/LayoutTests/platform/efl/TestExpectations	2015-08-11 18:05:40 UTC (rev 188263)
@@ -2253,3 +2253,6 @@
 
 # This test hardcodes the result of a platform-dependent font lookup algorithm.
 fast/text/fallback-language-han.html [ Skip ]
+
+# This test relies on iOS-specific font fallback.
+fast/text/arabic-glyph-cache-fill-combine.html [ ImageOnlyFailure ]

Modified: trunk/LayoutTests/platform/gtk/TestExpectations (188262 => 188263)


--- trunk/LayoutTests/platform/gtk/TestExpectations	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/LayoutTests/platform/gtk/TestExpectations	2015-08-11 18:05:40 UTC (rev 188263)
@@ -2420,3 +2420,6 @@
 
 # This test hardcodes the result of a platform-dependent font lookup algorithm.
 fast/text/fallback-language-han.html [ Skip ]
+
+# This test relies on iOS-specific font fallback.
+fast/text/arabic-glyph-cache-fill-combine.html [ ImageOnlyFailure ]

Modified: trunk/LayoutTests/platform/mac/TestExpectations (188262 => 188263)


--- trunk/LayoutTests/platform/mac/TestExpectations	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/LayoutTests/platform/mac/TestExpectations	2015-08-11 18:05:40 UTC (rev 188263)
@@ -1313,3 +1313,6 @@
 
 # Language-specific font fallback is disabled on certain versions of OS X
 webkit.org/b/147390 [ Mavericks Yosemite ElCapitan ] fast/text/fallback-language-han.html [ ImageOnlyFailure ]
+
+# This test relies on iOS-specific font fallback.
+[ Mavericks Yosemite ElCapitan ] fast/text/arabic-glyph-cache-fill-combine.html [ ImageOnlyFailure ]

Modified: trunk/LayoutTests/platform/win/TestExpectations (188262 => 188263)


--- trunk/LayoutTests/platform/win/TestExpectations	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/LayoutTests/platform/win/TestExpectations	2015-08-11 18:05:40 UTC (rev 188263)
@@ -3149,3 +3149,6 @@
 
 # This test hardcodes the result of a platform-dependent font lookup algorithm.
 fast/text/fallback-language-han.html [ Skip ]
+
+# This test relies on iOS-specific font fallback.
+fast/text/arabic-glyph-cache-fill-combine.html [ ImageOnlyFailure ]

Modified: trunk/Source/WebCore/ChangeLog (188262 => 188263)


--- trunk/Source/WebCore/ChangeLog	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/Source/WebCore/ChangeLog	2015-08-11 18:05:40 UTC (rev 188263)
@@ -1,3 +1,40 @@
+2015-08-11  Myles C. Maxfield  <[email protected]>
+
+        [iOS] Arabic letter Yeh is drawn in LastResort
+        https://bugs.webkit.org/show_bug.cgi?id=147862
+        <rdar://problem/22202935>
+
+        Reviewed by Darin Adler.
+
+        In order to perform font fallback, we must know which fonts support which characters. We
+        perform this check by asking each font to map a sequence of codepoints to glyphs, and
+        any glyphs which end up with a 0 value are unsupported by the font.
+
+        One of the mechanisms that we use to do this is to combine the code points into a string,
+        and tell Core Text to lay out the string. However, this is fundamentally a different
+        operation than the one we are trying to perform. Strings combine adjacent codepoints into
+        grapheme clusters, and CoreText operates on these. However, we are trying to gain
+        information regarding codepoints, not grapheme clusters.
+
+        Instead of taking this string-based approach, we should try harder to use Core Text
+        functions which operate on ordered collections of characters, rather than strings. In
+        particular, CTFontGetGlyphsForCharacters() and CTFontGetVerticalGlyphsForCharacters()
+        have the behavior we want where any unmapped characters end up with a 0 value glyph.
+
+        Previously, we were only using the result of those functions if they were successfully
+        able to map their entire input. However, given the fact that we can degrade gracefully
+        in the case of a partial mapping, we shouldn't need to bail completely to the
+        string-based approach should a partial mapping occur.
+
+        At some point we should delete the string-based approach entirely. However, this path
+        is still explicitly used for composite fonts. Fixing that use case is out of scope
+        for this patch.
+
+        Test: fast/text/arabic-glyph-cache-fill-combine.html
+
+        * platform/graphics/mac/GlyphPageMac.cpp:
+        (WebCore::GlyphPage::fill):
+
 2015-08-11  Chris Dumez  <[email protected]>
 
         The 'length' property on interface objects should be configurable

Modified: trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp (188262 => 188263)


--- trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp	2015-08-11 17:57:08 UTC (rev 188262)
+++ trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp	2015-08-11 18:05:40 UTC (rev 188263)
@@ -87,9 +87,11 @@
                 haveGlyphs = true;
             }
         }
-    } else if (!fontData->platformData().isCompositeFontReference() && ((fontData->platformData().widthVariant() == RegularWidth)
-        ? CTFontGetVerticalGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength)
-        : CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength))) {
+    } else if (!fontData->platformData().isCompositeFontReference()) {
+        if (fontData->platformData().widthVariant() == RegularWidth)
+            CTFontGetVerticalGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
+        else
+            CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
         // When buffer consists of surrogate pairs, CTFontGetVerticalGlyphsForCharacters and CTFontGetGlyphsForCharacters
         // place the glyphs at indices corresponding to the first character of each pair.
         ASSERT(!(bufferLength % length) && (bufferLength / length == 1 || bufferLength / length == 2));
@@ -103,6 +105,9 @@
             }
         }
     } else {
+        // FIXME: webkit.org/b/147859 This code is fundamentally broken. A string is not the same as an ordered sequence of codepoints. In particular, strings
+        // combine adjacent codepoints into grapheme clusters. We should delete this entire else {} block.
+
         // We ask CoreText for possible vertical variant glyphs
         RetainPtr<CFStringRef> string = adoptCF(CFStringCreateWithCharactersNoCopy(kCFAllocatorDefault, buffer, bufferLength, kCFAllocatorNull));
         RetainPtr<CFAttributedStringRef> attributedString = adoptCF(CFAttributedStringCreate(kCFAllocatorDefault, string.get(), fontData->getCFStringAttributes(0, fontData->hasVerticalGlyphs() ? Vertical : Horizontal)));

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to