Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(476)

Issue 1164533005: Don't double-inflate visual overflow rect for LayoutInlines (Closed)

Created:
5 years, 6 months ago by fs
Modified:
5 years, 6 months ago
CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Add additional test; TEs. #

Patch Set 3 : SP TEs. #

Total comments: 8

Patch Set 4 : Touch up Tc. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+36 lines, -5 lines) Patch
M LayoutTests/TestExpectations View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
A LayoutTests/fast/repaint/inline-outline-repaint-2.html View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
A + LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt View 1 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/layout/LayoutInline.cpp View 1 1 chunk +1 line, -4 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
fs
Chris, as discussed in https://codereview.chromium.org/1163913002/
5 years, 6 months ago (2015-06-08 13:11:23 UTC) #2
Julien - ping for review
lgtm, Levi should double check for some line-box related nastiness I could have missed. https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt ...
5 years, 6 months ago (2015-06-08 15:46:11 UTC) #3
fs
https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right): https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt#newcode9 LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40, 10], On 2015/06/08 15:46:11, Julien Chaffraix ...
5 years, 6 months ago (2015-06-08 16:15:37 UTC) #4
fs
https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right): https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt#newcode9 LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40, 10], On 2015/06/08 16:15:36, fs wrote: ...
5 years, 6 months ago (2015-06-08 16:29:12 UTC) #5
leviw_travelin_and_unemployed
LGTM.
5 years, 6 months ago (2015-06-08 20:16:00 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164533005/60001
5 years, 6 months ago (2015-06-08 20:47:43 UTC) #9
commit-bot: I haz the power
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196697
5 years, 6 months ago (2015-06-08 21:39:36 UTC) #10
Julien - ping for review
https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt File LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt (right): https://codereview.chromium.org/1164533005/diff/40001/LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt#newcode9 LayoutTests/fast/repaint/inline-outline-repaint-2-expected.txt:9: [215, 0, 40, 10], On 2015/06/08 at 16:29:12, fs ...
5 years, 6 months ago (2015-06-08 22:19:48 UTC) #11
fs
5 years, 6 months ago (2015-06-09 07:35:23 UTC) #12
Message was sent while issue was closed.
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

Powered by Google App Engine
This is Rietveld 408576698