Note scrollbar layer properties changed when position changes.

Note scrollbar layer properties changed when position changes.

The layer properties need to be changed when the current scroll position changes or else the damage will be ignored and it won't be painted.

BUG=272361
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217728

[danakj, 2013-08-13 19:09:51]

Can you add a test for this?

[danakj, 2013-08-13 19:11:02]

On 2013/08/13 19:09:51, danakj wrote:
> Can you add a test for this?

layer_tree_host_unittest_damage.cc might be an appropriate place. You can do
scrolling and see what the damage rect in the frame ends up being.

[piman, 2013-08-13 19:28:37]

drive-by: do we have a test (layout?) that makes sure that we don't blow up the
damage rect in case of redundant calls to SetCurrentPos/SetMaximum ?
I don't think we do, but if we were calling SetCurrentPos on every commit, we'd
make the damage as big as the view.

[danakj, 2013-08-13 19:32:40]

https://codereview.chromium.org/23102003/diff/1/cc/layers/scrollbar_layer_imp...
File cc/layers/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/23102003/diff/1/cc/layers/scrollbar_layer_imp...
cc/layers/scrollbar_layer_impl.cc:179: maximum_ = maximum;
Yes please early out here if maximum_ == maximum. Same for above.

[danakj, 2013-08-13 19:33:25]

I'd also like to understand why these work at all in GL mode..

[enne (OOO), 2013-08-13 19:43:13]

You'll also need to do this damaging when changing the thumb's parameters as
well (any function used as an input to ComputeThumbQuadRect).

[jbauman, 2013-08-14 17:57:20]

On 2013/08/13 19:11:02, danakj wrote:
> On 2013/08/13 19:09:51, danakj wrote:
> > Can you add a test for this?
> 
> layer_tree_host_unittest_damage.cc might be an appropriate place. You can do
> scrolling and see what the damage rect in the frame ends up being.

Yeah, I'm working on it. Setting up a layer tree that behaves correctly seems
pretty tricky, though....

[jbauman, 2013-08-14 21:08:40]

PTAL. Added a test and handling of other properties.

[danakj, 2013-08-14 21:29:14]

Thanks! Few comments for the test.

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:297: layer_tree_root_ =
Layer::Create();
does this need to be a member variable? usually they all do or none of them do.
i don't see you using this on the main thread outside of this function. (nit:
root_layer? layer_tree_root reads a lot like layer_tree_host :))

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:301:
layer_tree_root_->AddChild(content_layer);
nit: some whitespace in between blocks of calls for each layer object would
help, and grouping the creation at the top of each block for the calls on that
layer.

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:311:
LayerTreeHostDamageTest::SetupTree();
can you EXPECT here that the content_layer does not contain the scrollbar, so we
know the test is doing what it claims for all time?

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:350: case 2:
Can you add a case that changes the max scroll (thumb size)?

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:351: host_impl->ScheduleAnimation();
nit: SetNeedsRedraw() instead?

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:354:
root->children()[0]->SetPosition(gfx::Point(1,1));
Add a comment explaining what this case is doing and why it is testing the
scrollbar?

[enne (OOO), 2013-08-14 21:33:40]

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:333:
EXPECT_FALSE(root_damage.Contains(gfx::Rect(300, 300, 10, 100)));
Can Contains be changed to Intersects here to be more stringent?

[jbauman, 2013-08-14 21:36:00]

On 2013/08/14 21:33:40, enne wrote:
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_unittest_damage.cc (right):
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:333:
> EXPECT_FALSE(root_damage.Contains(gfx::Rect(300, 300, 10, 100)));
> Can Contains be changed to Intersects here to be more stringent?

Wouldn't Contains be more stringent?

[jbauman, 2013-08-14 21:36:11]

On 2013/08/14 21:36:00, jbauman wrote:
> On 2013/08/14 21:33:40, enne wrote:
> >
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> > File cc/trees/layer_tree_host_unittest_damage.cc (right):
> > 
> >
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> > cc/trees/layer_tree_host_unittest_damage.cc:333:
> > EXPECT_FALSE(root_damage.Contains(gfx::Rect(300, 300, 10, 100)));
> > Can Contains be changed to Intersects here to be more stringent?
> 
> Wouldn't Contains be more stringent?

Oops, nevermind.

[jbauman, 2013-08-14 22:10:35]

Ok, should be fixed.

On 2013/08/14 21:29:14, danakj wrote:
> Thanks! Few comments for the test.
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_unittest_damage.cc (right):
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:297: layer_tree_root_ =
> Layer::Create();
> does this need to be a member variable? usually they all do or none of them
do.
> i don't see you using this on the main thread outside of this function. (nit:
> root_layer? layer_tree_root reads a lot like layer_tree_host :))
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:301:
> layer_tree_root_->AddChild(content_layer);
> nit: some whitespace in between blocks of calls for each layer object would
> help, and grouping the creation at the top of each block for the calls on that
> layer.
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:311:
> LayerTreeHostDamageTest::SetupTree();
> can you EXPECT here that the content_layer does not contain the scrollbar, so
we
> know the test is doing what it claims for all time?
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:350: case 2:
> Can you add a case that changes the max scroll (thumb size)?
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:351:
host_impl->ScheduleAnimation();
> nit: SetNeedsRedraw() instead?
> 
>
https://codereview.chromium.org/23102003/diff/22001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:354:
> root->children()[0]->SetPosition(gfx::Point(1,1));
> Add a comment explaining what this case is doing and why it is testing the
> scrollbar?

[danakj, 2013-08-14 22:17:20]

Thanks! LGTM

https://codereview.chromium.org/23102003/diff/39001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23102003/diff/39001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:386:
MULTI_THREAD_TEST_F(LayerTreeHostDamageTestScrollbarDoesDamage);
nit: Can this be SINGLE_AND_MULTI_THREAD_TEST_F?

[enne (OOO), 2013-08-14 22:20:46]

lgtm2

https://codereview.chromium.org/23102003/diff/39001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/23102003/diff/39001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_damage.cc:371:
root->children()[0]->SetPosition(gfx::Point(1,1));
Could you also set the scrollbar to the same offset that it's already at and
check for no damage to address piman's concern in comment #4?

[jbauman, 2013-08-14 22:24:24]

On 2013/08/14 22:17:20, danakj wrote:
> Thanks! LGTM
> 
>
https://codereview.chromium.org/23102003/diff/39001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_unittest_damage.cc (right):
> 
>
https://codereview.chromium.org/23102003/diff/39001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_unittest_damage.cc:386:
> MULTI_THREAD_TEST_F(LayerTreeHostDamageTestScrollbarDoesDamage);
> nit: Can this be SINGLE_AND_MULTI_THREAD_TEST_F?

No, impl-thread scrolling and the single-threaded compositor don't play well
together (and I can't figure out a way to do this without impl thread scrolling.

[commit-bot: I haz the power, 2013-08-14 22:36:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/23102003/31001

[danakj, 2013-08-14 22:36:41]

On Wed, Aug 14, 2013 at 6:24 PM, <jbauman@chromium.org> wrote:

> No, impl-thread scrolling and the single-threaded compositor don't play
> well
> together (and I can't figure out a way to do this without impl thread
> scrolling.
>

Ah, okay!


>
>
https://codereview.chromium.**org/23102003/<https://codereview.chromium.org/2...
>

[commit-bot: I haz the power, 2013-08-15 01:46:24]

Change committed as 217728