Find In Page hides the text when text color matches text search hightlight color.

third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp

Index: third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
diff --git a/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp b/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
index 7e086cd149dff55c2bfd253a458424ddf5655e10..cb816e051a8c12a13f618b72d4ece97e35b72caf 100644
--- a/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
+++ b/third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp
@@ -65,11 +65,15 @@ void SVGInlineTextBoxPainter::paint(const PaintInfo& paintInfo, const LayoutPoin
         // TODO(chrishtr): passing the cull rect is incorrect.
         DrawingRecorder recorder(paintInfo.context, m_svgInlineTextBox, displayItemType, FloatRect(paintInfo.cullRect().m_rect));
         InlineTextBoxPainter(m_svgInlineTextBox).paintDocumentMarkers(
-            paintInfo.context, paintOffset, style,
-            textLayoutObject.scaledFont(), true);
+            paintInfo, paintOffset, style,
+            textLayoutObject.scaledFont(), InlineTextBoxPainter::DocumentMarkerPaintPhase::Background);
 
         if (!m_svgInlineTextBox.textFragments().isEmpty())
             paintTextFragments(paintInfo, parentLayoutObject);
+
+        InlineTextBoxPainter(m_svgInlineTextBox).paintDocumentMarkers(
+            paintInfo, paintOffset, style,
+            textLayoutObject.scaledFont(), InlineTextBoxPainter::DocumentMarkerPaintPhase::Foreground);
     }
 }
 
@@ -169,25 +173,17 @@ void SVGInlineTextBoxPainter::paintSelectionBackground(const PaintInfo& paintInf
     int startPosition, endPosition;
     m_svgInlineTextBox.selectionStartEnd(startPosition, endPosition);
 
-    int fragmentStartPosition = 0;
-    int fragmentEndPosition = 0;
     AffineTransform fragmentTransform;
-    unsigned textFragmentsSize = m_svgInlineTextBox.textFragments().size();
-    for (unsigned i = 0; i < textFragmentsSize; ++i) {
-        const SVGTextFragment& fragment = m_svgInlineTextBox.textFragments().at(i);
-
-        fragmentStartPosition = startPosition;
-        fragmentEndPosition = endPosition;
-        if (!m_svgInlineTextBox.mapStartEndPositionsIntoFragmentCoordinates(fragment, fragmentStartPosition, fragmentEndPosition))
-            continue;
-
+    Vector<SVGTextFragmentWithRange> fragmentInfoList = collectFragmentsInRange(startPosition, endPosition);
+    for (const SVGTextFragmentWithRange& fragmentWithRange : fragmentInfoList) {
+        const SVGTextFragment& fragment = fragmentWithRange.fragment;
         GraphicsContextStateSaver stateSaver(paintInfo.context);
         fragment.buildFragmentTransform(fragmentTransform);
         if (!fragmentTransform.isIdentity())
             paintInfo.context.concatCTM(fragmentTransform);
 
         paintInfo.context.setFillColor(backgroundColor);
-        paintInfo.context.fillRect(m_svgInlineTextBox.selectionRectForTextFragment(fragment, fragmentStartPosition, fragmentEndPosition, style), backgroundColor);
+        paintInfo.context.fillRect(m_svgInlineTextBox.selectionRectForTextFragment(fragment, fragmentWithRange.startPosition, fragmentWithRange.endPosition, style), backgroundColor);
     }
 }
 
@@ -422,53 +418,87 @@ void SVGInlineTextBoxPainter::paintText(const PaintInfo& paintInfo, const Comput
     }
 }
 
-void SVGInlineTextBoxPainter::paintTextMatchMarker(GraphicsContext& context, const LayoutPoint&, DocumentMarker* marker, const ComputedStyle& style, const Font& font)
+Vector<SVGTextFragmentWithRange> SVGInlineTextBoxPainter::collectTextMatches(const PaintInfo& paintInfo, DocumentMarker* marker)

[fs, 2015/12/18 11:35:17]

|paintInfo| isn't used.

[ramya.v, 2015/12/21 08:13:49]

