Optimize the characterNumberAtPositionCallback text query

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

 /*
  * Copyright (C) Research In Motion Limited 2010-2012. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 275 matching lines @@
 struct StartPositionOfCharacterData : QueryData {
     StartPositionOfCharacterData(unsigned queryPosition)
         : position(queryPosition)
     {
     }
 
     unsigned position;
     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);
     }
     return glyphPosition;
 }
 
 static bool startPositionOfCharacterCallback(QueryData* queryData, const SVGTextFragment& fragment)
 {
     StartPositionOfCharacterData* data = static_cast<StartPositionOfCharacterData*>(queryData);
@@ skipping 87 matching lines @@
 struct ExtentOfCharacterData : QueryData {
     ExtentOfCharacterData(unsigned queryPosition)
         : position(queryPosition)
     {
     }
 
     unsigned position;
     FloatRect extent;
 };
 
+static FloatRect physicalGlyphExtents(const QueryData* queryData, const SVGTextMetrics& metrics, const FloatPoint& glyphPosition)
+{
+    // TODO(fs): Negative glyph extents seems kind of weird to have, but
+    // presently it can occur in some cases (like Arabic.)
+    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)
+            glyphExtents.move(0, -glyphExtents.height());
+        else
+            glyphExtents.move(-glyphExtents.width(), 0);
+    }
+    return glyphExtents;
+}
+
 static inline FloatRect calculateGlyphBoundaries(const QueryData* queryData, const SVGTextFragment& fragment, int startPosition)
 {
-    float scalingFactor = queryData->textLineLayout.scalingFactor();
+    const float scalingFactor = queryData->textLineLayout.scalingFactor();
     ASSERT(scalingFactor);
+    const float baseline = queryData->textLineLayout.scaledFont().getFontMetrics().floatAscent() / scalingFactor;
 
-    FloatPoint glyphPosition = calculateGlyphPositionWithoutTransform(queryData, fragment, startPosition);
-    glyphPosition.move(0, -queryData->textLineLayout.scaledFont().getFontMetrics().floatAscent() / scalingFactor);
+    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& textMetricsValues = queryData->textLineLayout.layoutAttributes()->textMetricsValues();
-    auto metrics = findMetricsForCharacter(textMetricsValues, fragment, startPosition);
+    const MetricsList& metricsList = queryData->textLineLayout.layoutAttributes()->textMetricsValues();
+    auto metrics = findMetricsForCharacter(metricsList, 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));
-
-    // 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());
-        else
-            glyphPosition.move(-glyphSize.width(), 0);
-    }
-
-    FloatRect extent(glyphPosition, glyphSize);
+    FloatRect extent = physicalGlyphExtents(queryData, *metrics, glyphPosition);
     if (fragment.isTransformed()) {
         AffineTransform fragmentTransform = fragment.buildFragmentTransform(SVGTextFragment::TransformIgnoringTextLength);
         extent = fragmentTransform.mapRect(extent);
     }
     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);
 
     int startPosition = data->position;
     int endPosition = startPosition + 1;
     if (!mapStartEndPositionsIntoFragmentCoordinates(queryData, fragment, startPosition, endPosition))
         return false;
 
     data->extent = calculateGlyphBoundaries(queryData, fragment, startPosition);
@@ skipping 57 matching lines @@
         --index;
         offset += textBoxes[index]->len();
     }
     return offset;
 }
 
 static bool characterNumberAtPositionCallback(QueryData* queryData, const SVGTextFragment& fragment)
 {
     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;
             data->offsetInTextNode = logicalOffsetInTextNode(queryData->textLineLayout, queryData->textBox, offsetInBox);
             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;
 }
 
 int SVGTextQuery::characterNumberAtPosition(const FloatPoint& position) const
 {
     CharacterNumberAtPositionData data(position);
     spatialQuery(m_queryRootLayoutObject, &data, characterNumberAtPositionCallback);
     return data.characterNumberWithin(m_queryRootLayoutObject);
 }
 
 } // namespace blink