Scrollbar ThumbLength is not updated when window size is changed.

Scrollbar ThumbLength is not updated, when window size is changed.

When a pop-up is opened the window size is 367 and content size is 451
and scrollbar of thumblength 202 is created. Pop-Window height has been
resized to 451 because of JS . Before resizing of Pop-Window scrollbar
thumb_length of 202 is been applied. After resizing Pop-Window vertical
scrollbar is disabled during UpdateScrollbarGeometry(has_thumb_ = false). 

Now When PaintedScrollbarLayer::UpdateThumbAndTrackGeometry is called 
after pop-up window resize, as has_thumb_ = false , UpdateProperty on
thumb_length_ is not applied, due to which we are seeing the unwanted
scrollbar

BUG=400136
Committed: https://crrev.com/ba7e0499e10b1c65ead54148a7582e0c60be6c6d
Cr-Commit-Position: refs/heads/master@{#293218}

[MuVen, 2014-09-01 14:11:36]

sataya.m@samsung.com changed reviewers:
+ danakj@chromium.org

[MuVen, 2014-09-01 14:11:36]

PTAL.

[Sikugu_, 2014-09-01 14:46:19]

sivagunturi@chromium.org changed reviewers:
+ sivagunturi@chromium.org

[Sikugu_, 2014-09-01 14:46:20]

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

https://codereview.chromium.org/524373003/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:45: has_thumb_pre_(false) {
can we have some better name here like cache_thumb_value_?

[danakj, 2014-09-02 16:23:06]

Please leave a blank line between your first-line patch title and the
description below it.

https://github.com/erlang/otp/wiki/Writing-good-commit-messages

If the content no longer requires scrolling, how does updating the thumb help
anything? It seems like the bug is that there's a scrollbar at all?

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

https://codereview.chromium.org/524373003/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:193: if ((has_thumb_ != has_thumb_pre_) ||
has_thumb_) {
I don't understand, when has_thumb_ became false, you still want to update it?
But you don't have a thumb anymore.

[MuVen, 2014-09-02 16:42:04]

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

https://codereview.chromium.org/524373003/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:193: if ((has_thumb_ != has_thumb_pre_) ||
has_thumb_) {
When "Pin it" is clicked a pop-up of window height of 367 and content height of
451 is created, during that time has_thumb_ is true, but later bec of some js
window height is increased to 451.
When window height is increased to 451 has_thumb_ is false, and thumb_length
should be zero. As has_thumb_ is false UpdateProperty on thumb_length_ is not
updated to 0 
(which is 202 previously).

[MuVen, 2014-09-02 17:29:47]

Updated Bug 400136 with clear analysis. PTAL.

Thanks,

[danakj, 2014-09-02 18:38:56]

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

https://codereview.chromium.org/524373003/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:193: if ((has_thumb_ != has_thumb_pre_) ||
has_thumb_) {
On 2014/09/02 16:42:04, muven wrote:
> When "Pin it" is clicked a pop-up of window height of 367 and content height
of
> 451 is created, during that time has_thumb_ is true, but later bec of some js
> window height is increased to 451.
> When window height is increased to 451 has_thumb_ is false, and thumb_length
> should be zero. As has_thumb_ is false UpdateProperty on thumb_length_ is not
> updated to 0 
> (which is 202 previously).

Ok, then how about when has_thumb is false, we always UpdateProperty of these 2
things to 0?

Also can you please add unit tests for this.

[Sikugu_, 2014-09-03 05:47:05]

sivagunturi@chromium.org changed reviewers:
- sivagunturi@chromium.org

[MuVen, 2014-09-03 07:04:39]

PTAL. Test case added.

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

https://codereview.chromium.org/524373003/diff/20001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:193: if ((has_thumb_ != has_thumb_pre_) ||
has_thumb_) {
When has_thumb is false, UpdateProperty of ThumbLength() is calculated @
ScrollbarTheme::thumbLength().If ScrollBar is disabled Length will be 0. Test
Case is added.

[danakj, 2014-09-03 15:38:27]

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

https://codereview.chromium.org/524373003/diff/60001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:193: if ((cache_has_thumb_ != has_thumb_)
|| has_thumb_) {
I mean you don't need to add this cache_has_thumb_. I don't want to add a member
variable to this class when we don't need to.

When has_thumb_ is false, just UpdateProperty(0, &thumb_thickness_); etc. no? Or
just remove the if(has_thumb) if the ThumbLength() function does the right thing
already?

[MuVen, 2014-09-03 17:00:13]

On 2014/09/03 15:38:27, danakj wrote:
>
https://codereview.chromium.org/524373003/diff/60001/cc/layers/painted_scroll...
> File cc/layers/painted_scrollbar_layer.cc (right):
> 
>
https://codereview.chromium.org/524373003/diff/60001/cc/layers/painted_scroll...
> cc/layers/painted_scrollbar_layer.cc:193: if ((cache_has_thumb_ != has_thumb_)
> || has_thumb_) {
> I mean you don't need to add this cache_has_thumb_. I don't want to add a
member
> variable to this class when we don't need to.
> 
> When has_thumb_ is false, just UpdateProperty(0, &thumb_thickness_); etc. no?
Or
> just remove the if(has_thumb) if the ThumbLength() function does the right
thing
> already?

Yes, initial patchset1 was to remove if(has_thumb). ThumbLength() function does
the
right thing. Removed the cache_has_thumb_. Review comments addressed.

PTAL.

Thanks.

[danakj, 2014-09-03 17:20:51]

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

https://codereview.chromium.org/524373003/diff/80001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:204: if (track_rect_.IsEmpty() ||
scaled_track_rect.IsEmpty())
This function never destroys the thumb_resource_ or the track_resource_. Maybe
it should be dropping those resources rather than holding onto their memory when
the track_rect_ is empty, and/or the thumb is missing/empty? Maybe you can
follow up on this with another CL?

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

https://codereview.chromium.org/524373003/diff/80001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_unittest.cc:232:
scrollbar_layer->fake_scrollbar()->set_thumb_thickness(0);
Hm, I guess this is building the assumption that the real Scrollbar will do the
same thing when thumb is false. I don't feel super comfortable with that
assumption since we can't test it.

So, how about remove these 2 set_thumb_thickness/length calls here. And in the
scrollbar class if(has_thumb_) { use the values from Scrollbar } else { use 0 }

(Sorry to backpedal on this, I didn't realize it wasn't something we could test)

[MuVen, 2014-09-03 18:08:47]

changes done as you have suggested. PTAL.

Thanks.

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

https://codereview.chromium.org/524373003/diff/80001/cc/layers/painted_scroll...
cc/layers/painted_scrollbar_layer.cc:204: if (track_rect_.IsEmpty() ||
scaled_track_rect.IsEmpty())
On 2014/09/03 17:20:51, danakj wrote:
> This function never destroys the thumb_resource_ or the track_resource_. Maybe
> it should be dropping those resources rather than holding onto their memory
when
> the track_rect_ is empty, and/or the thumb is missing/empty? Maybe you can
> follow up on this with another CL?

Sure. Will work on this.

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

https://codereview.chromium.org/524373003/diff/80001/cc/layers/scrollbar_laye...
cc/layers/scrollbar_layer_unittest.cc:232:
scrollbar_layer->fake_scrollbar()->set_thumb_thickness(0);
On 2014/09/03 17:20:51, danakj wrote:
> Hm, I guess this is building the assumption that the real Scrollbar will do
the
> same thing when thumb is false. I don't feel super comfortable with that
> assumption since we can't test it.
> 
> So, how about remove these 2 set_thumb_thickness/length calls here. And in the
> scrollbar class if(has_thumb_) { use the values from Scrollbar } else { use 0
}
> 
> (Sorry to backpedal on this, I didn't realize it wasn't something we could
test)

Valid point !!! Changes Done.

[danakj, 2014-09-03 19:12:26]

LGTM

[danakj, 2014-09-03 19:12:31]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-03 19:17:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sataya.m@samsung.com/524373003/100001

[commit-bot: I haz the power, 2014-09-03 23:36:15]

Committed patchset #6 (id:100001) as c3dd07554fd3791e55862ff02bc5ea4040b2fd3b

[commit-bot: I haz the power, 2014-09-10 03:28:26]

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