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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 285103003: [RAL] Make sure RenderLayers are invalidated when moved. (Closed)

Created:
6 years, 7 months ago by dsinclair
Modified:
6 years, 6 months ago
CC:
blink-reviews, blink-reviews-rendering, zoltan1, eae+blinkwatch, leviw+renderwatch, blink-layers+watch_chromium.org, jchaffraix+rendering, pdr., rune+blink
Visibility:
Public.

Description

[RAL] Make sure RenderLayers are invalidated when moved. With Repaint-After-Layout we have to make sure that as layers are moved around we properly invalidate the old and new positions. To do this, I've updated the updateLayerPositions call to check if the previous and new repaint container offsets have changed. If so, I mark the object as needing invalidation. In order to not have an n^2 tree walk for each containerForRepaint() call I'm passing the repaint container down the tree as we recurse through updateLayerPositions. BUG=371640 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175388

Patch Set 1 : #

Patch Set 2 : Rebase to Master #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : Rebase to master. #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : Rebase to master #

Patch Set 9 : #

Patch Set 10 : Rebase to master #

Total comments: 8

Patch Set 11 : Rebase to master #

Patch Set 12 : #

Total comments: 8

Patch Set 13 : #

Patch Set 14 : Add test expectations for mac #

Total comments: 1

Patch Set 15 : #

Patch Set 16 : #

Total comments: 5

Patch Set 17 : Rebase to master #

Total comments: 2

Patch Set 18 : #

Total comments: 2

Patch Set 19 : #

Patch Set 20 : Rebase to master #

Patch Set 21 : Test Expectations #

