Changeset 195403 in webkit

Timestamp:

Jan 20, 2016, 11:58:28 PM (11 years ago)

Author:

bshafiei@apple.com

Message:

Merged r188377. rdar://problem/24208102

Location:

branches/safari-601-branch

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/text/system-font-punctuation.html (copied) (copied from trunk/LayoutTests/fast/text/system-font-punctuation.html )
• LayoutTests/platform/ios-simulator/fast/text/system-font-punctuation-expected.txt (copied) (copied from trunk/LayoutTests/platform/ios-simulator/fast/text/system-font-punctuation-expected.txt )
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/FontDescription.h (modified) (1 diff)
• Source/WebCore/platform/graphics/FontPlatformData.h (modified) (1 diff)
• Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderCombineText.cpp (modified) (1 diff)

branches/safari-601-branch/LayoutTests/ChangeLog

@@ +1 @@
+2016-01-20  Babak Shafiei  <bshafiei@apple.com>
+
+        Merge r188377.
+
+    2015-08-12  Myles C. Maxfield  <mmaxfield@apple.com>
+
+            [Cocoa] [CJK-configured device] System font has vertical punctuation
+            https://bugs.webkit.org/show_bug.cgi?id=147964
+            <rdar://problem/22256660>
+
+            Reviewed by Dean Jackson.
+
+            Make sure punctuation isn't vertical.
+
+            * fast/text/system-font-punctuation.html: Added.
+            * platform/ios-simulator/fast/text/system-font-punctuation-expected.txt: Added
+            * platform/mac/fast/text/system-font-punctuation-expected.txt: Added
+
 2016-01-20  Babak Shafiei  <bshafiei@apple.com>
 

branches/safari-601-branch/Source/WebCore/ChangeLog

@@ +1 @@
+2016-01-20  Babak Shafiei  <bshafiei@apple.com>
+
+        Merge r188377.
+
+    2015-08-12  Myles C. Maxfield  <mmaxfield@apple.com>
+
+            [Cocoa] [CJK-configured device] System font has vertical punctuation
+            https://bugs.webkit.org/show_bug.cgi?id=147964
+            <rdar://problem/22256660>
+
+            Reviewed by Dean Jackson.
+
+            GlyphPage::fill() has multiple code paths to accomplish its goal. It uses the shouldUseCoreText() helper
+            function to determine which one of the paths should be taken. However, not all of the code paths in
+            GlyphPage::fill() are able of handling all situations. Indeed, the CoreText code paths in GlyphPage::fill()
+            are only able to handle the situations which shouldUseCoreText() returns true for. This happens in the
+            following cases:
+
+            1. If the font is a composite font
+            2. If the font is used for text-combine
+            3. If the font has vertical glyphs
+
+            In r187693, I added one more case to this list: If the font is the system font. However, I failed to add
+            the necessary support to GlyphPage::fill() for this case. Becasue of this, we just happened to fall into
+            the case of vertical fonts (just by coincidence), which causes us to use
+            CTFontGetVerticalGlyphsForCharacters() instead of CTFontGetGlyphsForCharacters().
+
+            The solution is to adopt the same behavior we were using before r187693. Back then, we were using
+            CGFontGetGlyphsForUnichars(), which always returned horizontal glyphs. We should simply adopt this same
+            behavior, except in the Core Text case. Therefore, this patch is just a simple check to see if we are
+            using the system font when determining which Core Text function to use.
+
+            Test: fast/text/system-font-punctuation.html
+
+            * platform/graphics/FontDescription.h:
+            (WebCore::FontDescription::setWidthVariant):
+            * platform/graphics/FontPlatformData.h:
+            (WebCore::FontPlatformData::isForTextCombine):
+            * platform/graphics/mac/GlyphPageMac.cpp:
+            (WebCore::shouldUseCoreText):
+            (WebCore::GlyphPage::fill):
+            * rendering/RenderCombineText.cpp:
+            (WebCore::RenderCombineText::combineText):
+
 2016-01-20  Babak Shafiei  <bshafiei@apple.com>
 

branches/safari-601-branch/Source/WebCore/platform/graphics/FontDescription.h

@@ -178 +178 @@
     void setOrientation(FontOrientation orientation) { m_orientation = orientation; }
     void setNonCJKGlyphOrientation(NonCJKGlyphOrientation orientation) { m_nonCJKGlyphOrientation = orientation; }
-    void setWidthVariant(FontWidthVariant widthVariant) { m_widthVariant = widthVariant; }
+    void setWidthVariant(FontWidthVariant widthVariant) { m_widthVariant = widthVariant; } // Make sure new callers of this sync with FontPlatformData::isForTextCombine()!
     void setScript(UScriptCode s) { m_script = s; }
     void setFeatureSettings(FontFeatureSettings&& settings) { m_featureSettings = WTF::move(settings); }

branches/safari-601-branch/Source/WebCore/platform/graphics/FontPlatformData.h

@@ -136 +136 @@
     FontOrientation orientation() const { return m_orientation; }
     FontWidthVariant widthVariant() const { return m_widthVariant; }
+    bool isForTextCombine() const { return widthVariant() != RegularWidth; } // Keep in sync with callers of FontDescription::setWidthVariant().
 
     void setOrientation(FontOrientation orientation) { m_orientation = orientation; }

branches/safari-601-branch/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp

@@ -43 +43 @@
 static bool shouldUseCoreText(const UChar* buffer, unsigned bufferLength, const Font* fontData)
 {
+    // This needs to be kept in sync with GlyphPage::fill(). Currently, the CoreText paths are not able to handle
+    // every situtation. Returning true from this function in a new situation will require you to explicitly add
+    // handling for that situation in the CoreText paths of GlyphPage::fill().
     if (fontData->platformData().isCompositeFontReference() || fontData->isSystemFont())
         return true;
-    if (fontData->platformData().widthVariant() != RegularWidth || fontData->hasVerticalGlyphs()) {
+    if (fontData->platformData().isForTextCombine() || fontData->hasVerticalGlyphs()) {
         // Ideographs don't have a vertical variant or width variants.
         for (unsigned i = 0; i < bufferLength; ++i) {
@@ -89 +92 @@
         }
     } else if (!fontData->platformData().isCompositeFontReference()) {
-        if (fontData->platformData().widthVariant() == RegularWidth)
+        // Because we know the implementation of shouldUseCoreText(), if the font isn't for text combine and it isn't a system font,
+        // we know it must have vertical glyphs.
+        if (fontData->platformData().isForTextCombine() || fontData->isSystemFont())
+            CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
+        else
             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.

branches/safari-601-branch/Source/WebCore/rendering/RenderCombineText.cpp

@@ -118 +118 @@
         static const FontWidthVariant widthVariants[] = { HalfWidth, ThirdWidth, QuarterWidth };
         for (size_t i = 0 ; i < WTF_ARRAY_LENGTH(widthVariants) ; ++i) {
-            description.setWidthVariant(widthVariants[i]);
+            description.setWidthVariant(widthVariants[i]); // When modifying this, make sure to keep it in sync with FontPlatformData::isForTextCombine()!
 
             FontCascade compressedFont(description, style().fontCascade().letterSpacing(), style().fontCascade().wordSpacing());