Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259493004:
Fix Compositing of Opaque Scrolling Layers and Add Tests (Closed)
|
Created:
4 years, 4 months ago by Stephen Chennney Modified:
4 years, 3 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.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix Compositing of Opaque Scrolling Layers and Add Tests
Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer
method to PaintLayer to use it for detecting composited
scrolling. The method only uses data from PaintLayer, so there's
no reason to have it elsewhere.
Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi
devices to use the method.
Fix the check in CompositingReasonsFinder to detect opaque background scrollers
as a reason
Remove the unnecessary check for self painting layer in
PaintLayer::backgroundIsKnownToBeOpaqueInRect.
The code is always called from a self painting layer's GraphicsLayer, or it's
called by the recursive call checking childrens' opaqueness. The former case means
the layer is always self painting, while the latter seems incorrect - one should
not exclude, not include, child self painting layers.
Add tests for promotion and demotion of scrolling layers as they switch between
opaque and transparent backgrounds.
R=flackr@chromium.org, chrishtr@chromium.org
BUG=381840
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/87b0099aa31169f8c7a40b4f93b4a1e86ddf53a1
Cr-Commit-Position: refs/heads/master@{#417743}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add opacity check #
Total comments: 1
Patch Set 3 : Fix bugs and update tests #Patch Set 4 : Added test for style chnage #Patch Set 5 : Revert unnecessary change. #
Total comments: 11
Patch Set 6 : New tests. #
Total comments: 2
Patch Set 7 : Fix the tests. #
Total comments: 17
Patch Set 8 : Address review comments #
Total comments: 17
Patch Set 9 : Review comments addressed and test baselines. #
Total comments: 13
Patch Set 10 : More review updates #Patch Set 11 : Rebase and stop adding CompositingReasonOutOfFlowClipping #
Total comments: 2
Patch Set 12 : Fix comment. #Messages
Total messages: 90 (48 generated)
Description was changed from ========== Move shouldPaintBackgroundOntoScrollingContentsLayer to PaintLayer The CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method should be on PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. This also switches the check for compositing scrollers on low-dpi devices to use the method. R=flackr@chromium.org BUG=381840 ========== to ========== Move shouldPaintBackgroundOntoScrollingContentsLayer to PaintLayer The CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method should be on PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. This also switches the check for compositing scrollers on low-dpi devices to use the method. R=flackr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
I've had to juggle stuff around to use the correct check for compositing opaque scrollers. This now prevents Gmail scrolling from being promoted at all, which I guess fixes the bug I was seeing with the previous code, but doesn't mean we've succeeded.
The CQ bit was checked by schenney@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I originally had shouldPaintBackgroundOntoScrollingContentsLayer in PaintLayer but with it only being used in CompositedLayerMapping chris suggested I move it. I agree though that it makes sense in PaintLayer. https://codereview.chromium.org/2259493004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1504: && layer->shouldPaintBackgroundOntoScrollingContentsLayer(); You also need to check that the scrolling contents layer is opaque. See m_scrollingContentsLayer->setContentsOpaque call here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... You could probably do the same check if the layer opacity hasn't been set yet.
The CQ bit was checked by schenney@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...
Gmail still does not composit due to the lack of either background-attachment: local (which is meaningless in this case) or background-clip: padding-box, which doesn't affect the rendering either due to opaque borders. We will have to ask them to change that. But if you add the background-clip: padding-box to the gmail scrolling message view, with --enable-blink-features=CompositeOpaqueScrollers you get the scroller appearing on top of the content above it on the page. While I need to investigate that, it shouldn't block this patch. https://codereview.chromium.org/2259493004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1504: && layer->shouldPaintBackgroundOntoScrollingContentsLayer(); On 2016/08/19 03:19:30, flackr wrote: > You also need to check that the scrolling contents layer is opaque. See > m_scrollingContentsLayer->setContentsOpaque call here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > > You could probably do the same check if the layer opacity hasn't been set yet. Done.
I have a patch already written to handle all of the cases which are equivalent to padding-box with local attachment. Just waiting on my background attachment patch to land since they touch very related code. I will upload it though and link it to you to test with. Can you add some tests which verify that we have automatically promoted and that the scrolling contents layer is opaque (similar to my tests in compositing/overflow/overflow-scroll-with-opaque-background.html but without the will-change compositing trigger since it should be automatic now). https://codereview.chromium.org/2259493004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1503: bool backgroundSupportsLCDText = RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() Can we remove the flag now?
On 2016/08/19 17:36:53, flackr wrote: > I have a patch already written to handle all of the cases which are equivalent > to padding-box with local attachment. Just waiting on my background attachment > patch to land since they touch very related code. I will upload it though and > link it to you to test with. OK. Thanks for all the hard work on this. > Can you add some tests which verify that we have automatically promoted and that > the scrolling contents layer is opaque (similar to my tests in > compositing/overflow/overflow-scroll-with-opaque-background.html but without the > will-change compositing trigger since it should be automatic now). Yes, will do. > https://codereview.chromium.org/2259493004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2259493004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1503: bool > backgroundSupportsLCDText = > RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() > Can we remove the flag now? Not yet. We at least need that gmail problem fixed. We'll have the flag in M54 and then maybe remove it for M55.
On 2016/08/19 at 17:36:53, flackr wrote: > I have a patch already written to handle all of the cases which are equivalent to padding-box with local attachment. Just waiting on my background attachment patch to land since they touch very related code. I will upload it though and link it to you to test with. Patch for detecting local-equivalent backgrounds is here: https://codereview.chromium.org/2264663002 > > Can you add some tests which verify that we have automatically promoted and that the scrolling contents layer is opaque (similar to my tests in compositing/overflow/overflow-scroll-with-opaque-background.html but without the will-change compositing trigger since it should be automatic now). > > https://codereview.chromium.org/2259493004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2259493004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1503: bool backgroundSupportsLCDText = RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() > Can we remove the flag now?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> On 2016/08/19 17:36:53, flackr wrote: > > Can you add some tests which verify that we have automatically promoted and > that > > the scrolling contents layer is opaque (similar to my tests in > > compositing/overflow/overflow-scroll-with-opaque-background.html but without > the > > will-change compositing trigger since it should be automatic now). > > Yes, will do. It may have to be a unit test because, AFAIK, you can't set the necessary blink RuntimeEnableFeature from a layout test. I'll verify that first.
> On 2016/08/19 17:36:53, flackr wrote: > > Can you add some tests which verify that we have automatically promoted and > that > > the scrolling contents layer is opaque (similar to my tests in > > compositing/overflow/overflow-scroll-with-opaque-background.html but without > the > > will-change compositing trigger since it should be automatic now). > > Yes, will do. It may have to be a unit test because, AFAIK, you can't set the necessary blink RuntimeEnableFeature from a layout test. I'll verify that first.
On 2016/08/19 at 20:04:52, schenney wrote: > > On 2016/08/19 17:36:53, flackr wrote: > > > Can you add some tests which verify that we have automatically promoted and > > that > > > the scrolling contents layer is opaque (similar to my tests in > > > compositing/overflow/overflow-scroll-with-opaque-background.html but without > > the > > > will-change compositing trigger since it should be automatic now). > > > > Yes, will do. > > It may have to be a unit test because, AFAIK, you can't set the necessary blink RuntimeEnableFeature from a layout test. I'll verify that first. You can expose a method to set the feature from a test, see my sticky tests for an example: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/... Although we should be able to turn this on with the correct detection. Also, does it correctly de-promote / promote when the background changes?
On 2016/08/19 20:07:36, flackr wrote: > On 2016/08/19 at 20:04:52, schenney wrote: > > > On 2016/08/19 17:36:53, flackr wrote: > > > > Can you add some tests which verify that we have automatically promoted > and > > > that > > > > the scrolling contents layer is opaque (similar to my tests in > > > > compositing/overflow/overflow-scroll-with-opaque-background.html but > without > > > the > > > > will-change compositing trigger since it should be automatic now). > > > > > > Yes, will do. > > > > It may have to be a unit test because, AFAIK, you can't set the necessary > blink RuntimeEnableFeature from a layout test. I'll verify that first. > > You can expose a method to set the feature from a test, see my sticky tests for > an example: > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/... > > Although we should be able to turn this on with the correct detection. Yeah, I can modify InternalSettings.h to expose this, but it seems to be discouraged for RuntimeEnabledFeatures (very few have settings). Are you against a unit test in principle, or arguing for path of least resistance? > Also, does it correctly de-promote / promote when the background changes? I can test that in a unit test too, or layout test.
On 2016/08/19 at 20:43:58, schenney wrote: > On 2016/08/19 20:07:36, flackr wrote: > > On 2016/08/19 at 20:04:52, schenney wrote: > > > > On 2016/08/19 17:36:53, flackr wrote: > > > > > Can you add some tests which verify that we have automatically promoted > > and > > > > that > > > > > the scrolling contents layer is opaque (similar to my tests in > > > > > compositing/overflow/overflow-scroll-with-opaque-background.html but > > without > > > > the > > > > > will-change compositing trigger since it should be automatic now). > > > > > > > > Yes, will do. > > > > > > It may have to be a unit test because, AFAIK, you can't set the necessary > > blink RuntimeEnableFeature from a layout test. I'll verify that first. > > > > You can expose a method to set the feature from a test, see my sticky tests for > > an example: > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/... > > > > Although we should be able to turn this on with the correct detection. > > Yeah, I can modify InternalSettings.h to expose this, but it seems to be discouraged for RuntimeEnabledFeatures (very few have settings). Are you against a unit test in principle, or arguing for path of least resistance? Not against a unit test at all, as long as it can verify promotion and opaqueness :-). I do think though we should remove the flag as soon as possible so that we can catch bugs. > > > Also, does it correctly de-promote / promote when the background changes? > > I can test that in a unit test too, or layout test. Sounds good, I don't have a strong preference for which test type.
The CQ bit was checked by schenney@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...
On 2016/08/22 15:34:03, flackr wrote: > On 2016/08/19 at 20:43:58, schenney wrote: > > On 2016/08/19 20:07:36, flackr wrote: > > > On 2016/08/19 at 20:04:52, schenney wrote: > > > > > On 2016/08/19 17:36:53, flackr wrote: > > > > > > Can you add some tests which verify that we have automatically > promoted > > > and > > > > > that > > > > > > the scrolling contents layer is opaque (similar to my tests in > > > > > > compositing/overflow/overflow-scroll-with-opaque-background.html but > > > without > > > > > the > > > > > > will-change compositing trigger since it should be automatic now). Done by extending the existing unit test. Found that we were not promoting at all due to a missing check in the CompositingReasonsFinder. > > > Also, does it correctly de-promote / promote when the background changes? Oops, haven't tested for this yet. Will do.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 schenney@chromium.org to run a CQ dry run
The CQ bit was unchecked by schenney@chromium.org
The CQ bit was checked by schenney@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...
Ready for review, but I think it will have to wait until flackr's other patch lands and this can be rebased. https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: && layer->shouldPaintBackgroundOntoScrollingContentsLayer() My understanding is that here we cannot use the CompositedLayerMapping flag because it will not have been updated yet.
On 2016/08/26 18:01:41, Stephen Chennney wrote: > Ready for review, but I think it will have to wait until flackr's other patch > lands and this can be rebased. > > https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: && > layer->shouldPaintBackgroundOntoScrollingContentsLayer() > My understanding is that here we cannot use the CompositedLayerMapping flag > because it will not have been updated yet. And Gmail scrolls composited with --enable-blink-features=CompositeOpaqueScrollers provided we define background-clip: padding-box on the overflow scroll div. I believe flackr's patch addresses the latter issue, so gmail will not have to update their page to get the benefit.
https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger() || RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled()) { I don't understand why hasOverflowScrollTrigger() isn't sufficient. Isn't that still true whether the flag is on or off? https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: && layer->shouldPaintBackgroundOntoScrollingContentsLayer() On 2016/08/26 at 18:01:41, Stephen Chennney wrote: > My understanding is that here we cannot use the CompositedLayerMapping flag because it will not have been updated yet. I believe that's correct - but should be easy to test. Have you also verified that we have the up to date value for not having negative z index children? See !m_owningLayer.stackingNode()->hasNegativeZOrderList() at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:30: ASSERT_TRUE(paintLayer->graphicsLayerBackingForScrolling()->contentsOpaque()); nit: This should probably be EXPECT_TRUE https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:49: ASSERT_TRUE(!paintLayer->needsCompositedScrolling()); this and following: EXPECT_FALSE https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:79: ASSERT_TRUE(!paintLayer->graphicsLayerBackingForScrolling()); I feel like we should have layouttests for these cases (depromoted / promoted) as well just to ensure they are invalidated and repainted correctly.
https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger() || RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled()) { On 2016/08/26 18:24:23, flackr wrote: > I don't understand why hasOverflowScrollTrigger() isn't sufficient. Isn't that > still true whether the flag is on or off? hasOverflowScrollTrigger only checks for the OverflowScrollTrigger enum and that is only set based on the preferCompositingToLCDText setting. I don't want to mess with hasOverflowScrollTrigger() itself because PaintLayerCompositor uses it as a proxy for preferCompositingToLCDText. We don't want to mix up preferCompositingToLCDText and compositeOpaqueScrollers because the former causes a lot more stuff to get composited, and we don't want some parts of the system thinking things are composited and others not (that's why gmail was broken for me before this change). https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: && layer->shouldPaintBackgroundOntoScrollingContentsLayer() On 2016/08/26 18:24:23, flackr wrote: > On 2016/08/26 at 18:01:41, Stephen Chennney wrote: > > My understanding is that here we cannot use the CompositedLayerMapping flag > because it will not have been updated yet. > > I believe that's correct - but should be easy to test. > > Have you also verified that we have the up to date value for not having negative > z index children? See !m_owningLayer.stackingNode()->hasNegativeZOrderList() at > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... I'll check. https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:30: ASSERT_TRUE(paintLayer->graphicsLayerBackingForScrolling()->contentsOpaque()); On 2016/08/26 18:24:23, flackr wrote: > nit: This should probably be EXPECT_TRUE Shows how often I've written unit tests. I'll fix it. https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:79: ASSERT_TRUE(!paintLayer->graphicsLayerBackingForScrolling()); On 2016/08/26 18:24:23, flackr wrote: > I feel like we should have layouttests for these cases (depromoted / promoted) > as well just to ensure they are invalidated and repainted correctly. That's a good point. I'll create such things.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2259493004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1506: && layer->backgroundIsKnownToBeOpaqueInRect(toLayoutBox(layer->layoutObject())->paddingBoxRect()); We have a chicken and egg problem. The first thing required in backgroundIsKnownToBeOpaqueInRect is that the layer is self painting, but the code to determine self painting status calls this method to check if we need overflow scrolling, so we never set isSelfPaintingLayer. So here we need something else that is not concerned about what the layer is painting into but is only concerned with the background itself. Interestingly, in the unit test this manages to work because the layer is self painting, and I have no idea why yet.
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
I think this is basically ready to go. I need baselines for the new tests and I need to change some of the EXPECT_TRUE to EXPECT_FALSE in the unit tests. Review requested now so that I can have code right before undating the test results. https://codereview.chromium.org/2259493004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): https://codereview.chromium.org/2259493004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2401: return false; This check is redundant in all pre-existing callers except the one below in childBackgroundIsKnownToBeOpaqueInRect. All the other cases are GraphicsLayers calling it on their m_owningLayer, so it must be self painting. I verified that with CHECKs. https://codereview.chromium.org/2259493004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2443: if (childLayer->isPaintInvalidationContainer()) I think this means we do not need to check that the child is a selfPaintingLayer, per the previous logic. Regardless, that logic seems wrong. As far as I can tell, we should return false here is the child IS a self painting layer because it will not then contribute to the opacity of it's parent's layer. The code hasn't changed wince it was originally added in 2013, back in WebKit. So it doesn't surprise me if it's out of data.
Dry run: 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
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...) 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 schenney@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...
Description was changed from ========== Move shouldPaintBackgroundOntoScrollingContentsLayer to PaintLayer The CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method should be on PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. This also switches the check for compositing scrollers on low-dpi devices to use the method. R=flackr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. R=flackr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
schenney@chromium.org changed reviewers: + chrishtr@chromium.org
Description was changed from ========== Fix Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. R=flackr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. Add tests for promotion and demotion of scrolling layers as they switch between opaque and transparent backgrounds. R=flackr@chromium.org, chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
I'm going to break out the change to PaintLayer::backgroundIsKnownToBeOpaqueInRect to correct the selfPaintingLayer thing. Otherwise I'm not aware of outstanding issues.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Rob, once you're done reviewing I'll give it a once-over.
https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:42: if (!hasOpaqueCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) The negation of hasOpaqueCompositedScrollingContentsLayer is a little too loose of a condition, you should add a function that checks that we do not get a composited scrolling contents layer at all. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:68: able to draw text with subpixel anti-aliasing. This comments seems out of date - doesn't mention that we change the background. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html:31: if (!hasOpaqueCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) Same as above, check that we don't have a composited scrolling contents layer. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html:68: able to draw text with subpixel anti-aliasing. Comment also seems out of date. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html:3: <head> I've been discouraged in the past from adding these extra tags to LayoutTests if they're not necessary - but if they are they should probably also be added to the expected.html reference file. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:51: return paintLayer->shouldPaintBackgroundOntoScrollingContentsLayer(); We should probably move the tests rather than have CompositedLayerMappingTest test PaintLayer. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger() || RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled()) { You mentioned that hasOverflowScrollTrigger() basically proxies whether we prefer compositing to lcd text, but it seems like layerNeedsCompositedScrolling in PaintLayerScrollableArea.cpp already includes whether prefer compositing to lcd text. Assuming my following comment is not a problem, this condition can probably be removed altogether. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:146: if (layer->clipParent()) I'm concerned this condition combined with RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() could cause us to add a compositing reason to a scroller when it doesn't have an opaque background. We should test this.
I'll get a patch up shortly with all these and get tests running. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:42: if (!hasOpaqueCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) On 2016/08/30 21:14:49, flackr wrote: > The negation of hasOpaqueCompositedScrollingContentsLayer is a little too loose > of a condition, you should add a function that checks that we do not get a > composited scrolling contents layer at all. Done. Added hasCompositedScrollingContentsLayer and used that. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:68: able to draw text with subpixel anti-aliasing. On 2016/08/30 21:14:49, flackr wrote: > This comments seems out of date - doesn't mention that we change the background. Done. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html:31: if (!hasOpaqueCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) On 2016/08/30 21:14:49, flackr wrote: > Same as above, check that we don't have a composited scrolling contents layer. Done. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html:68: able to draw text with subpixel anti-aliasing. On 2016/08/30 21:14:49, flackr wrote: > Comment also seems out of date. Done. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background.html:3: <head> On 2016/08/30 21:14:49, flackr wrote: > I've been discouraged in the past from adding these extra tags to LayoutTests if > they're not necessary - but if they are they should probably also be added to > the expected.html reference file. Opinions differ, but since the other tests in this area don't do it, I won't either. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:51: return paintLayer->shouldPaintBackgroundOntoScrollingContentsLayer(); On 2016/08/30 21:14:49, flackr wrote: > We should probably move the tests rather than have CompositedLayerMappingTest > test PaintLayer. Done. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger() || RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled()) { On 2016/08/30 21:14:49, flackr wrote: > You mentioned that hasOverflowScrollTrigger() basically proxies whether we > prefer compositing to lcd text, but it seems like layerNeedsCompositedScrolling > in PaintLayerScrollableArea.cpp already includes whether prefer compositing to > lcd text. Assuming my following comment is not a problem, this condition can > probably be removed altogether. I don't think we can remove it for the scrolling case, but adjusting logic will clean it up and prevent confusion as to what applies where. https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:146: if (layer->clipParent()) On 2016/08/30 21:14:50, flackr wrote: > I'm concerned this condition combined with > RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() could cause us to add > a compositing reason to a scroller when it doesn't have an opaque background. We > should test this. I think I'll just adjust the logic.
The CQ bit was checked by schenney@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:146: if (layer->clipParent()) On 2016/08/31 at 20:50:20, Stephen Chennney wrote: > On 2016/08/30 21:14:50, flackr wrote: > > I'm concerned this condition combined with > > RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled() could cause us to add > > a compositing reason to a scroller when it doesn't have an opaque background. We > > should test this. > > I think I'll just adjust the logic. Sorry for the delay, I mistook this to mean you were going to have another comment later when this was done.
This will need to be rebased on my recently landed patch of course - but I think that's fairly mechanical - use the new upstream versions of the functions moved in this patch.
https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:51: if (!hasCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) Please use testharness.js + assert methods there for this rather than custom code. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html:43: result += "Fail.\n"; Same comment as on the other test. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background-will-change.html (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background-will-change.html:1: <script> codereview claims this is identical to compositing/overflow/overflow-scroll-with-opaque-background.html ? https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:146: directReasons |= CompositingReasonOutOfFlowClipping; Why doesn't this apply to low-DPI composited scrolling? https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (left): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:248: // both opaque and may only have an integer translation as its Are you checking for integer translation? https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.h:438: // the background can scroll with the content). When the background is also "can be painted into the composited scrolling contents layer when it exists" (last three words are my suggested addition) https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:58: "#scroller { overflow: scroll; height: 200px; width: 200px; background: white local content-box; border: 10px solid rgba(0, 255, 0, 0.5); }" Why did you change the test? Are you trying to test the 0.5 alpha on the border? If so that can be a second test. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:80: "#scroller { overflow: scroll; height: 200px; width: 200px; background: rgba(0, 255, 0, 0.5) local content-box; border: 10px solid rgba(0, 255, 0, 0.5); }" Same question here about why you needed to change the size.
I'll get a new patch up tomorrow, but I've meanwhile addressed the review questions. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:51: if (!hasCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) On 2016/09/02 01:19:38, chrishtr wrote: > Please use testharness.js + assert methods there for this rather than custom > code. I can't figure out how to do that and get a pixel result, which we want to make sure that we also render the right thing. If I use testharness.js without the testharnessreport.js the asserts don't cause a failure, but if I include the report js then we don't get the pixel results because testharnessreport.js clears the document body before creating it's output. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background-will-change.html (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-with-opaque-background-will-change.html:1: <script> On 2016/09/02 01:19:38, chrishtr wrote: > codereview claims this is identical to > compositing/overflow/overflow-scroll-with-opaque-background.html ? It is the same as the test previously named overflow-scroll-with-opaque-background.html. It's a rename of that test combined with a re-implementation of that test, since it is really testing the will-change effect rather than the opaque background case. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (left): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:248: // both opaque and may only have an integer translation as its On 2016/09/02 01:19:38, chrishtr wrote: > Are you checking for integer translation? I could DCHECK for it. I believe we force integer scrolling if needsCompositedScrolling is true, so we're OK. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayer.h:438: // the background can scroll with the content). When the background is also On 2016/09/02 01:19:38, chrishtr wrote: > "can be painted into the composited scrolling contents layer when it exists" > (last three words are my suggested addition) Will do. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:58: "#scroller { overflow: scroll; height: 200px; width: 200px; background: white local content-box; border: 10px solid rgba(0, 255, 0, 0.5); }" On 2016/09/02 01:19:38, chrishtr wrote: > Why did you change the test? Are you trying to test the 0.5 alpha on the border? > If so that can be a second test. I changed it to match the layout test setup. It's not necessary so I can remove it and will do.
https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/compositing/overflow/overflow-scroll-background-opaque-to-transparent.html:51: if (!hasCompositedScrollingContentsLayer(JSON.parse(window.internals.layerTreeAsText(document)))) On 2016/09/06 at 21:06:20, Stephen Chennney wrote: > On 2016/09/02 01:19:38, chrishtr wrote: > > Please use testharness.js + assert methods there for this rather than custom > > code. > > I can't figure out how to do that and get a pixel result, which we want to make sure that we also render the right thing. If I use testharness.js without the testharnessreport.js the asserts don't cause a failure, but if I include the report js then we don't get the pixel results because testharnessreport.js clears the document body before creating it's output. I see. Ok. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (left): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:248: // both opaque and may only have an integer translation as its On 2016/09/06 at 21:06:21, Stephen Chennney wrote: > On 2016/09/02 01:19:38, chrishtr wrote: > > Are you checking for integer translation? > > I could DCHECK for it. I believe we force integer scrolling if needsCompositedScrolling is true, so we're OK. The sum of the translations from the root must be integers to get LCD text. I think this needs more investigation. Please file a bug. and add a TODO here.
Still working on this. I need to get new baselines. I'll add some more comments when I have the new patch uploaded. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:146: directReasons |= CompositingReasonOutOfFlowClipping; On 2016/09/02 01:19:38, chrishtr wrote: > Why doesn't this apply to low-DPI composited scrolling? It presumably should, but note that flackr had concerns about adding this reason before we knew the layer would actually be composited, hence this refactoring. In the forthcoming patch I've looked at how the CompositingReasonOutOfFlowClipping reason is used and modified the single use location to add the reason directly, since it's in CompositingRequirementsUpdater and that's about the only place we have the necessary information to apply it correctly. https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (left): https://codereview.chromium.org/2259493004/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:248: // both opaque and may only have an integer translation as its On 2016/09/06 22:17:59, chrishtr wrote: > On 2016/09/06 at 21:06:21, Stephen Chennney wrote: > > On 2016/09/02 01:19:38, chrishtr wrote: > > > Are you checking for integer translation? > > > > I could DCHECK for it. I believe we force integer scrolling if > needsCompositedScrolling is true, so we're OK. > > The sum of the translations from the root must be integers to get LCD text. I > think this needs more investigation. Please file a bug. and add a TODO here. This comment on an earlier review explains why we have integer scroll offset: https://codereview.chromium.org/1826013002/#msg55 I've added the bug number and a comment, but not here, rather in CompositingReasonsFinder where it makes more sense. Here we are just adding CompositingReasonOverflowScrollingTouch for cases where we have already given up on getting LCD Text. The forthcoming patch reworks this comment to reflect what is actually happening.
https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (left): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger()) { Should hasOverflowScrollTrigger() now be deleted? Its other call site is PaintLayerCompositor::preferCompositingToLCDTextEnabled(), which could just consult document().page->settings().preferCompositingToLCDTextEnabled() instead. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: if (layer->clipParent()) { What went wrong exactly that it had to move from CompositingReasonFinder?
I missed a couple of requested changes. I'll deal with that now. flackr, could you look again at the changes to CompositingReasonFinder and CompositingRequirementsUpdater? https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/TestExpectations:53: crbug.com/381840 virtual/prefer_compositing_to_lcd_text/compositing/overflow/overflow-scroll-background-transparent-to-opaque.html [ Failure ] These fail because we composite regardless of background or composite scrollers flag when preferCompositingToLCDText. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (left): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger()) { This has been removed because layer->needsCompositedScrolling() takes care of the CompositingReasonOverflowScrollingTouch reason, while the CompositingReasonOutOfFlowClipping has been moved to the place it's used in CompositingRequirementsUpdater. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:147: directReasons |= CompositingReasonOutOfFlowClipping; This is only used in CompositingRequirementsUpdater, so move it there https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:150: directReasons |= CompositingReasonOverflowScrollingTouch; This check is all that remains. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (left): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:243: // therefore using grayscale AA. Clarified the meaning of this comment. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:245: // FIXME: it should also be possible to promote if the layer can This FIXME is redundant in that we have patches in flight or bugs to address every reason we can think of. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: if (layer->clipParent()) { This check was previously in CompositingReasonsFinder, but then the code here only applied when the preferCompositingToLCDText flag was set, and also did not apply if the m_hasCompositedScrollingAncestor flag was set in the code block above (where we add CompositingReasonOverflowScroll for things that will not be painting LCD text). Now it will apply when it needs to, and only when it needs to. Note that the effect of this is to add CompositingReasonsAssumedOverlap to some layers that previously may not have had it, but which it seems should have based on the logic for preferCompositingToLCDText. It could be I am misunderstanding and we only need this when we composite without LCD text, but that seems to suggest we need to avoid promoting scrollers if this condition would be hit.
The CQ bit was checked by schenney@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 think my previous round of comments addressed the other questions. https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (left): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:145: if (hasOverflowScrollTrigger()) { On 2016/09/07 20:38:07, chrishtr wrote: > Should hasOverflowScrollTrigger() now be deleted? > Its other call site is > PaintLayerCompositor::preferCompositingToLCDTextEnabled(), > which could just consult > document().page->settings().preferCompositingToLCDTextEnabled() instead. Yeah, I don't see why not.
https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: if (layer->clipParent()) { On 2016/09/07 at 20:38:40, Stephen Chennney wrote: > This check was previously in CompositingReasonsFinder, but then the code here only applied when the preferCompositingToLCDText flag was set, and also did not apply if the m_hasCompositedScrollingAncestor flag was set in the code block above (where we add CompositingReasonOverflowScroll for things that will not be painting LCD text). Now it will apply when it needs to, and only when it needs to. > > Note that the effect of this is to add CompositingReasonsAssumedOverlap to some layers that previously may not have had it, but which it seems should have based on the logic for preferCompositingToLCDText. It could be I am misunderstanding and we only need this when we composite without LCD text, but that seems to suggest we need to avoid promoting scrollers if this condition would be hit. I'm not sure this is correct, it seems to me this reason should be applied only if clipParent() is not a descendant of the nearest composited scroller. I.e. if you promote a scroller, you need to promote descendants for this reason, but if a scroller wasn't promoted then we don't need to promote these descendants.
https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: if (layer->clipParent()) { On 2016/09/07 20:57:06, flackr wrote: > On 2016/09/07 at 20:38:40, Stephen Chennney wrote: > > This check was previously in CompositingReasonsFinder, but then the code here > only applied when the preferCompositingToLCDText flag was set, and also did not > apply if the m_hasCompositedScrollingAncestor flag was set in the code block > above (where we add CompositingReasonOverflowScroll for things that will not be > painting LCD text). Now it will apply when it needs to, and only when it needs > to. > > > > Note that the effect of this is to add CompositingReasonsAssumedOverlap to > some layers that previously may not have had it, but which it seems should have > based on the logic for preferCompositingToLCDText. It could be I am > misunderstanding and we only need this when we composite without LCD text, but > that seems to suggest we need to avoid promoting scrollers if this condition > would be hit. > > I'm not sure this is correct, it seems to me this reason should be applied only > if clipParent() is not a descendant of the nearest composited scroller. I.e. if > you promote a scroller, you need to promote descendants for this reason, but if > a scroller wasn't promoted then we don't need to promote these descendants. Maybe the logic covers that. We're only here if currentRecursionData.m_hasCompositedScrollingAncestor is true, so we have a composited scrolling ancestor. I'm having a great deal of trouble figuring out the code immediately before this, but it seems to be only adding the CompositingReasonAssumedOverlap reason under some circumstances, which maybe covers your concerns. We have the option of totally removing CompositingReasonOutOfFlowClipping. It only gets added here now and is never used elsewhere, except if we dump reasons for debug or test.
https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: if (layer->clipParent()) { On 2016/09/07 at 21:08:19, Stephen Chennney wrote: > On 2016/09/07 20:57:06, flackr wrote: > > On 2016/09/07 at 20:38:40, Stephen Chennney wrote: > > > This check was previously in CompositingReasonsFinder, but then the code here > > only applied when the preferCompositingToLCDText flag was set, and also did not > > apply if the m_hasCompositedScrollingAncestor flag was set in the code block > > above (where we add CompositingReasonOverflowScroll for things that will not be > > painting LCD text). Now it will apply when it needs to, and only when it needs > > to. > > > > > > Note that the effect of this is to add CompositingReasonsAssumedOverlap to > > some layers that previously may not have had it, but which it seems should have > > based on the logic for preferCompositingToLCDText. It could be I am > > misunderstanding and we only need this when we composite without LCD text, but > > that seems to suggest we need to avoid promoting scrollers if this condition > > would be hit. > > > > I'm not sure this is correct, it seems to me this reason should be applied only > > if clipParent() is not a descendant of the nearest composited scroller. I.e. if > > you promote a scroller, you need to promote descendants for this reason, but if > > a scroller wasn't promoted then we don't need to promote these descendants. > > Maybe the logic covers that. We're only here if currentRecursionData.m_hasCompositedScrollingAncestor is true, so we have a composited scrolling ancestor. I'm having a great deal of trouble figuring out the code immediately before this, but it seems to be only adding the CompositingReasonAssumedOverlap reason under some circumstances, which maybe covers your concerns. > > We have the option of totally removing CompositingReasonOutOfFlowClipping. It only gets added here now and is never used elsewhere, except if we dump reasons for debug or test. Adding it here causes a promotion of the current layer because reasonsToComposite may have previously been CompositingReasonNone, right? That being said since as you pointed out we only get into this case if we already have an ancestor composited scroller it will most of the time be correct to promote the current element. Can you add a comment to the effect of "We only need to promote when the clipParent is not a descendant of the ancestor scroller". I think we don't want to actually do this check because it would require a walk but it's worth noting that we'll be promoting more often than we may need to.
The CQ bit was checked by schenney@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...
On 2016/09/07 22:38:41, flackr wrote: > https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp > (right): > > https://codereview.chromium.org/2259493004/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:282: > if (layer->clipParent()) { > On 2016/09/07 at 21:08:19, Stephen Chennney wrote: > > On 2016/09/07 20:57:06, flackr wrote: > > > On 2016/09/07 at 20:38:40, Stephen Chennney wrote: > > > > This check was previously in CompositingReasonsFinder, but then the code > here > > > only applied when the preferCompositingToLCDText flag was set, and also did > not > > > apply if the m_hasCompositedScrollingAncestor flag was set in the code block > > > above (where we add CompositingReasonOverflowScroll for things that will not > be > > > painting LCD text). Now it will apply when it needs to, and only when it > needs > > > to. > > > > > > > > Note that the effect of this is to add CompositingReasonsAssumedOverlap to > > > some layers that previously may not have had it, but which it seems should > have > > > based on the logic for preferCompositingToLCDText. It could be I am > > > misunderstanding and we only need this when we composite without LCD text, > but > > > that seems to suggest we need to avoid promoting scrollers if this condition > > > would be hit. > > > > > > I'm not sure this is correct, it seems to me this reason should be applied > only > > > if clipParent() is not a descendant of the nearest composited scroller. I.e. > if > > > you promote a scroller, you need to promote descendants for this reason, but > if > > > a scroller wasn't promoted then we don't need to promote these descendants. > > > > Maybe the logic covers that. We're only here if > currentRecursionData.m_hasCompositedScrollingAncestor is true, so we have a > composited scrolling ancestor. I'm having a great deal of trouble figuring out > the code immediately before this, but it seems to be only adding the > CompositingReasonAssumedOverlap reason under some circumstances, which maybe > covers your concerns. > > > > We have the option of totally removing CompositingReasonOutOfFlowClipping. It > only gets added here now and is never used elsewhere, except if we dump reasons > for debug or test. > > Adding it here causes a promotion of the current layer because > reasonsToComposite may have previously been CompositingReasonNone, right? > > That being said since as you pointed out we only get into this case if we > already have an ancestor composited scroller it will most of the time be correct > to promote the current element. Can you add a comment to the effect of "We only > need to promote when the clipParent is not a descendant of the ancestor > scroller". I think we don't want to actually do this check because it would > require a walk but it's worth noting that we'll be promoting more often than we > may need to. I've added that comment and am also now testing what happens if we do not add the CompositingReasonOutOfFlowClipping reason at all, so that there will be no promotion for layers that do not subsequently get the CompositingReasonAssumedOverlap reason when the unclippedDescendant list is processed. Meanwhile, I'm not aware of any additional outstanding issues. Christhr@, could you verify that your concerns have been addressed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 schenney@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...
It appears that never setting CompositingReasonOutOfFlowClipping makes no difference on Linux tests, but then maybe it will only hit on preferCompositingToLCDText tests, that I'm running now. Assuming they pass, and the trybots pass, I would really like to land this today, let it bake for a week with the flag off by default (to reveal side effects) and then turn the flag on by default in a week or two.
On 2016/09/09 18:33:25, Stephen Chennney wrote: > It appears that never setting CompositingReasonOutOfFlowClipping makes no > difference on Linux tests, but then maybe it will only hit on > preferCompositingToLCDText tests, that I'm running now. > > Assuming they pass, and the trybots pass, I would really like to land this > today, let it bake for a week with the flag off by default (to reveal side > effects) and then turn the flag on by default in a week or two. No layout tests on linux fail with the CompositeOpaqueScrollers flag enabled. Numerous fail with preferCompositingToLCDText indicating that we're doing a better job than the "composite everything" approach. We should consider removing preferCompositingToLCDText once we have scrollers compositing - it might resolve a lot of ChromeOS bugs. :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Looks good with small nit. https://codereview.chromium.org/2259493004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:239: // Add CompositingReasonOverflowScrollingTouch for layers that do not already have it but could. s/could/need it/
The CQ bit was checked by schenney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2259493004/#ps220001 (title: "Fix comment.")
Thanks all for the hard review slog. https://codereview.chromium.org/2259493004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp (right): https://codereview.chromium.org/2259493004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:239: // Add CompositingReasonOverflowScrollingTouch for layers that do not already have it but could. On 2016/09/09 20:32:14, chrishtr wrote: > s/could/need it/ Done.
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 Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. Add tests for promotion and demotion of scrolling layers as they switch between opaque and transparent backgrounds. R=flackr@chromium.org, chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. Add tests for promotion and demotion of scrolling layers as they switch between opaque and transparent backgrounds. R=flackr@chromium.org, chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Fix Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. Add tests for promotion and demotion of scrolling layers as they switch between opaque and transparent backgrounds. R=flackr@chromium.org, chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix Compositing of Opaque Scrolling Layers and Add Tests Move the CompositedLayerMapping::shouldPaintBackgroundOntoScrollingContentsLayer method to PaintLayer to use it for detecting composited scrolling. The method only uses data from PaintLayer, so there's no reason to have it elsewhere. Switch the check in PaintLayerScrollableArea for compositing scrollers on low-dpi devices to use the method. Fix the check in CompositingReasonsFinder to detect opaque background scrollers as a reason Remove the unnecessary check for self painting layer in PaintLayer::backgroundIsKnownToBeOpaqueInRect. The code is always called from a self painting layer's GraphicsLayer, or it's called by the recursive call checking childrens' opaqueness. The former case means the layer is always self painting, while the latter seems incorrect - one should not exclude, not include, child self painting layers. Add tests for promotion and demotion of scrolling layers as they switch between opaque and transparent backgrounds. R=flackr@chromium.org, chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/87b0099aa31169f8c7a40b4f93b4a1e86ddf53a1 Cr-Commit-Position: refs/heads/master@{#417743} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/87b0099aa31169f8c7a40b4f93b4a1e86ddf53a1 Cr-Commit-Position: refs/heads/master@{#417743} |