Unified diffs Side-by-side diffs Delta from patch set Stats (+351 lines, -13 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/fast/repaint/crbug-371640.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +63 lines, -0 lines 0 comments Download
A LayoutTests/fast/repaint/crbug-371640-2.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +66 lines, -0 lines 0 comments Download
A + LayoutTests/fast/repaint/crbug-371640-2-expected.txt View 1 2 3 4 5 6 1 chunk +4 lines, -2 lines 0 comments Download
A LayoutTests/fast/repaint/crbug-371640-3.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +71 lines, -0 lines 0 comments Download
A + LayoutTests/fast/repaint/crbug-371640-3-expected.txt View 1 2 3 4 5 6 1 chunk +3 lines, -2 lines 0 comments Download
A LayoutTests/fast/repaint/crbug-371640-4.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +69 lines, -0 lines 0 comments Download
A LayoutTests/fast/repaint/crbug-371640-4-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/fast/repaint/crbug-371640-expected.txt View 1 chunk +18 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderBlock.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +5 lines, -3 lines 0 comments Download
M Source/core/rendering/RenderLayer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M Source/core/rendering/RenderLayer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +22 lines, -4 lines 0 comments Download
M Source/core/rendering/RenderLayerModelObject.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 23 (0 generated)
dsinclair
PTAL. I don't think this is a great fix. It feels like a really big ...
6 years, 7 months ago (2014-05-14 20:44:13 UTC) #1
dsinclair
PTAL. This should be good to go now I think.
6 years, 7 months ago (2014-05-23 16:58:21 UTC) #2
ojan
Can you give more context on why are floats special? https://codereview.chromium.org/285103003/diff/200001/LayoutTests/fast/repaint/crbug-371640.html File LayoutTests/fast/repaint/crbug-371640.html (right): https://codereview.chromium.org/285103003/diff/200001/LayoutTests/fast/repaint/crbug-371640.html#newcode2 ...
6 years, 7 months ago (2014-05-24 05:14:29 UTC) #3
dsinclair
It seems that, if you have a positioned object inside a float, it's possible for ...
6 years, 7 months ago (2014-05-26 14:57:24 UTC) #4
Julien - ping for review
https://codereview.chromium.org/285103003/diff/240001/LayoutTests/fast/repaint/crbug-371640-3.html File LayoutTests/fast/repaint/crbug-371640-3.html (right): https://codereview.chromium.org/285103003/diff/240001/LayoutTests/fast/repaint/crbug-371640-3.html#newcode56 LayoutTests/fast/repaint/crbug-371640-3.html:56: <p>If the test worked correctly you should see 1 ...
6 years, 7 months ago (2014-05-27 12:34:53 UTC) #5
dsinclair
https://codereview.chromium.org/285103003/diff/240001/LayoutTests/fast/repaint/crbug-371640-3.html File LayoutTests/fast/repaint/crbug-371640-3.html (right): https://codereview.chromium.org/285103003/diff/240001/LayoutTests/fast/repaint/crbug-371640-3.html#newcode56 LayoutTests/fast/repaint/crbug-371640-3.html:56: <p>If the test worked correctly you should see 1 ...
6 years, 7 months ago (2014-05-27 15:37:12 UTC) #6
leviw_travelin_and_unemployed
https://codereview.chromium.org/285103003/diff/280001/Source/core/rendering/RenderBlock.cpp File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/285103003/diff/280001/Source/core/rendering/RenderBlock.cpp#newcode380 Source/core/rendering/RenderBlock.cpp:380: bool isFloatingOrRelPositioned = isFloating() || isRelPositioned(); Can this not ...
6 years, 7 months ago (2014-05-27 17:27:56 UTC) #7
dsinclair
PTAL. The previous solution fell apart when I was adding the test leviw@ requested for ...
6 years, 6 months ago (2014-05-28 20:32:17 UTC) #8
leviw_travelin_and_unemployed
https://codereview.chromium.org/285103003/diff/320001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/285103003/diff/320001/Source/core/rendering/RenderLayer.cpp#newcode312 Source/core/rendering/RenderLayer.cpp:312: if (renderer()->previousPositionFromRepaintContainer() != renderer()->positionFromRepaintContainer(newRepaintContainer)) This is unfortunate duplicate work. ...
6 years, 6 months ago (2014-05-28 20:51:54 UTC) #9
leviw_travelin_and_unemployed
https://codereview.chromium.org/285103003/diff/320001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/285103003/diff/320001/Source/core/rendering/RenderLayer.cpp#newcode312 Source/core/rendering/RenderLayer.cpp:312: if (renderer()->previousPositionFromRepaintContainer() != renderer()->positionFromRepaintContainer(newRepaintContainer)) On 2014/05/28 20:51:55, leviw wrote: ...
6 years, 6 months ago (2014-05-28 21:02:00 UTC) #10
dsinclair
PTAL. This depends on another CL that is currently in the CQ but should be ...
6 years, 6 months ago (2014-06-02 18:14:33 UTC) #11
leviw_travelin_and_unemployed
https://codereview.chromium.org/285103003/diff/480001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/285103003/diff/480001/Source/core/rendering/RenderLayer.cpp#newcode302 Source/core/rendering/RenderLayer.cpp:302: if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && geometryMap) { Whoa... does this mean ...
6 years, 6 months ago (2014-06-02 20:22:18 UTC) #12
dsinclair
https://codereview.chromium.org/285103003/diff/480001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/285103003/diff/480001/Source/core/rendering/RenderLayer.cpp#newcode302 Source/core/rendering/RenderLayer.cpp:302: if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled() && geometryMap) { On 2014/06/02 20:22:19, leviw ...
6 years, 6 months ago (2014-06-02 20:36:18 UTC) #13
leviw_travelin_and_unemployed
https://codereview.chromium.org/285103003/diff/500001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/285103003/diff/500001/Source/core/rendering/RenderLayer.cpp#newcode309 Source/core/rendering/RenderLayer.cpp:309: renderer()->setMayNeedPaintInvalidation(true); I still don't love this We're only okay ...
6 years, 6 months ago (2014-06-02 20:47:20 UTC) #14
dsinclair
https://codereview.chromium.org/285103003/diff/500001/Source/core/rendering/RenderLayer.cpp File Source/core/rendering/RenderLayer.cpp (right): https://codereview.chromium.org/285103003/diff/500001/Source/core/rendering/RenderLayer.cpp#newcode309 Source/core/rendering/RenderLayer.cpp:309: renderer()->setMayNeedPaintInvalidation(true); On 2014/06/02 20:47:21, leviw wrote: > I still ...
6 years, 6 months ago (2014-06-02 20:58:06 UTC) #15
leviw_travelin_and_unemployed
LGTM. This makes it a little clearer for me.
6 years, 6 months ago (2014-06-03 00:14:51 UTC) #16
dsinclair
The CQ bit was checked by dsinclair@chromium.org
6 years, 6 months ago (2014-06-03 14:01:02 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsinclair@chromium.org/285103003/540001
6 years, 6 months ago (2014-06-03 14:01:23 UTC) #18
dsinclair
The CQ bit was unchecked by dsinclair@chromium.org
6 years, 6 months ago (2014-06-03 14:47:40 UTC) #19
dsinclair
The CQ bit was checked by dsinclair@chromium.org
6 years, 6 months ago (2014-06-03 14:49:34 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dsinclair@chromium.org/285103003/560001
6 years, 6 months ago (2014-06-03 14:49:51 UTC) #21
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink ...
6 years, 6 months ago (2014-06-03 16:08:10 UTC) #22
commit-bot: I haz the power
6 years, 6 months ago (2014-06-03 16:57:39 UTC) #23
Message was sent while issue was closed. 
Change committed as 175388

Powered by Google App Engine
This is Rietveld 408576698