Changeset 151327 in webkit


Ignore:
Timestamp:
Jun 7, 2013, 10:59:01 AM (12 years ago)
Author:
rniwa@webkit.org
Message:

REGRESSION: Lines jump up and down while typing Chinese or Japanese
​https://bugs.webkit.org/show_bug.cgi?id=115931

Reviewed by Darin Adler.

The bug was caused by Font::width caching the width of text even when the font fallbacks existed when fallbackFonts
argument was null; because of this, a later call to Font::width was returning the width without filling up
fallbackFonts even if it was not null this time.

Fixed the bug by adding a local fallback fonts hash set, and checking the emptiness of this variable in Font::width.
Also added pass fallbackFonts around in various places to make use of the cached font fallbacks.

No new tests. Unfortunately I haven't been able to make a reliable reduction for this bug.

  • platform/graphics/Font.cpp:

(WebCore::Font::width):

  • rendering/RenderBlockLineLayout.cpp:

(WebCore::measureHyphenWidth):
(WebCore::setLogicalWidthForTextRun):
(WebCore::textWidth):
(WebCore::tryHyphenating):
(WebCore::RenderBlock::LineBreaker::nextSegmentBreak):

  • rendering/RenderText.cpp:

(WebCore::maxWordFragmentWidth):
(WebCore::RenderText::computePreferredLogicalWidths):