Done.

 {
-    // SVG is only interested in the TextMatch markers.
+    Vector<SVGTextFragmentWithRange> textMatchInfoList;
+
+    // SVG does not support grammar or spellcheck markers, so skip anything but TextMatch.
     if (marker->type() != DocumentMarker::TextMatch)
-        return;
+        return textMatchInfoList;
 
-    LayoutSVGInlineText& textLayoutObject = toLayoutSVGInlineText(*LineLayoutPaintShim::layoutObjectFrom(m_svgInlineTextBox.lineLayoutItem()));
+    if (!LineLayoutPaintShim::layoutObjectFrom(m_svgInlineTextBox.lineLayoutItem())->frame()->editor().markedTextMatchesAreHighlighted())
+        return textMatchInfoList;
 
-    AffineTransform fragmentTransform;
-    for (InlineTextBox* box = textLayoutObject.firstTextBox(); box; box = box->nextTextBox()) {
-        if (!box->isSVGInlineTextBox())
-            continue;
+    int markerStartPosition = std::max<int>(marker->startOffset() - m_svgInlineTextBox.start(), 0);

[pdr., 2015/12/19 02:39:45]

Can these be negative? If not, lets use unsigned (similarly for
collectFragmentsInRange).

[ramya.v, 2015/12/21 08:13:49]

This would depend on collectFragmentsInRange,
mapStartEndPositionsIntoFragmentCoordinates, selectionStartEnd etc.. which are
used in various places.

Something similar was done in webkit in
http://trac.webkit.org/changeset/170337 and reverted in 
http://trac.webkit.org/changeset/170947

Probably might need to check case by case. As of now added a TODO in
collectFragmentsInRange since that's a common function.

[fs, 2015/12/21 12:00:01]

I believe this one can - the only guarantee (made by
InlineTextBoxPainter::paintDocumentMarkers) is that there is overlap. (It's
worth noting that both of the operands to the subtraction is unsigned though so
we're relying on wrapping and unsigned -> int doing the right thing [which it
should in practice].) A different formulation would lend itself better to an
unsigned version though.

+    int markerEndPosition = std::min<int>(marker->endOffset() - m_svgInlineTextBox.start(), m_svgInlineTextBox.len());
 
-        SVGInlineTextBox* textBox = toSVGInlineTextBox(box);
+    if (markerStartPosition >= markerEndPosition)
+        return textMatchInfoList;
 
-        int markerStartPosition = std::max<int>(marker->startOffset() - textBox->start(), 0);
-        int markerEndPosition = std::min<int>(marker->endOffset() - textBox->start(), textBox->len());
+    textMatchInfoList = collectFragmentsInRange(markerStartPosition, markerEndPosition);

[fs, 2015/12/18 11:35:17]

Could just make this a tail-call ("return collectFragmentsInRange(...") and mark
|textMatchInfoList| const - maybe renaming it emptyTextMatchList or something.

[ramya.v, 2015/12/21 08:13:49]

Done.

+    return textMatchInfoList;
+}
 
