Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2461463004:
Resize background-attachment: fixed when inertTopControls is enabled. (Closed)
|
Created:
4 years, 1 month ago by bokan Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, kinuko+watch, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResize background-attachment: fixed when inertTopControls is enabled.
When inertTopControls is enabled, hiding the top controls doesn't change the
layout height, meaning we don't invalidate layout or paint. However,
background-attachment: fixed is sized to the viewport which *is* resized. In
this case we need to explicitly do some work to make sure the background
updates correctly.
If the background is composited, the GraphicsLayer needs to be resized so we
mark the frame as needing layout so that happens. If the background isn't
composited, we only need to mark the LayoutView (which paints the background)
as needing a paint invalidation.
LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded would previously
check dimensions against viewWidth and viewHeight to determine if an
invalidation is needed. I've refactored the method somewhat but
viewWidth|Height are actually layout dimensions so I've replaced them with
frameView()->visibleContentSize which reflects the size of the viewport.
Removed LayoutView::backgroundRect as it's unused.
BUG=565930
Committed: https://crrev.com/478af0332a09c2d8179dccf09a6d078a515ffbaf
Cr-Commit-Position: refs/heads/master@{#429736}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Improved tests and fixed nit #
Total comments: 1
Patch Set 4 : Added test for non-fixed-attachment #
Messages
Total messages: 31 (21 generated)
Description was changed from ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. BUG=565930 ========== to ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. BUG=565930 ==========
bokan@chromium.org changed reviewers: + vollick@chromium.org
The CQ bit was checked by bokan@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...
Ian, WDYT? For composited backgrounds, needsLayout might be a bit heavy handed, and I got this to work with setNeedsGraphicsLayerUpdate(GraphicsLayerUpdateLocal) and compositor()->setNeedsCompositingUpdate(CompositingUpdateAfterGeometryChange) instead but that required querying compositing state from a resize so that's clearly not the right place for it. A layout figures out the background layer needs to be resized and should be cheap since we didn't change the layout size, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bokan@chromium.org changed reviewers: + flackr@chromium.org
vollick@ recommended flackr@ might be a good reviewer for this.
https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:1496: // If root layer scrolls if on, we've already issued a full invalidation nit s/if on/is on https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4269: TEST_F(WebViewTest, ResizeCompositedAndFixedBackground) { We should test with/without root layer scrolls right? https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4332: compositor->fixedRootBackgroundLayer()->size().height()); Is the invalidation tested?
Description was changed from ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. BUG=565930 ========== to ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. Removed LayoutView::backgroundRect as its unused. BUG=565930 ==========
Description was changed from ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. Removed LayoutView::backgroundRect as its unused. BUG=565930 ========== to ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. Removed LayoutView::backgroundRect as it's unused. BUG=565930 ==========
Description was changed from ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. Removed LayoutView::backgroundRect as it's unused. BUG=565930 ========== to ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded would previously check dimensions against viewWidth and viewHeight to determine if an invalidation is needed. I've refactored the method somewhat but viewWidth|Height are actually layout dimensions so I've replaced them with frameView()->visibleContentSize which reflects the size of the viewport. Removed LayoutView::backgroundRect as it's unused. BUG=565930 ==========
All comments addressed, ptal. https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:1496: // If root layer scrolls if on, we've already issued a full invalidation On 2016/10/31 18:17:55, flackr wrote: > nit s/if on/is on Done. https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4269: TEST_F(WebViewTest, ResizeCompositedAndFixedBackground) { On 2016/10/31 18:17:55, flackr wrote: > We should test with/without root layer scrolls right? Good point, I've moved this to VisualViewportTest which is parameterized to test root-layer-scrolling. https://codereview.chromium.org/2461463004/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/tests/WebViewTest.cpp:4332: compositor->fixedRootBackgroundLayer()->size().height()); On 2016/10/31 18:17:55, flackr wrote: > Is the invalidation tested? I figured out how to test invalidations. Added a second test for the case where we don't composite the background.
The CQ bit was checked by bokan@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.
Looks good, just one more possible test case. https://codereview.chromium.org/2461463004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): https://codereview.chromium.org/2461463004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:2094: TEST_P(VisualViewportTest, ResizeCompositedAndFixedBackground) { We should also test the case of no fixed background where there is no layout or invalidation right?
On 2016/11/03 13:28:11, flackr wrote: > Looks good, just one more possible test case. > > https://codereview.chromium.org/2461463004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/VisualViewportTest.cpp (right): > > https://codereview.chromium.org/2461463004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/VisualViewportTest.cpp:2094: > TEST_P(VisualViewportTest, ResizeCompositedAndFixedBackground) { > We should also test the case of no fixed background where there is no layout or > invalidation right? Done
The CQ bit was checked by bokan@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...
lgtm
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 bokan@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 ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded would previously check dimensions against viewWidth and viewHeight to determine if an invalidation is needed. I've refactored the method somewhat but viewWidth|Height are actually layout dimensions so I've replaced them with frameView()->visibleContentSize which reflects the size of the viewport. Removed LayoutView::backgroundRect as it's unused. BUG=565930 ========== to ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded would previously check dimensions against viewWidth and viewHeight to determine if an invalidation is needed. I've refactored the method somewhat but viewWidth|Height are actually layout dimensions so I've replaced them with frameView()->visibleContentSize which reflects the size of the viewport. Removed LayoutView::backgroundRect as it's unused. BUG=565930 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded would previously check dimensions against viewWidth and viewHeight to determine if an invalidation is needed. I've refactored the method somewhat but viewWidth|Height are actually layout dimensions so I've replaced them with frameView()->visibleContentSize which reflects the size of the viewport. Removed LayoutView::backgroundRect as it's unused. BUG=565930 ========== to ========== Resize background-attachment: fixed when inertTopControls is enabled. When inertTopControls is enabled, hiding the top controls doesn't change the layout height, meaning we don't invalidate layout or paint. However, background-attachment: fixed is sized to the viewport which *is* resized. In this case we need to explicitly do some work to make sure the background updates correctly. If the background is composited, the GraphicsLayer needs to be resized so we mark the frame as needing layout so that happens. If the background isn't composited, we only need to mark the LayoutView (which paints the background) as needing a paint invalidation. LayoutView::setShouldDoFullPaintInvalidationOnResizeIfNeeded would previously check dimensions against viewWidth and viewHeight to determine if an invalidation is needed. I've refactored the method somewhat but viewWidth|Height are actually layout dimensions so I've replaced them with frameView()->visibleContentSize which reflects the size of the viewport. Removed LayoutView::backgroundRect as it's unused. BUG=565930 Committed: https://crrev.com/478af0332a09c2d8179dccf09a6d078a515ffbaf Cr-Commit-Position: refs/heads/master@{#429736} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/478af0332a09c2d8179dccf09a6d078a515ffbaf Cr-Commit-Position: refs/heads/master@{#429736} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