Location:
trunk/Source/WebCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r151325 r151327  
     12013-06-07  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        REGRESSION: Lines jump up and down while typing Chinese or Japanese
     4        https://bugs.webkit.org/show_bug.cgi?id=115931
     5
     6        Reviewed by Darin Adler.
     7
     8        The bug was caused by Font::width caching the width of text even when the font fallbacks existed when fallbackFonts
     9        argument was null; because of this, a later call to Font::width was returning the width without filling up
     10        fallbackFonts even if it was not null this time.
     11
     12        Fixed the bug by adding a local fallback fonts hash set, and checking the emptiness of this variable in Font::width.
     13        Also added pass fallbackFonts around in various places to make use of the cached font fallbacks.
     14
     15        No new tests. Unfortunately I haven't been able to make a reliable reduction for this bug.
     16
     17        * platform/graphics/Font.cpp:
     18        (WebCore::Font::width):
     19        * rendering/RenderBlockLineLayout.cpp:
     20        (WebCore::measureHyphenWidth):
     21        (WebCore::setLogicalWidthForTextRun):
     22        (WebCore::textWidth):
     23        (WebCore::tryHyphenating):
     24        (WebCore::RenderBlock::LineBreaker::nextSegmentBreak):
     25        * rendering/RenderText.cpp:
     26        (WebCore::maxWordFragmentWidth):
     27        (WebCore::RenderText::computePreferredLogicalWidths):
     28
    1292013-06-07  Zan Dobersek  <zdobersek@igalia.com>
    230

  • trunk/Source/WebCore/platform/graphics/Font.cpp

    r150897 r151327  
    313313        return *cacheEntry;
    314314
     315    HashSet<const SimpleFontData*> localFallbackFonts;
     316    if (!fallbackFonts)
     317        fallbackFonts = &localFallbackFonts;
     318
    315319    float result;
    316320    if (codePathToUse == Complex)
    … …  
    319323        result = floatWidthForSimpleText(run, fallbackFonts, glyphOverflow);
    320324
    321     if (cacheEntry && (!fallbackFonts || fallbackFonts->isEmpty()))
     325    if (cacheEntry && fallbackFonts->isEmpty())
    322326        *cacheEntry = result;
    323327    return result;

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

    r151295 r151327  
    807807}
    808808
    809 static inline float measureHyphenWidth(RenderText* renderer, const Font& font)
     809static inline float measureHyphenWidth(RenderText* renderer, const Font& font, HashSet<const SimpleFontData*>* fallbackFonts = 0)
    810810{
    811811    RenderStyle* style = renderer->style();
    812     return font.width(RenderBlock::constructTextRun(renderer, font, style->hyphenString().string(), style));
     812    return font.width(RenderBlock::constructTextRun(renderer, font, style->hyphenString().string(), style), fallbackFonts);
    813813}
    814814
    … …  
    831831
    832832static inline void setLogicalWidthForTextRun(RootInlineBox* lineBox, BidiRun* run, RenderText* renderer, float xPos, const LineInfo& lineInfo,
    833                                              GlyphOverflowAndFallbackFontsMap& textBoxDataMap, VerticalPositionCache& verticalPositionCache, WordMeasurements& wordMeasurements)
     833    GlyphOverflowAndFallbackFontsMap& textBoxDataMap, VerticalPositionCache& verticalPositionCache, WordMeasurements& wordMeasurements)
    834834{
    835835    HashSet<const SimpleFontData*> fallbackFonts;
    … …  
    854854    if (toInlineTextBox(run->m_box)->hasHyphen()) {
    855855        const Font& font = renderer->style(lineInfo.isFirstLine())->font();
    856         hyphenWidth = measureHyphenWidth(renderer, font);
     856        hyphenWidth = measureHyphenWidth(renderer, font, &fallbackFonts);
    857857    }
    858858    float measuredWidth = 0;
    … …  
    867867        int lastEndOffset = run->m_start;
    868868        for (size_t i = 0, size = wordMeasurements.size(); i < size && lastEndOffset < run->m_stop; ++i) {
    869             const WordMeasurement& wordMeasurement = wordMeasurements[i];
    870             if (wordMeasurement.width <=0 || wordMeasurement.startOffset == wordMeasurement.endOffset)
     869            WordMeasurement& wordMeasurement = wordMeasurements[i];
     870            if (wordMeasurement.width <= 0 || wordMeasurement.startOffset == wordMeasurement.endOffset)
    871871                continue;
    872872            if (wordMeasurement.renderer != renderer || wordMeasurement.startOffset != lastEndOffset || wordMeasurement.endOffset > run->m_stop)
    … …  
    876876            if (kerningIsEnabled && lastEndOffset == run->m_stop) {
    877877                int wordLength = lastEndOffset - wordMeasurement.startOffset;
    878                 measuredWidth += renderer->width(wordMeasurement.startOffset, wordLength, xPos + measuredWidth, lineInfo.isFirstLine());
     878                GlyphOverflow overflow;
     879                measuredWidth += renderer->width(wordMeasurement.startOffset, wordLength, xPos + measuredWidth, lineInfo.isFirstLine(),
     880                    &wordMeasurement.fallbackFonts, &overflow);
    879881                UChar c = renderer->characterAt(wordMeasurement.startOffset);
    880882                if (i > 0 && wordLength == 1 && (c == ' ' || c == '\t'))
    … …  
    25612563}
    25622564
    2563 static ALWAYS_INLINE float textWidth(RenderText* text, unsigned from, unsigned len, const Font& font, float xPos, bool isFixedPitch, bool collapseWhiteSpace, HashSet<const SimpleFontData*>* fallbackFonts = 0, TextLayout* layout = 0)
     2565static ALWAYS_INLINE float textWidth(RenderText* text, unsigned from, unsigned len, const Font& font, float xPos, bool isFixedPitch, bool collapseWhiteSpace, HashSet<const SimpleFontData*>& fallbackFonts, TextLayout* layout = 0)
    25642566{
    25652567    GlyphOverflow glyphOverflow;
    25662568    if (isFixedPitch || (!from && len == text->textLength()) || text->style()->hasTextCombine())
    2567         return text->width(from, len, font, xPos, fallbackFonts, &glyphOverflow);
     2569        return text->width(from, len, font, xPos, &fallbackFonts, &glyphOverflow);
    25682570
    25692571    if (layout)
    2570         return Font::width(*layout, from, len, fallbackFonts);
     2572        return Font::width(*layout, from, len, &fallbackFonts);
    25712573
    25722574    TextRun run = RenderBlock::constructTextRun(text, font, text, from, len, text->style());
    … …  
    25772579    run.setTabSize(!collapseWhiteSpace, text->style()->tabSize());
    25782580    run.setXPos(xPos);
    2579     return font.width(run, fallbackFonts, &glyphOverflow);
     2581    return font.width(run, &fallbackFonts, &glyphOverflow);
    25802582}
    25812583
    … …  
    26362638
    26372639#if !ASSERT_DISABLED
    2638     float prefixWidth = hyphenWidth + textWidth(text, lastSpace, prefixLength, font, xPos, isFixedPitch, collapseWhiteSpace) + lastSpaceWordSpacing;
     2640    HashSet<const SimpleFontData*> fallbackFonts;
     2641    float prefixWidth = hyphenWidth + textWidth(text, lastSpace, prefixLength, font, xPos, isFixedPitch, collapseWhiteSpace, fallbackFonts) + lastSpaceWordSpacing;
    26392642    ASSERT(xPos + prefixWidth <= availableWidth);
    26402643#else
    … …  
    30793082            // Non-zero only when kerning is enabled and TextLayout isn't used, in which case we measure
    30803083            // words with their trailing space, then subtract its width.
    3081             float wordTrailingSpaceWidth = (f.typesettingFeatures() & Kerning) && !textLayout ? f.width(constructTextRun(t, f, &space, 1, style)) + wordSpacing : 0;
     3084            HashSet<const SimpleFontData*> fallbackFonts;
     3085            float wordTrailingSpaceWidth = (f.typesettingFeatures() & Kerning) && !textLayout ? f.width(constructTextRun(t, f, &space, 1, style), &fallbackFonts) + wordSpacing : 0;
    30823086
    30833087            UChar lastCharacter = renderTextInfo.m_lineBreakIterator.lastCharacter();
    … …  
    30933097
    30943098                if (c == softHyphen && autoWrap && !hyphenWidth && style->hyphens() != HyphensNone) {
    3095                     hyphenWidth = measureHyphenWidth(t, f);
     3099                    hyphenWidth = measureHyphenWidth(t, f, &fallbackFonts);
    30963100                    width.addUncommittedWidth(hyphenWidth);
    30973101                }
    … …  
    31043108                    wrapW += charWidth;
    31053109                    bool midWordBreakIsBeforeSurrogatePair = U16_IS_LEAD(c) && current.m_pos + 1 < t->textLength() && U16_IS_TRAIL(t->characters()[current.m_pos + 1]);
    3106                     charWidth = textWidth(t, current.m_pos, midWordBreakIsBeforeSurrogatePair ? 2 : 1, f, width.committedWidth() + wrapW, isFixedPitch, collapseWhiteSpace, 0, textLayout);
     3110                    charWidth = textWidth(t, current.m_pos, midWordBreakIsBeforeSurrogatePair ? 2 : 1, f, width.committedWidth() + wrapW, isFixedPitch, collapseWhiteSpace, fallbackFonts, textLayout);
    31073111                    midWordBreak = width.committedWidth() + wrapW + charWidth > width.availableWidth();
    31083112                }
    … …  
    31383142                    float additionalTempWidth;
    31393143                    if (wordTrailingSpaceWidth && c == ' ')
    3140                         additionalTempWidth = textWidth(t, lastSpace, current.m_pos + 1 - lastSpace, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, &wordMeasurement.fallbackFonts, textLayout) - wordTrailingSpaceWidth;
     3144                        additionalTempWidth = textWidth(t, lastSpace, current.m_pos + 1 - lastSpace, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, wordMeasurement.fallbackFonts, textLayout) - wordTrailingSpaceWidth;
    31413145                    else
    3142                         additionalTempWidth = textWidth(t, lastSpace, current.m_pos - lastSpace, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, &wordMeasurement.fallbackFonts, textLayout);
     3146                        additionalTempWidth = textWidth(t, lastSpace, current.m_pos - lastSpace, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, wordMeasurement.fallbackFonts, textLayout);
     3147
     3148                    if (wordMeasurement.fallbackFonts.isEmpty() && !fallbackFonts.isEmpty())
     3149                        wordMeasurement.fallbackFonts.swap(fallbackFonts);
     3150                    fallbackFonts.clear();
    31433151
    31443152                    wordMeasurement.width = additionalTempWidth + wordSpacingForWordMeasurement;
    … …  
    31643172                        bool lineWasTooWide = false;
    31653173                        if (width.fitsOnLine() && currentCharacterIsWS && currentStyle->breakOnlyAfterWhiteSpace() && !midWordBreak) {
    3166                             float charWidth = textWidth(t, current.m_pos, 1, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, &wordMeasurement.fallbackFonts, textLayout) + (applyWordSpacing ? wordSpacing : 0);
     3174                            float charWidth = textWidth(t, current.m_pos, 1, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, wordMeasurement.fallbackFonts, textLayout) + (applyWordSpacing ? wordSpacing : 0);
    31673175                            // Check if line is too big even without the extra space
    31683176                            // at the end of the line. If it is not, do nothing.
    … …  
    33023310
    33033311            // IMPORTANT: current.m_pos is > length here!
    3304             float additionalTempWidth = ignoringSpaces ? 0 : textWidth(t, lastSpace, current.m_pos - lastSpace, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, &wordMeasurement.fallbackFonts, textLayout);
     3312            float additionalTempWidth = ignoringSpaces ? 0 : textWidth(t, lastSpace, current.m_pos - lastSpace, f, width.currentWidth(), isFixedPitch, collapseWhiteSpace, wordMeasurement.fallbackFonts, textLayout);
    33053313            wordMeasurement.startOffset = lastSpace;
    33063314            wordMeasurement.endOffset = current.m_pos;
    … …  
    33103318            float inlineLogicalTempWidth = inlineLogicalWidth(current.m_obj, !appliedStartWidth, includeEndWidth);
    33113319            width.addUncommittedWidth(additionalTempWidth + inlineLogicalTempWidth);
     3320
     3321            if (wordMeasurement.fallbackFonts.isEmpty() && !fallbackFonts.isEmpty())
     3322                wordMeasurement.fallbackFonts.swap(fallbackFonts);
     3323            fallbackFonts.clear();
    33123324
    33133325            if (collapseWhiteSpace && currentCharacterIsSpace && additionalTempWidth)

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

    r150922 r151327  
    894894}
    895895
    896 static float maxWordFragmentWidth(RenderText* renderer, RenderStyle* style, const Font& font, const UChar* word, int wordLength, int minimumPrefixLength, int minimumSuffixLength, int& suffixStart)
     896static float maxWordFragmentWidth(RenderText* renderer, RenderStyle* style, const Font& font, const UChar* word, int wordLength, int minimumPrefixLength, int minimumSuffixLength, int& suffixStart, HashSet<const SimpleFontData*>& fallbackFonts, GlyphOverflow& glyphOverflow)
    897897{
    898898    suffixStart = 0;
    … …  
    921921        run.setCharactersLength(fragmentWithHyphen.length());
    922922        run.setCharacterScanForCodePath(!renderer->canUseSimpleFontCodePath());
    923         float fragmentWidth = font.width(run);
     923        float fragmentWidth = font.width(run, &fallbackFonts, &glyphOverflow);
    924924
    925925        // Narrow prefixes are ignored. See tryHyphenating in RenderBlockLineLayout.cpp.
    … …  
    969969    // Non-zero only when kerning is enabled, in which case we measure words with their trailing
    970970    // space, then subtract its width.
    971     float wordTrailingSpaceWidth = f.typesettingFeatures() & Kerning ? f.width(RenderBlock::constructTextRun(this, f, &space, 1, styleToUse)) + wordSpacing : 0;
     971    float wordTrailingSpaceWidth = f.typesettingFeatures() & Kerning ? f.width(RenderBlock::constructTextRun(this, f, &space, 1, styleToUse), &fallbackFonts) + wordSpacing : 0;
    972972
    973973    // If automatic hyphenation is allowed, we keep track of the width of the widest word (or word
    … …  
    10701070            if (w > maxWordWidth) {
    10711071                int suffixStart;
    1072                 float maxFragmentWidth = maxWordFragmentWidth(this, styleToUse, f, characters() + i, wordLen, minimumPrefixLength, minimumSuffixLength, suffixStart);
     1072                float maxFragmentWidth = maxWordFragmentWidth(this, styleToUse, f, characters() + i, wordLen, minimumPrefixLength, minimumSuffixLength, suffixStart, fallbackFonts, glyphOverflow);
    10731073
    10741074                if (suffixStart) {
    … …  
    11511151                run.setXPos(leadWidth + currMaxWidth);
    11521152
    1153                 currMaxWidth += f.width(run);
     1153                currMaxWidth += f.width(run, &fallbackFonts);
    11541154                glyphOverflow.right = 0;
    11551155                needsWordSpacing = isSpace && !previousCharacterIsSpace && i == len - 1;
Note: See TracChangeset for help on using the changeset viewer.