Call SAC::DidScrollUpdate only for compositor-triggered scrolls. (issue 2842553003 by skobes@chromium.org)

1 view
Skip to first unread message

sko...@chromium.org

unread,
Apr 25, 2017, 4:35:47 PM4/25/17
to bo...@chromium.org, ael...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org
Reviewers: bokan, aelias
CL: https://codereview.chromium.org/2842553003/

Description:
Call SAC::DidScrollUpdate only for compositor-triggered scrolls.

For everything else we should rely on SAC::DidRequestShowFromMainThread.

Additionally, ensure that LayerTreeImpl::ShowScrollbars is called in synchronous
compositing mode (which commits directly to the active tree).

BUG=606395
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel


Affected files (+65, -114 lines):
M cc/layers/scrollbar_layer_unittest.cc
M cc/trees/layer_tree_host_impl.h
M cc/trees/layer_tree_host_impl.cc
M cc/trees/layer_tree_host_impl_unittest.cc
M cc/trees/layer_tree_impl.h
M cc/trees/layer_tree_impl.cc
M third_party/WebKit/LayoutTests/paint/invalidation/border-radius-repaint-2.html
M third_party/WebKit/LayoutTests/paint/invalidation/border-radius-repaint-2-expected.png
M third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html
M third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change2-expected.html


sko...@chromium.org

unread,
Apr 25, 2017, 4:36:44 PM4/25/17
to bo...@chromium.org, ael...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org
Note: on the review thread of http://crrev.com/2770293003 I said that layout
tests don't create ScrollbarAnimationController. That was wrong - they do, when
pixel dumps are requested. But bokan's idea of calling LTI::ShowScrollbars in
ActivateSyncTree didn't work, because it needs to happen before
UpdateDrawProperties, which is in
LTHI::UpdateSyncTreeAfterCommitOrImplSideInvalidation. So I am calling
ShowScrollbars there.

https://codereview.chromium.org/2842553003/

bo...@chromium.org