-        if (markerStartPosition >= markerEndPosition)
+Vector<SVGTextFragmentWithRange> SVGInlineTextBoxPainter::collectFragmentsInRange(int startPosition, int endPosition)
+{
+    Vector<SVGTextFragmentWithRange> fragmentInfoList;
+    const Vector<SVGTextFragment>& fragments = m_svgInlineTextBox.textFragments();
+    unsigned textFragmentsSize = fragments.size();
+    for (unsigned i = 0; i < textFragmentsSize; ++i) {

[fs, 2015/12/18 11:35:17]

Use range-based for-loop.

[ramya.v, 2015/12/21 08:13:49]

Done.

+        const SVGTextFragment& fragment = fragments.at(i);
+
+        int fragmentStartPosition = startPosition;
+        int fragmentEndPosition = endPosition;
+        if (!m_svgInlineTextBox.mapStartEndPositionsIntoFragmentCoordinates(fragment, fragmentStartPosition, fragmentEndPosition))
             continue;
 
-        const Vector<SVGTextFragment>& fragments = textBox->textFragments();
-        unsigned textFragmentsSize = fragments.size();
-        for (unsigned i = 0; i < textFragmentsSize; ++i) {
-            const SVGTextFragment& fragment = fragments.at(i);
-
-            int fragmentStartPosition = markerStartPosition;
-            int fragmentEndPosition = markerEndPosition;
-            if (!textBox->mapStartEndPositionsIntoFragmentCoordinates(fragment, fragmentStartPosition, fragmentEndPosition))
-                continue;
-
-            FloatRect fragmentRect = textBox->selectionRectForTextFragment(fragment, fragmentStartPosition, fragmentEndPosition, style);
-            fragment.buildFragmentTransform(fragmentTransform);
-
-            // Draw the marker highlight.
-            if (LineLayoutPaintShim::layoutObjectFrom(m_svgInlineTextBox.lineLayoutItem())->frame()->editor().markedTextMatchesAreHighlighted()) {
-                Color color = marker->activeMatch() ?
-                    LayoutTheme::theme().platformActiveTextSearchHighlightColor() :
-                    LayoutTheme::theme().platformInactiveTextSearchHighlightColor();
-                GraphicsContextStateSaver stateSaver(context);
-                if (!fragmentTransform.isIdentity())
-                    context.concatCTM(fragmentTransform);
-                context.setFillColor(color);
-                context.fillRect(fragmentRect, color);
-            }
+        fragmentInfoList.append(SVGTextFragmentWithRange(fragment, fragmentStartPosition, fragmentEndPosition));
+    }
+    return fragmentInfoList;
+}
+
+void SVGInlineTextBoxPainter::paintTextMatchMarkerForeground(const PaintInfo& paintInfo, const LayoutPoint& point, DocumentMarker* marker, const ComputedStyle& style, const Font& font)
+{
+    Vector<SVGTextFragmentWithRange> textMatchInfoList = collectTextMatches(paintInfo, marker);
+    if (textMatchInfoList.isEmpty())
+        return;
+
+    Color textColor = LayoutTheme::theme().platformTextSearchColor(marker->activeMatch());
+    AffineTransform fragmentTransform;
+    for (const SVGTextFragmentWithRange& textMatchInfo : textMatchInfoList) {
+        const SVGTextFragment& fragment = textMatchInfo.fragment;
+        GraphicsContextStateSaver stateSaver(paintInfo.context);
+        fragment.buildFragmentTransform(fragmentTransform);
+        if (!fragmentTransform.isIdentity())
+            paintInfo.context.concatCTM(fragmentTransform);
+
+        TextRun textRun = m_svgInlineTextBox.constructTextRun(style, fragment);
+        SkPaint paint;
+        if (setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {

[fs, 2015/12/18 11:35:17]

Since this does not depend on anything within the loop, it should be possible to
hoist - including the setColor etc.

Also, this can return false if 'fill' is 'none' or otherwise invalid (in general
if it should not produce any "ink"). IIUC, that's not an effect you want here?

Additionally, as long only ApplyToFillMode is used here you could just do
something like:

SkPaint fillPaint;
fillPaint.setColor(textColor.rgb());
fillPaint.setAntiAlias(true);

and then use that to paint. Which seems simpler. And when adding
ApplyToStrokeMode you'll likely need to do that because of how setupTextPaint
returns early when there's no paint to be applied.

Speaking of stroke - maybe you should add that to one of the (pixel) tests.

[ramya.v, 2015/12/21 08:13:49]

Moved setupPaint out of loop. As with other browsers even if fill is none, and
only stroke is specified both fill and stroke are painted. So for fill created a
generic paint as mentioned by you. For stroke used setupPaint and only if it is
successful painting stoke inside loop. Linux seems to be fine, Win and Mac seems
like fill not getting painted. (text-match-highlight-expected.png) Do I have to
change anything else? Please let me know your opinion

[fs, 2015/12/21 12:00:01]

That's odd... It does look like it happens for the non-highlight case too though
(lots of drop-outs noticable), so presumably this is a separate issue. Please
make a TC from that and file a bug. From the PoV of this CL we should hopefully
be fine though.

[ramya.v, 2015/12/21 13:01:55]

To file a bug, same test case, if I directly load in chrome browser in windows
its painting fine. Test results are showing this way.

[fs, 2015/12/21 13:13:24]

Ok, so it's in the test-runner only... better make sure to make that clear in
the bug report then.

+            paint.setColor(textColor.rgb());
+            paintText(paintInfo, textRun, fragment, textMatchInfo.startPosition, textMatchInfo.endPosition, paint);
         }
     }
 }
 
+void SVGInlineTextBoxPainter::paintTextMatchMarkerBackground(const PaintInfo& paintInfo, const LayoutPoint& point, DocumentMarker* marker, const ComputedStyle& style, const Font& font)
+{
+    Vector<SVGTextFragmentWithRange> textMatchInfoList = collectTextMatches(paintInfo, marker);
+    if (textMatchInfoList.isEmpty())
+        return;
+
+    Color color = LayoutTheme::theme().platformTextSearchHighlightColor(marker->activeMatch());
+    AffineTransform fragmentTransform;
+    for (const SVGTextFragmentWithRange& textMatchInfo : textMatchInfoList) {
+        const SVGTextFragment& fragment = textMatchInfo.fragment;
+        GraphicsContextStateSaver stateSaver(paintInfo.context);
+        fragment.buildFragmentTransform(fragmentTransform);
+        if (!fragmentTransform.isIdentity())
+            paintInfo.context.concatCTM(fragmentTransform);
+        FloatRect fragmentRect = m_svgInlineTextBox.selectionRectForTextFragment(fragment, textMatchInfo.startPosition, textMatchInfo.endPosition, style);
+        paintInfo.context.setFillColor(color);
+        paintInfo.context.fillRect(fragmentRect, color);

[fs, 2015/12/18 11:35:17]

Passing color to fillRect is redundant with setFillColor, so you can drop one or
the other. (If the latter, then it would be possible to make the state-saver
conditional.)

[ramya.v, 2015/12/21 08:13:49]

Done.

+    }
+}
+
 } // namespace blink