Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1826013002:
Enable compositing for opaque scrolling content on low DPI screens (Closed)
|
Created:
4 years, 9 months ago by Stephen Chennney Modified:
4 years, 4 months ago CC:
ajuma, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, paulirish, 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. |
DescriptionEnable compositing for opaque scrolling content on low DPI screens
If an overflow scroll element has an opaque background color we
will consider it for promotion to a composited layer, regardless
of the LCD text vs compositing reference. A runtime flag must be
enabled on the command line to test and use this feature. It will
be removed after we verify stability and fix related bugs.
R=flackr, chrishtr
BUG=381840
Committed: https://crrev.com/a0abcf517e027c793782d057982d992ae0ea4612
Cr-Commit-Position: refs/heads/master@{#411037}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Retry patch #Patch Set 3 : Catch more cases #Patch Set 4 : Fix integral scroll offset #Patch Set 5 : Do not promote floats #Patch Set 6 : Fix border issue and move logic to PaintLayerScrollableArea #Patch Set 7 : Add runtime flag #
Total comments: 6
Patch Set 8 : Disable feature by default, not enabled in dev builds #
Total comments: 9
Patch Set 9 : Fix up integer scroll logic #Patch Set 10 : Expectations. #Patch Set 11 : Just the code, no tests #Patch Set 12 : Add test and TODO for failure mode #
Total comments: 7
Patch Set 13 : Fix date (for real this time) #
Messages
Total messages: 93 (40 generated)
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/patch-status/1826013002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826013002/1
Description was changed from ========== Enable compositing for opaque scrolling content on low DPI screens WIP If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. Tests of content stacking on top of a scrolling div are failing, so there's a real bug. R= BUG=381840 ========== to ========== Enable compositing for opaque scrolling content on low DPI screens WIP If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. Tests of content stacking on top of a scrolling div are failing, so there's a real bug. R= BUG=381840 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Very interested in this CL moving forward! What are the next steps here?
On 2016/05/13 08:49:15, paulirish wrote: > Very interested in this CL moving forward! What are the next steps here? Rob Flack asked me about picking it up. I let him know he can take it if he would like. This patch broke several compositor tests in non-obvious ways, revealing existing bugs and/or due to me making the wrong decision on whether some code paths needed changing. I have some other work that is seriously affecting background image quality under zoom, after which I was planning to come back top this.
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1826013002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp (right): https://codereview.chromium.org/1826013002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositingReasonFinder.cpp:39: || !layer->layoutObject()->style()->visitedDependentColor(CSSPropertyBackgroundColor).hasAlpha(); You'll also have to ensure that the scroller is positioned at exact integer offsets and is not rotated, mirroring the LCD logic in cc.
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/patch-status/1826013002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826013002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/patch-status/1826013002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826013002/40001
I've tried to update all the checks for composited scrolling to use the opacity check. We clearly were not before. But test results locally don't look better, so more work is required. I believe this patch also addresses the integral scroll offset problem.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/patch-status/1826013002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1826013002/60001
The only change in the latest patch is to remove an illegal compositing state check in FrameView and to rebase (bringing in numerous other changes to files). All of the significant failures, the Debug asserts and the big differences in painting, exist with the --enable-prefer-compositing-to-lcd-text flag, and even reproduce in production Chrome on Mac. Given we've lived with those in production for a long time, it seems we could land this with the same failures and figure out how to fix the bug in other patches.
On 2016/06/01 at 19:13:59, schenney wrote: > The only change in the latest patch is to remove an illegal compositing state check in FrameView and to rebase (bringing in numerous other changes to files). > > All of the significant failures, the Debug asserts and the big differences in painting, exist with the --enable-prefer-compositing-to-lcd-text flag, and even reproduce in production Chrome on Mac. Given we've lived with those in production for a long time, it seems we could land this with the same failures and figure out how to fix the bug in other patches. I don't think that's a great idea. These are real bugs. We should fix them before rolling them out to more users.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The latest patch fixes issues with floats, but we still have significant failures for fast/overflow/overflow-with-local-background-attachment.html (border all wrong, for unknown reasons) fast/overflow/overflow-stacking.html (wrong stacking order) fast/clip/014.html (scrollbars not clipped) I'll look at the background one first, then the scrollbar clipping, then the stacking one. Feel free to chip in.
On 2016/06/02 19:42:20, Stephen Chennney wrote: > The latest patch fixes issues with floats, but we still have significant > failures for > > fast/overflow/overflow-with-local-background-attachment.html (border all wrong, > for unknown reasons) > fast/overflow/overflow-stacking.html (wrong stacking order) > fast/clip/014.html (scrollbars not clipped) > > I'll look at the background one first, then the scrollbar clipping, then the > stacking one. Feel free to chip in. I've fixed fast/overflow/overflow-with-local-background-attachment.html by preventing compositing with border decoration. Tomorrow I'll take the time to verify that we fail in all such cases without this patch. vollick@ has a fix for fast/clip/014.html, basically by catching an additional clip reason to not promote. vollick@ is looking at overflow stacking problems. flackr@ has figured out why we do not show lcd text even with the patch and is looking at a fix. Best way to proceed, probably, is to land other patches then rebase this one, as other issues already repro with --preferCompositingTo...
On 2016/06/02 at 20:56:14, schenney wrote: > On 2016/06/02 19:42:20, Stephen Chennney wrote: > > The latest patch fixes issues with floats, but we still have significant > > failures for > > > > fast/overflow/overflow-with-local-background-attachment.html (border all wrong, > > for unknown reasons) > > fast/overflow/overflow-stacking.html (wrong stacking order) > > fast/clip/014.html (scrollbars not clipped) > > > > I'll look at the background one first, then the scrollbar clipping, then the > > stacking one. Feel free to chip in. > > I've fixed fast/overflow/overflow-with-local-background-attachment.html by preventing compositing with border decoration. Tomorrow I'll take the time to verify that we fail in all such cases without this patch. > vollick@ has a fix for fast/clip/014.html, basically by catching an additional clip reason to not promote. > vollick@ is looking at overflow stacking problems. > flackr@ has figured out why we do not show lcd text even with the patch and is looking at a fix. > > Best way to proceed, probably, is to land other patches then rebase this one, as other issues already repro with --preferCompositingTo... Cool! What was the reason fixed fast/overflow/overflow-with-local-background-attachment.html doesn't allow compositing with border decoration?
On 2016/06/02 21:03:25, chrishtr wrote: > > Best way to proceed, probably, is to land other patches then rebase this one, > as other issues already repro with --preferCompositingTo... > > Cool! > > What was the reason fixed > fast/overflow/overflow-with-local-background-attachment.html doesn't allow > compositing with border decoration? The test has a dashed wide border on the overflow element, and when composited the layer is initially painted with the debug color or left unfilled so the negative space in the dot pattern is filled with some color (red in debug and black in release, IIRC) instead of being white. It's not worth filling every scrolling layer just to solve this rare use case, so disabling in the border decoration case seems the best option.
On 2016/06/03 17:26:00, Stephen Chennney wrote: > On 2016/06/02 21:03:25, chrishtr wrote: > > > > Best way to proceed, probably, is to land other patches then rebase this > one, > > as other issues already repro with --preferCompositingTo... > > > > Cool! > > > > What was the reason fixed > > fast/overflow/overflow-with-local-background-attachment.html doesn't allow > > compositing with border decoration? > > The test has a dashed wide border on the overflow element, and when composited > the layer is initially painted with the debug color or left unfilled so the > negative space in the dot pattern is filled with some color (red in debug and > black in release, IIRC) instead of being white. It's not worth filling every > scrolling layer just to solve this rare use case, so disabling in the border > decoration case seems the best option. The problem with fast/overflow/overflow-stacking.html is the fundamental compositing bug. The overlapping div has a negative margin and doesn't participate in overlap testing. flackr@ is going to look at requiring a PaintLayer for objects with negative margins.
On 2016/06/03 at 17:51:53, vollick wrote: > On 2016/06/03 17:26:00, Stephen Chennney wrote: > > On 2016/06/02 21:03:25, chrishtr wrote: > > > > > > Best way to proceed, probably, is to land other patches then rebase this > > one, > > > as other issues already repro with --preferCompositingTo... > > > > > > Cool! > > > > > > What was the reason fixed > > > fast/overflow/overflow-with-local-background-attachment.html doesn't allow > > > compositing with border decoration? > > > > The test has a dashed wide border on the overflow element, and when composited > > the layer is initially painted with the debug color or left unfilled so the > > negative space in the dot pattern is filled with some color (red in debug and > > black in release, IIRC) instead of being white. It's not worth filling every > > scrolling layer just to solve this rare use case, so disabling in the border > > decoration case seems the best option. > > The problem with fast/overflow/overflow-stacking.html is the fundamental compositing bug. > > The overlapping div has a negative margin and doesn't participate in overlap testing. > > flackr@ is going to look at requiring a PaintLayer for objects with negative margins. I don't think that will work. PaintLayers have other side-effects. If it's the FCB then I think we should just decide to just eat the regression or not.
On 2016/06/03 17:51:53, vollick wrote: > On 2016/06/03 17:26:00, Stephen Chennney wrote: > > On 2016/06/02 21:03:25, chrishtr wrote: > > > > > > Best way to proceed, probably, is to land other patches then rebase this > > one, > > > as other issues already repro with --preferCompositingTo... > > > > > > Cool! > > > > > > What was the reason fixed > > > fast/overflow/overflow-with-local-background-attachment.html doesn't allow > > > compositing with border decoration? > > > > The test has a dashed wide border on the overflow element, and when composited > > the layer is initially painted with the debug color or left unfilled so the > > negative space in the dot pattern is filled with some color (red in debug and > > black in release, IIRC) instead of being white. It's not worth filling every > > scrolling layer just to solve this rare use case, so disabling in the border > > decoration case seems the best option. > > The problem with fast/overflow/overflow-stacking.html is the fundamental > compositing bug. > > The overlapping div has a negative margin and doesn't participate in overlap > testing. > > flackr@ is going to look at requiring a PaintLayer for objects with negative > margins. This isn't a real fix, of course (that's SPv2), but it might be possible to catch a common form of the bug.
On 2016/06/03 17:59:24, vollick wrote: > On 2016/06/03 17:51:53, vollick wrote: > > On 2016/06/03 17:26:00, Stephen Chennney wrote: > > > On 2016/06/02 21:03:25, chrishtr wrote: > > > > > > > > Best way to proceed, probably, is to land other patches then rebase this > > > one, > > > > as other issues already repro with --preferCompositingTo... > > > > > > > > Cool! > > > > > > > > What was the reason fixed > > > > fast/overflow/overflow-with-local-background-attachment.html doesn't allow > > > > compositing with border decoration? > > > > > > The test has a dashed wide border on the overflow element, and when > composited > > > the layer is initially painted with the debug color or left unfilled so the > > > negative space in the dot pattern is filled with some color (red in debug > and > > > black in release, IIRC) instead of being white. It's not worth filling every > > > scrolling layer just to solve this rare use case, so disabling in the border > > > decoration case seems the best option. > > > > The problem with fast/overflow/overflow-stacking.html is the fundamental > > compositing bug. > > > > The overlapping div has a negative margin and doesn't participate in overlap > > testing. > > > > flackr@ is going to look at requiring a PaintLayer for objects with negative > > margins. > > This isn't a real fix, of course (that's SPv2), but it might be possible to > catch a common form of the bug. Ah, sorry Chris, I missed your last comment. I agree with you. As Rob pointed out more deficiencies with the approach, it now seems silly. Feels like we should eat it. Would you agree?
On 2016/06/03 at 18:00:37, vollick wrote: > On 2016/06/03 17:59:24, vollick wrote: > > On 2016/06/03 17:51:53, vollick wrote: > > > On 2016/06/03 17:26:00, Stephen Chennney wrote: > > > > On 2016/06/02 21:03:25, chrishtr wrote: > > > > > > > > > > Best way to proceed, probably, is to land other patches then rebase this > > > > one, > > > > > as other issues already repro with --preferCompositingTo... > > > > > > > > > > Cool! > > > > > > > > > > What was the reason fixed > > > > > fast/overflow/overflow-with-local-background-attachment.html doesn't allow > > > > > compositing with border decoration? > > > > > > > > The test has a dashed wide border on the overflow element, and when > > composited > > > > the layer is initially painted with the debug color or left unfilled so the > > > > negative space in the dot pattern is filled with some color (red in debug > > and > > > > black in release, IIRC) instead of being white. It's not worth filling every > > > > scrolling layer just to solve this rare use case, so disabling in the border > > > > decoration case seems the best option. > > > > > > The problem with fast/overflow/overflow-stacking.html is the fundamental > > > compositing bug. > > > > > > The overlapping div has a negative margin and doesn't participate in overlap > > > testing. > > > > > > flackr@ is going to look at requiring a PaintLayer for objects with negative > > > margins. > > > > This isn't a real fix, of course (that's SPv2), but it might be possible to > > catch a common form of the bug. > > Ah, sorry Chris, I missed your last comment. I agree with you. As Rob pointed out more deficiencies with the approach, it now seems silly. > > Feels like we should eat it. Would you agree? Well we're eating it already on all retina devices...I guess we can eat some more.
Description was changed from ========== Enable compositing for opaque scrolling content on low DPI screens WIP If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. Tests of content stacking on top of a scrolling div are failing, so there's a real bug. R= BUG=381840 ========== to ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. Also convert all ASSERT to DCHECK and related functions in FrameView (to get past presubmit). R=flackr BUG=381840 ==========
schenney@chromium.org changed reviewers: + flackr@chromium.org - chrishtr@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...
Updated patch puts the new composited scrolling changes behind a flag, so we can test more easily while we fix related bugs (particularly work through https://codereview.chromium.org/2068723002/ I think I need some test expectations for the changed tests.
Description was changed from ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. Also convert all ASSERT to DCHECK and related functions in FrameView (to get past presubmit). R=flackr BUG=381840 ========== to ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. Also convert all ASSERT to DCHECK and related functions in FrameView (to get past presubmit). R=flackr BUG=381840 ==========
On 2016/07/27 17:18:23, Stephen Chennney wrote: > Updated patch puts the new composited scrolling changes behind a flag, so we can > test more easily while we fix related bugs (particularly work through > https://codereview.chromium.org/2068723002/ > > I think I need some test expectations for the changed tests. Patch was wrong. Feature should not be experimental. Latest patch fixes it.
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_...)
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...
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:196: #if ENABLE(ASSERT) Why did you add the #if here? https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: || layer->layoutObject()->isFloating() Why the two new things listed here?
Also looking at why this patch seems to make some difference to another bug reproduction case, when it shouldn't have any effect if the flag is not enabled. https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:196: #if ENABLE(ASSERT) On 2016/07/27 18:45:44, chrishtr wrote: > Why did you add the #if here? Because m_hasBeenDisposed is only defined if ENABLE(ASSERT) and apparently DCHECK does not look at ENABLE(ASSERT) while the old Blink ASSERT does. There might be a corresponding ENABLE(CHECK) that I'll try to find. https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: || layer->layoutObject()->isFloating() On 2016/07/27 18:45:44, chrishtr wrote: > Why the two new things listed here? Because Floating layers trigger a isSelfPaintingLayer assertion if composited, and because dashed borders do not paint correctly if composited. That might be related to the one seen in the background-paints-in-foreground patch but the set of test failures is different. I think this case is due to shrinking the background to not draw in the border area, but we need it to if the border is dashed and the background is visible behind it.
https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: || layer->layoutObject()->isFloating() On 2016/07/27 at 19:13:26, Stephen Chennney wrote: > On 2016/07/27 18:45:44, chrishtr wrote: > > Why the two new things listed here? > > Because Floating layers trigger a isSelfPaintingLayer assertion if composited, and because dashed borders do not paint correctly if composited. That might be related to the one seen in the background-paints-in-foreground patch but the set of test failures is different. I think this case is due to shrinking the background to not draw in the border area, but we need it to if the border is dashed and the background is visible behind it. Please remove these then. Let's address them in a followup. I'd like to debug with you whether they will work (sounds like the second one might start working with flackr's patch). https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:690: if (layoutView.layer()->needsCompositedScrolling()) I don't think this will work, because the view is special and doesn't have overflow clipping. https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2202: LayoutViewItem layoutView = this->layoutViewItem(); Ditto. I think you should just write: if (m_frame->settings() && (!m_frame->settings()->preferCompositingToLCDTextEnabled() || RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled()))
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/07/27 19:26:01, chrishtr wrote: > https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: || > layer->layoutObject()->isFloating() > On 2016/07/27 at 19:13:26, Stephen Chennney wrote: > > On 2016/07/27 18:45:44, chrishtr wrote: > > > Why the two new things listed here? > > > > Because Floating layers trigger a isSelfPaintingLayer assertion if composited, > and because dashed borders do not paint correctly if composited. That might be > related to the one seen in the background-paints-in-foreground patch but the set > of test failures is different. I think this case is due to shrinking the > background to not draw in the border area, but we need it to if the border is > dashed and the background is visible behind it. > > Please remove these then. Let's address them in a followup. I'd like to debug > with you whether they will work (sounds like the second one might start working > with flackr's patch). I can do that, sure. > > https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameView.cpp:690: if > (layoutView.layer()->needsCompositedScrolling()) > I don't think this will work, because the view is special and doesn't have > overflow clipping. > > https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameView.cpp:2202: LayoutViewItem > layoutView = this->layoutViewItem(); > Ditto. > > I think you should just write: > > if (m_frame->settings() && > (!m_frame->settings()->preferCompositingToLCDTextEnabled() || > RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled())) The alternative is a new method on PaintLayerScrollableArea for needsLCDText, which is what this method is a proxy for. I'm looking at how hard that will be, because without it we'll always have integer scrolling when we turn the flag on for good.
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
I assume we shouldn't have to change the existing layout tests to get this patch to land - if they fail it represents real problem cases with the automatic promotion right? https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1507: || layer->layoutObject()->style()->hasBorderDecoration()); Why can we not use composited scrolling with a border decoration or floating elements? Won't this reduce the number of composited scrolling cases we have on High DPI today?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Turns out we don't need to do anything special about integer scroll offsets. If the user prefers LCD text (as indicated by !preferCompositingToLCDText) we give integer scroll offsets, and we are only invoking our new logic if thats already the case. If the user prefers compositing, we never even try to draw LCD text. This means all those changes to FrameView are not necessary. https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1505: || layer->layoutObject()->isFloating() On 2016/07/27 19:26:00, chrishtr wrote: > On 2016/07/27 at 19:13:26, Stephen Chennney wrote: > > On 2016/07/27 18:45:44, chrishtr wrote: > > > Why the two new things listed here? > > > > Because Floating layers trigger a isSelfPaintingLayer assertion if composited, > and because dashed borders do not paint correctly if composited. That might be > related to the one seen in the background-paints-in-foreground patch but the set > of test failures is different. I think this case is due to shrinking the > background to not draw in the border area, but we need it to if the border is > dashed and the background is visible behind it. > > Please remove these then. Let's address them in a followup. I'd like to debug > with you whether they will work (sounds like the second one might start working > with flackr's patch). Done.
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_...)
Expectations and everything probably passes now. I think we could land this pending any comments.
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 ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. Also convert all ASSERT to DCHECK and related functions in FrameView (to get past presubmit). R=flackr BUG=381840 ========== to ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. R=flackr, chrishtr BUG=381840 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So much confusion, so many patches. Sorry I overlooked some questions. https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:690: if (layoutView.layer()->needsCompositedScrolling()) On 2016/07/27 19:26:00, chrishtr wrote: > I don't think this will work, because the view is special and doesn't have > overflow clipping. > Done. https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2202: LayoutViewItem layoutView = this->layoutViewItem(); On 2016/07/27 19:26:00, chrishtr wrote: > Ditto. > > I think you should just write: > > if (m_frame->settings() && > (!m_frame->settings()->preferCompositingToLCDTextEnabled() || > RuntimeEnabledFeatures::compositeOpaqueScrollersEnabled())) We don't need anything at all. The logic works such that we can only be trying to use high DPI when the preferCompositingToLCDTextEnabled is false, so the existing check catches it. https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1507: || layer->layoutObject()->style()->hasBorderDecoration()); On 2016/07/27 22:17:27, flackr wrote: > Why can we not use composited scrolling with a border decoration or floating > elements? Won't this reduce the number of composited scrolling cases we have on > High DPI today? Dashed borders are broken in tests because we don't draw the background behind the dashes (might be a test problem using the wrong box). Floating layers give assertions about selfPaintingLayer. These tests fail today with existing promotion on HighDPI devices, I believe, but we don't really look at the results.
On 2016/07/27 22:17:27, flackr wrote: > I assume we shouldn't have to change the existing layout tests to get this patch > to land - if they fail it represents real problem cases with the automatic > promotion right? No tests should fail apart from the one I modified to be clearer when debugging. I changed the background on it which changed the test result. In particular, several tests had solid background colors that made it impossible to see whether the element had scrolled or not. Or they had solid backgrounds and were getting promoted but the test was explicitly checking what happens when something is not promoted. Those I just changed to add transparency to the background. I suppose that for this patch I don't need to change any tests, since the flag is not active anywhere, but I don't want spurious failures when we run layout tests locally with the flag. I only want to see real unresolved issues.
On 2016/07/28 at 14:00:45, schenney wrote: > On 2016/07/27 22:17:27, flackr wrote: > > I assume we shouldn't have to change the existing layout tests to get this patch > > to land - if they fail it represents real problem cases with the automatic > > promotion right? > > No tests should fail apart from the one I modified to be clearer when debugging. I changed the background on it which changed the test result. In particular, several tests had solid background colors that made it impossible to see whether the element had scrolled or not. Or they had solid backgrounds and were getting promoted but the test was explicitly checking what happens when something is not promoted. Those I just changed to add transparency to the background. > > I suppose that for this patch I don't need to change any tests, since the flag is not active anywhere, but I don't want spurious failures when we run layout tests locally with the flag. I only want to see real unresolved issues. Understood, this means these tests would also have failed if run in High DPI where we would promote them currently. https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1507: || layer->layoutObject()->style()->hasBorderDecoration()); On 2016/07/28 at 13:55:38, Stephen Chennney wrote: > On 2016/07/27 22:17:27, flackr wrote: > > Why can we not use composited scrolling with a border decoration or floating > > elements? Won't this reduce the number of composited scrolling cases we have on > > High DPI today? > > Dashed borders are broken in tests because we don't draw the background behind the dashes (might be a test problem using the wrong box). Floating layers give assertions about selfPaintingLayer. > > These tests fail today with existing promotion on HighDPI devices, I believe, but we don't really look at the results. Understood, sounds good. Can we land these as a separate change with high dpi tests that will fail if we don't have these extra conditions?
https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1507: || layer->layoutObject()->style()->hasBorderDecoration()); On 2016/07/28 14:24:25, flackr wrote: > On 2016/07/28 at 13:55:38, Stephen Chennney wrote: > > On 2016/07/27 22:17:27, flackr wrote: > > > Why can we not use composited scrolling with a border decoration or floating > > > elements? Won't this reduce the number of composited scrolling cases we have > on > > > High DPI today? > > > > Dashed borders are broken in tests because we don't draw the background behind > the dashes (might be a test problem using the wrong box). Floating layers give > assertions about selfPaintingLayer. > > > > These tests fail today with existing promotion on HighDPI devices, I believe, > but we don't really look at the results. > > Understood, sounds good. Can we land these as a separate change with high dpi > tests that will fail if we don't have these extra conditions? I'm not sure what you mean. Do you mean leave out these extra conditions in the code and address them in a separate change? If so, Chris also thought so and the latest patch set does that. The follow up plan in that case is to fix the underlying issues so that we don't need the extra rejection conditions.
https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1507: || layer->layoutObject()->style()->hasBorderDecoration()); On 2016/07/28 at 14:50:47, Stephen Chennney wrote: > On 2016/07/28 14:24:25, flackr wrote: > > On 2016/07/28 at 13:55:38, Stephen Chennney wrote: > > > On 2016/07/27 22:17:27, flackr wrote: > > > > Why can we not use composited scrolling with a border decoration or floating > > > > elements? Won't this reduce the number of composited scrolling cases we have > > on > > > > High DPI today? > > > > > > Dashed borders are broken in tests because we don't draw the background behind > > the dashes (might be a test problem using the wrong box). Floating layers give > > assertions about selfPaintingLayer. > > > > > > These tests fail today with existing promotion on HighDPI devices, I believe, > > but we don't really look at the results. > > > > Understood, sounds good. Can we land these as a separate change with high dpi > > tests that will fail if we don't have these extra conditions? > > I'm not sure what you mean. Do you mean leave out these extra conditions in the code and address them in a separate change? If so, Chris also thought so and the latest patch set does that. The follow up plan in that case is to fix the underlying issues so that we don't need the extra rejection conditions. Yes, basically, we can land these extra conditions ahead of this patch (or after I suppose since we're not turning on promotion by default yet). Since it reproduces on high dpi you should be able to demonstrate that these conditions are necessary with a test in one of our VirtualTestSuites which enables high dpi: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/VirtualTe...
There should be no layout test changes in this CL. Please put them in a followup if needed, and ideally by adding new tests rather than mutating existing ones.
On 2016/07/28 16:06:47, chrishtr wrote: > There should be no layout test changes in this CL. Please put them in a followup > if needed, > and ideally by adding new tests rather than mutating existing ones. This test, compositing/change-compositing-settings.html, is explicitly testing that things are promoted or not based on the preferCompositingToLCDText flag, assuming that the flag is the only thing that will get a scroll composited. Now that we have something else that gets things composited, we need to modify the test to isolate the flag's effect and not the background's effect. Would you rather I renamed the test to explicitly reflect that it's testing the flag? Having said that I agree there should be a test to verify that solid background gets layers promoted if the compositeOpaqueBackgrounds flag is set. That means we need to change window.internals to make the flag query-able (or is that automatic). And I'll move the other test changes to a different patches. Some are about improving the test to make it possible to verify the result visually. Some should be duplicated for the composite scrollers/don't composite scrollers cases. Again that requires access in test internals.
On 2016/07/28 at 16:24:52, schenney wrote: > On 2016/07/28 16:06:47, chrishtr wrote: > > There should be no layout test changes in this CL. Please put them in a followup > > if needed, > > and ideally by adding new tests rather than mutating existing ones. > > This test, compositing/change-compositing-settings.html, is explicitly testing that things are promoted or not based on the preferCompositingToLCDText flag, assuming that the flag is the only thing that will get a scroll composited. Now that we have something else that gets things composited, we need to modify the test to isolate the flag's effect and not the background's effect. Would you rather I renamed the test to explicitly reflect that it's testing the flag? The flag is not set for testing, so I don't see why this test should change. I agree it's good to test the flag in this CL (via a new layout test for safety). > > Having said that I agree there should be a test to verify that solid background gets layers promoted if the compositeOpaqueBackgrounds flag is set. That means we need to change window.internals to make the flag query-able (or is that automatic). > > And I'll move the other test changes to a different patches. Some are about improving the test to make it possible to verify the result visually. Some should be duplicated for the composite scrollers/don't composite scrollers cases. Again that requires access in test internals.
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: This issue passed the CQ dry run.
Finally, ready to land, I think. We have a unit test, some more todos, and no layout test changes.
https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1508: // TODO(schenney) Tests fail if we do not also exclude layer->layoutObject()->isFloating() (asserts) and isFloating should have been fixed by https://codereview.chromium.org/2191953002. The other is fixed by better blacklisting above. Maybe just remove this TODO? https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:25: DisableCompositingQueryAsserts compositingQueryEnabler; Why do you need this? The lifecycle should be up to date.
On 2016/08/04 at 16:18:11, schenney wrote: > Finally, ready to land, I think. We have a unit test, some more todos, and no layout test changes. I really think we shouldn't land this patch until we can base it on my patch https://codereview.chromium.org/2196583002 (which should be ready to land) and turn it on by default. The condition you're using for when to promote isn't correct as it needs to be based on the same conditions I've added in my patch (PaintLayer::shouldPaintBackgroundOntoForeground and PaintLayer::backgroundIsKnownToBeOpaqueInRect) since if we don't paint the background in the foreground or if it's not opaque we can't composite without losing LCD text.
My intention was to land independent of flackr's patch and update the check in PaintLayerScrollableArea to use the logic flackr has added in PaintLayer, because that it the correct logic. I added to TODO to track that. I don't mind which order we land in. One advantage of landing this first is that flackr can apply his patch and easily verify we get the LCD text. The advantage of landing background-into-foreground painting first is that I can remove the TODO before submitting. Chris as tiebreaker. https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1508: // TODO(schenney) Tests fail if we do not also exclude layer->layoutObject()->isFloating() (asserts) and On 2016/08/04 16:35:44, chrishtr wrote: > isFloating should have been fixed by https://codereview.chromium.org/2191953002. > The other is fixed by > better blacklisting above. Maybe just remove this TODO? Yep. Forgot you fixed that. https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/08/04 16:35:44, chrishtr wrote: > 2016 Done. https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:25: DisableCompositingQueryAsserts compositingQueryEnabler; On 2016/08/04 16:35:44, chrishtr wrote: > Why do you need this? The lifecycle should be up to date. Artifact of an earlier iteration of the test. Gone.
On 2016/08/04 at 17:35:25, schenney wrote: > My intention was to land independent of flackr's patch and update the check in PaintLayerScrollableArea to use the logic flackr has added in PaintLayer, because that it the correct logic. I added to TODO to track that. > > I don't mind which order we land in. One advantage of landing this first is that flackr can apply his patch and easily verify we get the LCD text. FWIW, I have been verifying that we still get LCD text by forcing a composited layer with "will-change: transform" on the scroller :-) > The advantage of landing background-into-foreground painting first is that I can remove the TODO before submitting. My concern is that the promotion logic should be tightly coupled to the background painting into scrolling contents logic. > > Chris as tiebreaker. > > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): > > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1508: // TODO(schenney) Tests fail if we do not also exclude layer->layoutObject()->isFloating() (asserts) and > On 2016/08/04 16:35:44, chrishtr wrote: > > isFloating should have been fixed by https://codereview.chromium.org/2191953002. > > The other is fixed by > > better blacklisting above. Maybe just remove this TODO? > > Yep. Forgot you fixed that. > > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): > > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. > On 2016/08/04 16:35:44, chrishtr wrote: > > 2016 > > Done. > > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:25: DisableCompositingQueryAsserts compositingQueryEnabler; > On 2016/08/04 16:35:44, chrishtr wrote: > > Why do you need this? The lifecycle should be up to date. > > Artifact of an earlier iteration of the test. Gone.
https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp (right): https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/08/04 at 17:35:25, Stephen Chennney wrote: > On 2016/08/04 16:35:44, chrishtr wrote: > > 2016 > > Done. Still not done. Update the patchset?
On 2016/08/09 18:03:40, chrishtr wrote: > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp > (right): > > https://codereview.chromium.org/1826013002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PaintLayerScrollableAreaTest.cpp:1: // > Copyright 2014 The Chromium Authors. All rights reserved. > On 2016/08/04 at 17:35:25, Stephen Chennney wrote: > > On 2016/08/04 16:35:44, chrishtr wrote: > > > 2016 > > > > Done. > > Still not done. Update the patchset? Not sure what happened there. Done now, and the TODO updated.
The CQ bit was checked by chrishtr@chromium.org
lgtm
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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by schenney@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 ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. R=flackr, chrishtr BUG=381840 ========== to ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. R=flackr, chrishtr BUG=381840 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. R=flackr, chrishtr BUG=381840 ========== to ========== Enable compositing for opaque scrolling content on low DPI screens If an overflow scroll element has an opaque background color we will consider it for promotion to a composited layer, regardless of the LCD text vs compositing reference. A runtime flag must be enabled on the command line to test and use this feature. It will be removed after we verify stability and fix related bugs. R=flackr, chrishtr BUG=381840 Committed: https://crrev.com/a0abcf517e027c793782d057982d992ae0ea4612 Cr-Commit-Position: refs/heads/master@{#411037} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a0abcf517e027c793782d057982d992ae0ea4612 Cr-Commit-Position: refs/heads/master@{#411037} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
