Fix ShapeResult::selectionRect handles RTL runs in wrong order

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp

 /*
  * Copyright (c) 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013 BlackBerry Limited. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 57 matching lines @@
 struct ShapeResult::RunInfo {
     RunInfo(const SimpleFontData* font, hb_direction_t dir, hb_script_t script,
         unsigned startIndex, unsigned numGlyphs, unsigned numCharacters)
         : m_fontData(const_cast<SimpleFontData*>(font)), m_direction(dir), m_script(script)
         , m_startIndex(startIndex), m_numCharacters(numCharacters)
         , m_numGlyphs(numGlyphs), m_width(0.0f)
     {
         m_glyphData.resize(m_numGlyphs);
     }
 
+    bool rtl() const { return HB_DIRECTION_IS_BACKWARD(m_direction); }
+    float xPositionForVisualOffset(unsigned) const;
     float xPositionForOffset(unsigned) const;
     int characterIndexForXPosition(float) const;
     void setGlyphAndPositions(unsigned index, uint16_t glyphId, float advance,
         float offsetX, float offsetY);
 
     void addAdvance(unsigned index, float advance)
     {
         m_glyphData[index].advance += advance;
     }
 
     size_t glyphToCharacterIndex(size_t i) const
     {
         return m_startIndex + m_glyphData[i].characterIndex;
     }
 
     RefPtr<SimpleFontData> m_fontData;
     hb_direction_t m_direction;
     hb_script_t m_script;
     Vector<HarfBuzzRunGlyphData> m_glyphData;
     unsigned m_startIndex;
     unsigned m_numCharacters;
     unsigned m_numGlyphs;
     float m_width;
 };
 
+float ShapeResult::RunInfo::xPositionForVisualOffset(unsigned offset) const
+{
+    ASSERT(offset < m_numCharacters);
+    if (rtl())
+        offset = m_numCharacters - offset - 1;
+    return xPositionForOffset(offset);
+}
+
 float ShapeResult::RunInfo::xPositionForOffset(unsigned offset) const
 {
     ASSERT(offset <= m_numCharacters);
     unsigned glyphIndex = 0;
     float position = 0;
-    if (m_direction == HB_DIRECTION_RTL) {
+    if (rtl()) {
         while (glyphIndex < m_numGlyphs && m_glyphData[glyphIndex].characterIndex > offset) {
             position += m_glyphData[glyphIndex].advance;
             ++glyphIndex;
         }
         // For RTL, we need to return the right side boundary of the character.
         // Add advance of glyphs which are part of the character.
         while (glyphIndex < m_numGlyphs - 1 && m_glyphData[glyphIndex].characterIndex == m_glyphData[glyphIndex + 1].characterIndex) {
             position += m_glyphData[glyphIndex].advance;
             ++glyphIndex;
         }
@@ skipping 12 matching lines @@
     ASSERT(targetX <= m_width);
     float currentX = 0;
     float currentAdvance = m_glyphData[0].advance;
     unsigned glyphIndex = 0;
 
     // Sum up advances that belong to the first character.
     while (glyphIndex < m_numGlyphs - 1 && m_glyphData[glyphIndex].characterIndex == m_glyphData[glyphIndex + 1].characterIndex)
         currentAdvance += m_glyphData[++glyphIndex].advance;
     currentAdvance = currentAdvance / 2.0;
     if (targetX <= currentAdvance)
-        return m_direction == HB_DIRECTION_RTL ? m_numCharacters : 0;
+        return rtl() ? m_numCharacters : 0;
 
     currentX = currentAdvance;
     ++glyphIndex;
     while (glyphIndex < m_numGlyphs) {
         unsigned prevCharacterIndex = m_glyphData[glyphIndex - 1].characterIndex;
         float prevAdvance = currentAdvance;
         currentAdvance = m_glyphData[glyphIndex].advance;
         while (glyphIndex < m_numGlyphs - 1 && m_glyphData[glyphIndex].characterIndex == m_glyphData[glyphIndex + 1].characterIndex)
             currentAdvance += m_glyphData[++glyphIndex].advance;
         currentAdvance = currentAdvance / 2.0;
         float nextX = currentX + prevAdvance + currentAdvance;
         if (currentX <= targetX && targetX <= nextX)
-            return m_direction == HB_DIRECTION_RTL ? prevCharacterIndex : m_glyphData[glyphIndex].characterIndex;
+            return rtl() ? prevCharacterIndex : m_glyphData[glyphIndex].characterIndex;
         currentX = nextX;
         ++glyphIndex;
     }
 
-    return m_direction == HB_DIRECTION_RTL ? 0 : m_numCharacters;
+    return rtl() ? 0 : m_numCharacters;
 }
 
 void ShapeResult::RunInfo::setGlyphAndPositions(unsigned index,
     uint16_t glyphId, float advance, float offsetX, float offsetY)
 {
     HarfBuzzRunGlyphData& data = m_glyphData[index];
     data.glyph = glyphId;
     data.advance = advance;
     data.offset = FloatSize(offsetX, offsetY);
 }
@@ skipping 233 matching lines @@
 
     // The absoluteFrom and absoluteTo arguments represent the start/end offset
     // for the entire run, from/to are continuously updated to be relative to
     // the current word (ShapeResult instance).
     int from = absoluteFrom;
     int to = absoluteTo;
 
     unsigned totalNumCharacters = 0;
     for (unsigned j = 0; j < results.size(); j++) {
         RefPtr<ShapeResult> result = results[j];
+        if (direction == RTL) {
+            // Convert logical offsets to visual offsets, because results are in

[wkorman, 2015/11/02 04:29:20]

Is visual the same as physical? Which name is preferred?

[kojii, 2015/11/02 05:19:44]

IIUC Unicode bidi reordering uses logical/visual while CSS directional
properties use logical/physical. Existing comments in HarfBuzzShaper is
consistent on that.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

+            // logical order while runs are in visual order.
+            if (!foundFromX && from >= 0 && static_cast<unsigned>(from) < result->numCharacters())
+                from = result->numCharacters() - from - 1;
+            if (!foundToX && to >= 0 && static_cast<unsigned>(to) < result->numCharacters())
+                to = result->numCharacters() - to - 1;
+            currentX -= result->width();
+        }
         for (unsigned i = 0; i < result->m_runs.size(); i++) {
             if (!result->m_runs[i])
                 continue;
-            if (direction == RTL)
-                currentX -= result->m_runs[i]->m_width;
+            ASSERT((direction == RTL) == result->m_runs[i]->rtl());
             int numCharacters = result->m_runs[i]->m_numCharacters;
             if (!foundFromX && from >= 0 && from < numCharacters) {
-                fromX = result->m_runs[i]->xPositionForOffset(from) + currentX;
+                fromX = result->m_runs[i]->xPositionForVisualOffset(from) + currentX;
                 foundFromX = true;
             } else {
                 from -= numCharacters;
             }
 
             if (!foundToX && to >= 0 && to < numCharacters) {
-                toX = result->m_runs[i]->xPositionForOffset(to) + currentX;
+                toX = result->m_runs[i]->xPositionForVisualOffset(to) + currentX;
                 foundToX = true;
             } else {
                 to -= numCharacters;
             }
 
             if (foundFromX && foundToX)
                 break;
-            if (direction != RTL)
-                currentX += result->m_runs[i]->m_width;
+            currentX += result->m_runs[i]->m_width;
         }
+        if (direction == RTL)
+            currentX -= result->width();
         totalNumCharacters += result->numCharacters();
     }
 
     // The position in question might be just after the text.
     if (!foundFromX && absoluteFrom == totalNumCharacters) {
         fromX = direction == RTL ? 0 : totalWidth;
         foundFromX = true;
     }
     if (!foundToX && absoluteTo == totalNumCharacters) {
         toX = direction == RTL ? 0 : totalWidth;
@@ skipping 868 matching lines @@
     }
 
     // Don't need to check m_textRun.allowsTrailingExpansion() since it's covered by !m_expansionOpportunityCount above
     spacing += nextExpansionPerOpportunity();
     m_isAfterExpansion = true;
     return spacing;
 }
 
 
 } // namespace blink