New issue
Advanced search Search tips

Issue 918699 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocking:
issue 836890



Sign in to add a comment

ScrollbarLayerTest.ShouldScrollNonOverlayOnMainThread fails with use_layer_lists

Project Member Reported by pdr@chromium.org, Jan 2

Issue description

ScrollbarLayerTest.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?
 
Cc: bokan@chromium.org
Status: Assigned (was: Untriaged)
Just FYI, the reason scrollbars "scroll" on main thread was so that we could grab and drag the thumb with touch. 
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?
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.
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?
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment