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

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

Modifying both text color and text highlight color when text is
matched. Similar approach is followed in other browsers like IE,
Firefox, Safari for active matches.

IE: text color-black, highlight color-yellow
Firefox: text color-white, highlight color-green
Safari: text color-black, highlight color-yellow for active match.

BUG=512354
Committed: https://crrev.com/9fb75b70f133a81c3f4aa2b442cd06980cd3a7dc
Cr-Commit-Position: refs/heads/master@{#366382}

[ramya.v, 2015-11-25 11:51:55]

ramya.v@samsung.com changed reviewers:
+ pdr@chromium.org

[ramya.v, 2015-11-25 11:51:56]

PTAL!

Thanks

[rwlbuis, 2015-11-30 15:39:52]

rob.buis@samsung.com changed reviewers:
+ rob.buis@samsung.com

[rwlbuis, 2015-11-30 15:39:53]

Some comments.

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTheme.cpp (right):

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:830: return Color(0, 0,
0); // Black.
You should be able to use Color::black here, that way you do not need the
comment.

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:835: return Color(0, 0,
0); // Black.
Ditto.

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h:36: void
paintTextMatchMarker(GraphicsContext*, const LayoutPoint& boxOrigin,
DocumentMarker*, const ComputedStyle&, const Font&, bool = true);
You'll need a descriptive variable name here, or a descriptive enum (class).
Same in the other headers.

[ramya.v, 2015-12-01 03:52:41]

Hi Rob,
Updated as per review comments. PTAL!

Thanks

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTheme.cpp (right):

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:830: return Color(0, 0,
0); // Black.
On 2015/11/30 15:39:52, rwlbuis wrote:
> You should be able to use Color::black here, that way you do not need the
> comment.

Done.

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:835: return Color(0, 0,
0); // Black.
On 2015/11/30 15:39:52, rwlbuis wrote:
> Ditto.

Done.

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h:36: void
paintTextMatchMarker(GraphicsContext*, const LayoutPoint& boxOrigin,
DocumentMarker*, const ComputedStyle&, const Font&, bool = true);
On 2015/11/30 15:39:52, rwlbuis wrote:
> You'll need a descriptive variable name here, or a descriptive enum (class).
> Same in the other headers.

Done.

[pdr., 2015-12-01 23:58:48]

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/repaint/text-match.html (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/repaint/text-match.html:4: function
highlightRange(id, start, end, active) {
Indent 4, or 2, but not both.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTheme.h (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.h:120: virtual Color
platformActiveTextSearchHighlightColor() const;
Lets make these non-virtual too.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.h:122: virtual Color
platformActiveTextSearchColor() const;
Why are these virtual?

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.h:123: virtual Color
platformInactiveTextSearchColor() const;
Because all callsites have the same ternary statement, lets just refactor this
code entirely:
Color platformTextSearchHighlightColor(bool activeMatch) const;
Color platformTextSearchTextColor(bool activeMatch) const;

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.h (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.h:114: virtual void
paintTextMatchMarker(GraphicsContext*, const LayoutPoint& boxOrigin,
DocumentMarker*, const ComputedStyle&, const Font&, bool background = true)
const;
"bool background" is not a good name, but I have a suggestion below that lets us
remove it by splitting this into two functions.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:225:
SVGInlineTextBoxPainter(*this).paintTextMatchMarker(context, point, marker,
style, font);
You also need to update this function,
SVGInlineTextBoxPainter::paintTextMatchMarker. Please add a test for the svg
text match colors too, to prove it works.

Just create a second test like the one you added in this patch, but use this
instead of the span:
<svg width="300px" height="100px">
  <text x="20" y="20" style="font-family: Ahem; font-size: 20px;
fill:#FF9632;">findMe</text>
</svg>

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:831: void
InlineTextBoxPainter::paintTextMatchMarker(GraphicsContext* pt, const
LayoutPoint& boxOrigin, DocumentMarker* marker, const ComputedStyle& style,
const Font& font, bool background)
This function is getting more spaghetti by the day. It really does two distinct
things now: paintTextMatchMarkerForeground and paintTextMatchMarkerBackground.
Lets separate that functionality into two functions and extract out the
positioning code (spos, epos, textrun, etc) into a third function.

I think we can really clean up this class by removing the other duplicated
positioning logic found in paintSingleCompositionBackgroundRun and
paintDocumentMarker.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:842: // Optionally
highlight the text
This is a funny comment since it's all this function does. Lets remove it.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:852: // Change the
text color
This comment is not useful, lets remove it.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:855:
LayoutTheme::theme().platformActiveTextSearchColor() :
Indent these lines.

[ramya.v, 2015-12-10 09:39:37]

@pdr 
Modified as per review comments. PTAL!

Thanks

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/repaint/text-match.html (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/repaint/text-match.html:4: function
highlightRange(id, start, end, active) {
On 2015/12/01 23:58:48, pdr wrote:
> Indent 4, or 2, but not both.

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTheme.h (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.h:120: virtual Color
platformActiveTextSearchHighlightColor() const;
On 2015/12/01 23:58:48, pdr wrote:
> Lets make these non-virtual too.

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.h:122: virtual Color
platformActiveTextSearchColor() const;
On 2015/12/01 23:58:48, pdr wrote:
> Why are these virtual?

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTheme.h:123: virtual Color
platformInactiveTextSearchColor() const;
On 2015/12/01 23:58:48, pdr wrote:
> Because all callsites have the same ternary statement, lets just refactor this
> code entirely:
> Color platformTextSearchHighlightColor(bool activeMatch) const;
> Color platformTextSearchTextColor(bool activeMatch) const;

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineTextBox.h (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineTextBox.h:114: virtual void
paintTextMatchMarker(GraphicsContext*, const LayoutPoint& boxOrigin,
DocumentMarker*, const ComputedStyle&, const Font&, bool background = true)
const;
On 2015/12/01 23:58:48, pdr wrote:
> "bool background" is not a good name, but I have a suggestion below that lets
us
> remove it by splitting this into two functions.

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:225:
SVGInlineTextBoxPainter(*this).paintTextMatchMarker(context, point, marker,
style, font);
On 2015/12/01 23:58:48, pdr wrote:
> You also need to update this function,
> SVGInlineTextBoxPainter::paintTextMatchMarker. Please add a test for the svg
> text match colors too, to prove it works.
> 
> Just create a second test like the one you added in this patch, but use this
> instead of the span:
> <svg width="300px" height="100px">
>   <text x="20" y="20" style="font-family: Ahem; font-size: 20px;
> fill:#FF9632;">findMe</text>
> </svg>

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:831: void
InlineTextBoxPainter::paintTextMatchMarker(GraphicsContext* pt, const
LayoutPoint& boxOrigin, DocumentMarker* marker, const ComputedStyle& style,
const Font& font, bool background)
On 2015/12/01 23:58:48, pdr wrote:
> This function is getting more spaghetti by the day. It really does two
distinct
> things now: paintTextMatchMarkerForeground and paintTextMatchMarkerBackground.
> Lets separate that functionality into two functions and extract out the
> positioning code (spos, epos, textrun, etc) into a third function.
> 
> I think we can really clean up this class by removing the other duplicated
> positioning logic found in paintSingleCompositionBackgroundRun and
> paintDocumentMarker.



Separated code into two distinct functions.
Can I take up extracting duplicated code and cleaning up the class in a separate
cl?

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:842: // Optionally
highlight the text
On 2015/12/01 23:58:48, pdr wrote:
> This is a funny comment since it's all this function does. Lets remove it.

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:852: // Change the
text color
On 2015/12/01 23:58:48, pdr wrote:
> This comment is not useful, lets remove it.

Done.

https://codereview.chromium.org/1468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:855:
LayoutTheme::theme().platformActiveTextSearchColor() :
On 2015/12/01 23:58:48, pdr wrote:
> Indent these lines.

Done.

[pdr., 2015-12-14 22:53:26]

Getting very close! Just straightforward plumbing issues now.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:836: int sPos =
std::max(marker->startOffset() - m_inlineTextBox.start(), (unsigned)0);
I'm okay with doing this as a followup, but lets add a TODO so nobody forgets:
// TODO(ramya.v): Extract this into a helper function and share the many copies
of this code.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:840: if
(LineLayoutPaintShim::layoutObjectFrom(m_inlineTextBox.lineLayoutItem())->frame()->editor().markedTextMatchesAreHighlighted())
{
Lets move this up (here, and in paintTextMatchMarkerBackground):

void InlineTextBoxPainter::paintTextMatchMarkerForeground...
{
    if
(!LineLayoutPaintShim::layoutObjectFrom(m_inlineTextBox.lineLayoutItem())->frame()->editor().markedTextMatchesAreHighlighted())
        return false;

    ...
    int sPos = std::max(marker->startOffset() - m_inlineTextBox.start(),
(unsigned)0);
    ...
    Color textColor = marker->activeMatch() ?
    ...
}

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:411: // SVG is
only interested in the TextMatch markers.
This comment only says what the if statement below it does :)

How about:
// SVG does not support grammar or spellcheck markers, so skip anything but
TextMatch.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:417: for
(InlineTextBox* box = textLayoutObject.firstTextBox(); box; box =
box->nextTextBox()) {
This code is too much copy & paste from
SVGInlineTextBoxPainter::paintTextMatchMarkerBackground. Unlike the copied code
in InlineTextBoxPainter::paintTextMatchMarkerForeground, this code is all from
this patch so I think we should fix it in this patch.

Can you think of a way to reduce the copy & pasted code between this function
and paintTextMatchMarkerBackground? Helper functions, restructuring the code,
etc?

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:453:
markerStyle->setFillPaintColor(textColor);
ComputedStyle is too big to be copying here. Can you plumb this color through
without copying style?

[ramya.v, 2015-12-15 06:12:24]

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:836: int sPos =
std::max(marker->startOffset() - m_inlineTextBox.start(), (unsigned)0);
On 2015/12/14 22:53:26, pdr wrote:
> I'm okay with doing this as a followup, but lets add a TODO so nobody forgets:
> // TODO(ramya.v): Extract this into a helper function and share the many
copies
> of this code.

Done.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:840: if
(LineLayoutPaintShim::layoutObjectFrom(m_inlineTextBox.lineLayoutItem())->frame()->editor().markedTextMatchesAreHighlighted())
{
On 2015/12/14 22:53:26, pdr wrote:
> Lets move this up (here, and in paintTextMatchMarkerBackground):
> 
> void InlineTextBoxPainter::paintTextMatchMarkerForeground...
> {
>     if
>
(!LineLayoutPaintShim::layoutObjectFrom(m_inlineTextBox.lineLayoutItem())->frame()->editor().markedTextMatchesAreHighlighted())
>         return false;
> 
>     ...
>     int sPos = std::max(marker->startOffset() - m_inlineTextBox.start(),
> (unsigned)0);
>     ...
>     Color textColor = marker->activeMatch() ?
>     ...
> }

Done.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:411: // SVG is
only interested in the TextMatch markers.
On 2015/12/14 22:53:26, pdr wrote:
> This comment only says what the if statement below it does :)
> 
> How about:
> // SVG does not support grammar or spellcheck markers, so skip anything but
> TextMatch.

Done.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:417: for
(InlineTextBox* box = textLayoutObject.firstTextBox(); box; box =
box->nextTextBox()) {
On 2015/12/14 22:53:26, pdr wrote:
> This code is too much copy & paste from
> SVGInlineTextBoxPainter::paintTextMatchMarkerBackground. Unlike the copied
code
> in InlineTextBoxPainter::paintTextMatchMarkerForeground, this code is all from
> this patch so I think we should fix it in this patch.
> 
> Can you think of a way to reduce the copy & pasted code between this function
> and paintTextMatchMarkerBackground? Helper functions, restructuring the code,
> etc?

Done.

https://codereview.chromium.org/1468913002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:453:
markerStyle->setFillPaintColor(textColor);
On 2015/12/14 22:53:26, pdr wrote:
> ComputedStyle is too big to be copying here. Can you plumb this color through
> without copying style?

I tried plumbing color through GraphicsContext using setFillColor and calling
drawText (something similar done in paintTextWithShadows) but the color is not
getting reflected. Can you please suggest if there is any other way to achieve
the same.

[pdr., 2015-12-16 06:03:42]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[pdr., 2015-12-16 06:04:23]

+cc fs@opera too.

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:25: struct
TextMatchInfo {
Almost the same datastrcture is used for both selection background painting and
SVGInlineTextBoxPainter::paintText. I wonder if this could be named
"SVGTextFramentWithStartEnd" and used throughout SVGInlineTextBoxPainter?

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:47: void
paintTextMatchMarkerHelper(const PaintInfo&, DocumentMarker*,
Vector<TextMatchInfo>&);
A slightly cleaner function call would be:
Vector<TextMatchInfo> collectTextMatches(...);

To really solve the issue with which color is used for painting text, I think we
will need to dis-entangle the text painting code, maybe as a separate patch
before this one.

SVGInlineTextBoxPainter::paintTextWithShadows already does a bit of everything,
maybe that can be cleaned up? Just glancing at the code (so take this with a
grain of salt), maybe something like:
1) Split SVGInlineTextBoxPainter::paintTextWithShadows into:
  1.a) Create SkPaint SVGInlineTextBoxPainter::setupTextPaint(...)
  1.b) Create void SVGInlineTextBoxPainter::paintText(..., SkPaint);
  (ensuring the scalingFactor transform gets plumbed around properly might be a
little tricky)
2) Use SVGInlineTextBoxPainter::paintText in this patch but pass in a custom
SkPaint that just has the text colors you want, or possibly use setupTextPaint
and then override the colors to be what you want.

[fs, 2015-12-16 12:15:01]

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:25: struct
TextMatchInfo {
On 2015/12/16 at 06:04:23, pdr wrote:
> Almost the same datastrcture is used for both selection background painting
and SVGInlineTextBoxPainter::paintText. I wonder if this could be named
"SVGTextFramentWithStartEnd" and used throughout SVGInlineTextBoxPainter?

Bikeshed: SVGTextFragmentWithRange

On a related note, looks like the method that constructs these walk all the
Inline(Text)Boxes of the text node - isn't this method called on a specific
(SVG)InlineTextBox already so m_svgInlineTextBox could be used (and hence no
need to store the pointer here.) It might make sense to fix/cleanup that first
though.

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:47: void
paintTextMatchMarkerHelper(const PaintInfo&, DocumentMarker*,
Vector<TextMatchInfo>&);
On 2015/12/16 at 06:04:23, pdr wrote:
> A slightly cleaner function call would be:
> Vector<TextMatchInfo> collectTextMatches(...);
> 
> To really solve the issue with which color is used for painting text, I think
we will need to dis-entangle the text painting code, maybe as a separate patch
before this one.
> 
> SVGInlineTextBoxPainter::paintTextWithShadows already does a bit of
everything, maybe that can be cleaned up? Just glancing at the code (so take
this with a grain of salt), maybe something like:
> 1) Split SVGInlineTextBoxPainter::paintTextWithShadows into:
>   1.a) Create SkPaint SVGInlineTextBoxPainter::setupTextPaint(...)
>   1.b) Create void SVGInlineTextBoxPainter::paintText(..., SkPaint);
>   (ensuring the scalingFactor transform gets plumbed around properly might be
a little tricky)

Yeah a split that would make sense. I don't think you need to worry about the
"scalingFactor transform" - if it's the one I'm thinking of it's only used to
(potentially) setup a paint server. The only other assumption around that is
that the CTM isn't changed (which I think should be easy to guarantee.)

> 2) Use SVGInlineTextBoxPainter::paintText in this patch but pass in a custom
SkPaint that just has the text colors you want, or possibly use setupTextPaint
and then override the colors to be what you want.

Using setupTextPaint might be a little overkill - we'd only need it for setting
up the bits that affect geometry (stroke basically - which I notice isn't
painted with this CL currently).The stroke-setup could quite easily be split out
too though if needed.

[ramya.v, 2015-12-17 07:53:22]

@pdr/fr

Modified data structure as per review comments.
PTAL!

Thanks.

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:25: struct
TextMatchInfo {
On 2015/12/16 12:15:00, fs wrote:
> On 2015/12/16 at 06:04:23, pdr wrote:
> > Almost the same datastrcture is used for both selection background painting
> and SVGInlineTextBoxPainter::paintText. I wonder if this could be named
> "SVGTextFramentWithStartEnd" and used throughout SVGInlineTextBoxPainter?
> 
> Bikeshed: SVGTextFragmentWithRange
> 
> On a related note, looks like the method that constructs these walk all the
> Inline(Text)Boxes of the text node - isn't this method called on a specific
> (SVG)InlineTextBox already so m_svgInlineTextBox could be used (and hence no
> need to store the pointer here.) It might make sense to fix/cleanup that first
> though.

Done.

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:47: void
paintTextMatchMarkerHelper(const PaintInfo&, DocumentMarker*,
Vector<TextMatchInfo>&);
On 2015/12/16 12:15:01, fs wrote:
> On 2015/12/16 at 06:04:23, pdr wrote:
> > A slightly cleaner function call would be:
> > Vector<TextMatchInfo> collectTextMatches(...);
> > 
> > To really solve the issue with which color is used for painting text, I
think
> we will need to dis-entangle the text painting code, maybe as a separate patch
> before this one.
> > 
> > SVGInlineTextBoxPainter::paintTextWithShadows already does a bit of
> everything, maybe that can be cleaned up? Just glancing at the code (so take
> this with a grain of salt), maybe something like:
> > 1) Split SVGInlineTextBoxPainter::paintTextWithShadows into:
> >   1.a) Create SkPaint SVGInlineTextBoxPainter::setupTextPaint(...)
> >   1.b) Create void SVGInlineTextBoxPainter::paintText(..., SkPaint);
> >   (ensuring the scalingFactor transform gets plumbed around properly might
be
> a little tricky)
> 
> Yeah a split that would make sense. I don't think you need to worry about the
> "scalingFactor transform" - if it's the one I'm thinking of it's only used to
> (potentially) setup a paint server. The only other assumption around that is
> that the CTM isn't changed (which I think should be easy to guarantee.)
> 
> > 2) Use SVGInlineTextBoxPainter::paintText in this patch but pass in a custom
> SkPaint that just has the text colors you want, or possibly use setupTextPaint
> and then override the colors to be what you want.
> 
> Using setupTextPaint might be a little overkill - we'd only need it for
setting
> up the bits that affect geometry (stroke basically - which I notice isn't
> painted with this CL currently).The stroke-setup could quite easily be split
out
> too though if needed.


Added the patch to disentangle the text painting code in
https://codereview.chromium.org/1530853002/
Once that lands will modify paint to plumb color before calling paintText.

[fs, 2015-12-17 10:10:55]

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:322: break;
Nit: Maybe drop this too now that TextMatch is in the same bucket as
Composition.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:844: Color
textColor = marker->activeMatch() ?
Could just pass the predicate as the argument instead:

LayoutTheme::theme().platformTextSearchColor(marker->activeMatch())

(which would be more in line with bool-argument use.)

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:850: // When we
use the text as a clip, we only care about the alpha, thus we make all the
colors black.
This comment doesn't appear to make much sense in this context - I see it was
just moved, but I don't think it made sense previously either.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:876: Color color =
marker->activeMatch() ?
Same suggestion as for the equivalent code above (pass predicate as argument).

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:879:
GraphicsContext& pt = paintInfo.context;
pt -> context

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:76:
textLayoutObject.scaledFont(), false);
Maybe we should change this 'background' bool to an enum { MarkerForeground,
MarkerBackground } or some such. I guess we can punt on that though.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:179: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
Using a range-based loop here would be an improvement:

for (const SVGTextFragmentWithRange& fragmentWithRange : fragmentInfoList)

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:416: for
(InlineTextBox* box = textLayoutObject.firstTextBox(); box; box =
box->nextTextBox()) {
I still don't think this additional nesting is needed.

Actually, the way the code is written right now it essentially depends on not
really doing anything for any other inline-box (collectFragments operate on
m_svgInlineTextBox and not the box that's iterated here.)

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:431:
textMatchInfoList.append(fragmentList.at(i));
Use appendVector or appendRange instead. Per above it shouldn't be needed
though.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:437:
Vector<SVGTextFragmentWithRange> SVGInlineTextBoxPainter::collectFragments(int
startPosition, int endPosition)
collectFragmentsInRange or something more narrow like that seems more
appropriate.

That being said though, I'm a bit irked by the potential for allocator churn
here (especially for the <textPath> case where there'll potentially be lots of
fragments.) I guess we can wish for few fragments for now though...

Could probably use a "functional" iteration style, but let's go with this for
now.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:442: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
Use a range-based for-loop.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:455:
fragmentInfoList.append(fragmentInfo);
With a constructor for SVGTextFragmentWithRange, this would turn into:

fragmentInfoList.append(SVGTextFragmentWithRange(fragment, ..., ...));

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:467: Color
textColor = marker->activeMatch() ?
Same comment as for other instances of this.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:472: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
Use range-based for-loop.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:493: Color
color = marker->activeMatch() ?
Again.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:498: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
Use range-based for-loop.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:26: const
SVGTextFragment* fragment;
Maybe add a constructor and make this a reference instead.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:27: int
fragmentStartPosition;
I don't think we need to be quite this verbose. Dropping the 'fragment' prefix
seems entirely reasonable to me (it should be possible to infer from the
context).

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:38: virtual void
paintTextMatchMarkerBackground(const PaintInfo&, const LayoutPoint&,
DocumentMarker*, const ComputedStyle&, const Font&);
Why are these virtual?

[ramya.v, 2015-12-18 03:43:33]

Updated as per review comments.
PTAL!

Thanks

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:47: void
paintTextMatchMarkerHelper(const PaintInfo&, DocumentMarker*,
Vector<TextMatchInfo>&);
On 2015/12/17 07:53:22, ramya.v wrote:
> On 2015/12/16 12:15:01, fs wrote:
> > On 2015/12/16 at 06:04:23, pdr wrote:
> > > A slightly cleaner function call would be:
> > > Vector<TextMatchInfo> collectTextMatches(...);
> > > 
> > > To really solve the issue with which color is used for painting text, I
> think
> > we will need to dis-entangle the text painting code, maybe as a separate
patch
> > before this one.
> > > 
> > > SVGInlineTextBoxPainter::paintTextWithShadows already does a bit of
> > everything, maybe that can be cleaned up? Just glancing at the code (so take
> > this with a grain of salt), maybe something like:
> > > 1) Split SVGInlineTextBoxPainter::paintTextWithShadows into:
> > >   1.a) Create SkPaint SVGInlineTextBoxPainter::setupTextPaint(...)
> > >   1.b) Create void SVGInlineTextBoxPainter::paintText(..., SkPaint);
> > >   (ensuring the scalingFactor transform gets plumbed around properly might
> be
> > a little tricky)
> > 
> > Yeah a split that would make sense. I don't think you need to worry about
the
> > "scalingFactor transform" - if it's the one I'm thinking of it's only used
to
> > (potentially) setup a paint server. The only other assumption around that is
> > that the CTM isn't changed (which I think should be easy to guarantee.)
> > 
> > > 2) Use SVGInlineTextBoxPainter::paintText in this patch but pass in a
custom
> > SkPaint that just has the text colors you want, or possibly use
setupTextPaint
> > and then override the colors to be what you want.
> > 
> > Using setupTextPaint might be a little overkill - we'd only need it for
> setting
> > up the bits that affect geometry (stroke basically - which I notice isn't
> > painted with this CL currently).The stroke-setup could quite easily be split
> out
> > too though if needed.
> 
> 
> Added the patch to disentangle the text painting code in
> https://codereview.chromium.org/1530853002/
> Once that lands will modify paint to plumb color before calling paintText.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:322: break;
On 2015/12/17 10:10:54, fs wrote:
> Nit: Maybe drop this too now that TextMatch is in the same bucket as
> Composition.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:844: Color
textColor = marker->activeMatch() ?
On 2015/12/17 10:10:54, fs wrote:
> Could just pass the predicate as the argument instead:
> 
> LayoutTheme::theme().platformTextSearchColor(marker->activeMatch())
> 
> (which would be more in line with bool-argument use.)

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:850: // When we
use the text as a clip, we only care about the alpha, thus we make all the
colors black.
On 2015/12/17 10:10:54, fs wrote:
> This comment doesn't appear to make much sense in this context - I see it was
> just moved, but I don't think it made sense previously either.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:876: Color color =
marker->activeMatch() ?
On 2015/12/17 10:10:54, fs wrote:
> Same suggestion as for the equivalent code above (pass predicate as argument).

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:879:
GraphicsContext& pt = paintInfo.context;
On 2015/12/17 10:10:54, fs wrote:
> pt -> context

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:76:
textLayoutObject.scaledFont(), false);
On 2015/12/17 10:10:55, fs wrote:
> Maybe we should change this 'background' bool to an enum { MarkerForeground,
> MarkerBackground } or some such. I guess we can punt on that though.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:179: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
On 2015/12/17 10:10:55, fs wrote:
> Using a range-based loop here would be an improvement:
> 
> for (const SVGTextFragmentWithRange& fragmentWithRange : fragmentInfoList)

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:416: for
(InlineTextBox* box = textLayoutObject.firstTextBox(); box; box =
box->nextTextBox()) {
On 2015/12/17 10:10:54, fs wrote:
> I still don't think this additional nesting is needed.
> 
> Actually, the way the code is written right now it essentially depends on not
> really doing anything for any other inline-box (collectFragments operate on
> m_svgInlineTextBox and not the box that's iterated here.)

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:431:
textMatchInfoList.append(fragmentList.at(i));
On 2015/12/17 10:10:55, fs wrote:
> Use appendVector or appendRange instead. Per above it shouldn't be needed
> though.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:437:
Vector<SVGTextFragmentWithRange> SVGInlineTextBoxPainter::collectFragments(int
startPosition, int endPosition)
On 2015/12/17 10:10:54, fs wrote:
> collectFragmentsInRange or something more narrow like that seems more
> appropriate.
> 
> That being said though, I'm a bit irked by the potential for allocator churn
> here (especially for the <textPath> case where there'll potentially be lots of
> fragments.) I guess we can wish for few fragments for now though...
> 
> Could probably use a "functional" iteration style, but let's go with this for
> now.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:442: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
On 2015/12/17 10:10:55, fs wrote:
> Use a range-based for-loop.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:455:
fragmentInfoList.append(fragmentInfo);
On 2015/12/17 10:10:55, fs wrote:
> With a constructor for SVGTextFragmentWithRange, this would turn into:
> 
> fragmentInfoList.append(SVGTextFragmentWithRange(fragment, ..., ...));

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:467: Color
textColor = marker->activeMatch() ?
On 2015/12/17 10:10:55, fs wrote:
> Same comment as for other instances of this.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:472: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
On 2015/12/17 10:10:55, fs wrote:
> Use range-based for-loop.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:493: Color
color = marker->activeMatch() ?
On 2015/12/17 10:10:55, fs wrote:
> Again.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:498: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
On 2015/12/17 10:10:54, fs wrote:
> Use range-based for-loop.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:26: const
SVGTextFragment* fragment;
On 2015/12/17 10:10:55, fs wrote:
> Maybe add a constructor and make this a reference instead.

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:27: int
fragmentStartPosition;
On 2015/12/17 10:10:55, fs wrote:
> I don't think we need to be quite this verbose. Dropping the 'fragment' prefix
> seems entirely reasonable to me (it should be possible to infer from the
> context).

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:38: virtual void
paintTextMatchMarkerBackground(const PaintInfo&, const LayoutPoint&,
DocumentMarker*, const ComputedStyle&, const Font&);
On 2015/12/17 10:10:55, fs wrote:
> Why are these virtual?

Done.

https://codereview.chromium.org/1468913002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:38: virtual void
paintTextMatchMarkerBackground(const PaintInfo&, const LayoutPoint&,
DocumentMarker*, const ComputedStyle&, const Font&);
On 2015/12/17 10:10:55, fs wrote:
> Why are these virtual?

Since I've split paintTextMatchMarker took these in the same line. Since there
is no subclass to this class which needs to override this, removing for now.

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

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:421:
Vector<SVGTextFragmentWithRange>
SVGInlineTextBoxPainter::collectTextMatches(const PaintInfo& paintInfo,
DocumentMarker* marker)
|paintInfo| isn't used.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:438:
textMatchInfoList = collectFragmentsInRange(markerStartPosition,
markerEndPosition);
Could just make this a tail-call ("return collectFragmentsInRange(...") and mark
|textMatchInfoList| const - maybe renaming it emptyTextMatchList or something.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:447: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
Use range-based for-loop.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:477: if
(setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {
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.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:500:
paintInfo.context.fillRect(fragmentRect, color);
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.)

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

Just a few small nits. With fs's comments addressed this will be in pretty good
shape.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h:31: enum class
DocumentMarkerPaintPhase { Foreground, Background };
Wdyt about moving this up so you don't need to qualify this? e.g.,
"InlineTextBoxPainter::DocumentMarkerPaintPhase::Foreground"?

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:432: int
markerStartPosition = std::max<int>(marker->startOffset() -
m_svgInlineTextBox.start(), 0);
Can these be negative? If not, lets use unsigned (similarly for
collectFragmentsInRange).

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:53:
Vector<SVGTextFragmentWithRange> collectTextMatches(const PaintInfo&,
DocumentMarker*);
Nit: const

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:54:
Vector<SVGTextFragmentWithRange> collectFragmentsInRange(int startPosition, int
endPosition);
Nit2: const

[ramya.v, 2015-12-21 08:13:50]

PTAL!

Thanks

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.h:31: enum class
DocumentMarkerPaintPhase { Foreground, Background };
On 2015/12/19 02:39:45, pdr wrote:
> Wdyt about moving this up so you don't need to qualify this? e.g.,
> "InlineTextBoxPainter::DocumentMarkerPaintPhase::Foreground"?

Done.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:421:
Vector<SVGTextFragmentWithRange>
SVGInlineTextBoxPainter::collectTextMatches(const PaintInfo& paintInfo,
DocumentMarker* marker)
On 2015/12/18 11:35:17, fs wrote:
> |paintInfo| isn't used.

Done.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:432: int
markerStartPosition = std::max<int>(marker->startOffset() -
m_svgInlineTextBox.start(), 0);
On 2015/12/19 02:39:45, pdr wrote:
> Can these be negative? If not, lets use unsigned (similarly for
> collectFragmentsInRange).

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.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:438:
textMatchInfoList = collectFragmentsInRange(markerStartPosition,
markerEndPosition);
On 2015/12/18 11:35:17, fs wrote:
> Could just make this a tail-call ("return collectFragmentsInRange(...") and
mark
> |textMatchInfoList| const - maybe renaming it emptyTextMatchList or something.

Done.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:447: for
(unsigned i = 0; i < textFragmentsSize; ++i) {
On 2015/12/18 11:35:17, fs wrote:
> Use range-based for-loop.

Done.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:477: if
(setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {
On 2015/12/18 11:35:17, fs wrote:
> 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.

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

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:500:
paintInfo.context.fillRect(fragmentRect, color);
On 2015/12/18 11:35:17, fs wrote:
> 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.)

Done.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:53:
Vector<SVGTextFragmentWithRange> collectTextMatches(const PaintInfo&,
DocumentMarker*);
On 2015/12/19 02:39:45, pdr wrote:
> Nit: const

Done.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:54:
Vector<SVGTextFragmentWithRange> collectFragmentsInRange(int startPosition, int
endPosition);
On 2015/12/19 02:39:45, pdr wrote:
> Nit2: const

Done.

[ramya.v, 2015-12-21 11:35:04]

Patchset #20 (id:380001) has been deleted

[fs, 2015-12-21 12:00:02]

Just a few more nits.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:432: int
markerStartPosition = std::max<int>(marker->startOffset() -
m_svgInlineTextBox.start(), 0);
On 2015/12/19 at 02:39:45, pdr wrote:
> Can these be negative? If not, lets use unsigned (similarly for
collectFragmentsInRange).

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.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:477: if
(setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {
On 2015/12/21 at 08:13:49, ramya.v wrote:
> On 2015/12/18 11:35:17, fs wrote:
> > 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.
> 
> 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

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.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:53:
Vector<SVGTextFragmentWithRange> collectTextMatches(const PaintInfo&,
DocumentMarker*);
On 2015/12/21 at 08:13:50, ramya.v wrote:
> On 2015/12/19 02:39:45, pdr wrote:
> > Nit: const
> 
> Done.

I would've thought that pdr meant for the method to be const-qualified (i.e
const at "the end" rather than the beginning.) Making |fragmentInfoList| et.c
(i.e the "receivers") const would make sense too.

Ditto below.

https://codereview.chromium.org/1468913002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:470: bool
shouldApplyToStrokeMode  = false;
Nit: shouldPaintStroke

https://codereview.chromium.org/1468913002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:471: if
(setupTextPaint(paintInfo, style, ApplyToStrokeMode, strokePaint)) {
This will also apply text-shadow, so that should be cleared below as well
(strokePaint.setLooper(nullptr))

[ramya.v, 2015-12-21 13:01:56]

PTAL!

Thanks

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:477: if
(setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {
On 2015/12/21 12:00:01, fs wrote:
> On 2015/12/21 at 08:13:49, ramya.v wrote:
> > On 2015/12/18 11:35:17, fs wrote:
> > > 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.
> > 
> > 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
> 
> 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.



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.

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.h:53:
Vector<SVGTextFragmentWithRange> collectTextMatches(const PaintInfo&,
DocumentMarker*);
On 2015/12/21 12:00:01, fs wrote:
> On 2015/12/21 at 08:13:50, ramya.v wrote:
> > On 2015/12/19 02:39:45, pdr wrote:
> > > Nit: const
> > 
> > Done.
> 
> I would've thought that pdr meant for the method to be const-qualified (i.e
> const at "the end" rather than the beginning.) Making |fragmentInfoList| et.c
> (i.e the "receivers") const would make sense too.
> 
> Ditto below.

Done.

https://codereview.chromium.org/1468913002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:470: bool
shouldApplyToStrokeMode  = false;
On 2015/12/21 12:00:01, fs wrote:
> Nit: shouldPaintStroke

Done.

https://codereview.chromium.org/1468913002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:471: if
(setupTextPaint(paintInfo, style, ApplyToStrokeMode, strokePaint)) {
On 2015/12/21 12:00:01, fs wrote:
> This will also apply text-shadow, so that should be cleared below as well
> (strokePaint.setLooper(nullptr))

Done.

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

lgtm

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:477: if
(setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {
On 2015/12/21 at 13:01:55, ramya.v wrote:
> On 2015/12/21 12:00:01, fs wrote:
> > On 2015/12/21 at 08:13:49, ramya.v wrote:
> > > On 2015/12/18 11:35:17, fs wrote:
> > > > 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.
> > > 
> > > 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
> > 
> > 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.
> 
> 
> 
> 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.

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

[ramya.v, 2015-12-21 13:18:00]

The CQ bit was checked by ramya.v@samsung.com

[commit-bot: I haz the power, 2015-12-21 13:18:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1468913002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1468913002/420001

[ramya.v, 2015-12-21 13:34:28]

On 2015/12/21 13:13:24, fs wrote:
> lgtm
> 
>
https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/1468913002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:477: if
> (setupTextPaint(paintInfo, style, ApplyToFillMode, paint)) {
> On 2015/12/21 at 13:01:55, ramya.v wrote:
> > On 2015/12/21 12:00:01, fs wrote:
> > > On 2015/12/21 at 08:13:49, ramya.v wrote:
> > > > On 2015/12/18 11:35:17, fs wrote:
> > > > > 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.
> > > > 
> > > > 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
> > > 
> > > 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.
> > 
> > 
> > 
> > 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.
> 
> Ok, so it's in the test-runner only... better make sure to make that clear in
> the bug report then.

Done (In https://code.google.com/p/chromium/issues/detail?id=571380)

[commit-bot: I haz the power, 2015-12-21 14:45:04]

Committed patchset #21 (id:420001)

[commit-bot: I haz the power, 2015-12-21 14:46:35]

Description was changed from

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

Modifying both text color and text highlight color when text is
matched. Similar approach is followed in other browsers like IE,
Firefox, Safari for active matches.

IE: text color-black, highlight color-yellow
Firefox: text color-white, highlight color-green
Safari: text color-black, highlight color-yellow for active match.

BUG=512354
==========

to

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

Modifying both text color and text highlight color when text is
matched. Similar approach is followed in other browsers like IE,
Firefox, Safari for active matches.

IE: text color-black, highlight color-yellow
Firefox: text color-white, highlight color-green
Safari: text color-black, highlight color-yellow for active match.

BUG=512354

Committed: https://crrev.com/9fb75b70f133a81c3f4aa2b442cd06980cd3a7dc
Cr-Commit-Position: refs/heads/master@{#366382}
==========

[commit-bot: I haz the power, 2015-12-21 14:46:36]

Patchset 21 (id:??) landed as
https://crrev.com/9fb75b70f133a81c3f4aa2b442cd06980cd3a7dc
Cr-Commit-Position: refs/heads/master@{#366382}

[dmurph, 2015-12-21 22:06:15]

On 2015/12/21 at 14:46:36, commit-bot wrote:
> Patchset 21 (id:??) landed as
https://crrev.com/9fb75b70f133a81c3f4aa2b442cd06980cd3a7dc
> Cr-Commit-Position: refs/heads/master@{#366382}

I think this incorrectly rebaselined for win10 and winxp.  I'll change test
expectations.