unread,
Apr 25, 2017, 6:56:49 PM4/25/17
to sko...@chromium.org, ael...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode352
cc/trees/layer_tree_host_impl.cc:352: active_tree_->ShowScrollbars();
I was a little confused until I checked the ShowScrollbars
implementation. Could we rename it to something like
ShowScrollbarsIfRequestedFromMain? Something to clarify that it's not
just forcing display of scrollbars.

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode12218
cc/trees/layer_tree_host_impl_unittest.cc:12218:
scrollbar_1_animation_controller->DidScrollUpdate();
Do you know why this wasn't previously needed?

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_impl.cc#newcode941
cc/trees/layer_tree_impl.cc:941: if (IsActiveTree() &&
OuterViewportScrollLayer()) {
Use LTHI::ViewportMainScrollLayer rather than OuterViewportScrollLayer
(it's an alias for the outer layer but makes clear where we reference
the viewport).

https://codereview.chromium.org/2842553003/diff/20001/third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html
File
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html
(right):

https://codereview.chromium.org/2842553003/diff/20001/third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html#newcode5
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html:5:
internals.settings.setMockScrollbarsEnabled(true);
Why are these window resize tests using overlay scrollbars? Turning them
on like this is kind of half baked since there's a mismatch between what
Blink this the status is and other parts of Chrome: crbug.com/661783

If it's just to prevent layout changes due to scrollbars appearing,
could we use overflow: hidden here too?

https://codereview.chromium.org/2842553003/

sko...@chromium.org

unread,
Apr 26, 2017, 8:46:32 PM4/26/17
to bo...@chromium.org, ael...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org
On 2017/04/25 22:56:48, bokan wrote:
> I was a little confused until I checked the ShowScrollbars
implementation. Could
> we rename it to something like ShowScrollbarsIfRequestedFromMain?
Something to
> clarify that it's not just forcing display of scrollbars.

Renamed to HandleScrollbarShowRequestsFromMain.


https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode12218
cc/trees/layer_tree_host_impl_unittest.cc:12218:
scrollbar_1_animation_controller->DidScrollUpdate();
On 2017/04/25 22:56:48, bokan wrote:
> Do you know why this wasn't previously needed?

The test was counting on the scrollbar to be made initially visible
through LayerTreeImpl::RegisterScrollbar during the call to
scrollbar_1->SetScrollElementId.

RegisterScrollbar no longer triggers SAC::DidScrollUpdate, so we have to
call it manually here.

(Note that we still want scrollbars to be initially visible, but we
can't enforce this through RegisterScrollbar anymore, since it may occur
before the scroll bounds are pushed, in which case SAC::DidScrollUpdate
would have no effect due to SAC::ApplyOpacityToScrollbars setting
effective_opacity to 0. Instead we rely on showing from main when the
first layout calls PLSA::VisibleSizeChanged.)


https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_impl.cc#newcode941
cc/trees/layer_tree_impl.cc:941: if (IsActiveTree() &&
OuterViewportScrollLayer()) {
On 2017/04/25 22:56:48, bokan wrote:
> Use LTHI::ViewportMainScrollLayer rather than OuterViewportScrollLayer
(it's an
> alias for the outer layer but makes clear where we reference the
viewport).

Done.


https://codereview.chromium.org/2842553003/diff/20001/third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html
File
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html
(right):

https://codereview.chromium.org/2842553003/diff/20001/third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html#newcode5
third_party/WebKit/LayoutTests/paint/invalidation/window-resize-no-layout-change1-expected.html:5:
internals.settings.setMockScrollbarsEnabled(true);
On 2017/04/25 22:56:48, bokan wrote:
> Why are these window resize tests using overlay scrollbars? Turning
them on like
> this is kind of half baked since there's a mismatch between what Blink
this the
> status is and other parts of Chrome: crbug.com/661783
>
> If it's just to prevent layout changes due to scrollbars appearing,
could we use
> overflow: hidden here too?

bo...@chromium.org

unread,
Apr 27, 2017, 3:38:58 PM4/27/17
to sko...@chromium.org, ael...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org
Thanks, lgtm



https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_host_impl_unittest.cc#newcode12218
cc/trees/layer_tree_host_impl_unittest.cc:12218:
scrollbar_1_animation_controller->DidScrollUpdate();
On 2017/04/27 00:46:31, skobes wrote:
> On 2017/04/25 22:56:48, bokan wrote:
> > Do you know why this wasn't previously needed?
>
> The test was counting on the scrollbar to be made initially visible
through
> LayerTreeImpl::RegisterScrollbar during the call to
> scrollbar_1->SetScrollElementId.
>
> RegisterScrollbar no longer triggers SAC::DidScrollUpdate, so we have
to call it
> manually here.
>
> (Note that we still want scrollbars to be initially visible, but we
can't
> enforce this through RegisterScrollbar anymore, since it may occur
before the
> scroll bounds are pushed, in which case SAC::DidScrollUpdate would
have no
> effect due to SAC::ApplyOpacityToScrollbars setting effective_opacity
to 0.
> Instead we rely on showing from main when the first layout calls
> PLSA::VisibleSizeChanged.)

Got it, thanks for the explanation.

https://codereview.chromium.org/2842553003/

sko...@chromium.org

unread,
Apr 27, 2017, 5:46:39 PM4/27/17
to bo...@chromium.org, ael...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org
Thanks! Waiting on aelias for cc/OWNERS.

https://codereview.chromium.org/2842553003/

ael...@chromium.org

unread,
Apr 27, 2017, 6:28:04 PM4/27/17
to sko...@chromium.org, bo...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org, chromium...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Apr 27, 2017, 9:39:12 PM4/27/17
to sko...@chromium.org, ael...@chromium.org, bo...@chromium.org, commi...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org, chromium...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Apr 27, 2017, 10:54:32 PM4/27/17
to sko...@chromium.org, ael...@chromium.org, bo...@chromium.org, commi...@chromium.org, blink-...@chromium.org, cc-...@chromium.org, chao...@chromium.org, chromium...@chromium.org
Reply all
Reply to author
Forward
0 new messages