ScrollbarLayerTest.ShouldScrollNonOverlayOnMainThread fails with use_layer_lists |
||
Issue descriptionScrollbarLayerTest.ShouldScrollNonOverlayOnMainThread tests that PaintedScrollbarLayer calls AddMainThreadScrollingReasons(MainThreadScrollingReason::kScrollbarScrolling), but we do not have per-layer reasons with use_layer_lists (see: Layer::GetMainThreadScrollingReasons). The task in this bug is to find out why we don't need to set MainThreadScrollingReason::kScrollbarScrolling with BGPT. How do we force main-thread scrolling for painted scrollbar layers with BGPT?
,
Jan 3
We have a is_scrollbar bit set on cc::Layer and cc::LayerImpl. It's used in LayerTreeHostImpl::IsTouchDraggingScrollbar to force main-thread scrolling. Do we still need the main thread scrolling reasons for this?
,
Jan 3
Perhaps not? I wonder: without it, does the compositor know to send the scroll to the associated scroller? If so then I think we can just remove it.
,
Jan 3
I think I could figure this out if I could figure out how to write a testcase for it :P What would fail if this feature were broken? We need a case where the main-thread would alter the scroll (i.e., the compositor shouldn't handle it), but can't that only be done with things that we already only main-thread scroll such as event handlers?
,
Jan 3
Thumb dragging is only implemented only in Blink in Scrollbar::GestureEvent and listens for GestureScroll events (unfortunately). I think #2 was implemented not too long ago so the main thread scroll reason is probably unneeded now. As for test, if you setup a composited scroller and can touch-drag the thumb and also wheel scrolling over the scrollbar causes scrolling of the scroller - I think you're good to go.
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a82f222db5cd5a30501f6264e6d2ef0e9368a08a commit a82f222db5cd5a30501f6264e6d2ef0e9368a08a Author: Philip Rogers <pdr@chromium.org> Date: Fri Jan 04 23:44:34 2019 Do not set main thread scrolling reasons for painted scrollbar In [1], main thread scrolling reasons were moved from cc::Layer to cc::ScrollNode. That patch missed a case where painted scrollbar layers always set main thread scrolling reasons for non-overlay scrollbars. This is not possible to support with main thread reasons on ScrollNode because it would force main thread scrolling for the entire scroll layer. This functionality is not actually needed anymore because LayerTreeHostImpl::ScrollBegin now falls back to the main thread when touch-dragging a scrollbar. LayerTreeHostImplTestMultiScrollable::ScrollHitTestOnScrollbar tests the main codepath for falling back to the main thread when touch-dragging. I also manually checked that touch-scrolling on the scrollbar and thumb work as expected with this patch. [1] https://crrev.com/617642 Bug: 918699 Change-Id: If8c9cf528639266e49956c5513c2a04b23829622 Reviewed-on: https://chromium-review.googlesource.com/c/1396439 Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#620109} [modify] https://crrev.com/a82f222db5cd5a30501f6264e6d2ef0e9368a08a/cc/layers/painted_scrollbar_layer.cc [modify] https://crrev.com/a82f222db5cd5a30501f6264e6d2ef0e9368a08a/cc/layers/scrollbar_layer_unittest.cc
,
Jan 9
|
||
►
Sign in to add a comment |
||
Comment 1 by bokan@chromium.org
, Jan 3Status: Assigned (was: Untriaged)