Optimize the characterNumberAtPositionCallback text query

third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp

Index: third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp
diff --git a/third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp b/third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp
index 8c3173e3171fb19055ac71e968374813c92f7cd8..ac581197255d351d179ad426e62846964117ecc0 100644
--- a/third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp
+++ b/third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp
@@ -293,27 +293,27 @@ struct StartPositionOfCharacterData : QueryData {
     FloatPoint startPosition;
 };
 
-static FloatPoint calculateGlyphPositionWithoutTransform(const QueryData* queryData, const SVGTextFragment& fragment, unsigned offsetInFragment)
+static FloatPoint logicalGlyphPositionToPhysical(const QueryData* queryData, const SVGTextFragment& fragment, float logicalGlyphOffset)
 {
-    float glyphOffsetInDirection = calculateGlyphRange(queryData, fragment, 0, offsetInFragment);
-
+    float physicalGlyphOffset = logicalGlyphOffset;
     if (!queryData->textBox->isLeftToRightDirection()) {
         float fragmentExtent = queryData->isVerticalText ? fragment.height : fragment.width;
-        glyphOffsetInDirection = fragmentExtent - glyphOffsetInDirection;
+        physicalGlyphOffset = fragmentExtent - logicalGlyphOffset;
     }
 
     FloatPoint glyphPosition(fragment.x, fragment.y);
     if (queryData->isVerticalText)
-        glyphPosition.move(0, glyphOffsetInDirection);
+        glyphPosition.move(0, physicalGlyphOffset);
     else
-        glyphPosition.move(glyphOffsetInDirection, 0);
+        glyphPosition.move(physicalGlyphOffset, 0);
 
     return glyphPosition;
 }
 
 static FloatPoint calculateGlyphPosition(const QueryData* queryData, const SVGTextFragment& fragment, unsigned offsetInFragment)
 {
-    FloatPoint glyphPosition = calculateGlyphPositionWithoutTransform(queryData, fragment, offsetInFragment);
+    float glyphOffsetInDirection = calculateGlyphRange(queryData, fragment, 0, offsetInFragment);
+    FloatPoint glyphPosition = logicalGlyphPositionToPhysical(queryData, fragment, glyphOffsetInDirection);
     if (fragment.isTransformed()) {
         AffineTransform fragmentTransform = fragment.buildFragmentTransform(SVGTextFragment::TransformIgnoringTextLength);
         glyphPosition = fragmentTransform.mapPoint(glyphPosition);
@@ -421,33 +421,41 @@ struct ExtentOfCharacterData : QueryData {
     FloatRect extent;
 };
 
-static inline FloatRect calculateGlyphBoundaries(const QueryData* queryData, const SVGTextFragment& fragment, int startPosition)
+static FloatRect physicalGlyphExtents(const QueryData* queryData, const SVGTextMetrics& metrics, const FloatPoint& glyphPosition)
 {
-    float scalingFactor = queryData->textLineLayout.scalingFactor();
-    ASSERT(scalingFactor);
-
-    FloatPoint glyphPosition = calculateGlyphPositionWithoutTransform(queryData, fragment, startPosition);
-    glyphPosition.move(0, -queryData->textLineLayout.scaledFont().getFontMetrics().floatAscent() / scalingFactor);
-
-    // Use the SVGTextMetrics computed by SVGTextMetricsBuilder (which spends
-    // time attempting to compute more correct glyph bounds already, handling
-    // cursive scripts to some degree.)
-    const MetricsList& textMetricsValues = queryData->textLineLayout.layoutAttributes()->textMetricsValues();
-    auto metrics = findMetricsForCharacter(textMetricsValues, fragment, startPosition);
-
     // TODO(fs): Negative glyph extents seems kind of weird to have, but
     // presently it can occur in some cases (like Arabic.)
-    FloatSize glyphSize(std::max<float>(metrics->width(), 0), std::max<float>(metrics->height(), 0));
+    FloatRect glyphExtents(
+        glyphPosition,
+        FloatSize(std::max<float>(metrics.width(), 0), std::max<float>(metrics.height(), 0)));
 
     // If RTL, adjust the starting point to align with the LHS of the glyph bounding box.
     if (!queryData->textBox->isLeftToRightDirection()) {
         if (queryData->isVerticalText)
-            glyphPosition.move(0, -glyphSize.height());
+            glyphExtents.move(0, -glyphExtents.height());
         else
-            glyphPosition.move(-glyphSize.width(), 0);
+            glyphExtents.move(-glyphExtents.width(), 0);
     }
+    return glyphExtents;
+}
+
+static inline FloatRect calculateGlyphBoundaries(const QueryData* queryData, const SVGTextFragment& fragment, int startPosition)
+{
+    const float scalingFactor = queryData->textLineLayout.scalingFactor();
+    ASSERT(scalingFactor);
+    const float baseline = queryData->textLineLayout.scaledFont().getFontMetrics().floatAscent() / scalingFactor;
 
-    FloatRect extent(glyphPosition, glyphSize);
+    float glyphOffsetInDirection = calculateGlyphRange(queryData, fragment, 0, startPosition);
+    FloatPoint glyphPosition = logicalGlyphPositionToPhysical(queryData, fragment, glyphOffsetInDirection);
+    glyphPosition.move(0, -baseline);
+
+    // Use the SVGTextMetrics computed by SVGTextMetricsBuilder (which spends

[pdr., 2016/03/21 22:55:56]

Not your fault but can you clean this comment up? What does "to some degree"
mean?

[fs, 2016/03/22 10:55:30]

Well I think I wrote this comment, so somewhat my fault =). I think the "to some
degree" was getting at the poor quality of the advances/bounds in those cases at
the time of writing - hopefully it's better now.

Parenthesis removed.

+    // time attempting to compute more correct glyph bounds already, handling
+    // cursive scripts to some degree.)
+    const MetricsList& metricsList = queryData->textLineLayout.layoutAttributes()->textMetricsValues();
+    auto metrics = findMetricsForCharacter(metricsList, fragment, startPosition);
+
+    FloatRect extent = physicalGlyphExtents(queryData, *metrics, glyphPosition);
     if (fragment.isTransformed()) {
         AffineTransform fragmentTransform = fragment.buildFragmentTransform(SVGTextFragment::TransformIgnoringTextLength);
         extent = fragmentTransform.mapRect(extent);
@@ -455,14 +463,6 @@ static inline FloatRect calculateGlyphBoundaries(const QueryData* queryData, con
     return extent;
 }
 
-static inline FloatRect calculateFragmentBoundaries(LineLayoutSVGInlineText textLineLayout, const SVGTextFragment& fragment)
-{
-    float scalingFactor = textLineLayout.scalingFactor();
-    ASSERT(scalingFactor);
-    float baseline = textLineLayout.scaledFont().getFontMetrics().floatAscent() / scalingFactor;
-    return fragment.boundingBox(baseline);
-}
-
 static bool extentOfCharacterCallback(QueryData* queryData, const SVGTextFragment& fragment)
 {
     ExtentOfCharacterData* data = static_cast<ExtentOfCharacterData*>(queryData);
@@ -540,18 +540,27 @@ static bool characterNumberAtPositionCallback(QueryData* queryData, const SVGTex
 {
     CharacterNumberAtPositionData* data = static_cast<CharacterNumberAtPositionData*>(queryData);
 
+    const float scalingFactor = data->textLineLayout.scalingFactor();
+    ASSERT(scalingFactor);
+    const float baseline = data->textLineLayout.scaledFont().getFontMetrics().floatAscent() / scalingFactor;
+
     // Test the query point against the bounds of the entire fragment first.
-    FloatRect fragmentExtents = calculateFragmentBoundaries(queryData->textLineLayout, fragment);
-    if (!fragmentExtents.contains(data->position))
+    if (!fragment.boundingBox(baseline).contains(data->position))
         return false;
 
+    AffineTransform fragmentTransform = fragment.buildFragmentTransform(SVGTextFragment::TransformIgnoringTextLength);
+
     // Iterate through the glyphs in this fragment, and check if their extents
     // contain the query point.
-    const Vector<SVGTextMetrics>& textMetrics = queryData->textLineLayout.layoutAttributes()->textMetricsValues();
-    unsigned textMetricsOffset = fragment.metricsListOffset;
+    MetricsList::const_iterator metrics =
+        data->textLineLayout.layoutAttributes()->textMetricsValues().begin() + fragment.metricsListOffset;
     unsigned fragmentOffset = 0;
+    float glyphOffset = 0;
     while (fragmentOffset < fragment.length) {
-        FloatRect extent = calculateGlyphBoundaries(queryData, fragment, fragmentOffset);
+        FloatPoint glyphPosition = logicalGlyphPositionToPhysical(data, fragment, glyphOffset);
+        glyphPosition.move(0, -baseline);
+
+        FloatRect extent = fragmentTransform.mapRect(physicalGlyphExtents(data, *metrics, glyphPosition));
         if (extent.contains(data->position)) {
             // Compute the character offset of the glyph within the text node.
             unsigned offsetInBox = fragment.characterOffset - queryData->textBox->start() + fragmentOffset;
@@ -559,8 +568,9 @@ static bool characterNumberAtPositionCallback(QueryData* queryData, const SVGTex
             data->hitLayoutItem = LineLayoutItem(data->textLineLayout);
             return true;
         }
-        fragmentOffset += textMetrics[textMetricsOffset].length();
-        textMetricsOffset++;
+        fragmentOffset += metrics->length();
+        glyphOffset += data->isVerticalText ? metrics->height() : metrics->width();
+        ++metrics;
     }
     return false;
 }