Scroll with Layers in views::ScrollView

Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e
Cr-Commit-Position: refs/heads/master@{#411889}

[tapted, 2016-07-28 05:32:26]

Description was changed from

==========
Scroll with Layers in views::ScrollView

BUG=615948, 594907, 605131
==========

to

==========
Scroll with Layers in views::ScrollView

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[tapted, 2016-07-28 06:34:37]

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

[commit-bot: I haz the power, 2016-07-28 06:34:56]

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, 2016-07-28 06:46:19]

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

[commit-bot: I haz the power, 2016-07-28 06:46:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[tapted, 2016-07-28 07:22:37]

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

[commit-bot: I haz the power, 2016-07-28 07:22:47]

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, 2016-07-28 07:24:51]

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

[commit-bot: I haz the power, 2016-07-28 07:24:52]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tapted, 2016-07-29 03:18:05]

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

[commit-bot: I haz the power, 2016-07-29 03:18:28]

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, 2016-07-29 03:59:43]

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

[commit-bot: I haz the power, 2016-07-29 03:59:44]

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_...)

[tapted, 2016-07-29 06:42:29]

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

[commit-bot: I haz the power, 2016-07-29 06:42:44]

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, 2016-07-29 07:30:26]

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

[commit-bot: I haz the power, 2016-07-29 07:30:26]

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_...)

[tapted, 2016-07-29 07:38:44]

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

[commit-bot: I haz the power, 2016-07-29 07:39: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, 2016-07-29 08:26:02]

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

[commit-bot: I haz the power, 2016-07-29 08:26:03]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-08-02 06:34:47]

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

[commit-bot: I haz the power, 2016-08-02 06:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-08-02 06:40:42]

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

[commit-bot: I haz the power, 2016-08-02 06:40:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-08-02 06:45:30]

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

[commit-bot: I haz the power, 2016-08-02 06:45:44]

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, 2016-08-02 07:32:33]

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

[commit-bot: I haz the power, 2016-08-02 07:32:34]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-08-02 08:18:00]

Patchset #8 (id:140001) has been deleted

[tapted, 2016-08-02 08:18:11]

Patchset #7 (id:120001) has been deleted

[tapted, 2016-08-02 08:18:21]

Patchset #3 (id:40001) has been deleted

[tapted, 2016-08-02 12:28:08]

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

[commit-bot: I haz the power, 2016-08-02 12:28:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-08-02 12:47:32]

Description was changed from

==========
Scroll with Layers in views::ScrollView

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView is given a layer-backed "contents container" layer
which is clipped to the existing ScrollView viewport. Tests and classes
such as TreeView assumed a particular view hierarchy in ScrollView, so
this is made explicit.

View::EnclosingScrollView is added to reliably obtain the scroll view
that may contain a view. This is inspired by -[NSView
enclosingScrollView] from AppKit.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[tapted, 2016-08-02 12:47:33]

tapted@chromium.org changed reviewers:
+ sky@chromium.org, vollick@chromium.org

[tapted, 2016-08-02 13:14:35]

Hi sky and ian, please take a look. (cc danakj in case you're peeking at email
during the hackathon :).

This seems like a good initial step, split out from the "Big Picture" CL in
http://crrev.com/2189583004 which gets elastic scrolling all going for a
views::ScrollView - but there's lots of stuff there. It links to the follow-ups
for things like adding phase information on the ui::Events and my idea for
including overscroll as a cc:Layer property (currently it's only a property of
the LayerTree root viewport).

There's a doc here with a bit more background:
https://docs.google.com/document/d/1v5CRDQEtOB302djv94StPPPOYXbLNkhQXu6X97AuM...

Thanks!

[commit-bot: I haz the power, 2016-08-02 13:37:52]

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

[commit-bot: I haz the power, 2016-08-02 13:37:53]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-08-04 07:21:19]

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

