Add a "logical query mode" to SVGTextQuery

Source/core/layout/svg/SVGTextQuery.cpp

Index: Source/core/layout/svg/SVGTextQuery.cpp
diff --git a/Source/core/layout/svg/SVGTextQuery.cpp b/Source/core/layout/svg/SVGTextQuery.cpp
index 490237a0386c0c4defe10d7ee1c2c110497d6082..159b33f0baa5557f51c525f3ac917cebc2127698 100644
--- a/Source/core/layout/svg/SVGTextQuery.cpp
+++ b/Source/core/layout/svg/SVGTextQuery.cpp
@@ -37,14 +37,14 @@ namespace blink {
 struct QueryData {
     QueryData()
         : isVerticalText(false)
-        , processedCharacters(0)
+        , currentOffset(0)
         , textLayoutObject(0)
         , textBox(0)
     {
     }
 
     bool isVerticalText;
-    unsigned processedCharacters;
+    unsigned currentOffset;
     LayoutSVGInlineText* textLayoutObject;
     const SVGInlineTextBox* textBox;
 };
@@ -101,36 +101,60 @@ static void collectTextBoxesInFlowBox(InlineFlowBox* flowBox, Vector<SVGInlineTe
 
 typedef bool ProcessTextFragmentCallback(QueryData*, const SVGTextFragment&);
 
-static bool executeQuery(LayoutObject* queryRoot, QueryData* queryData, ProcessTextFragmentCallback fragmentCallback)
+static bool queryTextBox(QueryData* queryData, const SVGInlineTextBox* textBox, ProcessTextFragmentCallback fragmentCallback)
+{
+    queryData->textBox = textBox;
+    queryData->textLayoutObject = &toLayoutSVGInlineText(textBox->layoutObject());
+
+    queryData->isVerticalText = textBox->layoutObject().style()->svgStyle().isVerticalWritingMode();
+    const Vector<SVGTextFragment>& fragments = textBox->textFragments();
+
+    // Loop over all text fragments in this text box, firing a callback for each.
+    unsigned fragmentCount = fragments.size();

[pdr., 2015/04/16 02:34:58]

Nit: range-based for loop would be slightly easier to read. (Similarly for for
loops in spatialQuery and logicalQuery)

[fs, 2015/04/16 14:22:21]

Changed this loop and the one in spatialQuery. The ones in logicalQuery() are
over line boxes, and I'm not aware of an adapted that would handle that - is
there one?

+    for (unsigned i = 0; i < fragmentCount; ++i) {
+        if (fragmentCallback(queryData, fragments.at(i)))
+            return true;
+    }
+    return false;
+}
+
+static void spatialQuery(LayoutObject* queryRoot, QueryData* queryData, ProcessTextFragmentCallback fragmentCallback)

[pdr., 2015/04/16 02:34:58]

Nit: Could you add a one-line comment here and above logicalQuery?

[fs, 2015/04/16 14:22:21]

Done. (2-lines though.)

 {
     Vector<SVGInlineTextBox*> textBoxes;
     collectTextBoxesInFlowBox(flowBoxForLayoutObject(queryRoot), textBoxes);
 
-    unsigned processedCharacters = 0;
     unsigned textBoxCount = textBoxes.size();
 
     // Loop over all text boxes
     for (unsigned textBoxPosition = 0; textBoxPosition < textBoxCount; ++textBoxPosition) {
-        queryData->textBox = textBoxes[textBoxPosition];
-        queryData->textLayoutObject = &toLayoutSVGInlineText(queryData->textBox->layoutObject());
-        ASSERT(queryData->textLayoutObject->style());
+        if (queryTextBox(queryData, textBoxes[textBoxPosition], fragmentCallback))
+            return;
+    }
+}
 
-        queryData->isVerticalText = queryData->textLayoutObject->style()->svgStyle().isVerticalWritingMode();
-        const Vector<SVGTextFragment>& fragments = queryData->textBox->textFragments();
+static void logicalQuery(LayoutObject* queryRoot, QueryData* queryData, ProcessTextFragmentCallback fragmentCallback)
+{
+    if (!queryRoot)
+        return;
 
-        // Loop over all text fragments in this text box, firing a callback for each.
-        unsigned fragmentCount = fragments.size();
-        for (unsigned i = 0; i < fragmentCount; ++i) {
-            const SVGTextFragment& fragment = fragments.at(i);
-            if (fragmentCallback(queryData, fragment))
-                return true;
+    // Walk the layout tree in pre-order, starting at the specified root, and
+    // run the query for each text node.
+    for (LayoutObject* layoutObject = queryRoot->slowFirstChild(); layoutObject; layoutObject = layoutObject->nextInPreOrder(queryRoot)) {
+        if (!layoutObject->isSVGInlineText())
+            continue;
 
-            processedCharacters += fragment.length;
-        }
+        LayoutSVGInlineText& textLayoutObject = toLayoutSVGInlineText(*layoutObject);
+        ASSERT(textLayoutObject.style());
 
-        queryData->processedCharacters = processedCharacters;
+        // TODO(fs): Allow filtering the search earlier, since we should be
+        // able to trivially reject (prune) at least some of the queries.
+
+        for (InlineTextBox* textBox = textLayoutObject.firstTextBox(); textBox; textBox = textBox->nextTextBox()) {

[pdr., 2015/04/16 02:34:58]

Does this need to be updated for each box?
for (...) {
    queryData->currentOffset += textBox.len();
    if (queryTextBox...)
...
}

If not, could we rename currentOffset to currentLayoutTextOffset or similar?

[fs, 2015/04/16 14:22:21]

Ouch, yes it should (or rather "needs to") of course... =/ - this causes lots of
ripples and complications...

+            if (queryTextBox(queryData, toSVGInlineTextBox(textBox), fragmentCallback))
+                return;
+        }
+        queryData->currentOffset += textLayoutObject.textLength();
     }
-    return false;
 }
 
 static void modifyStartEndPositionsRespectingLigatures(const QueryData* queryData, const SVGTextFragment& fragment, int& startPosition, int& endPosition)
@@ -167,9 +191,11 @@ static void modifyStartEndPositionsRespectingLigatures(const QueryData* queryDat
 
 static bool mapStartEndPositionsIntoFragmentCoordinates(const QueryData* queryData, const SVGTextFragment& fragment, int& startPosition, int& endPosition)
 {
+    unsigned boxStart = queryData->currentOffset + queryData->textBox->start();
+
     // Make <startPosition, endPosition> offsets relative to the current text box.
-    startPosition -= queryData->processedCharacters;
-    endPosition -= queryData->processedCharacters;
+    startPosition -= boxStart;
+    endPosition -= boxStart;
 
     // Reuse the same logic used for text selection & painting, to map our
     // query start/length into start/endPositions of the current text fragment.
@@ -191,8 +217,8 @@ static bool numberOfCharactersCallback(QueryData*, const SVGTextFragment&)
 unsigned SVGTextQuery::numberOfCharacters() const
 {
     QueryData data;
-    executeQuery(m_queryRootLayoutObject, &data, numberOfCharactersCallback);
-    return data.processedCharacters;
+    logicalQuery(m_queryRootLayoutObject, &data, numberOfCharactersCallback);
+    return data.currentOffset;
 }
 
 // textLength() implementation
@@ -215,7 +241,7 @@ static bool textLengthCallback(QueryData* queryData, const SVGTextFragment& frag
 float SVGTextQuery::textLength() const
 {
     TextLengthData data;
-    executeQuery(m_queryRootLayoutObject, &data, textLengthCallback);
+    logicalQuery(m_queryRootLayoutObject, &data, textLengthCallback);
     return data.textLength;
 }
 
@@ -251,7 +277,7 @@ static bool subStringLengthCallback(QueryData* queryData, const SVGTextFragment&
 float SVGTextQuery::subStringLength(unsigned startPosition, unsigned length) const
 {
     SubStringLengthData data(startPosition, length);
-    executeQuery(m_queryRootLayoutObject, &data, subStringLengthCallback);
+    logicalQuery(m_queryRootLayoutObject, &data, subStringLengthCallback);
     return data.subStringLength;
 }
 
@@ -318,7 +344,7 @@ static bool startPositionOfCharacterCallback(QueryData* queryData, const SVGText
 FloatPoint SVGTextQuery::startPositionOfCharacter(unsigned position) const
 {
     StartPositionOfCharacterData data(position);
-    executeQuery(m_queryRootLayoutObject, &data, startPositionOfCharacterCallback);
+    logicalQuery(m_queryRootLayoutObject, &data, startPositionOfCharacterCallback);
     return data.startPosition;
 }
 
@@ -352,7 +378,7 @@ static bool endPositionOfCharacterCallback(QueryData* queryData, const SVGTextFr
 FloatPoint SVGTextQuery::endPositionOfCharacter(unsigned position) const
 {
     EndPositionOfCharacterData data(position);
-    executeQuery(m_queryRootLayoutObject, &data, endPositionOfCharacterCallback);
+    logicalQuery(m_queryRootLayoutObject, &data, endPositionOfCharacterCallback);
     return data.endPosition;
 }
 
@@ -392,7 +418,7 @@ static bool rotationOfCharacterCallback(QueryData* queryData, const SVGTextFragm
 float SVGTextQuery::rotationOfCharacter(unsigned position) const
 {
     RotationOfCharacterData data(position);
-    executeQuery(m_queryRootLayoutObject, &data, rotationOfCharacterCallback);
+    logicalQuery(m_queryRootLayoutObject, &data, rotationOfCharacterCallback);
     return data.rotation;
 }
 
@@ -484,7 +510,7 @@ static bool extentOfCharacterCallback(QueryData* queryData, const SVGTextFragmen
 FloatRect SVGTextQuery::extentOfCharacter(unsigned position) const
 {
     ExtentOfCharacterData data(position);
-    executeQuery(m_queryRootLayoutObject, &data, extentOfCharacterCallback);
+    logicalQuery(m_queryRootLayoutObject, &data, extentOfCharacterCallback);
     return data.extent;
 }
 
@@ -492,12 +518,36 @@ FloatRect SVGTextQuery::extentOfCharacter(unsigned position) const
 struct CharacterNumberAtPositionData : QueryData {
     CharacterNumberAtPositionData(const FloatPoint& queryPosition)
         : position(queryPosition)
+        , hitLayoutObject(nullptr)
+        , offsetInTextNode(0)
     {
     }
 
+    int characterNumberWithin(const LayoutObject* queryRoot) const;
+
     FloatPoint position;
+    LayoutObject* hitLayoutObject;
+    int offsetInTextNode;

[pdr., 2015/04/16 02:34:58]

(not for this patch)
Should we be using unsigned long or long here for overflow handling?

[fs, 2015/04/16 14:22:21]

I don't see how this particular value would overflow, but the value we derive
from it (below, characterNumberWithin) could (in very extreme cases) I suppose.
This code (and I guess InlineBox in a wider sense) use signed and unsigned types
a bit too interchangeably (for my stomach) - so it's kind of hard to side with
one or the other. The return value here is speced as (IDL) long (==int) though,
so that's the primary motivation for that type here.

 };
 
+int CharacterNumberAtPositionData::characterNumberWithin(const LayoutObject* queryRoot) const
+{
+    if (!hitLayoutObject)

[pdr., 2015/04/16 02:34:58]

Can you add a comment here stating that this is per-spec?
https://svgwg.org/svg2-draft/single-page.html#text-__svg__SVGTextContentEleme...

[fs, 2015/04/16 14:22:21]

Done.

+        return -1;
+    ASSERT(queryRoot);
+    int characterNumber = offsetInTextNode;
+
+    // Accumulate the lengths of all the text nodes preceding the target layout
+    // object within the queried root, to get the complete character number.
+    for (const LayoutObject* layoutObject = hitLayoutObject->previousInPreOrder(queryRoot);
+        layoutObject; layoutObject = layoutObject->previousInPreOrder(queryRoot)) {
+        if (!layoutObject->isSVGInlineText())
+            continue;
+        characterNumber += toLayoutSVGInlineText(layoutObject)->textLength();
+    }
+    return characterNumber;
+}
+
 static bool characterNumberAtPositionCallback(QueryData* queryData, const SVGTextFragment& fragment)
 {
     CharacterNumberAtPositionData* data = static_cast<CharacterNumberAtPositionData*>(queryData);
@@ -516,10 +566,9 @@ static bool characterNumberAtPositionCallback(QueryData* queryData, const SVGTex
     while (fragmentOffset < fragment.length) {
         calculateGlyphBoundaries(queryData, fragment, fragmentOffset, extent);
         if (extent.contains(data->position)) {
-            // Compute the character offset of the glyph within the text box
-            // and add to processedCharacters.
-            unsigned characterOffset = fragment.characterOffset + fragmentOffset;
-            data->processedCharacters += characterOffset - data->textBox->start();
+            // Compute the character offset of the glyph within the text node.
+            data->offsetInTextNode = fragment.characterOffset + fragmentOffset;
+            data->hitLayoutObject = data->textLayoutObject;
             return true;
         }
         fragmentOffset += textMetrics[textMetricsOffset].length();
@@ -531,10 +580,8 @@ static bool characterNumberAtPositionCallback(QueryData* queryData, const SVGTex
 int SVGTextQuery::characterNumberAtPosition(const FloatPoint& position) const
 {
     CharacterNumberAtPositionData data(position);
-    if (!executeQuery(m_queryRootLayoutObject, &data, characterNumberAtPositionCallback))
-        return -1;
-
-    return data.processedCharacters;
+    spatialQuery(m_queryRootLayoutObject, &data, characterNumberAtPositionCallback);
+    return data.characterNumberWithin(m_queryRootLayoutObject);
 }
 
 }