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

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

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

Additionally, ensure that show-from-main requests are processed in synchronous
compositing mode (which commits directly to the active tree).

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

Review-Url: https://codereview.chromium.org/2842553003
Cr-Commit-Position: refs/heads/master@{#467855}
Committed: https://chromium.googlesource.com/chromium/src/+/697a467f819ce09da5209a3df13b8b92f33e35a4

[skobes, 2017-04-25 02:32:44]

Description was changed from

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

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

BUG=606395
==========

to

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

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

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

[skobes, 2017-04-25 02:32:55]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 02:33:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2017-04-25 02:44:02]

Description was changed from

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

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

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

to

==========
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
==========

[commit-bot: I haz the power, 2017-04-25 03:41:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 03:41:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[skobes, 2017-04-25 18:42:46]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 18:43:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2017-04-25 20:35:46]

skobes@chromium.org changed reviewers:
+ aelias@chromium.org, bokan@chromium.org

[skobes, 2017-04-25 20:36:43]

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.

[commit-bot: I haz the power, 2017-04-25 20:39:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 20:39:57]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-04-25 22:56:48]

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

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_hos...
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_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_hos...
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_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_imp...
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/Layo...
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/Layo...
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?

[skobes, 2017-04-27 00:40:50]

Patchset #3 (id:40001) has been deleted

[skobes, 2017-04-27 00:45:30]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 00:46:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2017-04-27 00:46:31]

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

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:352: active_tree_->ShowScrollbars();
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_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_hos...
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_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_imp...
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/Layo...
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/Layo...
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?

Done.

[skobes, 2017-04-27 02:03:54]

Description was changed from

==========
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
==========

to

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

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

Additionally, ensure that show-from-main requests are processed in synchronous
compositing mode (which commits directly to the active tree).

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

[commit-bot: I haz the power, 2017-04-27 02:30:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 02:30:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[skobes, 2017-04-27 18:28:37]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 18:29:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 19:00:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 19:00:02]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[bokan, 2017-04-27 19:38:58]

Thanks, lgtm

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

https://codereview.chromium.org/2842553003/diff/20001/cc/trees/layer_tree_hos...
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.

[skobes, 2017-04-27 20:01:52]

The CQ bit was checked by skobes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 20:03:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 20:26:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 20:26:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[skobes, 2017-04-27 21:46:38]

Thanks!  Waiting on aelias for cc/OWNERS.

[aelias_OOO_until_Jul13, 2017-04-27 22:28:04]

lgtm

[skobes, 2017-04-28 01:37:51]

The CQ bit was checked by skobes@chromium.org

[commit-bot: I haz the power, 2017-04-28 01:39:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 02:54:21]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493343471141550,
"parent_rev": "c538713fa88d7fb0a831fbaa3b7dedafb9b5cdbf", "commit_rev":
"697a467f819ce09da5209a3df13b8b92f33e35a4"}

[commit-bot: I haz the power, 2017-04-28 02:54:30]

Description was changed from

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

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

Additionally, ensure that show-from-main requests are processed in synchronous
compositing mode (which commits directly to the active tree).

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

to

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

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

Additionally, ensure that show-from-main requests are processed in synchronous
compositing mode (which commits directly to the active tree).

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

Review-Url: https://codereview.chromium.org/2842553003
Cr-Commit-Position: refs/heads/master@{#467855}
Committed:
https://chromium.googlesource.com/chromium/src/+/697a467f819ce09da5209a3df13b...
==========

[commit-bot: I haz the power, 2017-04-28 02:54:32]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/697a467f819ce09da5209a3df13b...