Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243973002:
Fix scroll chaining in CC for non-default RootScroller. (Closed)
DescriptionFix scroll chaining in CC for non-default RootScroller.
Since the RootScroller proposal allows arbitrary elements to become the outer
viewport scroll layer, the scroll chaining behavior need a bit of an
adjustment. Specifically, we shouldn't chain past the outer viewport scroll
layer. The current code assumes that once we hit the outer viewport scroll
layer, we can continue chaining since we'll get to the inner viewport scroll
layer.
In this CL, we'll add the inner viewport scroll layer to the scroll chain and
immediately break when we encounter the outer viewport scroll layer, relying on
the fact that the scrolling code will use cc::Viewport to scroll the inner
viewport scroll layer.
Also added a test to make sure TopControls movement works correctly when a non-
default outer viewport layer is used.
BUG=505516
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/bc115b4fc5960bfbac9b53b1db4639b648bf59bf
Cr-Commit-Position: refs/heads/master@{#413439}
Patch Set 1 #Patch Set 2 #
Total comments: 4
Patch Set 3 : Addressed Tim's comments #
Messages
Total messages: 26 (18 generated)
Description was changed from ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layers, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. BUG=505516 ========== to ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layers, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layers, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
bokan@chromium.org changed reviewers: + tdresser@chromium.org
Description was changed from ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- defaut outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Tim, ptal.
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.
In CL description: defaut -> default LGTM, tests look great. https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3022: ScrollNode* inner_scroll_node = Maybe inner_viewport_scroll_node? https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6389: // Test that scroll chain correctly when a child scroller on the page (e.g. a Test that scroll*s*
Description was changed from ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- defaut outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- default outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3022: ScrollNode* inner_scroll_node = On 2016/08/15 13:37:41, tdresser wrote: > Maybe inner_viewport_scroll_node? Done. https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2243973002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:6389: // Test that scroll chain correctly when a child scroller on the page (e.g. a On 2016/08/15 13:37:41, tdresser wrote: > Test that scroll*s* Done.
bokan@chromium.org changed reviewers: + ajuma@chromium.org
+ajuma@ for OWNER
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.
lgtm
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2243973002/#ps40001 (title: "Addressed Tim's comments")
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 scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- default outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- default outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- default outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Fix scroll chaining in CC for non-default RootScroller. Since the RootScroller proposal allows arbitrary elements to become the outer viewport scroll layer, the scroll chaining behavior need a bit of an adjustment. Specifically, we shouldn't chain past the outer viewport scroll layer. The current code assumes that once we hit the outer viewport scroll layer, we can continue chaining since we'll get to the inner viewport scroll layer. In this CL, we'll add the inner viewport scroll layer to the scroll chain and immediately break when we encounter the outer viewport scroll layer, relying on the fact that the scrolling code will use cc::Viewport to scroll the inner viewport scroll layer. Also added a test to make sure TopControls movement works correctly when a non- default outer viewport layer is used. BUG=505516 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/bc115b4fc5960bfbac9b53b1db4639b648bf59bf Cr-Commit-Position: refs/heads/master@{#413439} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bc115b4fc5960bfbac9b53b1db4639b648bf59bf Cr-Commit-Position: refs/heads/master@{#413439} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