[commit-bot: I haz the power, 2016-08-04 07:21:29]

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, 2016-08-04 08:17:59]

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

[commit-bot: I haz the power, 2016-08-04 08:18:00]

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...)

[tapted, 2016-08-04 13:19:02]

vollick: ping? If you get a chance.. I guess all the src/cc and ui/compositor
reviewers are pretty busy this week :/

https://codereview.chromium.org/2188133002/diff/200001/ui/views/controls/scro...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2188133002/diff/200001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:226: DCHECK(!a_view->layer());
added a dcheck here per the comments in the overview CL - crrev/2189583004 . Of
course, client code could just create a layer after calling SetContents, so I'm
not sure how useful it is..

I also did an audit with combinations of git grep and codesearch -- I didn't
find anything currently using a layer-backed contents. The closest was
message_center_view.cc setting the ScrollView itself to have a layer, but not
the contents -
https://cs.chromium.org/chromium/src/ui/message_center/views/message_center_v...
(this went in for r153049 - I think so it can be swiped off the screen in ash
smoothly)

[sky, 2016-08-04 18:58:39]

Were you able to make this work without contents_container_ as talked about in
the other patch?

[tapted, 2016-08-05 03:49:40]

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

[commit-bot: I haz the power, 2016-08-05 03:49:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-08-05 04:21:47]

Description was changed from

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView is given a layer-backed "contents container" layer
which is clipped to the existing ScrollView viewport. Tests and classes
such as TreeView assumed a particular view hierarchy in ScrollView, so
this is made explicit.

View::EnclosingScrollView is added to reliably obtain the scroll view
that may contain a view. This is inspired by -[NSView
enclosingScrollView] from AppKit.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[tapted, 2016-08-05 04:22:34]

Patchset #9 (id:220001) has been deleted

[tapted, 2016-08-05 04:24:40]

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

[commit-bot: I haz the power, 2016-08-05 04:24:59]

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, 2016-08-05 05:22:55]

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

[commit-bot: I haz the power, 2016-08-05 05:22:56]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-08-05 06:13:53]

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

[commit-bot: I haz the power, 2016-08-05 06:14:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-08-05 07:12:31]

On 2016/08/04 18:58:39, sky wrote:
> Were you able to make this work without contents_container_ as talked about in
> the other patch?

I've Done this now since PatchSet 9. It works.. and things like
TreeView/TableView seem happy. But I did have to tweak a couple of
ScrollViewTest test cases.

Also it does "break" views_examples -- e.g. the "SmallSquare" case in the
ScrollView example now fills the viewport when it's added rather than tucking up
into the top left corner. Also, resizing the window to be large, then small
again doesn't re-layout the view, so it accumulates scrollers when being made
smaller.

Also spotted a couple of things that would have regressed (see below). Fixed,
but they're not related to the removal of contents_container_

