Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2621133002:
Return main thread scroll layer in FindScrollLayerForDeviceViewportPoint (Closed)
DescriptionReturn main thread scroll layer FindScrollLayerForDeviceViewportPoint
In FindScrollLayerForDeviceViewportPoint, we return null for main thread scroll
layer. But some calls only use the layer to look up animation controller and
that null return will make it wrong.
In the patch we return a layer without checking it is scroll in main thread.
BUG=679804
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2621133002
Cr-Commit-Position: refs/heads/master@{#443943}
Committed: https://chromium.googlesource.com/chromium/src/+/38ec90922bb8f6e9c2b6aaacdc27c13ae6dde8f2
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix #Patch Set 3 : test fix #
Total comments: 1
Patch Set 4 : add todo back #Patch Set 5 : add test #
Total comments: 4
Patch Set 6 : bokan comments addressed #
Total comments: 1
Patch Set 7 : return current layer when it is main thread scrolling #Messages
Total messages: 38 (20 generated)
Description was changed from ========== Remove main thread scroll checking in LayerTreeHostImpl::MouseMoveAt In LayerTreeHostImpl::MouseMoveAt, we find the scroll layer only for look up scrollbar animation controller so we need to know is it main thread scrolling. In the patch we add a method LayerTreeHostImpl::FindScrollLayerByLayer to find the scroll layer. BUG=679804 ========== to ========== Remove main thread scroll checking in LayerTreeHostImpl::MouseMoveAt In LayerTreeHostImpl::MouseMoveAt, we find the scroll layer only for look up scrollbar animation controller so we need to know is it main thread scrolling. In the patch we add a method LayerTreeHostImpl::FindScrollLayerByLayer to find the scroll layer. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Remove main thread scroll checking in LayerTreeHostImpl::MouseMoveAt In LayerTreeHostImpl::MouseMoveAt, we find the scroll layer only for look up scrollbar animation controller so we need to know is it main thread scrolling. In the patch we add a method LayerTreeHostImpl::FindScrollLayerByLayer to find the scroll layer. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Remove main thread scroll checker in LayerTreeHostImpl::MouseMoveAt In LayerTreeHostImpl::MouseMoveAt, we find the scroll layer only for look up scrollbar animation controller so we need to know is it main thread scrolling. In the patch we add a method LayerTreeHostImpl::FindScrollLayerByLayer to find the scroll layer. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
PTAL. Maybe we should also change the name of FindScrollLayerForDeviceViewportPoint and FindScrollLayerByLayer.
https://codereview.chromium.org/2621133002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2621133002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2578: LayerImpl* LayerTreeHostImpl::FindScrollLayerByLayer( This method avoids the walk up the parent layers looking for a scrollable one. i.e. if the hit layer isn't itself scrollable this wont work. Instead, I think you should modify FindScrollLayerForDeviceViewportPoint to return a layer even if it's main thread scrolling. Since it returns whether the layer scrolls on main, you can modify the other call sites that expect NULL to check scrolls_on_main_thread.
The CQ bit was checked by chaopeng@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by chaopeng@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.
PTAL. Thank you.
https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (left): https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3247: // scrolling on main thread, as well. What extra checking is this TODO referring to?
chaopeng@chromium.org changed reviewers: + sahel@chromium.org
On 2017/01/13 16:19:17, bokan wrote: > https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (left): > > https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:3247: // scrolling on main thread, as well. > What extra checking is this TODO referring to? I think this TODO means make FindScrollLayerForDeviceViewportPoint can return a scroll layer on main threading. sahel@ PTAL. Thank you.
On 2017/01/13 16:33:32, chaopeng wrote: > On 2017/01/13 16:19:17, bokan wrote: > > > https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... > > File cc/trees/layer_tree_host_impl.cc (left): > > > > > https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... > > cc/trees/layer_tree_host_impl.cc:3247: // scrolling on main thread, as well. > > What extra checking is this TODO referring to? > > I think this TODO means make FindScrollLayerForDeviceViewportPoint can return a > scroll layer on main threading. > sahel@ PTAL. Thank you. My read of it is that some of the code below the TODO is unnecessary once FindScrollLayerForDeviceViewportPoint finds the main thread layer as well.
Description was changed from ========== Remove main thread scroll checker in LayerTreeHostImpl::MouseMoveAt In LayerTreeHostImpl::MouseMoveAt, we find the scroll layer only for look up scrollbar animation controller so we need to know is it main thread scrolling. In the patch we add a method LayerTreeHostImpl::FindScrollLayerByLayer to find the scroll layer. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Return main thread scroll layer FindScrollLayerForDeviceViewportPoint In FindScrollLayerForDeviceViewportPoint, we return null for main thread scroll layer. But some calls only use the layer to look up animation controller and that null return will make it wrong. In the patch we return a layer without checking it is scroll in main thread. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/01/13 16:34:33, bokan wrote: > On 2017/01/13 16:33:32, chaopeng wrote: > > On 2017/01/13 16:19:17, bokan wrote: > > > > > > https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... > > > File cc/trees/layer_tree_host_impl.cc (left): > > > > > > > > > https://codereview.chromium.org/2621133002/diff/40001/cc/trees/layer_tree_hos... > > > cc/trees/layer_tree_host_impl.cc:3247: // scrolling on main thread, as well. > > > What extra checking is this TODO referring to? > > > > I think this TODO means make FindScrollLayerForDeviceViewportPoint can return > a > > scroll layer on main threading. > > sahel@ PTAL. Thank you. > > My read of it is that some of the code below the TODO is unnecessary once > FindScrollLayerForDeviceViewportPoint finds the main thread layer as well. sahel@ said the todo means FindScrollLayerForDeviceViewportPoint can return scroll layer when mouse over scrollbar. I added it back.
Please update the TODO to make that clearer. Also, please add a test that checks that MouseMoveAt over a scroll layer that has a main thread reason correctly calls DidMouseMoveNear on the ScrollbarAnimationController.
Patchset #5 (id:80001) has been deleted
Added test and updated TODO comments, PTAL. https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:11654: root_scroll->set_main_thread_scrolling_reasons( Can child layer be not main thread scroll if the parent has main_thread_scrolling_reasons?
lgtm % comments https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3245: // scrolling on main thread when mouse over scrollbar as well. nit: when mouse *is* over scrollbar. https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:11626: void LayerTreeHostImplTest::SetupMouseMoveAtTestScrollbarStates( IMO, TestScrollbarStates would be a better name https://codereview.chromium.org/2621133002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:11654: root_scroll->set_main_thread_scrolling_reasons( On 2017/01/13 19:15:28, chaopeng wrote: > Can child layer be not main thread scroll if the parent has > main_thread_scrolling_reasons? Yes, they're independent so a child that scrolls on impl can have a parent that scrolls on main. A child that scrolls on main can have a parent that scrolls on impl. That said, in this case I'd also make the child main thread scroll.
chaopeng@chromium.org changed reviewers: + weiliangc@chromium.org
weiliangc@chromium.org: Please review changes
https://codereview.chromium.org/2621133002/diff/120001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2621133002/diff/120001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:2539: return potentially_scrolling_layer_impl Why not return just active_tree->LayerById(scroll_node->owning_layer_id) here? It seems like when scroll_node is scrolling on main, returning potentially_scrolling_layer_impl would return a layer scrolling on impl, is this expected?
On 2017/01/16 18:31:15, weiliangc wrote: > https://codereview.chromium.org/2621133002/diff/120001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2621133002/diff/120001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_impl.cc:2539: return potentially_scrolling_layer_impl > Why not return just active_tree->LayerById(scroll_node->owning_layer_id) here? > > It seems like when scroll_node is scrolling on main, returning > potentially_scrolling_layer_impl would return a layer scrolling on impl, is this > expected? Yes, we should just return the main thread layer. Updated. PTAL. Thank you.
LGTM
Also, please wrap the description at 80 columns
Description was changed from ========== Return main thread scroll layer FindScrollLayerForDeviceViewportPoint In FindScrollLayerForDeviceViewportPoint, we return null for main thread scroll layer. But some calls only use the layer to look up animation controller and that null return will make it wrong. In the patch we return a layer without checking it is scroll in main thread. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Return main thread scroll layer FindScrollLayerForDeviceViewportPoint In FindScrollLayerForDeviceViewportPoint, we return null for main thread scroll layer. But some calls only use the layer to look up animation controller and that null return will make it wrong. In the patch we return a layer without checking it is scroll in main thread. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/01/16 19:35:56, bokan wrote: > Also, please wrap the description at 80 columns LGTM As we discussed with Chao, the current TODO is accurate.
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2621133002/#ps140001 (title: "return current layer when it is main thread scrolling")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484597982367090,
"parent_rev": "8fa239362956ce98ce4bd9f3b540050231a33179", "commit_rev":
"38ec90922bb8f6e9c2b6aaacdc27c13ae6dde8f2"}
Message was sent while issue was closed.
Description was changed from ========== Return main thread scroll layer FindScrollLayerForDeviceViewportPoint In FindScrollLayerForDeviceViewportPoint, we return null for main thread scroll layer. But some calls only use the layer to look up animation controller and that null return will make it wrong. In the patch we return a layer without checking it is scroll in main thread. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Return main thread scroll layer FindScrollLayerForDeviceViewportPoint In FindScrollLayerForDeviceViewportPoint, we return null for main thread scroll layer. But some calls only use the layer to look up animation controller and that null return will make it wrong. In the patch we return a layer without checking it is scroll in main thread. BUG=679804 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2621133002 Cr-Commit-Position: refs/heads/master@{#443943} Committed: https://chromium.googlesource.com/chromium/src/+/38ec90922bb8f6e9c2b6aaacdc27... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/38ec90922bb8f6e9c2b6aaacdc27... |
