Changeset 129175 in webkit

Timestamp:

Sep 20, 2012 4:16:16 PM (12 years ago)

Author:

bashi@chromium.org

Message:

[Chromium] Improve glyph selection of HarfBuzzShaper
​https://bugs.webkit.org/show_bug.cgi?id=97164

Reviewed by Tony Chang.

Source/WebCore:

Take into account clusters for selection.

Test: fast/text/international/hebrew-selection.html

• platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:

(WebCore::HarfBuzzShaper::HarfBuzzRun::applyShapeResult): Removed m_logClusters.
m_logCluster is no longer used.
(WebCore::HarfBuzzShaper::HarfBuzzRun::characterIndexForXPosition):

• If targetX is in the left side of the first cluster, return the leftmost character index.
• If targetX is in the right side of the last cluster, return the rightmost character index.
• If targetX is between the right side of the cluster N and the left side of the cluster N+1, then:
  • return N+1 for LTR.
  • return N for RTL.

(WebCore::HarfBuzzShaper::HarfBuzzRun::xPositionForOffset):
Find the cluster of index in question, then:

• return the left side boundary of the cluster for LTR.
• return the right side boundary of the cluster for RTL.
• platform/graphics/harfbuzz/ng/HarfBuzzShaper.h:

(HarfBuzzRun):

LayoutTests:

Added a test for complex text selection.

• fast/text/international/hebrew-selection-expected.html: Added.
• fast/text/international/hebrew-selection.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/text/international/hebrew-selection-expected.html (added)
• LayoutTests/fast/text/international/hebrew-selection.html (added)
• LayoutTests/platform/chromium/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp (modified) (5 diffs)
• Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-09-20  Kenichi Ishibashi  <bashi@chromium.org>
+
+        [Chromium] Improve glyph selection of HarfBuzzShaper
+        https://bugs.webkit.org/show_bug.cgi?id=97164
+
+        Reviewed by Tony Chang.
+
+        Added a test for complex text selection.
+
+        * fast/text/international/hebrew-selection-expected.html: Added.
+        * fast/text/international/hebrew-selection.html: Added.
+
 2012-09-20  Dirk Pranke  <dpranke@chromium.org>
 

trunk/LayoutTests/platform/chromium/TestExpectations

@@ -3559 +3559 @@
 webkit.org/b/97179 http/tests/css/link-css-disabled-value-with-slow-loading-sheet.html [ Failure Pass ]
 
-
+# The test should pass after harfbuzz transition.
+webkit.org/b/97164 [ Linux ] fast/text/international/hebrew-selection.html [ Failure ]
+

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-09-20  Kenichi Ishibashi  <bashi@chromium.org>
+
+        [Chromium] Improve glyph selection of HarfBuzzShaper
+        https://bugs.webkit.org/show_bug.cgi?id=97164
+
+        Reviewed by Tony Chang.
+
+        Take into account clusters for selection.
+
+        Test: fast/text/international/hebrew-selection.html
+
+        * platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:
+        (WebCore::HarfBuzzShaper::HarfBuzzRun::applyShapeResult): Removed m_logClusters.
+        m_logCluster is no longer used.
+        (WebCore::HarfBuzzShaper::HarfBuzzRun::characterIndexForXPosition):
+        - If targetX is in the left side of the first cluster, return the leftmost character index.
+        - If targetX is in the right side of the last cluster, return the rightmost character index.
+        - If targetX is between the right side of the cluster N and the left side of the cluster N+1, then:
+          - return N+1 for LTR.
+          - return N for RTL.
+        (WebCore::HarfBuzzShaper::HarfBuzzRun::xPositionForOffset):
+        Find the cluster of index in question, then:
+        - return the left side boundary of the cluster for LTR.
+        - return the right side boundary of the cluster for RTL.
+        * platform/graphics/harfbuzz/ng/HarfBuzzShaper.h:
+        (HarfBuzzRun):
+
 2012-09-20  Tony Chang  <tony@chromium.org>
 

trunk/Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp

@@ -87 +87 @@
     m_advances.resize(m_numGlyphs);
     m_glyphToCharacterIndexes.resize(m_numGlyphs);
-    m_logClusters.resize(m_numCharacters);
     m_offsets.resize(m_numGlyphs);
 
@@ -93 +92 @@
     for (unsigned i = 0; i < m_numGlyphs; ++i)
         m_glyphToCharacterIndexes[i] = infos[i].cluster;
-
-    // Fill logical clusters
-    unsigned index = 0;
-    while (index < m_numGlyphs) {
-        unsigned nextIndex = index + 1;
-        while (nextIndex < m_numGlyphs && infos[index].cluster == infos[nextIndex].cluster)
-            ++nextIndex;
-        if (rtl()) {
-            int nextCluster = nextIndex < m_numGlyphs ? infos[nextIndex].cluster : -1;
-            for (int j = infos[index].cluster; j > nextCluster; --j)
-                m_logClusters[j] = index;
-        } else {
-            unsigned nextCluster = nextIndex < m_numGlyphs ? infos[nextIndex].cluster : m_numCharacters;
-            for (unsigned j = infos[index].cluster; j < nextCluster; ++j)
-                m_logClusters[j] = index;
-        }
-        index = nextIndex;
-    }
 }
 
@@ -124 +105 @@
     ASSERT(targetX <= m_width);
     float currentX = 0;
-    float prevAdvance = 0;
-    for (unsigned i = 0; i < m_numGlyphs; ++i) {
-        float currentAdvance = m_advances[i] / 2.0;
+    float currentAdvance = m_advances[0];
+    unsigned glyphIndex = 0;
+
+    // Sum up advances that belong to a character.
+    while (glyphIndex < m_numGlyphs - 1 && m_glyphToCharacterIndexes[glyphIndex] == m_glyphToCharacterIndexes[glyphIndex + 1])
+        currentAdvance += m_advances[++glyphIndex];
+    currentAdvance = currentAdvance / 2.0;
+    if (targetX <= currentAdvance)
+        return rtl() ? m_numCharacters : 0;
+
+    ++glyphIndex;
+    while (glyphIndex < m_numGlyphs) {
+        unsigned prevCharacterIndex = m_glyphToCharacterIndexes[glyphIndex - 1];
+        float prevAdvance = currentAdvance;
+        currentAdvance = m_advances[glyphIndex];
+        while (glyphIndex < m_numGlyphs - 1 && m_glyphToCharacterIndexes[glyphIndex] == m_glyphToCharacterIndexes[glyphIndex + 1])
+            currentAdvance += m_advances[++glyphIndex];
+        currentAdvance = currentAdvance / 2.0;
         float nextX = currentX + prevAdvance + currentAdvance;
         if (currentX <= targetX && targetX <= nextX)
-            return m_glyphToCharacterIndexes[i] + (rtl() ? 1 : 0);
+            return rtl() ? prevCharacterIndex : m_glyphToCharacterIndexes[glyphIndex];
         currentX = nextX;
         prevAdvance = currentAdvance;
+        ++glyphIndex;
     }
 
@@ -140 +137 @@
 {
     ASSERT(offset < m_numCharacters);
-    unsigned glyphIndex = m_logClusters[offset];
-    ASSERT(glyphIndex <= m_numGlyphs);
+    unsigned glyphIndex = 0;
     float position = 0;
-    for (unsigned i = 0; i < glyphIndex; ++i)
-        position += m_advances[i];
-    if (rtl())
+    if (rtl()) {
+        while (glyphIndex < m_numGlyphs && m_glyphToCharacterIndexes[glyphIndex] > offset) {
+            position += m_advances[glyphIndex];
+            ++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_glyphToCharacterIndexes[glyphIndex] == m_glyphToCharacterIndexes[glyphIndex + 1]) {
+            position += m_advances[glyphIndex];
+            ++glyphIndex;
+        }
         position += m_advances[glyphIndex];
+    } else {
+        while (glyphIndex < m_numGlyphs && m_glyphToCharacterIndexes[glyphIndex] < offset) {
+            position += m_advances[glyphIndex];
+            ++glyphIndex;
+        }
+    }
     return position;
 }
@@ -158 +168 @@
         int nextPosition = position;
         U16_NEXT(source, nextPosition, length, character);
-        // Don't normalize tabs as they are not treated as spaces for word-end
+        // Don't normalize tabs as they are not treated as spaces for word-end.
         if (Font::treatAsSpace(character) && character != '\t')
             character = ' ';

trunk/Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.h

@@ -95 +95 @@
         Vector<uint16_t, 256> m_glyphs;
         Vector<float, 256> m_advances;
-        Vector<uint16_t, 256> m_logClusters;
         Vector<uint16_t, 256> m_glyphToCharacterIndexes;
         Vector<FloatPoint, 256> m_offsets;