https://codereview.chromium.org/2188133002/diff/200001/ui/views/controls/scro...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2188133002/diff/200001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:195: // TODO(tapted): Support setting the
background to something other than white,
Without this TODO - the ExtensionInstall dialog would get a white scrollview on
a grey background :/. But it's a small change to include it (done) - see what
you think. We're close to launching a dev-channel experiment
(http://crbug.com/633436), so I shouldn't regress things..

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
File ui/views/controls/scroll_view_unittest.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view_unittest.cc:570: EXPECT_EQ(gfx::Size(kWidth,
kMinHeight), scroll_view.contents()->size());
Technically.... this violates StaticSizedView::GetMinimumSize(), but I don't
think there's a use-case in Chrome that isn't satisfied.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/views/view.cc#newco...
ui/views/view.cc:449: gfx::Transform View::GetTransform() const {
This is new since patchset 8 too. Without it events are incorrectly offset when
the view has scrolled. Included test coverage.

[commit-bot: I haz the power, 2016-08-05 07:22:24]

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

[commit-bot: I haz the power, 2016-08-05 07:22:25]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-05 15:51:42]

Nice, LGTM

[sky, 2016-08-05 15:51:59]

I only looked at views. I'm assuming Ian is looking at the rest.

[Ian Vollick, 2016-08-09 14:00:44]

vollick@chromium.org changed reviewers:
+ ymalik@chromium.org

[Ian Vollick, 2016-08-09 14:00:47]

vollick@chromium.org changed reviewers:
- ymalik@chromium.org

[Ian Vollick, 2016-08-09 14:00:49]

https://codereview.chromium.org/2188133002/diff/280001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:583: return false;  // TODO(tapted): Should this
return true?
I think this should only return true if we're asynchronously scrolling; seems
correct for this to return false.

I hadn't seen this before and it looks like it's only used to detect when we'll
do impl-thread scroll animations. +ymalik in case I've got this wrong.

https://codereview.chromium.org/2188133002/diff/280001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/compositor/layer.cc...
ui/compositor/layer.cc:761:
cc_layer_->SetScrollParent(parent_clip_layer->cc_layer_);
This should not be necessary. The concept of "scroll parent" is very weird and
should only be used for web content.

https://codereview.chromium.org/2188133002/diff/280001/ui/compositor/layer.cc...
ui/compositor/layer.cc:762:
cc_layer_->SetIsContainerForFixedPositionLayers(true);
Do we used fixed pos layers in ui::?

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:29: base::FEATURE_DISABLED_BY_DEFAULT
I'm sure this is a dumb question, but why just mac?

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:82: const bool scrolls_with_layers =
viewport->layer() != nullptr;
Should this use the ScrollsWithLayers helper function you added? (Which,
presumably, would just wrap up the same check).

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:93: int y =
CheckScrollBounds(viewport->height(), view->height(), offset.y());
Do you need a test where the scroll bounds are updated?

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:99: view->SetBounds(-x, -y, view->width(),
view->height());
nit/bikeshed/ignorable: I realize that this was not introduced by your patch,
but it seems strange that function called CheckScrollBounds would modify the
bounds. SanitizeScrollBounds, FixScrollBounds, or UpdateScrollBounds maybe?

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:210:
Background::CreateSolidBackground(background_color_));
I presume that the reason you've duplicated a bit of code that's in
::SetBackgroundColor is that you only want to affect the content layer here?

[tapted, 2016-08-10 03:33:25]

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

[commit-bot: I haz the power, 2016-08-10 03:33:43]

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, 2016-08-10 05:29:22]

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

[commit-bot: I haz the power, 2016-08-10 05:29:24]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-08-10 05:39:04]

https://codereview.chromium.org/2188133002/diff/280001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:583: return false;  // TODO(tapted): Should this
return true?
On 2016/08/09 14:00:49, vollick wrote:
> I think this should only return true if we're asynchronously scrolling; seems
> correct for this to return false.
> 
> I hadn't seen this before and it looks like it's only used to detect when
we'll
> do impl-thread scroll animations. +ymalik in case I've got this wrong.

Done.

https://codereview.chromium.org/2188133002/diff/280001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/compositor/layer.cc...
ui/compositor/layer.cc:761:
cc_layer_->SetScrollParent(parent_clip_layer->cc_layer_);
On 2016/08/09 14:00:49, vollick wrote:
> This should not be necessary. The concept of "scroll parent" is very weird and
> should only be used for web content.

Done.

https://codereview.chromium.org/2188133002/diff/280001/ui/compositor/layer.cc...
ui/compositor/layer.cc:762:
cc_layer_->SetIsContainerForFixedPositionLayers(true);
On 2016/08/09 14:00:49, vollick wrote:
> Do we used fixed pos layers in ui::?

