Move nonscrollable axis gloweffect suppression to renderer.

Move nonscrollable axis gloweffect suppression to renderer.

on nonscrollable axis gloweffect should not be invoked, on diagonal scroll in some cases horizontal axis is disabled, and when scrolling diagonally this invokes horizontal glow , but as per android UX horizontal glow shouldnt be display as it is nonscrollable. This change optimizes such calls and avoids unnecessary IPC calls.

The change is behind a CC setting since Android WebView
needs to send overscroll notifications to apps even on nonscrollable axes.

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

[MuVen, 2014-04-09 13:57:08]

PTAL

I have written the code base on following table. I have taken the values for all
the working glow cases. considering many values i have written this table.

Considering all these cases we did_scroll_x = 1 did_scroll_y = 1  glow cannot
appear. Can we add check for that aelias ?
I have verified on long scroll_delta even.

(copy paste on notepad++ for better look)

DIRECTION     					unused_root_delta         did_scroll_x      did_scroll_y

TOPGLOW        					0, < 0  EX:(0,    -10)   		0      			0
BOTTOMGLOW     					0, > 0  EX:(0,     10)    		0      			0
RIGHTGLOW  					    > 0, 0  EX:(10,     0)    		0      			0
LEFTGLOW       					< 0, 0  EX:(-10,    0)    		0      			0

BOTTOMGLOW RIGHTGLOW		    > 0, > 0 EX:(10,   10)    		0      			0
BOTTOMGLOW LEFTGLOW			    < 0, > 0 EX:(-10,  10)    		0      			0
TOPGLOW LEFTGLOW 				< 0, < 0 EX:(-10, -10)    		0      			0
TOPGLOW RIGHTGLOW				> 0, < 0 EX:(10,  -10)    		0      			0

RIGHTGLOW BOTTOM UP SCROLLING    > 0, 0 EX:(10,     0)    		0      			1
LEFTGLOW BOTTOM UP SCROLLING     < 0, 0 EX:(-10,    0)    		0      			1
TOPGLOW LEFT RIGHT SCROLLING     0, < 0 EX:(0,    -10)   		1      			0
BOTTOMGLOW LEFT RIGHT SCROLLING  0, > 0 EX:(0,     10)  		1      			0



thanks,
MuVen.

[MuVen, 2014-04-09 15:52:19]

DIRECTION     			unused_root_delta         did_scroll_x      did_scroll_y

TOPGLOW        			0, < 0  EX:(0,    -10) 		0      			0
BOTTOMGLOW     			0, > 0  EX:(0,     10) 		0      			0
RIGHTGLOW  			> 0, 0  EX:(10,     0) 		0      			0
LEFTGLOW       			< 0, 0  EX:(-10,    0) 		0      			0

BOTTOMGLOW RIGHTGLOW	       > 0, > 0 EX:(10,   10)  		0      			0
BOTTOMGLOW LEFTGLOW	       < 0, > 0 EX:(-10,  10)  	0      			0
TOPGLOW LEFTGLOW 	       < 0, < 0 EX:(-10, -10)    		0      			0
TOPGLOW RIGHTGLOW	       > 0, < 0 EX:(10,  -10)    		0      			0

RIGHTGLOW BOTTOM UP SCROLLING    > 0, 0 EX:(10,     0)    		0      			1
LEFTGLOW BOTTOM UP SCROLLING     < 0, 0 EX:(-10,    0)    		0      			1
TOPGLOW LEFT RIGHT SCROLLING     0, < 0 EX:(0,    -10)   		1      			0
BOTTOMGLOW LEFT RIGHT SCROLLING  0, > 0 EX:(0,     10)  		1      			0

[MuVen, 2014-04-09 15:53:07]

DIRECTION     			unused_root_delta         did_scroll_x      did_scroll_y

TOPGLOW        			0, < 0  EX:(0,    -10) 		0      			0
BOTTOMGLOW     			0, > 0  EX:(0,     10) 		0      			0
RIGHTGLOW  			> 0, 0  EX:(10,     0) 		0      			0
LEFTGLOW       			< 0, 0  EX:(-10,    0) 		0      			0

BOTTOMGLOW RIGHTGLOW	       > 0, > 0 EX:(10,   10)  		0      			0
BOTTOMGLOW LEFTGLOW	       < 0, > 0 EX:(-10,  10)    	0      			0
TOPGLOW LEFTGLOW 	       < 0, < 0 EX:(-10, -10)    	0      			0
TOPGLOW RIGHTGLOW	       > 0, < 0 EX:(10,  -10)    	0      			0

