Refine cull rect for SVGInlineTextBox painting

Refine cull rect for SVGInlineTextBox painting

The PaintInfo cull rect can (will) change between paints, so use
the moral equivalent of logicalOverflowRect() to compute a more
accurate cull rect instead.

This fixes under-invalidation in the following tests:

 svg/dynamic-updates/SVGTextElement-dom-lengthAdjust-attr.html,
 svg/dynamic-updates/SVGTextElement-dom-textLength-attr.html,
 svg/dynamic-updates/SVGTextElement-svgdom-lengthAdjust-prop.html and
 svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html

Exposed by https://codereview.chromium.org/2104943005

BUG=231560
Committed: https://crrev.com/1d600ed3a61c66119bf732d3b17d3e8372c8cdb7
Cr-Commit-Position: refs/heads/master@{#404265}

[fs, 2016-07-05 11:07:02]

Description was changed from

==========
Refine cull rect for SVGInlineTextBox painting

The PaintInfo cull rect can (will) change between paints, so use
logicalOverflowRect() to compute a more accurate cull rect instead.

This fixes under-invalidation in the following tests:

 svg/dynamic-updates/SVGTextElement-dom-lengthAdjust-attr.html,
 svg/dynamic-updates/SVGTextElement-dom-textLength-attr.html,
 svg/dynamic-updates/SVGTextElement-svgdom-lengthAdjust-prop.html and
 svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html

Exposed by https://codereview.chromium.org/2104943005

BUG=231560
==========

to

==========
Refine cull rect for SVGInlineTextBox painting

The PaintInfo cull rect can (will) change between paints, so use
the moral equivalent of logicalOverflowRect() to compute a more
accurate cull rect instead.

This fixes under-invalidation in the following tests:

 svg/dynamic-updates/SVGTextElement-dom-lengthAdjust-attr.html,
 svg/dynamic-updates/SVGTextElement-dom-textLength-attr.html,
 svg/dynamic-updates/SVGTextElement-svgdom-lengthAdjust-prop.html and
 svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html

Exposed by https://codereview.chromium.org/2104943005

BUG=231560
==========

[fs, 2016-07-05 14:31:08]

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

[chrishtr, 2016-07-06 20:53:20]

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:70: // We
compute the paint rect with what looks like the logical values, to match the
Factor all this into a method saying it's computing the cull rect for painting?

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:75:
LayoutSize(m_svgInlineTextBox.logicalWidth(),
m_svgInlineTextBox.logicalHeight()));
Physical, not logical.

[pdr., 2016-07-06 21:04:20]

LGTM with Chris's comments addressed.

If you want, you can add the following to the patch description:
This patch has a side benefit of using the correct opaque bounds in
spv2 since we no longer have infinite non-opaque bounds. This fixes
lcd text antialiasing bugs on many tests and we pass 193 new tests with
spv2 enabled (1360 failures to 1167).

[pdr., 2016-07-06 21:06:46]

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:36: static bool
paintsMarkerHighlights(const LayoutObject& layoutObject)
Make InlineTextBoxPainter's paintsMarkerHighlights a protected member function
and use it here too?

[fs, 2016-07-06 21:32:41]

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:36: static bool
paintsMarkerHighlights(const LayoutObject& layoutObject)
On 2016/07/06 at 21:06:46, pdr. wrote:
> Make InlineTextBoxPainter's paintsMarkerHighlights a protected member function
and use it here too?

SVGInlineTextBoxPainter are not related to InlineTextBoxPainter by inheritance
(only by name and function), but I can add it as a public (static) entry point -
or a protected one with a friend declaration (I think I prefer the former.) We
could probably add an inheritance relation-ship here, but that might call for an
unproportional amount of extra work. Other options?

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:70: // We
compute the paint rect with what looks like the logical values, to match the
On 2016/07/06 at 20:53:20, chrishtr wrote:
> Factor all this into a method saying it's computing the cull rect for
painting?

Will do.

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:75:
LayoutSize(m_svgInlineTextBox.logicalWidth(),
m_svgInlineTextBox.logicalHeight()));
On 2016/07/06 at 20:53:20, chrishtr wrote:
> Physical, not logical.

Per the comment above this code, this is intentionally using logical* (because
"weirdness and legacy" presumably). I can make sure the helper method I'll add
starts with 'physical' though.

[pdr., 2016-07-06 21:46:47]

On 2016/07/06 at 21:32:41, fs wrote:
>
https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:36: static
bool paintsMarkerHighlights(const LayoutObject& layoutObject)
> On 2016/07/06 at 21:06:46, pdr. wrote:
> > Make InlineTextBoxPainter's paintsMarkerHighlights a protected member
function and use it here too?
> 
> SVGInlineTextBoxPainter are not related to InlineTextBoxPainter by inheritance
(only by name and function), but I can add it as a public (static) entry point -
or a protected one with a friend declaration (I think I prefer the former.) We
could probably add an inheritance relation-ship here, but that might call for an
unproportional amount of extra work. Other options?

Oops, I forgot about how those classes don't inherit. Yeah, a public static on
InlineTextboxPainter sounds good to me too.

[fs, 2016-07-06 23:33:30]

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2124553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:70: // We
compute the paint rect with what looks like the logical values, to match the
On 2016/07/06 at 21:32:41, fs wrote:
> On 2016/07/06 at 20:53:20, chrishtr wrote:
> > Factor all this into a method saying it's computing the cull rect for
painting?
> 
> Will do.

Moved into a boundsFor... method.

[fs, 2016-07-07 21:29:40]

The CQ bit was checked by fs@opera.com

[fs, 2016-07-07 21:29:40]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2124553002/#ps60001
(title: "Add boundsForDrawingRecorder; expose helper from InlineTextBoxPainter")

[commit-bot: I haz the power, 2016-07-07 21:30:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 22:50:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-07 22:52:58]

Description was changed from

==========
Refine cull rect for SVGInlineTextBox painting

The PaintInfo cull rect can (will) change between paints, so use
the moral equivalent of logicalOverflowRect() to compute a more
accurate cull rect instead.

This fixes under-invalidation in the following tests:

 svg/dynamic-updates/SVGTextElement-dom-lengthAdjust-attr.html,
 svg/dynamic-updates/SVGTextElement-dom-textLength-attr.html,
 svg/dynamic-updates/SVGTextElement-svgdom-lengthAdjust-prop.html and
 svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html

Exposed by https://codereview.chromium.org/2104943005

BUG=231560
==========

to

==========
Refine cull rect for SVGInlineTextBox painting

The PaintInfo cull rect can (will) change between paints, so use
the moral equivalent of logicalOverflowRect() to compute a more
accurate cull rect instead.

This fixes under-invalidation in the following tests:

 svg/dynamic-updates/SVGTextElement-dom-lengthAdjust-attr.html,
 svg/dynamic-updates/SVGTextElement-dom-textLength-attr.html,
 svg/dynamic-updates/SVGTextElement-svgdom-lengthAdjust-prop.html and
 svg/dynamic-updates/SVGTextElement-svgdom-textLength-prop.html

Exposed by https://codereview.chromium.org/2104943005

BUG=231560

Committed: https://crrev.com/1d600ed3a61c66119bf732d3b17d3e8372c8cdb7
Cr-Commit-Position: refs/heads/master@{#404265}
==========

[commit-bot: I haz the power, 2016-07-07 22:52:59]

Patchset 4 (id:??) landed as
https://crrev.com/1d600ed3a61c66119bf732d3b17d3e8372c8cdb7
Cr-Commit-Position: refs/heads/master@{#404265}