cc: Do not allow gesture-scrolling 'overflow[-{x|y}]:hidden' layers.

cc: Do not allow gesture-scrolling 'overflow[-{x|y}]:hidden' layers.

BUG=175502
R=jamesr@chromium.org

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

[sadrul, 2013-10-19 22:42:10]

This fix is based on my understanding of
https://code.google.com/p/chromium/issues/detail?id=175502#c23 This breaks a few
tests, but it should be possible to fix those. Does this approach still look
like a good fix?

[danakj, 2013-10-21 15:14:42]

https://codereview.chromium.org/30793002/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/30793002/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:345: scroll.set_x(0.f);
This would eat the whole scroll but not perform it, ie return unscrolled would
return 0 along this axis. Is that the intent here?

[sadrul, 2013-10-21 16:19:13]

https://codereview.chromium.org/30793002/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/30793002/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:345: scroll.set_x(0.f);
On 2013/10/21 15:14:42, danakj wrote:
> This would eat the whole scroll but not perform it, ie return unscrolled would
> return 0 along this axis. Is that the intent here?

That wasn't intentional. Fixed. Thanks!

[jamesr1, 2013-10-21 16:36:42]

I don't believe this is sufficient for custom styled scrollbars. You will
need to pipe two bits for user scrollability (one for each axis) from blink.
On Oct 21, 2013 9:19 AM, <sadrul@chromium.org> wrote:

>
> https://codereview.chromium.**org/30793002/diff/1/cc/layers/**
>
layer_impl.cc<https://codereview.chromium.org/30793002/diff/1/cc/layers/layer_impl.cc>
> File cc/layers/layer_impl.cc (right):
>
> https://codereview.chromium.**org/30793002/diff/1/cc/layers/**
>
layer_impl.cc#newcode345<https://codereview.chromium.org/30793002/diff/1/cc/layers/layer_impl.cc#newcode345>
> cc/layers/layer_impl.cc:345: scroll.set_x(0.f);
> On 2013/10/21 15:14:42, danakj wrote:
>
>> This would eat the whole scroll but not perform it, ie return
>>
> unscrolled would
>
>> return 0 along this axis. Is that the intent here?
>>
>
> That wasn't intentional. Fixed. Thanks!
>
>
https://codereview.chromium.**org/30793002/<https://codereview.chromium.org/3...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2013-10-21 22:31:32]

On 2013/10/21 16:36:42, jamesr1 wrote:
> I don't believe this is sufficient for custom styled scrollbars. You will
> need to pipe two bits for user scrollability (one for each axis) from blink.

Done. The blink-side patch is at: https://codereview.chromium.org/33413002/

I have added a couple of tests here. PTAL.

[danakj, 2013-10-21 22:34:40]

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h#newcod...
cc/layers/layer.h:256: user_scrollable_vertical_ = vertical;
This doesn't need to SetNeedsCommit()?

[jamesr, 2013-10-21 22:36:34]

lgtm

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:465: return InputHandler::ScrollIgnored;
nit: it'd be useful to TRACE_EVENT this bail-out like the other ones so it's
easier to diagnose failure-to-scroll issues

https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:792:
overflow->SetUserScrollable(false, true);
callsites like this are really hard to read - what does this mean?

i think exposing the pair:
set_user_scrollable_horizontal(bool)
set_user_scrollable_vertical(bool)

while slightly more verbose would be worth it in increased readability

[sadrul, 2013-10-21 22:39:03]

https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:792:
overflow->SetUserScrollable(false, true);
On 2013/10/21 22:36:34, jamesr wrote:
> callsites like this are really hard to read - what does this mean?
> 
> i think exposing the pair:
> set_user_scrollable_horizontal(bool)
> set_user_scrollable_vertical(bool)
> 
> while slightly more verbose would be worth it in increased readability

Should this be done for the blink change in
https://codereview.chromium.org/33413002/ too? Or just from WebLayerImpl
onwards?

[jamesr, 2013-10-21 22:42:49]

On 2013/10/21 22:39:03, sadrul wrote:
>
https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl_unittest.cc:792:
> overflow->SetUserScrollable(false, true);
> On 2013/10/21 22:36:34, jamesr wrote:
> > callsites like this are really hard to read - what does this mean?
> > 
> > i think exposing the pair:
> > set_user_scrollable_horizontal(bool)
> > set_user_scrollable_vertical(bool)
> > 
> > while slightly more verbose would be worth it in increased readability
> 
> Should this be done for the blink change in
> https://codereview.chromium.org/33413002/ too? Or just from WebLayerImpl
> onwards?

This is arbitrary, but I think the Blink API is more reasonable as one call
since the overhead of changing cross-repo API is higher and the calls do not map
directly onto setters/getters.

[sadrul, 2013-10-21 23:11:16]

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h#newcod...
cc/layers/layer.h:256: user_scrollable_vertical_ = vertical;
On 2013/10/21 22:34:41, danakj wrote:
> This doesn't need to SetNeedsCommit()?

It probably does! Done.

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:465: return InputHandler::ScrollIgnored;
On 2013/10/21 22:36:34, jamesr wrote:
> nit: it'd be useful to TRACE_EVENT this bail-out like the other ones so it's
> easier to diagnose failure-to-scroll issues

Done.

https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/30793002/diff/150001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:792:
overflow->SetUserScrollable(false, true);
On 2013/10/21 22:39:03, sadrul wrote:
> On 2013/10/21 22:36:34, jamesr wrote:
> > callsites like this are really hard to read - what does this mean?
> > 
> > i think exposing the pair:
> > set_user_scrollable_horizontal(bool)
> > set_user_scrollable_vertical(bool)
> > 
> > while slightly more verbose would be worth it in increased readability
> 
> Should this be done for the blink change in
> https://codereview.chromium.org/33413002/ too? Or just from WebLayerImpl
> onwards?

Done (replaced LayerImpl::SetUserScrollable() with
LayerImpl::set_user_scrollable_*())

[danakj, 2013-10-21 23:30:09]

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h#newcod...
cc/layers/layer.h:256: user_scrollable_vertical_ = vertical;
On 2013/10/21 23:11:17, sadrul wrote:
> On 2013/10/21 22:34:41, danakj wrote:
> > This doesn't need to SetNeedsCommit()?
> 
> It probably does! Done.

Ok, can you add it to layer_unittest.cc then?

[sadrul, 2013-10-21 23:33:22]

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/30793002/diff/150001/cc/layers/layer.h#newcod...
cc/layers/layer.h:256: user_scrollable_vertical_ = vertical;
On 2013/10/21 23:30:09, danakj wrote:
> On 2013/10/21 23:11:17, sadrul wrote:
> > On 2013/10/21 22:34:41, danakj wrote:
> > > This doesn't need to SetNeedsCommit()?
> > 
> > It probably does! Done.
> 
> Ok, can you add it to layer_unittest.cc then?

Done.

[sadrul, 2013-10-22 03:36:47]

Committed patchset #8 manually as r230032 (presubmit successful).