RIGHTGLOW BOTTOM UP SCROLLING    > 0, 0 EX:(10,     0)   	0      			1
LEFTGLOW BOTTOM UP SCROLLING     < 0, 0 EX:(-10,    0)   	0      			1
TOPGLOW LEFT RIGHT SCROLLING     0, < 0 EX:(0,    -10)   	1      			0
BOTTOMGLOW LEFT RIGHT SCROLLING  0, > 0 EX:(0,     10)  	1      			0

[MuVen, 2014-04-09 16:05:09]

+jdduke

[jdduke (slow), 2014-04-09 17:35:00]

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2370: if
(!settings_.using_synchronous_renderer_compositor) {
I don't think we want to piggyback on this setting.  We should just add a new
setting, say, |report_overscroll_only_for_scrollable_axes| (aelias@ may have a
better name), and we set it appropriately in |RenderWidgetCompositor::Create|
(for now based on the using_synchronous_renderer_compositor setting).

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2375: // Horizontal Glow Disabled as scrolling
on Horizontal axes is impossible.
I wonder if we should just use:

if (InnerViewportScrollLayer()->MaxScrollOffset().x() <= 0)
  unused_root_delta.set_x(0);

and similar for the y axis.  Then there's no need to check the other axis, and
no need to early return.  I'm not sure if we need an epsilon check; I used a
ceil previously because I was observing small float errors between viewport and
content sizes, but maybe the MaxScrollOffset does not suffer this problem
(aelias@ can you confirm?)

[aelias_OOO_until_Jul13, 2014-04-09 19:43:42]

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2370: if
(!settings_.using_synchronous_renderer_compositor) {
On 2014/04/09 17:35:00, jdduke wrote:
> I don't think we want to piggyback on this setting.  We should just add a new
> setting, say, |report_overscroll_only_for_scrollable_axes| (aelias@ may have a
> better name), and we set it appropriately in |RenderWidgetCompositor::Create|
> (for now based on the using_synchronous_renderer_compositor setting).
> 

"report_overscroll_only_for_scrollable_axes" sounds OK to me.

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2375: // Horizontal Glow Disabled as scrolling
on Horizontal axes is impossible.
On 2014/04/09 17:35:00, jdduke wrote:
> I wonder if we should just use:
> 
> if (InnerViewportScrollLayer()->MaxScrollOffset().x() <= 0)
>   unused_root_delta.set_x(0);
> 
> and similar for the y axis.  Then there's no need to check the other axis, and
> no need to early return.  I'm not sure if we need an epsilon check; I used a
> ceil previously because I was observing small float errors between viewport
and
> content sizes, but maybe the MaxScrollOffset does not suffer this problem
> (aelias@ can you confirm?)

This sounds fine.  If MaxScrollOffset suffers from the problem, we can correct
the problem there.

For future-proofing for the new pinch zoom mode, we should check
InnerViewportScrollLayer()->MaxScrollOffset().x() <= 0 &&
OuterViewportScrollLayer()->MaxScrollOffset().x() <= 0  (although only the inner
viewport is used right now).

[jdduke (slow), 2014-04-09 22:51:47]

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/230223006/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2312: if (std::abs(unused_root_delta.x()) <
kEpsilon)
It might make sense for us to move the zero-ing code here, so that all such code
is in a single place in this function.

[MuVen, 2014-04-10 03:49:40]

Hi Jdduke,

we can implement in this way and i guess we dont need any other checks  whether
viewport width < rootLayerwidth and all i guess. And can  this be made generic ?

 if (layer_impl == InnerViewportScrollLayer()) {
      unused_root_delta.Subtract(applied_delta);
      if (std::abs(unused_root_delta.x()) < kEpsilon)
        unused_root_delta.set_x(0.0f);
      if (std::abs(unused_root_delta.y()) < kEpsilon)
        unused_root_delta.set_y(0.0f);  
      if(std::abs(InnerViewportScrollLayer()->MaxScrollOffset().x()) <= 0)
        unused_root_delta.set_x(0.0f);
      if(std::abs(InnerViewportScrollLayer()->MaxScrollOffset().y()) <= 0)
        unused_root_delta.set_y(0.0f);     
    }

Please share your views .

[MuVen, 2014-04-10 04:54:23]

PTAL.

[jdduke (slow), 2014-04-10 14:16:02]

https://codereview.chromium.org/230223006/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/230223006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2318: gfx::SizeF ceiled_viewport_size =
You shouldn't need to check both viewport size and the max scroll offset,
something like this should be sufficient:

if (active_tree_->TotalMaxScrollOffset().x() <= kEpsilon)
  unused_root_delta.set_x(0.0f):
if (active_tree_->TotalMaxScrollOffset().y() <= kEpsilon)
  unused_root_delta.set_y(0.0f):

https://codereview.chromium.org/230223006/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/230223006/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:966: gfx::SizeF
ceiled_viewport_size =
Please remove the corresponding |set_*_overscroll_enabled| methods on
OverscrollGlow, as well as the member variables and clamping that they trigger
(in content/browser/android/overscroll_glow.{cc,h}).

[aelias_OOO_until_Jul13, 2014-04-10 16:28:36]

https://codereview.chromium.org/230223006/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/230223006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2322: if
(std::abs(InnerViewportScrollLayer()->MaxScrollOffset().x()) <= 0)
Please also check OuterViewportScrollLayer as I mentioned earlier.  For context,
this is for when this approach launches in the future:
https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...

[MuVen, 2014-04-10 16:31:36]

Hi jdduke,

i have one recommendation on this patch to avoid dependency on kEpsilon.


 const float kEpsilon = 0.1f;
    if (layer_impl == InnerViewportScrollLayer()) {
      unused_root_delta.Subtract(applied_delta);
      if (std::abs(unused_root_delta.x()) < kEpsilon)
        unused_root_delta.set_x(0.0f);
      if (std::abs(unused_root_delta.y()) < kEpsilon)
        unused_root_delta.set_y(0.0f);
  
++      if(!layer_impl->HasScrollbar(HORIZONTAL))
++        unused_root_delta.set_x(0.0f);
++      if(!layer_impl->HasScrollbar(VERTICAL))
++        unused_root_delta.set_y(0.0f);

    }


let me know your comments on this.

[aelias_OOO_until_Jul13, 2014-04-10 17:50:50]

On 2014/04/10 16:31:36, muven wrote:
> Hi jdduke,
> 
> i have one recommendation on this patch to avoid dependency on kEpsilon.
> 
> 
>  const float kEpsilon = 0.1f;
>     if (layer_impl == InnerViewportScrollLayer()) {
>       unused_root_delta.Subtract(applied_delta);
>       if (std::abs(unused_root_delta.x()) < kEpsilon)
>         unused_root_delta.set_x(0.0f);
>       if (std::abs(unused_root_delta.y()) < kEpsilon)
>         unused_root_delta.set_y(0.0f);
>   
> ++      if(!layer_impl->HasScrollbar(HORIZONTAL))
> ++        unused_root_delta.set_x(0.0f);
> ++      if(!layer_impl->HasScrollbar(VERTICAL))
> ++        unused_root_delta.set_y(0.0f);
> 
>     }
> 
> 
> leg me know your comments on this.

Not a good idea, since scrollbars aren't necessarily one-to-one with
scrollability.  Scrollbars could be turned off for purely cosmetic reasons.

[MuVen, 2014-04-11 03:31:25]

As mentioned by jdduke, he has recommended to use
active_tree_->TotalMaxScrollOffset() ; Logic is not related only to
innerviewport scrolllayer, does this still need to add check for
outerviewportscrolllayer ?

[aelias_OOO_until_Jul13, 2014-04-11 03:35:12]

On 2014/04/11 03:31:25, muven wrote:
> As mentioned by jdduke, he has recommended to use
> active_tree_->TotalMaxScrollOffset() ; Logic is not related only to
> innerviewport scrolllayer, does this still need to add check for
> outerviewportscrolllayer ?

Yes, TotalMaxScrollOffset should work fine.

[MuVen, 2014-04-11 04:04:10]

PTAL, update as per review comments.

[aelias_OOO_until_Jul13, 2014-04-11 07:01:49]

lgtm

[MuVen, 2014-04-11 07:04:15]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2014-04-11 07:04:33]

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

[aelias_OOO_until_Jul13, 2014-04-11 07:10:34]

Adding piman@ for content/renderer/gpu OWNERS rubberstamp.

I also updated your patch description.  In the future, please keep lines below
80 characters, provide more details, and link to a bug number.

[piman, 2014-04-11 07:36:03]

lgtm

[commit-bot: I haz the power, 2014-04-11 08:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-11 08:23:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[MuVen, 2014-04-11 08:27:01]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2014-04-11 08:27:06]

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

[MuVen, 2014-04-11 11:04:52]

The CQ bit was unchecked by sataya.m@samsung.com

[MuVen, 2014-04-11 11:04:53]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2014-04-11 13:42:13]

Change committed as 263226