Ah, I must have added this in a very early iteration that tried to swap the
scrolling layer with the layer tree host's inner_viewport_scroll_layer_ on each
scroll event. There's a DCHECK in LayerTreeHost::FinishCommitOnImplThread that
would fail. But I don't think I need it now - this doesn't seem to get used on
the impl side. Removed.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:29: base::FEATURE_DISABLED_BY_DEFAULT
On 2016/08/09 14:00:49, vollick wrote:
> I'm sure this is a dumb question, but why just mac?

Mac needs it for elasticity, but (I think!) the plan is to roll it out
everywhere once any kinks have been sorted out on Mac.

So with the approach here, it will still be behind a flag on non-mac, and e.g.,
ChromeOS uses a lot of transparent UI, so it can be scrutinized this way.
(Whereas on Mac, UI using toolkit-views is only just rolling out, so is already
under a lot of scrutiny).

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:82: const bool scrolls_with_layers =
viewport->layer() != nullptr;
On 2016/08/09 14:00:49, vollick wrote:
> Should this use the ScrollsWithLayers helper function you added? (Which,
> presumably, would just wrap up the same check).

Added a comment - for now, the header row is still scrolling without layers (it
doesn't need elasticity and has less to repaint), so it's not a direct
correspondence.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:93: int y =
CheckScrollBounds(viewport->height(), view->height(), offset.y());
On 2016/08/09 14:00:49, vollick wrote:
> Do you need a test where the scroll bounds are updated?

Ooh, good catch

  DCHECK_EQ(offset.x(), x);
  DCHECK_EQ(offset.y(), y);

doesn't fail views_unittests here - doh. Added a test to cover this.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:99: view->SetBounds(-x, -y, view->width(),
view->height());
On 2016/08/09 14:00:49, vollick wrote:
> nit/bikeshed/ignorable: I realize that this was not introduced by your patch,
> but it seems strange that function called CheckScrollBounds would modify the
> bounds. SanitizeScrollBounds, FixScrollBounds, or UpdateScrollBounds maybe?

I went with ConstrainScrollToBounds - I think that's a good fit.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:210:
Background::CreateSolidBackground(background_color_));
On 2016/08/09 14:00:49, vollick wrote:
> I presume that the reason you've duplicated a bit of code that's in
> ::SetBackgroundColor is that you only want to affect the content layer here?

That's also true, but the reason I had in mind is that we should check for the
presence of a Background* being already set on |a_view|, and not clobber that
background here (i.e. it would only get clobbered if the client code also calls
SetBackgroundColor() with something other than transparent). However,
SetBackgroundColor() can't have that same check since there will "always" be a
Background* after calling SetContents(). Hence, I couldn't just call
SetBackgroundColor here.

[tapted, 2016-08-11 20:58:37]

vollick: ping?

[Ian Vollick, 2016-08-12 13:57:42]

lgtm with nits.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:29: base::FEATURE_DISABLED_BY_DEFAULT
On 2016/08/10 at 05:39:04, tapted (OOO soon) wrote:
> On 2016/08/09 14:00:49, vollick wrote:
> > I'm sure this is a dumb question, but why just mac?
> 
> Mac needs it for elasticity, but (I think!) the plan is to roll it out
everywhere once any kinks have been sorted out on Mac.
> 
> So with the approach here, it will still be behind a flag on non-mac, and
e.g., ChromeOS uses a lot of transparent UI, so it can be scrutinized this way.
(Whereas on Mac, UI using toolkit-views is only just rolling out, so is already
under a lot of scrutiny).

Ah, makes sense. Thanks for the explanation.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:99: view->SetBounds(-x, -y, view->width(),
view->height());
On 2016/08/10 at 05:39:04, tapted (OOO soon) wrote:
> On 2016/08/09 14:00:49, vollick wrote:
> > nit/bikeshed/ignorable: I realize that this was not introduced by your
patch,
> > but it seems strange that function called CheckScrollBounds would modify the
> > bounds. SanitizeScrollBounds, FixScrollBounds, or UpdateScrollBounds maybe?
> 
> I went with ConstrainScrollToBounds - I think that's a good fit.

