cc: Make PaintedScrollbarLayer not use ContentsScalingLayer

cc: Make PaintedScrollbarLayer not use ContentsScalingLayer

ContentsScalingLayer depends on the main thread running calculate draw
properties and updating contents scale and content bounds.  In order to
switch to using property trees (which don't currently calculate these
values), PaintedScrollbarLayer needs to calculate contents scale
internally.

This also moves towards removing ContentsScalingLayer (currently also
used by TiledLayer and HeadsUpDisplayLayer), which once it's gone will
allow contents scale and content bounds to removed entirely from Layer
and LayerImpl.

Committed: https://crrev.com/467829ba34bd4244b8415e35149e72ce81386ad6
Cr-Commit-Position: refs/heads/master@{#316137}

[enne (OOO), 2015-02-11 21:41:57]

enne@chromium.org changed reviewers:
+ danakj@chromium.org

[enne (OOO), 2015-02-11 21:41:59]

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
File cc/layers/painted_scrollbar_layer.cc (right):

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
cc/layers/painted_scrollbar_layer.cc:203: float scale =
layer_tree_host()->device_scale_factor();
This is a change in behavior from ContentsScalingLayer, which defaults to using
device scale factor + page scale factor when the layer has a transform that
scales can't be extracted from.  I don't worry very hard about pixelated
scrollbars on transformed layers during page zoom, but I thought I'd mention
that it's a subtle change in behavior.

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
cc/layers/painted_scrollbar_layer.cc:219: SetNeedsDisplay();
This replicates ContentsScalingLayer::Update, although it also SetNeedsDisplay
when the bounds changed, which unit tests needed, and seems more correct.

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
cc/layers/painted_scrollbar_layer.cc:229: UpdateInternalContentScale();
As UpdateInternalContentScale can invalidate, it needs to be in this auto reset
block to avoid an extra commit.

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
cc/layers/painted_scrollbar_layer.cc:253: updated = true;
I don't know why this didn't set updated, but it seemed like an oversight.

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
File cc/layers/painted_scrollbar_layer.h (right):

https://codereview.chromium.org/913133004/diff/1/cc/layers/painted_scrollbar_...
cc/layers/painted_scrollbar_layer.h:87: float internal_content_scale_;
Internal is a weird name, but I wanted to differentiate from Layer(Impl)
functions and members.

https://codereview.chromium.org/913133004/diff/1/cc/layers/scrollbar_layer_un...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/1/cc/layers/scrollbar_layer_un...
cc/layers/scrollbar_layer_unittest.cc:694: // Force output surface creation for
renderer capabilities.
Previously the MaxTextureSize (which reached into renderer capabilities, which
dchecks output surface lost) only happened during CDP, which doesn't happen in
these tests.  Because Update does happen (where the max texture size clamping
moved to), they suddenly need an output surface.

https://codereview.chromium.org/913133004/diff/1/cc/layers/scrollbar_layer_un...
cc/layers/scrollbar_layer_unittest.cc:776: created += num_updates[j] * 2;
This test used to recreate the LTH every time, but rather than refactoring the
creation function, I just accumulated the values here.  The test is ultimately
the same.

[enne (OOO), 2015-02-11 23:04:22]

I added another patch because it turned out I wasn't scaling the thumb rect by
device scale (and we had no tests to verify this).  This required moving around
a bunch of stuff in scrollbar_layer_unittest, sorry.

[danakj, 2015-02-12 19:35:44]

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:94:
gfx::ToCeiledSize(gfx::ScaleSize(bounds(), scale, scale));
nit: only need to pass scale once

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:113:
scrollbar_layer->set_internal_content_scale_and_bounds(
internal_contents_scale thoughout? should nominate for best name award

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl.cc:94:
occlusion_in_content_space.GetUnoccludedContentRect(thumb_quad_rect);
The occlusion is in unscaled space (from the draw transform), so does this rect
work with it now?

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl_unittest.cc:39:
scrollbar_layer_impl->set_internal_content_scale_and_bounds(1.f, layer_size);
maybe we should have some scale here. would it pass?

https://codereview.chromium.org/913133004/diff/20001/cc/layers/scrollbar_laye...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_unittest.cc:110: typedef base::hash_map<UIResourceId,
UIResourceBitmap> UIResourceBitmapMap;
nit: prefer using

[enne (OOO), 2015-02-12 21:55:56]

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:94:
gfx::ToCeiledSize(gfx::ScaleSize(bounds(), scale, scale));
On 2015/02/12 19:35:43, danakj wrote:
> nit: only need to pass scale once

Done.

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:113:
scrollbar_layer->set_internal_content_scale_and_bounds(
On 2015/02/12 19:35:43, danakj wrote:
> internal_contents_scale thoughout? should nominate for best name award

Who named that?? T_T

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl.cc:94:
occlusion_in_content_space.GetUnoccludedContentRect(thumb_quad_rect);
On 2015/02/12 19:35:43, danakj wrote:
> The occlusion is in unscaled space (from the draw transform), so does this
rect
> work with it now?

OOPS.

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl_unittest.cc:39:
scrollbar_layer_impl->set_internal_content_scale_and_bounds(1.f, layer_size);
On 2015/02/12 19:35:43, danakj wrote:
> maybe we should have some scale here. would it pass?

Excellent call.  It wouldn't.  Fixed.

https://codereview.chromium.org/913133004/diff/20001/cc/layers/scrollbar_laye...
File cc/layers/scrollbar_layer_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/20001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_unittest.cc:110: typedef base::hash_map<UIResourceId,
UIResourceBitmap> UIResourceBitmapMap;
On 2015/02/12 19:35:43, danakj wrote:
> nit: prefer using

Done.

https://codereview.chromium.org/913133004/diff/20001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_unittest.cc:385: TEST_F(ScrollbarLayerTest,
ThumbRectDeviceScale) {
I removed this test in favor of adding a few rect checks to the occlusion test. 
It's a little weird to do there, but I can't use ComputeThumbRect anymore (as it
just does layer rects) and there's a lotttt of setup to get a layer to produce
quads.

https://codereview.chromium.org/913133004/diff/40001/cc/test/layer_test_commo...
File cc/test/layer_test_common.cc (right):

https://codereview.chromium.org/913133004/diff/40001/cc/test/layer_test_commo...
cc/test/layer_test_common.cc:80: if
(!quad->quadTransform().IsIdentityOrIntegerTranslation()) {
Now that the occlusion test has a scale transform in it, I had to adjust this to
make the test framework not yell.

[danakj, 2015-02-12 22:18:05]

LGTM

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl.cc:96: visible_thumb_quad_rect,
internal_contents_scale_);
Do you need to intersect with the scaled_thumb_quad_rect? We normally do that..

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl_unittest.cc:74: gfx::Rect
scaled_thumb_rect = gfx::ScaleToEnclosingRect(thumb_rect, scale);
Can you leave a comment making note that this is testing something other than
occlusion?

Then if we decide to delete this test one day it hopefully alerts us.

[enne (OOO), 2015-02-12 22:26:28]

New patchsets have been uploaded after l-g-t-m from danakj@chromium.org

[enne (OOO), 2015-02-12 22:26:34]

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl.cc:96: visible_thumb_quad_rect,
internal_contents_scale_);
On 2015/02/12 22:18:04, danakj wrote:
> Do you need to intersect with the scaled_thumb_quad_rect? We normally do
that..

I can't imagine a case where the unscaled thumb contains the unscaled visible,
but the scaled thumb does not contain the scaled visible.

I know we normally do that, but it kind of seems not necessary here.

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
File cc/layers/painted_scrollbar_layer_impl_unittest.cc (right):

https://codereview.chromium.org/913133004/diff/40001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer_impl_unittest.cc:74: gfx::Rect
scaled_thumb_rect = gfx::ScaleToEnclosingRect(thumb_rect, scale);
On 2015/02/12 22:18:05, danakj wrote:
> Can you leave a comment making note that this is testing something other than
> occlusion?
> 
> Then if we decide to delete this test one day it hopefully alerts us.

Done.

[enne (OOO), 2015-02-12 22:35:53]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-02-12 22:36:37]

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

[commit-bot: I haz the power, 2015-02-12 23:47:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-12 23:48:36]

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

[enne (OOO), 2015-02-12 23:55:26]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/913393004/ by enne@chromium.org.

The reason for reverting is: Causes DCHECKs in layout tests..

[enne (OOO), 2015-02-13 00:39:35]

I used the wrong (unscaled) track rect when passing the opaque rect to the quad.
 I added a check for this in the test and verified that the layout tests don't
DCHECK anymore.

[enne (OOO), 2015-02-13 00:39:38]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-02-13 00:40:48]

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

[danakj, 2015-02-13 00:43:42]

Thanks LGTM

[commit-bot: I haz the power, 2015-02-13 02:39:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-13 02:40:45]

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