⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 188377 in webkit


Ignore:
Timestamp:
Aug 12, 2015, 10:36:21 PM (11 years ago)
Author:
mmaxfield@apple.com
Message:

[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.

Source/WebCore:

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):

LayoutTests:

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
Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r188376 r188377  
     12015-08-12  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [Cocoa] [CJK-configured device] System font has vertical punctuation
     4        https://bugs.webkit.org/show_bug.cgi?id=147964
     5        <rdar://problem/22256660>
     6
     7        Reviewed by Dean Jackson.
     8
     9        Make sure punctuation isn't vertical.
     10
     11        * fast/text/system-font-punctuation.html: Added.
     12        * platform/ios-simulator/fast/text/system-font-punctuation-expected.txt: Added
     13        * platform/mac/fast/text/system-font-punctuation-expected.txt: Added
     14
    1152015-08-12  Alexey Proskuryakov  <ap@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r188375 r188377  
     12015-08-12  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [Cocoa] [CJK-configured device] System font has vertical punctuation
     4        https://bugs.webkit.org/show_bug.cgi?id=147964
     5        <rdar://problem/22256660>
     6
     7        Reviewed by Dean Jackson.
     8
     9        GlyphPage::fill() has multiple code paths to accomplish its goal. It uses the shouldUseCoreText() helper
     10        function to determine which one of the paths should be taken. However, not all of the code paths in
     11        GlyphPage::fill() are able of handling all situations. Indeed, the CoreText code paths in GlyphPage::fill()
     12        are only able to handle the situations which shouldUseCoreText() returns true for. This happens in the
     13        following cases:
     14
     15        1. If the font is a composite font
     16        2. If the font is used for text-combine
     17        3. If the font has vertical glyphs
     18
     19        In r187693, I added one more case to this list: If the font is the system font. However, I failed to add
     20        the necessary support to GlyphPage::fill() for this case. Becasue of this, we just happened to fall into
     21        the case of vertical fonts (just by coincidence), which causes us to use
     22        CTFontGetVerticalGlyphsForCharacters() instead of CTFontGetGlyphsForCharacters().
     23
     24        The solution is to adopt the same behavior we were using before r187693. Back then, we were using
     25        CGFontGetGlyphsForUnichars(), which always returned horizontal glyphs. We should simply adopt this same
     26        behavior, except in the Core Text case. Therefore, this patch is just a simple check to see if we are
     27        using the system font when determining which Core Text function to use.
     28
     29        Test: fast/text/system-font-punctuation.html
     30
     31        * platform/graphics/FontDescription.h:
     32        (WebCore::FontDescription::setWidthVariant):
     33        * platform/graphics/FontPlatformData.h:
     34        (WebCore::FontPlatformData::isForTextCombine):
     35        * platform/graphics/mac/GlyphPageMac.cpp:
     36        (WebCore::shouldUseCoreText):
     37        (WebCore::GlyphPage::fill):
     38        * rendering/RenderCombineText.cpp:
     39        (WebCore::RenderCombineText::combineText):
     40
    1412015-08-12  Jinyoung Hur  <hur.ims@navercorp.com>
    242

  • trunk/Source/WebCore/platform/graphics/FontDescription.h

    r187626 r188377  
    125125    void setOrientation(FontOrientation orientation) { m_orientation = orientation; }
    126126    void setNonCJKGlyphOrientation(NonCJKGlyphOrientation orientation) { m_nonCJKGlyphOrientation = orientation; }
    127     void setWidthVariant(FontWidthVariant widthVariant) { m_widthVariant = widthVariant; }
     127    void setWidthVariant(FontWidthVariant widthVariant) { m_widthVariant = widthVariant; } // Make sure new callers of this sync with FontPlatformData::isForTextCombine()!
    128128    void setLocale(const AtomicString&);
    129129    void setFeatureSettings(PassRefPtr<FontFeatureSettings> settings) { m_featureSettings = settings; }

  • trunk/Source/WebCore/platform/graphics/FontPlatformData.h

    r187806 r188377  
    138138    FontOrientation orientation() const { return m_orientation; }
    139139    FontWidthVariant widthVariant() const { return m_widthVariant; }
     140    bool isForTextCombine() const { return widthVariant() != RegularWidth; } // Keep in sync with callers of FontDescription::setWidthVariant().
    140141
    141142    void setOrientation(FontOrientation orientation) { m_orientation = orientation; }

  • trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp

    r188263 r188377  
    4343static bool shouldUseCoreText(const UChar* buffer, unsigned bufferLength, const Font* fontData)
    4444{
     45    // This needs to be kept in sync with GlyphPage::fill(). Currently, the CoreText paths are not able to handle
     46    // every situtation. Returning true from this function in a new situation will require you to explicitly add
     47    // handling for that situation in the CoreText paths of GlyphPage::fill().
    4548    if (fontData->platformData().isCompositeFontReference() || fontData->isSystemFont())
    4649        return true;
    47     if (fontData->platformData().widthVariant() != RegularWidth || fontData->hasVerticalGlyphs()) {
     50    if (fontData->platformData().isForTextCombine() || fontData->hasVerticalGlyphs()) {
    4851        // Ideographs don't have a vertical variant or width variants.
    4952        for (unsigned i = 0; i < bufferLength; ++i) {
    … …  
    8992        }
    9093    } else if (!fontData->platformData().isCompositeFontReference()) {
    91         if (fontData->platformData().widthVariant() == RegularWidth)
     94        // Because we know the implementation of shouldUseCoreText(), if the font isn't for text combine and it isn't a system font,
     95        // we know it must have vertical glyphs.
     96        if (fontData->platformData().isForTextCombine() || fontData->isSystemFont())
     97            CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
     98        else
    9299            CTFontGetVerticalGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
    93         else
    94             CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
    95100        // When buffer consists of surrogate pairs, CTFontGetVerticalGlyphsForCharacters and CTFontGetGlyphsForCharacters
    96101        // place the glyphs at indices corresponding to the first character of each pair.

  • trunk/Source/WebCore/rendering/RenderCombineText.cpp

    r182609 r188377  
    118118        static const FontWidthVariant widthVariants[] = { HalfWidth, ThirdWidth, QuarterWidth };
    119119        for (size_t i = 0 ; i < WTF_ARRAY_LENGTH(widthVariants) ; ++i) {
    120             description.setWidthVariant(widthVariants[i]);
     120            description.setWidthVariant(widthVariants[i]); // When modifying this, make sure to keep it in sync with FontPlatformData::isForTextCombine()!
    121121
    122122            FontCascade compressedFont(description, style().fontCascade().letterSpacing(), style().fontCascade().wordSpacing());
Note: See TracChangeset for help on using the changeset viewer.