Hide pinch viewport scrollbars when near minimum scale.

Hide pinch viewport scrollbars when near minimum scale.

This patch prevents the overlay scrollbars from showing up when we're near the
minimum scale. The threshold can be set per-platform; by default it's 0 so the
scrollbars will be visible regardless of the scale. On Windows/Linux/CrOS we'd
like to prevent the scrollbars from overlaying over the window scrollbars so
we set a small non-0 threshold.

BUG=446411
Committed: https://crrev.com/c784a6f02ed8f2cffe131c2c067c4805cd752610
Cr-Commit-Position: refs/heads/master@{#313431}

[bokan, 2015-01-26 20:01:00]

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

[bokan, 2015-01-26 20:01:17]

Alexandre, ptal. Thanks.

[aelias_OOO_until_Jul13, 2015-01-26 20:13:10]

This logic doesn't seem to fit that well in the animation controller, as it's
orthogonal to the fade animation.  How about instead hiding the scrollbar via
some non-opacity property like hideLayerAndSubtree from LayerImpl?

Alternatively, have you considered reordering the scrollbar tree from Blink tree
such that the nonoverlay one is on top?

[bokan, 2015-01-26 20:36:39]

On 2015/01/26 20:13:10, aelias wrote:
> This logic doesn't seem to fit that well in the animation controller, as it's
> orthogonal to the fade animation.  How about instead hiding the scrollbar via
> some non-opacity property like hideLayerAndSubtree from LayerImpl?

I'll give that a look.

> 
> Alternatively, have you considered reordering the scrollbar tree from Blink
tree
> such that the nonoverlay one is on top?

My gut feeling is that would look weird; I'll give the above a try first.

[bokan, 2015-01-27 19:10:06]

Got the same effect using SetHideLayerAndSubtree, ptal.

[aelias_OOO_until_Jul13, 2015-01-27 19:35:05]

lgtm modulo comments below

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:417: min_page_scale_factor() +
settings().scrollbar_show_scale_threshold;
I generally prefer scales to be multiplied together rather than added, adding
can have strange nonlinear effects.  Please make this a multiplication.

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.cc:37: scrollbar_show_scale_threshold(0),
Default should be 1 if it's a multiplication.

https://codereview.chromium.org/877863002/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/877863002/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:408:
settings.scrollbar_show_scale_threshold = 0.05f;
This should be 1.05.

[aelias_OOO_until_Jul13, 2015-01-27 19:52:29]

Sorry, I just remembered an additional issue. 
ScrollbarLayerImplBase::PushPropertiesTo currently has a hack to preserve
impl-side opacity to avoid getting clobbered on commit.  That should be extended
to preserve hideLayerAndSubtree value to avoid an analogous problem with this.

[bokan, 2015-01-27 20:41:55]

On 2015/01/27 19:52:29, aelias wrote:
> Sorry, I just remembered an additional issue. 
> ScrollbarLayerImplBase::PushPropertiesTo currently has a hack to preserve
> impl-side opacity to avoid getting clobbered on commit.  That should be
extended
> to preserve hideLayerAndSubtree value to avoid an analogous problem with this.

Done.

[bokan, 2015-01-27 20:42:02]

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:417: min_page_scale_factor() +
settings().scrollbar_show_scale_threshold;
On 2015/01/27 19:35:05, aelias wrote:
> I generally prefer scales to be multiplied together rather than added, adding
> can have strange nonlinear effects.  Please make this a multiplication.

Done.

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/877863002/diff/40001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.cc:37: scrollbar_show_scale_threshold(0),
On 2015/01/27 19:35:05, aelias wrote:
> Default should be 1 if it's a multiplication.

Done.

https://codereview.chromium.org/877863002/diff/40001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/877863002/diff/40001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:408:
settings.scrollbar_show_scale_threshold = 0.05f;
On 2015/01/27 19:35:05, aelias wrote:
> This should be 1.05.

Done.

[aelias_OOO_until_Jul13, 2015-01-27 20:44:06]

https://codereview.chromium.org/877863002/diff/60001/cc/trees/layer_tree_impl...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/877863002/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl_unittest.cc:27: settings.scrollbar_show_scale_threshold
= 0.1f;
Look like this also should be updated to 1.1f.

[bokan, 2015-01-27 20:48:32]

https://codereview.chromium.org/877863002/diff/60001/cc/trees/layer_tree_impl...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/877863002/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl_unittest.cc:27: settings.scrollbar_show_scale_threshold
= 0.1f;
On 2015/01/27 20:44:06, aelias wrote:
> Look like this also should be updated to 1.1f.

Right, done.

[aelias_OOO_until_Jul13, 2015-01-27 20:51:40]

lgtm

[bokan, 2015-01-27 20:52:12]

bokan@chromium.org changed reviewers:
+ jbauman@chromium.org

[bokan, 2015-01-27 20:52:30]

+jbauman@ for RenderWidgetCompositor

[jbauman, 2015-01-28 00:26:03]

lgtm

[bokan, 2015-01-28 00:29:28]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2015-01-28 00:29:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877863002/80001

[commit-bot: I haz the power, 2015-01-28 04:12:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-28 04:14:23]

Patchset 5 (id:??) landed as
https://crrev.com/c784a6f02ed8f2cffe131c2c067c4805cd752610
Cr-Commit-Position: refs/heads/master@{#313431}