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

Issue 2244873002: Fix PaintPropertyTreeBuilder for root layer scrolling. (Closed)

Created:
4 years, 4 months ago by szager1
Modified:
4 years, 4 months ago
Reviewers:
skobes, pdr., Xianzhu, trchen
CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src@rls-enable
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix PaintPropertyTreeBuilder for root layer scrolling. With RLS enabled, paint property trees will not use any properties set on the FrameView; instead, all properties will come from the LayoutView. BUG=490942 R=pdr@chromium.org,chrishtr@chromium.org,skobes@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/6ea7c61e26c872c72d7df02b1c4027b4bcd31588 Cr-Commit-Position: refs/heads/master@{#413343}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Disabled exact geometry checks when RLS is enabled #

Total comments: 26

Patch Set 3 : nits #

Patch Set 4 : Delete redundant test #

Patch Set 5 : TODO #

Total comments: 2

Patch Set 6 : No null clip for LocalBorderBoxProperties #

Patch Set 7 : Fix test expectation for rootClip #

Unified diffs Side-by-side diffs Delta from patch set Stats (+265 lines, -164 lines) Patch
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp View 1 2 3 4 5 8 chunks +61 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp View 1 2 3 4 5 6 53 chunks +200 lines, -153 lines 0 comments Download

Messages

Total messages: 75 (28 generated)
szager1
4 years, 4 months ago (2016-08-12 22:21:53 UTC) #1
skobes
I will defer to chrishtr and pdr on the core/paint changes. https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): ...
4 years, 4 months ago (2016-08-12 22:26:46 UTC) #4
szager1
https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode1131 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/12 22:26:46, skobes wrote: ...
4 years, 4 months ago (2016-08-12 22:44:31 UTC) #7
pdr.
https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode1131 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/12 at 22:44:31, szager1 ...
4 years, 4 months ago (2016-08-13 02:25:38 UTC) #8
pdr.
On 2016/08/13 at 02:25:38, pdr. wrote: > https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): > > https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode1131 ...
4 years, 4 months ago (2016-08-13 02:41:54 UTC) #9
szager1
On 2016/08/13 02:41:54, pdr. wrote: > On 2016/08/13 at 02:25:38, pdr. wrote: > > > ...
4 years, 4 months ago (2016-08-17 00:38:01 UTC) #15
szager1
Rebased and fixed tests, PTAL.
4 years, 4 months ago (2016-08-17 00:38:33 UTC) #16
skobes
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode1139 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) This still makes me nervous ...
4 years, 4 months ago (2016-08-17 01:38:24 UTC) #17
pdr.
+cc trchen for review
4 years, 4 months ago (2016-08-17 03:24:35 UTC) #21
pdr.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode1139 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/17 at 01:38:24, skobes ...
4 years, 4 months ago (2016-08-17 04:30:41 UTC) #22
trchen
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode1139 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/17 04:30:41, pdr. wrote: ...
4 years, 4 months ago (2016-08-17 05:57:48 UTC) #23
pdr.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp#newcode17 third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:17: namespace blink { On 2016/08/17 at 04:30:41, pdr. wrote: ...
4 years, 4 months ago (2016-08-17 06:35:44 UTC) #24
szager1
On 2016/08/17 06:35:44, pdr. wrote: > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:17: > namespace blink { > On 2016/08/17 at ...
4 years, 4 months ago (2016-08-17 19:21:29 UTC) #25
szager1
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode53 third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:53: } On 2016/08/17 05:57:47, trchen wrote: > I think ...
4 years, 4 months ago (2016-08-17 19:21:35 UTC) #26
pdr.
On 2016/08/17 at 19:21:29, szager wrote: > On 2016/08/17 06:35:44, pdr. wrote: > > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:17: ...
4 years, 4 months ago (2016-08-17 20:24:37 UTC) #27
szager1
On 2016/08/17 20:24:37, pdr. wrote: > On 2016/08/17 at 19:21:29, szager wrote: > > On ...
4 years, 4 months ago (2016-08-17 20:38:19 UTC) #28
pdr.
LGTM Please wait for an LGTM from trchen.
4 years, 4 months ago (2016-08-17 21:00:05 UTC) #29
skobes
lgtm2
4 years, 4 months ago (2016-08-17 21:01:34 UTC) #30
trchen
On 2016/08/17 20:38:19, szager1 wrote: > On 2016/08/17 20:24:37, pdr. wrote: > > On 2016/08/17 ...
4 years, 4 months ago (2016-08-17 21:05:23 UTC) #31
pdr.
On 2016/08/17 at 21:05:23, trchen wrote: > On 2016/08/17 20:38:19, szager1 wrote: > > On ...
4 years, 4 months ago (2016-08-17 21:08:14 UTC) #32
skobes
On 2016/08/17 21:05:23, trchen wrote: > The root cause is due to root LayoutView getting ...
4 years, 4 months ago (2016-08-17 21:12:59 UTC) #33
trchen
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode58 third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:58: frameView.y() + layoutView->location().y() + context.current.paintOffset.y()); On 2016/08/17 19:21:34, szager1 ...
4 years, 4 months ago (2016-08-17 21:30:55 UTC) #34
trchen
By the way, I'm tired of writing this everywhere: Settings* settings = object.document().settings(); forceScrollingForLayoutView = ...
4 years, 4 months ago (2016-08-17 21:33:53 UTC) #35
trchen
On 2016/08/17 21:12:59, skobes wrote: > On 2016/08/17 21:05:23, trchen wrote: > > The root ...
4 years, 4 months ago (2016-08-17 21:39:30 UTC) #36
skobes
On 2016/08/17 21:33:53, trchen wrote: > By the way, I'm tired of writing this everywhere: ...
4 years, 4 months ago (2016-08-17 21:43:36 UTC) #37
skobes
On 2016/08/17 21:39:30, trchen wrote: > We will still do scrolling through LayoutView, but we'll ...
4 years, 4 months ago (2016-08-18 00:34:17 UTC) #38
trchen
On 2016/08/18 00:34:17, skobes wrote: > On 2016/08/17 21:39:30, trchen wrote: > > We will ...
4 years, 4 months ago (2016-08-18 00:43:48 UTC) #39
szager1
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode58 third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:58: frameView.y() + layoutView->location().y() + context.current.paintOffset.y()); On 2016/08/17 21:30:55, trchen ...
4 years, 4 months ago (2016-08-19 01:52:13 UTC) #40
pdr.
@trchen, would you be okay with landing this as-is and resolving the frameview/layoutview issue when ...
4 years, 4 months ago (2016-08-19 02:09:45 UTC) #41
pdr.
On 2016/08/19 at 02:09:45, pdr. wrote: > @trchen, would you be okay with landing this ...
4 years, 4 months ago (2016-08-19 19:52:01 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/100001
4 years, 4 months ago (2016-08-19 19:53:37 UTC) #44
pdr.
On 2016/08/19 at 19:52:01, pdr. wrote: > On 2016/08/19 at 02:09:45, pdr. wrote: > > ...
4 years, 4 months ago (2016-08-19 20:04:14 UTC) #45
trchen
On 2016/08/19 20:04:14, pdr. wrote: > On 2016/08/19 at 19:52:01, pdr. wrote: > > On ...
4 years, 4 months ago (2016-08-19 20:46:08 UTC) #46
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/281391)
4 years, 4 months ago (2016-08-19 21:09:20 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/120001
4 years, 4 months ago (2016-08-19 22:10:44 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/277521)
4 years, 4 months ago (2016-08-20 00:43:23 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/120001
4 years, 4 months ago (2016-08-20 01:00:54 UTC) #55
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/277635)
4 years, 4 months ago (2016-08-20 02:57:26 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/120001
4 years, 4 months ago (2016-08-20 03:19:46 UTC) #59
commit-bot: I haz the power
Exceeded global retry quota
4 years, 4 months ago (2016-08-20 04:50:10 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/120001
4 years, 4 months ago (2016-08-20 09:34:28 UTC) #63
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/126527)
4 years, 4 months ago (2016-08-20 10:11:51 UTC) #65
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/120001
4 years, 4 months ago (2016-08-20 15:41:09 UTC) #67
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/126568)
4 years, 4 months ago (2016-08-20 16:59:58 UTC) #69
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2244873002/120001
4 years, 4 months ago (2016-08-20 17:08:18 UTC) #71
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 4 months ago (2016-08-20 18:18:41 UTC) #73
commit-bot: I haz the power
4 years, 4 months ago (2016-08-20 18:20:09 UTC) #75
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/6ea7c61e26c872c72d7df02b1c4027b4bcd31588
Cr-Commit-Position: refs/heads/master@{#413343}

Powered by Google App Engine
This is Rietveld 408576698