I like it.

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:657: // and commits a frame, which isn't true
in some tests.
Please create a bug for this and mention it in the comment.

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:259: // touched on the impl layer tree.
I presume that scroll to offset can now cause scroll deltas to be collected by
the LTHI making the first DCHECK fail, but we shouldn't be pinch-zooming, so the
page scale delta should still be 1.f, right?

If that's true, can you reinstate this code for the 2nd DCHECK, and add a
comment as to why we might get scroll deltas in the single thread proxy (since
it's counterintuitive).

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:798:
layer_tree_host_->ApplyScrollAndScale(scroll_info.get());
Again, please add a comment as this is counterintuitive.

[tapted, 2016-08-13 07:15:38]

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

[commit-bot: I haz the power, 2016-08-13 07:15:52]

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, 2016-08-13 08:09:10]

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

[commit-bot: I haz the power, 2016-08-13 08:09:11]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-08-13 09:03:53]

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
File ui/views/controls/scroll_view.cc (right):

https://codereview.chromium.org/2188133002/diff/280001/ui/views/controls/scro...
ui/views/controls/scroll_view.cc:657: // and commits a frame, which isn't true
in some tests.
On 2016/08/12 13:57:42, vollick wrote:
> Please create a bug for this and mention it in the comment.

Done.

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:259: // touched on the impl layer tree.
On 2016/08/12 13:57:42, vollick wrote:
> I presume that scroll to offset can now cause scroll deltas to be collected by
> the LTHI making the first DCHECK fail, but we shouldn't be pinch-zooming, so
the
> page scale delta should still be 1.f, right?
> 
> If that's true, can you reinstate this code for the 2nd DCHECK, and add a
> comment as to why we might get scroll deltas in the single thread proxy (since
> it's counterintuitive).

Done (reinstated in DoBeginMainFrame() since the ProcessScrollDeltas() is needed
for the DCHECK).

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2188133002/diff/300001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:798:
layer_tree_host_->ApplyScrollAndScale(scroll_info.get());
On 2016/08/12 13:57:42, vollick wrote:
> Again, please add a comment as this is counterintuitive.

Done.

[tapted, 2016-08-13 09:05:57]

The CQ bit was checked by tapted@chromium.org

[tapted, 2016-08-13 09:05:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/2188133002/#ps340001
(title: "bugref, scale_delta DCHECK + comment")

[commit-bot: I haz the power, 2016-08-13 09:06:09]

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, 2016-08-13 09:09:52]

Description was changed from

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-13 09:09:53]

Committed patchset #14 (id:340001)

[commit-bot: I haz the power, 2016-08-13 09:11:22]

Description was changed from

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Scroll with Layers in views::ScrollView

Initially, do this only on Mac, or with
--enable-features=ToolkitViewsScrollWithLayers.

This is a first step for bringing up elastic scrolling of a
views::ScrollView on Mac. Still, by itself, it results in significant
performance improvements for scrolling in toolkit-views and allows the
UI thread to keep pace with the high rate of touchpad scroll events
produced by Cocoa on Mac.

To take advantage of existing scroll handling for smooth-scrolling and
elasticity, later CLs will route scroll events through the
cc::InputHandler (i.e. LayerTreeHostImpl). Since the ui::Compositor is
single-threaded on the UI thread, there is no distinct impl side. For a
consistent event flow, perform all scrolling on the impl side. This
requires plumbing through a way for a views::ScrollView to set the
scroll offset on the impl side directly, in response to an input event.

views::ScrollView now makes its contents layer-backed and clips
it to the layer added to the existing ScrollView viewport.

BUG=615948, 594907, 605131
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e
Cr-Commit-Position: refs/heads/master@{#411889}
==========

[commit-bot: I haz the power, 2016-08-13 09:11:24]

Patchset 14 (id:??) landed as
https://crrev.com/4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e
Cr-Commit-Position: refs/heads/master@{#411889}