Changeset 76137 in webkit


Ignore:
Timestamp:
Jan 19, 2011 10:29:15 AM (13 years ago)
Author:
evan@chromium.org
Message:

2011-01-18 Evan Martin <​evan@chromium.org>

Reviewed by Tony Chang.

[chromium] simplify complex text code, fixing a handful of layout tests
​https://bugs.webkit.org/show_bug.cgi?id=52661

  • platform/chromium/test_expectations.txt: Mark 11 tests as expected to pass.

2011-01-18 Evan Martin <​evan@chromium.org>

Reviewed by Tony Chang.

[chromium] simplify complex text code, fixing a handful of layout tests
​https://bugs.webkit.org/show_bug.cgi?id=52661

Change ComplexTextControllerLinux to lay out RTL text to the left from
the starting point. (Previously it always went to the right.) This allows
us to map pixel offsets more directly into offsets within the text runs,
simplifying a few of the text-fiddling functions (they no longer need to
track the current position, as ComplexTextController now does it).

  • platform/graphics/chromium/ComplexTextControllerLinux.cpp: (WebCore::ComplexTextController::ComplexTextController): (WebCore::ComplexTextController::reset): (WebCore::ComplexTextController::setGlyphXPositions):
  • platform/graphics/chromium/ComplexTextControllerLinux.h: (WebCore::ComplexTextController::offsetX):
  • platform/graphics/chromium/FontLinux.cpp: (WebCore::Font::drawComplexText): (WebCore::Font::floatWidthForComplexText): (WebCore::glyphIndexForXPositionInScriptRun): (WebCore::Font::offsetForPositionForComplexText): (WebCore::Font::selectionRectForComplexText):
Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r76133 r76137  
     12011-01-18  Evan Martin  <evan@chromium.org>
     2
     3        Reviewed by Tony Chang.
     4
     5        [chromium] simplify complex text code, fixing a handful of layout tests
     6        https://bugs.webkit.org/show_bug.cgi?id=52661
     7
     8        * platform/chromium/test_expectations.txt: Mark 11 tests as expected to pass.
     9
    1102011-01-19  Michael Saboff  <msaboff@apple.com>
    211

  • trunk/LayoutTests/platform/chromium/test_expectations.txt

    r76127 r76137  
    30803080BUGCR69571 : plugins/destroy-on-setwindow.html = CRASH
    30813081
    3082 // Failing after r75720
    3083 BUGCR69612 LINUX : editing/selection/extend-to-line-boundary.html = TEXT
    3084 BUGCR69612 LINUX : fast/text/atsui-negative-spacing-features.html = IMAGE
    3085 BUGCR69612 LINUX : fast/text/atsui-spacing-features.html = IMAGE
    3086 BUGCR69612 LINUX : fast/text/drawBidiText.html = IMAGE
    3087 BUGCR69612 LINUX : fast/text/international/bidi-AN-after-empty-run.html = IMAGE
    3088 BUGCR69612 LINUX : fast/text/international/bidi-CS-after-AN.html = IMAGE
    3089 BUGCR69612 LINUX : fast/text/international/bidi-linebreak-002.html = IMAGE
    3090 BUGCR69612 LINUX : fast/text/international/bidi-linebreak-003.html = IMAGE
    3091 BUGCR69612 LINUX : fast/text/international/bidi-mirror-he-ar.html = IMAGE
    3092 BUGCR69612 LINUX : fast/text/international/bidi-neutral-run.html = IMAGE
    3093 BUGCR69612 LINUX : fast/text/international/hebrew-vowels.html = IMAGE
    3094 
    30953082// Failing after r75768
    30963083BUGCR69639 : http/tests/loading/cross-origin-XHR-willLoadRequest.html = TEXT

  • trunk/Source/WebCore/ChangeLog

    r76136 r76137  
     12011-01-18  Evan Martin  <evan@chromium.org>
     2
     3        Reviewed by Tony Chang.
     4
     5        [chromium] simplify complex text code, fixing a handful of layout tests
     6        https://bugs.webkit.org/show_bug.cgi?id=52661
     7
     8        Change ComplexTextControllerLinux to lay out RTL text to the left from
     9        the starting point.  (Previously it always went to the right.)  This allows
     10        us to map pixel offsets more directly into offsets within the text runs,
     11        simplifying a few of the text-fiddling functions (they no longer need to
     12        track the current position, as ComplexTextController now does it).
     13
     14        * platform/graphics/chromium/ComplexTextControllerLinux.cpp:
     15        (WebCore::ComplexTextController::ComplexTextController):
     16        (WebCore::ComplexTextController::reset):
     17        (WebCore::ComplexTextController::setGlyphXPositions):
     18        * platform/graphics/chromium/ComplexTextControllerLinux.h:
     19        (WebCore::ComplexTextController::offsetX):
     20        * platform/graphics/chromium/FontLinux.cpp:
     21        (WebCore::Font::drawComplexText):
     22        (WebCore::Font::floatWidthForComplexText):
     23        (WebCore::glyphIndexForXPositionInScriptRun):
     24        (WebCore::Font::offsetForPositionForComplexText):
     25        (WebCore::Font::selectionRectForComplexText):
     26
    1272011-01-19  Pavel Feldman  <pfeldman@chromium.org>
    228

  • trunk/Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp

    r75756 r76137  
    4848ComplexTextController::ComplexTextController(const TextRun& run, unsigned startingX, const Font* font)
    4949    : m_font(font)
    50     , m_startingX(startingX)
    51     , m_offsetX(m_startingX)
    5250    , m_run(getNormalizedTextRun(run, m_normalizedRun, m_normalizedBuffer))
    5351    , m_wordSpacingAdjustment(0)
    … …  
    7674    m_item.stringLength = m_run.length();
    7775
    78     reset();
     76    reset(startingX);
    7977}
    8078
    … …  
    138136}
    139137
    140 void ComplexTextController::reset()
     138void ComplexTextController::reset(unsigned offset)
    141139{
    142140    m_indexOfNextScriptRun = 0;
    143     m_offsetX = m_startingX;
     141    m_offsetX = offset;
    144142}
    145143
    … …  
    278276
    279277    // Iterate through the glyphs in logical order, flipping for RTL where necessary.
    280     // In RTL mode all variables are positive except m_xPositions, which starts from m_offsetX and runs negative.
    281     // It is fixed up in a second pass below.
     278    // Glyphs are positioned starting from m_offsetX; in RTL mode they go leftwards from there.
    282279    for (size_t i = 0; i < m_item.num_glyphs; ++i) {
    283280        while (static_cast<unsigned>(logClustersIndex) < m_item.item.length && logClusters()[logClustersIndex] < i)
    … …  
    304301        position += advance;
    305302    }
    306     const double width = position;
    307 
    308     // Now that we've computed the total width, do another pass to fix positioning for RTL.
    309     if (isRTL) {
    310         for (size_t i = 0; i < m_item.num_glyphs; ++i)
    311             m_xPositions[i] += width;
    312     }
    313 
    314     m_pixelWidth = std::max(width, 0.0);
    315     m_offsetX += m_pixelWidth;
     303    m_pixelWidth = std::max(position, 0.0);
     304    m_offsetX += m_pixelWidth * rtlFlip;
    316305}
    317306

  • trunk/Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.h

    r75756 r76137  
    6969    // WebKit uses this to justify text.
    7070    void setPadding(int);
    71     void reset();
     71    void reset(unsigned offset);
    7272    // Advance to the next script run, returning false when the end of the
    7373    // TextRun has been reached.
    … …  
    8787    // Set the x offset for the next script run. This affects the values in
    8888    // |xPositions|
    89     void setXOffsetToZero() { m_offsetX = 0; }
    9089    bool rtl() const { return m_run.rtl(); }
    9190    const uint16_t* glyphs() const { return m_glyphs16; }
    … …  
    115114    const unsigned numCodePoints() const { return m_numCodePoints; }
    116115
     116    // Return the current pixel position of the controller.
     117    const unsigned offsetX() const { return m_offsetX; }
     118
    117119    const FontPlatformData* fontPlatformDataForScriptRun() { return reinterpret_cast<FontPlatformData*>(m_item.font->userData); }
    118120
    … …  
    138140    SkScalar* m_xPositions; // A vector of x positions for each glyph.
    139141    ssize_t m_indexOfNextScriptRun; // Indexes the script run in |m_run|.
    140     const unsigned m_startingX; // Offset in pixels of the first script run.
    141142    unsigned m_offsetX; // Offset in pixels to the start of the next script run.
    142143    unsigned m_pixelWidth; // Width (in px) of the current script run.

  • trunk/Source/WebCore/platform/graphics/chromium/FontLinux.cpp

    r75756 r76137  
    207207    controller.setPadding(run.padding());
    208208
     209    if (run.rtl()) {
     210        // FIXME: this causes us to shape the text twice -- once to compute the width and then again
     211        // below when actually rendering.  Change ComplexTextController to match platform/mac and
     212        // platform/chromium/win by having it store the shaped runs, so we can reuse the results.
     213        controller.reset(point.x() + controller.widthOfFullRun());
     214        // We need to set the padding again because ComplexTextController layout consumed the value.
     215        // Fixing the above problem would help here too.
     216        controller.setPadding(run.padding());
     217    }
     218
    209219    while (controller.nextScriptRun()) {
    210220        if (fill) {
    … …  
    232242    controller.setWordSpacingAdjustment(wordSpacing());
    233243    controller.setLetterSpacingAdjustment(letterSpacing());
     244    controller.setPadding(run.padding());
    234245    return controller.widthOfFullRun();
    235246}
    … …  
    240251    // position and halfway through the current glyph.
    241252    // FIXME: this code probably belongs in ComplexTextController.
    242     int lastX = controller.rtl() ? controller.width() : 0;
     253    int lastX = controller.offsetX() - (controller.rtl() ? -controller.width() : controller.width());
    243254    for (int glyphIndex = 0; static_cast<unsigned>(glyphIndex) < controller.length(); ++glyphIndex) {
    244255        int advance = truncateFixedPointToInteger(controller.advances()[glyphIndex]);
    … …  
    258269    // FIXME: This truncation is not a problem for HTML, but only affects SVG, which passes floating-point numbers
    259270    // to Font::offsetForPosition(). Bug http://webkit.org/b/40673 tracks fixing this problem.
    260     int x = static_cast<int>(xFloat);
     271    int targetX = static_cast<int>(xFloat);
    261272
    262273    // (Mac code ignores includePartialGlyphs, and they don't know what it's
    … …  
    265276    controller.setWordSpacingAdjustment(wordSpacing());
    266277    controller.setLetterSpacingAdjustment(letterSpacing());
    267 
    268     // If this is RTL text, the first glyph from the left is actually the last
    269     // code point. So we need to know how many code points there are total in
    270     // order to subtract. This is different from the length of the TextRun
    271     // because UTF-16 surrogate pairs are a single code point, but 32-bits long.
    272     // In LTR we leave this as 0 so that we get the correct value for
    273     // |basePosition|, below.
    274     unsigned totalCodePoints = 0;
    275     if (controller.rtl()) {
    276         ssize_t offset = 0;
    277         while (offset < run.length()) {
    278             utf16_to_code_point(run.characters(), run.length(), &offset);
    279             totalCodePoints++;
    280         }
    281     }
    282 
    283     unsigned basePosition = totalCodePoints;
    284 
    285     // For RTL:
    286     //   code-point order:  abcd efg hijkl
    287     //   on screen:         lkjih gfe dcba
    288     //                                ^   ^
    289     //                                |   |
    290     //                  basePosition--|   |
    291     //                 totalCodePoints----|
    292     // Since basePosition is currently the total number of code-points, the
    293     // first thing we do is decrement it so that it's pointing to the start of
    294     // the current script-run.
    295     //
    296     // For LTR, basePosition is zero so it already points to the start of the
    297     // first script run.
     278    controller.setPadding(run.padding());
     279    if (run.rtl()) {
     280        // See FIXME in drawComplexText.
     281        controller.reset(controller.widthOfFullRun());
     282        controller.setPadding(run.padding());
     283    }
     284
     285    unsigned basePosition = 0;
     286
     287    int x = controller.offsetX();
    298288    while (controller.nextScriptRun()) {
    299         if (controller.rtl())
    300             basePosition -= controller.numCodePoints();
    301 
    302         if (x >= 0 && static_cast<unsigned>(x) < controller.width()) {
    303             // The x value in question is within this script run. We consider
    304             // each glyph in presentation order and stop when we find the one
    305             // covering this position.
    306             const int glyphIndex = glyphIndexForXPositionInScriptRun(controller, x);
     289        int nextX = controller.offsetX();
     290
     291        if (std::min(x, nextX) <= targetX && targetX <= std::max(x, nextX)) {
     292            // The x value in question is within this script run.
     293            const int glyphIndex = glyphIndexForXPositionInScriptRun(controller, targetX);
    307294
    308295            // Now that we have a glyph index, we have to turn that into a
    … …  
    325312        }
    326313
    327         x -= controller.width();
    328 
    329         if (!controller.rtl())
    330             basePosition += controller.numCodePoints();
     314        basePosition += controller.numCodePoints();
    331315    }
    332316
    … …  
    343327    controller.setWordSpacingAdjustment(wordSpacing());
    344328    controller.setLetterSpacingAdjustment(letterSpacing());
    345 
    346     // Base will point to the x offset for the start of the current script run. Note that, in
    347     // the LTR case, width will be 0.
    348     int base = controller.rtl() ? controller.widthOfFullRun() : 0;
    349 
    350     controller.reset();
     329    controller.setPadding(run.padding());
     330    if (run.rtl()) {
     331        // See FIXME in drawComplexText.
     332        controller.reset(controller.widthOfFullRun());
     333        controller.setPadding(run.padding());
     334    }
     335
     336    // Iterate through the script runs in logical order, searching for the run covering the positions of interest.
    351337    while (controller.nextScriptRun() && (fromX == -1 || toX == -1)) {
    352         // ComplexTextController will helpfully accululate the x offsets for different
    353         // script runs for us. For this code, however, we always want the x offsets
    354         // to start from zero so we call this before each script run.
    355         controller.setXOffsetToZero();
    356 
    357         if (controller.rtl())
    358             base -= controller.width();
    359 
    360338        if (fromX == -1 && from >= 0 && static_cast<unsigned>(from) < controller.numCodePoints()) {
    361339            // |from| is within this script run. So we index the clusters log to
    … …  
    363341            // position.
    364342            int glyph = controller.logClusters()[from];
    365             fromX = base + controller.xPositions()[glyph];
     343            fromX = controller.xPositions()[glyph];
    366344            if (controller.rtl())
    367345                fromX += truncateFixedPointToInteger(controller.advances()[glyph]);
    … …  
    371349        if (toX == -1 && to >= 0 && static_cast<unsigned>(to) < controller.numCodePoints()) {
    372350            int glyph = controller.logClusters()[to];
    373             toX = base + controller.xPositions()[glyph];
     351            toX = controller.xPositions()[glyph];
    374352            if (controller.rtl())
    375353                toX += truncateFixedPointToInteger(controller.advances()[glyph]);
    376354        } else
    377355            to -= controller.numCodePoints();
    378 
    379         if (!controller.rtl())
    380             base += controller.width();
    381356    }
    382357
    383358    // The position in question might be just after the text.
    384     const int endEdge = base;
    385     if (fromX == -1 && !from)
    386         fromX = endEdge;
    387     if (toX == -1 && !to)
    388         toX = endEdge;
     359    if (fromX == -1)
     360        fromX = controller.offsetX();
     361    if (toX == -1)
     362        toX = controller.offsetX();
    389363
    390364    ASSERT(fromX != -1 && toX != -1);
Note: See TracChangeset for help on using the changeset viewer.