|
|
Created:
4 years, 4 months ago by szager1 Modified:
4 years, 4 months ago 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. |
DescriptionFix 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 #
Messages
Total messages: 75 (28 generated)
The CQ bit was checked by pdr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I will defer to chrishtr and pdr on the core/paint changes. https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) The purpose of this check is to pull the scrollbars out of the page scale layer so they don't grow and shrink with pinch zoom. Don't we want to keep that behavior in slimming paint v2?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/12 22:26:46, skobes wrote: > The purpose of this check is to pull the scrollbars out of the page scale layer > so they don't grow and shrink with pinch zoom. Don't we want to keep that > behavior in slimming paint v2? As I understand it -- and this was all explained to me by pdr@ -- all of this GraphicsLayer stuff is basically obsolete when SPv2 is enabled. There's only one graphics layer, and the compositor does the rest. So really, most or all of CompositedLayerMapping will go away. The reason I put this here is because it prevents a crash, but with SPv2 this probably all pointless and obsolete anyway.
https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/12 at 22:44:31, szager1 wrote: > On 2016/08/12 22:26:46, skobes wrote: > > The purpose of this check is to pull the scrollbars out of the page scale layer > > so they don't grow and shrink with pinch zoom. Don't we want to keep that > > behavior in slimming paint v2? > > As I understand it -- and this was all explained to me by pdr@ -- all of this GraphicsLayer stuff is basically obsolete when SPv2 is enabled. There's only one graphics layer, and the compositor does the rest. So really, most or all of CompositedLayerMapping will go away. > > The reason I put this here is because it prevents a crash, but with SPv2 this probably all pointless and obsolete anyway. We do want to maintain the pinch/zoom behavior, but using transform nodes instead of graphics layers. I don't think anyone has thought about how either pinch/zoom or page scale will hook up in spv2 though. For pinch/zoom, could we add a special-case transform property node on layoutView that is before the transform node? We can't reuse the layoutView's transform property since it can get used (e.g., http://jsbin.com/zabufa). Any way we go, we'll need special logic to use one transform node for the scrollbars but another for the contents :/ SPV2 will indeed get rid of GraphicsLayer altogether, deferring to cc for what gets layerized.
On 2016/08/13 at 02:25:38, pdr. wrote: > https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): > > https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) > On 2016/08/12 at 22:44:31, szager1 wrote: > > On 2016/08/12 22:26:46, skobes wrote: > > > The purpose of this check is to pull the scrollbars out of the page scale layer > > > so they don't grow and shrink with pinch zoom. Don't we want to keep that > > > behavior in slimming paint v2? > > > > As I understand it -- and this was all explained to me by pdr@ -- all of this GraphicsLayer stuff is basically obsolete when SPv2 is enabled. There's only one graphics layer, and the compositor does the rest. So really, most or all of CompositedLayerMapping will go away. > > > > The reason I put this here is because it prevents a crash, but with SPv2 this probably all pointless and obsolete anyway. > > We do want to maintain the pinch/zoom behavior, but using transform nodes instead of graphics layers. I don't think anyone has thought about how either pinch/zoom or page scale will hook up in spv2 though. For pinch/zoom, could we add a special-case transform property node on layoutView that is before the transform node? We can't reuse the layoutView's transform property since it can get used (e.g., http://jsbin.com/zabufa). Any way we go, we'll need special logic to use one transform node for the scrollbars but another for the contents :/ > > SPV2 will indeed get rid of GraphicsLayer altogether, deferring to cc for what gets layerized. Skobes pointed out that I was confused here... In the example, the LayoutView does not have a transform but instead that's owned by the html's LayoutBlockFlow. What properties can apply to a LayoutView? We may be able to re-use some of it's ObjectPaintProperties, even if special code will still be needed to ensure they apply in the correct order for scrollbars.
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by szager@chromium.org to run a CQ dry run
Description was changed from ========== 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 ========== to ========== 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 ==========
szager@chromium.org changed reviewers: + wangxianzhu@chromium.org - chrishtr@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/13 02:41:54, pdr. wrote: > On 2016/08/13 at 02:25:38, pdr. wrote: > > > https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... > > File > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > (right): > > > > > https://codereview.chromium.org/2244873002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1131: > if (m_isMainFrameLayoutViewLayer && > !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) > > On 2016/08/12 at 22:44:31, szager1 wrote: > > > On 2016/08/12 22:26:46, skobes wrote: > > > > The purpose of this check is to pull the scrollbars out of the page scale > layer > > > > so they don't grow and shrink with pinch zoom. Don't we want to keep that > > > > behavior in slimming paint v2? > > > > > > As I understand it -- and this was all explained to me by pdr@ -- all of > this GraphicsLayer stuff is basically obsolete when SPv2 is enabled. There's > only one graphics layer, and the compositor does the rest. So really, most or > all of CompositedLayerMapping will go away. > > > > > > The reason I put this here is because it prevents a crash, but with SPv2 > this probably all pointless and obsolete anyway. > > > > We do want to maintain the pinch/zoom behavior, but using transform nodes > instead of graphics layers. I don't think anyone has thought about how either > pinch/zoom or page scale will hook up in spv2 though. For pinch/zoom, could we > add a special-case transform property node on layoutView that is before the > transform node? We can't reuse the layoutView's transform property since it can > get used (e.g., http://jsbin.com/zabufa). Any way we go, we'll need special > logic to use one transform node for the scrollbars but another for the contents > :/ > > > > SPV2 will indeed get rid of GraphicsLayer altogether, deferring to cc for what > gets layerized. > > Skobes pointed out that I was confused here... In the example, the LayoutView > does not have a transform but instead that's owned by the html's > LayoutBlockFlow. > > What properties can apply to a LayoutView? We may be able to re-use some of it's > ObjectPaintProperties, even if special code will still be needed to ensure they > apply in the correct order for scrollbars. With this change, LayoutView will have: - paintOffsetTranslation, which would encompass, e.g., the border on an iframe - scrollTranslation, just like any other scrolling LayoutBox - overflowClip, just like any other scrolling LayoutBox
Rebased and fixed tests, PTAL.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) This still makes me nervous but if the paint team is ok with it then *shrug*. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:357: if (object.isLayoutView()) { Why does the LayoutView have a separate path here? It's a little unclear what the difference is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pdr@chromium.org changed reviewers: + trchen@chromium.org
+cc trchen for review
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/17 at 01:38:24, skobes wrote: > This still makes me nervous but if the paint team is ok with it then *shrug*. I agree but we just aren't far enough along to block you on this :( I've filed https://crbug.com/638473 for this work. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:128: if (object.isLayoutView()) Can you add a comment here about why we don't clear the paint offset translation, below? https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:17: namespace blink { This patch fails about 900 new LayoutTests when run with: --additional-driver-flag=--enable-slimming-paint-v2 --additional-driver-flag=--root-layer-scrolls --retry-failures There looks to be two classes of failures: 1) Nothing renders if a frame scrollbar is present 2) Random pixel differences--maybe a snapping difference somewhere? https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:21: , public ::testing::WithParamInterface<FrameSettingOverrideFunction> { This approach of reusing the existing tests and adding rootTransform/rootClip/etc is pretty cool https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:233: TEST_P(PaintPropertyTreeBuilderTest, DISABLED_FrameScrollingRootLayerScrolls) Can you remove DISABLED_ from this test? I think it needs some minor fixing but nothing major.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1139: if (m_isMainFrameLayoutViewLayer && !RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2016/08/17 04:30:41, pdr. wrote: > On 2016/08/17 at 01:38:24, skobes wrote: > > This still makes me nervous but if the paint team is ok with it then *shrug*. > > I agree but we just aren't far enough along to block you on this :( > > I've filed https://crbug.com/638473 for this work. I think this line is totally fine. The long term goal is to bypass core/layout/compositing. Today we are still using PaintLayerCompositor just because it is a pain to duplicate some basic setup. If possible I don't even want LayoutView to create CLM... Because it pops main frame scrollbars to a graphics layer. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:53: } I think we should not create effect nodes for LayoutView explicitly here. I recall that accessibility folks are interested in applying filters on LayoutView, so LayoutView may have real effect nodes in the future. My mental model is that every LayoutObject that is a stacking context (or stacked for being positioned) should create an effect node, but we don't actually create them if they can be safely omitted. For the case of root LayoutView, effect node can't be omitted because someone needs to create root surface. I'd rather do this in PaintPropertyTreeBuilder::updateEffect() line #202 instead of here. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:58: frameView.y() + layoutView->location().y() + context.current.paintOffset.y()); It feels weird. We should not include LayoutView location here as it defeats the purpose of RLS --- eliminate cronyism between FrameView and LayoutView. And doing FrameView location here is weird. I think these should stay in frameView.preTranslation, as they belong to Widget, not LayoutView. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:128: if (object.isLayoutView()) On 2016/08/17 04:30:41, pdr. wrote: > Can you add a comment here about why we don't clear the paint offset > translation, below? I'd rather add CHECK(!object.isLayoutView()) on line #109 to make sure LayoutView never have paint offset. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:357: if (object.isLayoutView()) { On 2016/08/17 01:38:24, skobes wrote: > Why does the LayoutView have a separate path here? It's a little unclear what > the difference is. Yea... I can't spot any difference either. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:384: if (object.isLayoutView()) { It is unfortunate that canContainFixedPositionObjects() returns true for LayoutView. I think LayoutView should behave as if canContainFixedPositionObjects() being false, whether in RLS mode or not.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:17: namespace blink { On 2016/08/17 at 04:30:41, pdr. wrote: > This patch fails about 900 new LayoutTests when run with: > --additional-driver-flag=--enable-slimming-paint-v2 --additional-driver-flag=--root-layer-scrolls --retry-failures > > There looks to be two classes of failures: > 1) Nothing renders if a frame scrollbar is present > 2) Random pixel differences--maybe a snapping difference somewhere? I debugged the first issue down a bit. Because we're using a different main GraphicsLayer now, we need to paint that GraphicsLayer and use its PaintController. Something like the following might work, or we could move around the GraphicsLayers in the visual viewport. +++ b/third_party/WebKit/Source/core/frame/FrameView.cpp @@ -2663,6 +2663,13 @@ void FrameView::synchronizedPaint() ASSERT(!view.isNull()); forAllNonThrottledFrameViews([](FrameView& frameView) { frameView.lifecycle().advanceTo(DocumentLifecycle::InPaint); }); + PaintLayer* layer = layoutViewItem().layer(); + if (RuntimeEnabledFeatures::slimmingPaintV2Enabled() && layer && layer->hasCompositedLayerMapping()) { + if (GraphicsLayer* mainGraphicsLayer = layer->compositedLayerMapping()->mainGraphicsLayer()) + synchronizedPaintRecursively(mainGraphicsLayer); + if (GraphicsLayer* scrollingContentsLayer = layer->compositedLayerMapping()->scrollingContentsLayer()) + synchronizedPaintRecursively(scrollingContentsLayer); + } // A null graphics layer can occur for painting of SVG images that are not parented into the main frame tree, // or when the FrameView is the main frame view of a page overlay. The page overlay is in the layer tree of // the host page and will be painted during synchronized painting of the host page. @@ -2719,10 +2726,12 @@ void FrameView::pushPaintArtifactToCompositor() ASSERT(layer); if (!layer->hasCompositedLayerMapping()) return; - GraphicsLayer* rootGraphicsLayer = layer->compositedLayerMapping()->mainGraphicsLayer(); - if (!rootGraphicsLayer->drawsContent()) + GraphicsLayer* contentsLayer = layer->compositedLayerMapping()->scrollingContentsLayer(); + if (!contentsLayer) + contentsLayer = layer->compositedLayerMapping()->mainGraphicsLayer(); + if (!contentsLayer || !contentsLayer->drawsContent()) return; - const PaintArtifact& paintArtifact = rootGraphicsLayer->getPaintController().paintArtifact(); + const PaintArtifact& paintArtifact = contentsLayer->getPaintController().paintArtifact(); https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:233: TEST_P(PaintPropertyTreeBuilderTest, DISABLED_FrameScrollingRootLayerScrolls) On 2016/08/17 at 04:30:41, pdr. wrote: > Can you remove DISABLED_ from this test? I think it needs some minor fixing but nothing major. Err, delete this.
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 04:30:41, pdr. wrote: > > This patch fails about 900 new LayoutTests when run with: > > --additional-driver-flag=--enable-slimming-paint-v2 > --additional-driver-flag=--root-layer-scrolls --retry-failures > > > > There looks to be two classes of failures: > > 1) Nothing renders if a frame scrollbar is present > > 2) Random pixel differences--maybe a snapping difference somewhere? I don't think those failures should block this patch. That's stuff that needs to be fixed elsewhere. We already know that spv2 and rls are not compatible, but this patch is a step in the right direction. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:233: > TEST_P(PaintPropertyTreeBuilderTest, DISABLED_FrameScrollingRootLayerScrolls) > On 2016/08/17 at 04:30:41, pdr. wrote: > > Can you remove DISABLED_ from this test? I think it needs some minor fixing > but nothing major. > > Err, delete this. Done.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:53: } On 2016/08/17 05:57:47, trchen wrote: > I think we should not create effect nodes for LayoutView explicitly here. I > recall that accessibility folks are interested in applying filters on > LayoutView, so LayoutView may have real effect nodes in the future. > > My mental model is that every LayoutObject that is a stacking context (or > stacked for being positioned) should create an effect node, but we don't > actually create them if they can be safely omitted. For the case of root > LayoutView, effect node can't be omitted because someone needs to create root > surface. > > I'd rather do this in PaintPropertyTreeBuilder::updateEffect() line #202 instead > of here. Done. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:58: frameView.y() + layoutView->location().y() + context.current.paintOffset.y()); On 2016/08/17 05:57:47, trchen wrote: > It feels weird. We should not include LayoutView location here as it defeats the > purpose of RLS --- eliminate cronyism between FrameView and LayoutView. > > And doing FrameView location here is weird. I think these should stay in > frameView.preTranslation, as they belong to Widget, not LayoutView. Once root layer scrolling is enabled, FrameView should not have any paint properties at all. Separately, wkorman@ and I are working on removing all geometry from FrameView. When that happens, the values that currently get stuffed into FrameView::x() and FrameView::y() will instead go into LayoutView::location(), and we will update this along with every other invocation of FrameView geometry code. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:128: if (object.isLayoutView()) On 2016/08/17 05:57:47, trchen wrote: > On 2016/08/17 04:30:41, pdr. wrote: > > Can you add a comment here about why we don't clear the paint offset > > translation, below? > > I'd rather add CHECK(!object.isLayoutView()) on line #109 to make sure > LayoutView never have paint offset. With root layer scrolling, every LayoutView must have a paint offset translation which is initialized prior to calling deriveBorderBoxFromContainerContext. That's why it's created here, and that's why we don't clear it in updatePaintOffsetTranslation. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:357: if (object.isLayoutView()) { On 2016/08/17 05:57:47, trchen wrote: > On 2016/08/17 01:38:24, skobes wrote: > > Why does the LayoutView have a separate path here? It's a little unclear what > > the difference is. > > Yea... I can't spot any difference either. The difference is that LayoutView will get a scroll transform even when the scroll offset is (0, 0). That makes it match the current behavior of FrameView::scrollTranslation(). I refactored the code to eliminate redundancy. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:384: if (object.isLayoutView()) { On 2016/08/17 05:57:47, trchen wrote: > It is unfortunate that canContainFixedPositionObjects() returns true for > LayoutView. I think LayoutView should behave as if > canContainFixedPositionObjects() being false, whether in RLS mode or not. Be that as it may, I think that changing the return value of canContainFixedPositionObjects would open a pretty big can of worms. The body element is not a fixed position container, so what than *would* be the top-level fixed position container?
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: > > namespace blink { > > On 2016/08/17 at 04:30:41, pdr. wrote: > > > This patch fails about 900 new LayoutTests when run with: > > > --additional-driver-flag=--enable-slimming-paint-v2 > > --additional-driver-flag=--root-layer-scrolls --retry-failures > > > > > > There looks to be two classes of failures: > > > 1) Nothing renders if a frame scrollbar is present > > > 2) Random pixel differences--maybe a snapping difference somewhere? > > > I don't think those failures should block this patch. That's stuff that needs to be fixed elsewhere. We already know that spv2 and rls are not compatible, but this patch is a step in the right direction. I agree that we can push these issues to a later patch. It's worth noting that this patch actually does get us really close... 900 failures == ~13000 passes :). Can you file a bug about the FrameView's GraphicsLayer choice being wrong for SPV2 and put a comment above the CompositedLayerMapping change with: // TODO(pdr): Ensure painting uses the correct GraphicsLayer when root-layer-scrolls is enabled (https://crbug.com/blah).
On 2016/08/17 20:24:37, pdr. wrote: > 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: > > > namespace blink { > > > On 2016/08/17 at 04:30:41, pdr. wrote: > > > > This patch fails about 900 new LayoutTests when run with: > > > > --additional-driver-flag=--enable-slimming-paint-v2 > > > --additional-driver-flag=--root-layer-scrolls --retry-failures > > > > > > > > There looks to be two classes of failures: > > > > 1) Nothing renders if a frame scrollbar is present > > > > 2) Random pixel differences--maybe a snapping difference somewhere? > > > > > > I don't think those failures should block this patch. That's stuff that needs > to be fixed elsewhere. We already know that spv2 and rls are not compatible, > but this patch is a step in the right direction. > > I agree that we can push these issues to a later patch. It's worth noting that > this patch actually does get us really close... 900 failures == ~13000 passes > :). Can you file a bug about the FrameView's GraphicsLayer choice being wrong > for SPV2 and put a comment above the CompositedLayerMapping change with: > // TODO(pdr): Ensure painting uses the correct GraphicsLayer when > root-layer-scrolls is enabled (https://crbug.com/blah). Done.
LGTM Please wait for an LGTM from trchen.
lgtm2
On 2016/08/17 20:38:19, szager1 wrote: > On 2016/08/17 20:24:37, pdr. wrote: > > 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: > > > > namespace blink { > > > > On 2016/08/17 at 04:30:41, pdr. wrote: > > > > > This patch fails about 900 new LayoutTests when run with: > > > > > --additional-driver-flag=--enable-slimming-paint-v2 > > > > --additional-driver-flag=--root-layer-scrolls --retry-failures > > > > > > > > > > There looks to be two classes of failures: > > > > > 1) Nothing renders if a frame scrollbar is present > > > > > 2) Random pixel differences--maybe a snapping difference somewhere? > > > > > > > > > I don't think those failures should block this patch. That's stuff that > needs > > to be fixed elsewhere. We already know that spv2 and rls are not compatible, > > but this patch is a step in the right direction. > > > > I agree that we can push these issues to a later patch. It's worth noting that > > this patch actually does get us really close... 900 failures == ~13000 passes > > :). Can you file a bug about the FrameView's GraphicsLayer choice being wrong > > for SPV2 and put a comment above the CompositedLayerMapping change with: > > // TODO(pdr): Ensure painting uses the correct GraphicsLayer when > > root-layer-scrolls is enabled (https://crbug.com/blah). > > Done. The root cause is due to root LayoutView getting composited, and foreground contents getting popped into CLM::m_scrollingContentsLayer instead of CLM::m_graphicsLayer. Ideally SPv2 should use the software painting path, but instead we simply force compositing but disable all compositing triggers. I wonder if it's possible to disable CLM on LayoutView and paint everything into PLC::m_rootContentLayer instead... For short-term workaround, I think we can change line #2641 of FrameView::pushPaintArtifactToCompositor(): - GraphicsLayer* rootGraphicsLayer = layer->compositedLayerMapping()->mainGraphicsLayer(); + CompositedLayerMapping& clm = *layer->compositedLayerMapping(); + GraphicsLayer* rootGraphicsLayer = clm.scrollingContentsLayer() ? clm.scrollingContentsLayer() : clm.mainGraphicsLayer();
On 2016/08/17 at 21:05:23, trchen wrote: > On 2016/08/17 20:38:19, szager1 wrote: > > On 2016/08/17 20:24:37, pdr. wrote: > > > 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: > > > > > namespace blink { > > > > > On 2016/08/17 at 04:30:41, pdr. wrote: > > > > > > This patch fails about 900 new LayoutTests when run with: > > > > > > --additional-driver-flag=--enable-slimming-paint-v2 > > > > > --additional-driver-flag=--root-layer-scrolls --retry-failures > > > > > > > > > > > > There looks to be two classes of failures: > > > > > > 1) Nothing renders if a frame scrollbar is present > > > > > > 2) Random pixel differences--maybe a snapping difference somewhere? > > > > > > > > > > > > I don't think those failures should block this patch. That's stuff that > > needs > > > to be fixed elsewhere. We already know that spv2 and rls are not compatible, > > > but this patch is a step in the right direction. > > > > > > I agree that we can push these issues to a later patch. It's worth noting that > > > this patch actually does get us really close... 900 failures == ~13000 passes > > > :). Can you file a bug about the FrameView's GraphicsLayer choice being wrong > > > for SPV2 and put a comment above the CompositedLayerMapping change with: > > > // TODO(pdr): Ensure painting uses the correct GraphicsLayer when > > > root-layer-scrolls is enabled (https://crbug.com/blah). > > > > Done. > > The root cause is due to root LayoutView getting composited, and foreground contents getting popped into CLM::m_scrollingContentsLayer instead of CLM::m_graphicsLayer. > > Ideally SPv2 should use the software painting path, but instead we simply force compositing but disable all compositing triggers. I wonder if it's possible to disable CLM on LayoutView and paint everything into PLC::m_rootContentLayer instead... > > For short-term workaround, I think we can change line #2641 of FrameView::pushPaintArtifactToCompositor(): > - GraphicsLayer* rootGraphicsLayer = layer->compositedLayerMapping()->mainGraphicsLayer(); > + CompositedLayerMapping& clm = *layer->compositedLayerMapping(); > + GraphicsLayer* rootGraphicsLayer = clm.scrollingContentsLayer() ? clm.scrollingContentsLayer() : clm.mainGraphicsLayer(); I agree, I put my hacked up version of this in https://crbug.com/638719. I think we can do this as a followup though, because without RLS we don't change behavior. Do you agree?
On 2016/08/17 21:05:23, trchen wrote: > The root cause is due to root LayoutView getting composited, and foreground > contents getting popped into CLM::m_scrollingContentsLayer instead of > CLM::m_graphicsLayer. > > Ideally SPv2 should use the software painting path, but instead we simply force > compositing but disable all compositing triggers. I wonder if it's possible to > disable CLM on LayoutView and paint everything into PLC::m_rootContentLayer > instead... My understanding of SPv2 is pretty fuzzy, but for root layer scrolling this seems backwards to me. We've been aiming to do scrolling through the LayoutView's CLM instead of the PLC layers.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... 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 wrote: > On 2016/08/17 05:57:47, trchen wrote: > > It feels weird. We should not include LayoutView location here as it defeats > the > > purpose of RLS --- eliminate cronyism between FrameView and LayoutView. > > > > And doing FrameView location here is weird. I think these should stay in > > frameView.preTranslation, as they belong to Widget, not LayoutView. > > Once root layer scrolling is enabled, FrameView should not have any paint > properties at all. Separately, wkorman@ and I are working on removing all > geometry from FrameView. When that happens, the values that currently get > stuffed into FrameView::x() and FrameView::y() will instead go into > LayoutView::location(), and we will update this along with every other > invocation of FrameView geometry code. That'd be great! I tried to do that 2 weeks ago but there are still many things rely on the (broken) Widget-based coordinate conversion. In the mean time I think the best strategy is to match old-world behavior. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:128: if (object.isLayoutView()) On 2016/08/17 19:21:34, szager1 wrote: > On 2016/08/17 05:57:47, trchen wrote: > > On 2016/08/17 04:30:41, pdr. wrote: > > > Can you add a comment here about why we don't clear the paint offset > > > translation, below? > > > > I'd rather add CHECK(!object.isLayoutView()) on line #109 to make sure > > LayoutView never have paint offset. > > With root layer scrolling, every LayoutView must have a paint offset translation > which is initialized prior to calling deriveBorderBoxFromContainerContext. > That's why it's created here, and that's why we don't clear it in > updatePaintOffsetTranslation. Does that match the behavior of old-world compositing? I think we do clear paintOffset and use TransformDisplayItem to do widget positioning even with RLS on. See PartPainter.cpp:114 and FramePainter.cpp:61. I don't think RLS excluded them. I appreciate that you tried to propagate paintOffset into sub-frame. It will be a good thing in the long term, if we can also propagate sub-pixel layout size into sub-frames. However things are not ready yet today, and paint invalidation may go mad if our paint offset doesn't match the old-world. :( I think this explains some of the sub-pixel layout test failures. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:384: if (object.isLayoutView()) { On 2016/08/17 19:21:34, szager1 wrote: > On 2016/08/17 05:57:47, trchen wrote: > > It is unfortunate that canContainFixedPositionObjects() returns true for > > LayoutView. I think LayoutView should behave as if > > canContainFixedPositionObjects() being false, whether in RLS mode or not. > > Be that as it may, I think that changing the return value of > canContainFixedPositionObjects would open a pretty big can of worms. The body > element is not a fixed position container, so what than *would* be the top-level > fixed position container? I think it should return nullptr and the element would know the viewport being the fixed-pos container. It is plain wrong to treat LayoutView as fixed-pos container especially in RLS mode, because we'd use the wrong box. For example: <div id="fixed-container" style="overflow:scroll; transform:translateZ(0)"> <div style="position:fixed"></div> </div> The transformed element would be the fixed-pos container for the inner fixed-pos element. However the fixed-pos element stick to the SCROLLED padding box of it, not the border box. In comparison, for the case of RLS LayoutView, fixed-pos element actually wants to stick to the border box. This discrepancy caused a lot of special-casing in our geometry conversion function, which can be avoided. :( That said, I agree it is a big can of worms. I'd avoid changing canContainFixedPositionObjects() at this moment. I think we can simply reuse pdr's code that excludes LayoutView as fixed-pos container. What would break if we simply treat LayoutView as regular containing block? https://codereview.chromium.org/2244873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:360: } This is difficult to read. How about this: bool forceScrollingForLayoutView = false; if (object.isLayoutView()) { Settings* settings = object.document().settings(); forceScrollingForLayoutView = settings && settings->rootLayerScrolls(); } if (forceScrollingForLayoutView || !scrollOffset.isZero() || layer->scrollsOverflow()) {
By the way, I'm tired of writing this everywhere: Settings* settings = object.document().settings(); forceScrollingForLayoutView = settings && settings->rootLayerScrolls(); If I made a CL to move RLS from Settings to REF, would people be happy about it? Or perhaps someone already tried but it couldn't be done?
On 2016/08/17 21:12:59, skobes wrote: > On 2016/08/17 21:05:23, trchen wrote: > > The root cause is due to root LayoutView getting composited, and foreground > > contents getting popped into CLM::m_scrollingContentsLayer instead of > > CLM::m_graphicsLayer. > > > > Ideally SPv2 should use the software painting path, but instead we simply > force > > compositing but disable all compositing triggers. I wonder if it's possible to > > disable CLM on LayoutView and paint everything into PLC::m_rootContentLayer > > instead... > > My understanding of SPv2 is pretty fuzzy, but for root layer scrolling this > seems backwards to me. We've been aiming to do scrolling through the > LayoutView's CLM instead of the PLC layers. We will still do scrolling through LayoutView, but we'll do software scrolling than CLM-based scrolling for SPv2. Anyway, I think pdr's workaround is good and we don't need to include that in this CL. Better if we can eliminate PLC all together, but the software paint path have been deserted for years. I couldn't estimate how much efforts need to be spent to make it great again...
On 2016/08/17 21:33:53, trchen wrote: > By the way, I'm tired of writing this everywhere: > Settings* settings = object.document().settings(); > forceScrollingForLayoutView = settings && settings->rootLayerScrolls(); > > If I made a CL to move RLS from Settings to REF, would people be happy about it? SGTM. I probably should have made this an REF to begin with. :)
On 2016/08/17 21:39:30, trchen wrote: > We will still do scrolling through LayoutView, but we'll do software scrolling > than CLM-based scrolling for SPv2. Anyway, I think pdr's workaround is good and > we don't need to include that in this CL. Ah ok... I have been thinking of PLC as being like the CLM for the FrameView scrolling paths. But if scrolling is no longer done by moving GraphicsLayers around then we can kill the scrolling layers in both PLC and CLM. > Better if we can eliminate PLC all together, but the software paint path have > been deserted for years. I couldn't estimate how much efforts need to be spent > to make it great again... Does "software paint" mean non-composited scrolling? Those paths are not dead, since we use them on low DPI. But maybe we are talking about different things.
On 2016/08/18 00:34:17, skobes wrote: > On 2016/08/17 21:39:30, trchen wrote: > > We will still do scrolling through LayoutView, but we'll do software scrolling > > than CLM-based scrolling for SPv2. Anyway, I think pdr's workaround is good > and > > we don't need to include that in this CL. > > Ah ok... I have been thinking of PLC as being like the CLM for the FrameView > scrolling paths. But if scrolling is no longer done by moving GraphicsLayers > around then we can kill the scrolling layers in both PLC and CLM. Well, for SPv2. With RLS + old-world, we can indeed eliminate most layers in PLC. CLM still needs to support traditional composited scrolling. > > Better if we can eliminate PLC all together, but the software paint path have > > been deserted for years. I couldn't estimate how much efforts need to be spent > > to make it great again... > > Does "software paint" mean non-composited scrolling? Those paths are not dead, > since we use them on low DPI. But maybe we are talking about different things. I'm referring to disabling compositing mode. i.e. without creating PLC at all. --force-compositing-mode has been the default for years. I don't think the flag exists anymore. I expect invalidation code would panic when they found paint invalidation container to be nullptr.
https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... 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 wrote: > On 2016/08/17 19:21:34, szager1 wrote: > > On 2016/08/17 05:57:47, trchen wrote: > > > It feels weird. We should not include LayoutView location here as it defeats > > the > > > purpose of RLS --- eliminate cronyism between FrameView and LayoutView. > > > > > > And doing FrameView location here is weird. I think these should stay in > > > frameView.preTranslation, as they belong to Widget, not LayoutView. > > > > Once root layer scrolling is enabled, FrameView should not have any paint > > properties at all. Separately, wkorman@ and I are working on removing all > > geometry from FrameView. When that happens, the values that currently get > > stuffed into FrameView::x() and FrameView::y() will instead go into > > LayoutView::location(), and we will update this along with every other > > invocation of FrameView geometry code. > > That'd be great! I tried to do that 2 weeks ago but there are still many things > rely on the (broken) Widget-based coordinate conversion. > > In the mean time I think the best strategy is to match old-world behavior. This entire patch is intended match old-world behavior, while removing all paint property nodes from FrameView when root layer scrolling is enabled, which is in my opinion the only sane thing to do. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:128: if (object.isLayoutView()) On 2016/08/17 21:30:55, trchen wrote: > On 2016/08/17 19:21:34, szager1 wrote: > > On 2016/08/17 05:57:47, trchen wrote: > > > On 2016/08/17 04:30:41, pdr. wrote: > > > > Can you add a comment here about why we don't clear the paint offset > > > > translation, below? > > > > > > I'd rather add CHECK(!object.isLayoutView()) on line #109 to make sure > > > LayoutView never have paint offset. > > > > With root layer scrolling, every LayoutView must have a paint offset > translation > > which is initialized prior to calling deriveBorderBoxFromContainerContext. > > That's why it's created here, and that's why we don't clear it in > > updatePaintOffsetTranslation. > > Does that match the behavior of old-world compositing? I think we do clear > paintOffset and use TransformDisplayItem to do widget positioning even with RLS > on. See PartPainter.cpp:114 and FramePainter.cpp:61. I don't think RLS excluded > them. > > I appreciate that you tried to propagate paintOffset into sub-frame. It will be > a good thing in the long term, if we can also propagate sub-pixel layout size > into sub-frames. However things are not ready yet today, and paint invalidation > may go mad if our paint offset doesn't match the old-world. :( > > I think this explains some of the sub-pixel layout test failures. I'm not sure what "long term" is here. Chromium is broken with either spv2 or rls enabled; with both of them enabled, it is doubly broken. I don't see the value of trying to make it less broken by compromising this patch, which is code that only runs when both features are enabled. To put it another way: if your primary concern is not to regress spv2, independent of rls, then this patch is neutral. But if you ever want rls and spv2 to work together, then I would argue that this patch is the correct approach, and if it causes test failures, then they need to be fixed elsewhere. https://codereview.chromium.org/2244873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:384: if (object.isLayoutView()) { On 2016/08/17 21:30:55, trchen wrote: > On 2016/08/17 19:21:34, szager1 wrote: > > On 2016/08/17 05:57:47, trchen wrote: > > > It is unfortunate that canContainFixedPositionObjects() returns true for > > > LayoutView. I think LayoutView should behave as if > > > canContainFixedPositionObjects() being false, whether in RLS mode or not. > > > > Be that as it may, I think that changing the return value of > > canContainFixedPositionObjects would open a pretty big can of worms. The body > > element is not a fixed position container, so what than *would* be the > top-level > > fixed position container? > > I think it should return nullptr and the element would know the viewport being > the fixed-pos container. It is plain wrong to treat LayoutView as fixed-pos > container especially in RLS mode, because we'd use the wrong box. For example: > > <div id="fixed-container" style="overflow:scroll; transform:translateZ(0)"> > <div style="position:fixed"></div> > </div> > > The transformed element would be the fixed-pos container for the inner fixed-pos > element. However the fixed-pos element stick to the SCROLLED padding box of it, > not the border box. In comparison, for the case of RLS LayoutView, fixed-pos > element actually wants to stick to the border box. This discrepancy caused a lot > of special-casing in our geometry conversion function, which can be avoided. :( > > That said, I agree it is a big can of worms. I'd avoid changing > canContainFixedPositionObjects() at this moment. I think we can simply reuse > pdr's code that excludes LayoutView as fixed-pos container. What would break if > we simply treat LayoutView as regular containing block? This is exactly the code that special-cases LayoutView for this reason. At the time that this code runs, context.current is the scroll offset of the container, which is correct for scrollable div's, but incorrect for the LayoutView. So, this change grabs the pre-scroll-translation transform to use for context.fixedPosition. https://codereview.chromium.org/2244873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2244873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:360: } On 2016/08/17 21:30:55, trchen wrote: > This is difficult to read. How about this: > bool forceScrollingForLayoutView = false; > if (object.isLayoutView()) { > Settings* settings = object.document().settings(); > forceScrollingForLayoutView = settings && settings->rootLayerScrolls(); > } > if (forceScrollingForLayoutView || !scrollOffset.isZero() || > layer->scrollsOverflow()) { Done.
@trchen, would you be okay with landing this as-is and resolving the frameview/layoutview issue when we switch spv2 to depend on rls?
On 2016/08/19 at 02:09:45, pdr. wrote: > @trchen, would you be okay with landing this as-is and resolving the frameview/layoutview issue when we switch spv2 to depend on rls? szager is leaving for vacation and trchen is OOO today. I'd like to build on this patch next week so I think we should go ahead and land this as-is, and address the frameview/layoutview issue in a followup. LGTM
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/19 at 19:52:01, pdr. wrote: > On 2016/08/19 at 02:09:45, pdr. wrote: > > @trchen, would you be okay with landing this as-is and resolving the frameview/layoutview issue when we switch spv2 to depend on rls? > > szager is leaving for vacation and trchen is OOO today. I'd like to build on this patch next week so I think we should go ahead and land this as-is, and address the frameview/layoutview issue in a followup. > > LGTM Two cool graphs with and without this patch: http://wince.sfo.corp.google.com:8766/no-rls.svg http://wince.sfo.corp.google.com:8766/rls.svg
On 2016/08/19 20:04:14, pdr. wrote: > On 2016/08/19 at 19:52:01, pdr. wrote: > > On 2016/08/19 at 02:09:45, pdr. wrote: > > > @trchen, would you be okay with landing this as-is and resolving the > frameview/layoutview issue when we switch spv2 to depend on rls? > > > > szager is leaving for vacation and trchen is OOO today. I'd like to build on > this patch next week so I think we should go ahead and land this as-is, and > address the frameview/layoutview issue in a followup. > > > > LGTM > > Two cool graphs with and without this patch: > http://wince.sfo.corp.google.com:8766/no-rls.svg > http://wince.sfo.corp.google.com:8766/rls.svg lgtm. Yes, absolutely no need to block an otherwise good CL for little side issues. :)
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/2244873002/#ps120001 (title: "Fix test expectation for rootClip")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by szager@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6ea7c61e26c872c72d7df02b1c4027b4bcd31588 Cr-Commit-Position: refs/heads/master@{#413343} |