Don't double-inflate visual overflow rect for LayoutInlines

Don't double-inflate visual overflow rect for LayoutInlines

linesVisualOverflowBoundingBox() already includes overflow from outlines
for the line (flow) boxes. Inflating with the outline again in
LayoutInline::clippedOverflowRect would mean doing it twice, and yield a
larger paint invalidation rect than required.

BUG=495368
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196697

[fs, 2015-06-08 10:58:10]

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

[fs, 2015-06-08 13:11:23]

Chris, as discussed in https://codereview.chromium.org/1163913002/

[Julien - ping for review, 2015-06-08 15:46:11]

lgtm, Levi should double check for some line-box related nastiness I could have
missed.

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right):

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40,
10],
Why do we have this unneeded invalidation?

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/inline-outline-repaint-2.html (right):

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2.html:8: .foo {
foo :(

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2.html:23:
document.querySelector('div').style.color = 'green';
document.getElementsByClassName['container'][0] (or even better switch container
to be an id and use getElementById) would show the intent a lot more.

[fs, 2015-06-08 16:15:37]

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right):

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40,
10],
On 2015/06/08 15:46:11, Julien Chaffraix - PST wrote:
> Why do we have this unneeded invalidation?

That's the LayoutText (shouldDoFullPaintInvalidation() and
layoutDidGetCalledSinceLastFrame() return true - reason is
PaintInvalidationLayoutObjectInsertion)

Can file a bug about it if you like.

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/inline-outline-repaint-2.html (right):

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2.html:8: .foo {
On 2015/06/08 15:46:11, Julien Chaffraix - PST wrote:
> foo :(

Restructured a bit and dropped the foo (in favor of "... > span"

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2.html:23:
document.querySelector('div').style.color = 'green';
On 2015/06/08 15:46:11, Julien Chaffraix - PST wrote:
> document.getElementsByClassName['container'][0] (or even better switch
container
> to be an id and use getElementById) would show the intent a lot more.

Changed to using an id.

[fs, 2015-06-08 16:29:12]

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right):

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40,
10],
On 2015/06/08 16:15:36, fs wrote:
> On 2015/06/08 15:46:11, Julien Chaffraix - PST wrote:
> > Why do we have this unneeded invalidation?
> 
> ... reason is PaintInvalidationLayoutObjectInsertion

Uhm, make that PaintInvalidationStyleChange...

[leviw_travelin_and_unemployed, 2015-06-08 20:16:00]

LGTM.

[fs, 2015-06-08 20:47:12]

The CQ bit was checked by fs@opera.com

[fs, 2015-06-08 20:47:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1164533005/#ps60001
(title: "Touch up Tc.")

[commit-bot: I haz the power, 2015-06-08 20:47:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164533005/60001

[commit-bot: I haz the power, 2015-06-08 21:39:36]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196697

[Julien - ping for review, 2015-06-08 22:19:48]

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right):

https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40,
10],
On 2015/06/08 at 16:29:12, fs wrote:
> On 2015/06/08 16:15:36, fs wrote:
> > On 2015/06/08 15:46:11, Julien Chaffraix - PST wrote:
> > > Why do we have this unneeded invalidation?
> > 
> > ... reason is PaintInvalidationLayoutObjectInsertion
> 
> Uhm, make that PaintInvalidationStyleChange...

It's weird that we do 2 invalidations when we don't move. We should just do a
single one and that would suggest some rectangle being wrong so it's bug-worthy.

[fs, 2015-06-09 07:35:23]

On 2015/06/08 22:19:48, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
> File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right):
> 
>
https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repain...
> LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40,
> 10],
> On 2015/06/08 at 16:29:12, fs wrote:
> > On 2015/06/08 16:15:36, fs wrote:
> > > On 2015/06/08 15:46:11, Julien Chaffraix - PST wrote:
> > > > Why do we have this unneeded invalidation?
> > > 
> > > ... reason is PaintInvalidationLayoutObjectInsertion
> > 
> > Uhm, make that PaintInvalidationStyleChange...
> 
> It's weird that we do 2 invalidations when we don't move. We should just do a
> single one and that would suggest some rectangle being wrong so it's
bug-worthy.

Filed crbug.com/498119