Fix another inappropriate overscroll glow.

Fix another inappropriate overscroll glow.

r259729 fixed one bad overscroll glow, but introduced a regression in another
case.  When a scroll amount fell between 0.01 and 0.1, the early break caused
the amount to fail to be subtracted from unused_scroll_delta at all, so all such
scrolls resulted in a glow.

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

[MuVen, 2014-03-31 09:57:37]

Hi Danakj/Aelias,

I found the same issue reproducible when you zoom in to the max scale in the
same page.

This is because of minute fraction values appended during the scaling. i thought
reusing the move_threshold value and added safe gaurd check !(did_scroll_x &&
did_scroll_y) which is required as when the glow effect is shown did_scroll_x
and did_scroll_y both of them should be [0,0] (for top, left, right, bottom,
top-left, top-right, bottom-left, bottom-right) or 1,0/ 0, 1(diagonal scrolling
case). 

[1,1] values represent that both the layers are scrollable. in this case glow
need not be active.

kindly let know your comments.

Thanks,
~MuVen.

[aelias_OOO_until_Jul13, 2014-04-01 02:28:19]

Please apply my alternate proposal below and also add a unit test (I suggest
just scrolling by an amount between 0.01 and 0.1 as that should reproduce the
issue.)

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

https://codereview.chromium.org/218883005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2267: break;
Hmm, even with your latest changes, I notice another bug that there is a small
overscroll glow at the end of flings (when they slow down completely).

I believe the bug is simply that this "break" here sometimes prevents the
subtraction from happening at all.  Could you move up the
unused_root_delta.Subtract clause right above "If the layer wasn't able to move"
comment?  Then remove the other changes in this patch.

[MuVen, 2014-04-01 03:29:07]

Hi aelias,

i agree with your patch as i saw many times from the logs that break has occured

without subtracting. ill move the subtract clause as u mentioned.

I have quick question on your proposal kindly clarify.

You dont want  const float kOverscrollEpsilon = 0.01f; to be changed to 
move_threshold ?


Along with this i propose this safe gaurd check which removes unnecessary glow
effects, 

bool did_overscroll =
       !unused_root_delta.IsZero() && !(did_scroll_x && did_scroll_y);

i have verified in many scenearios (working usecases) , glow wont be initiated
when 
did_scroll_x = 1 and did_scroll_y =1. Kindly reconsider on this safe gaurd
check. This gives
OPTIMISES glow effect completely .

let me know your opinion , base on that ill upload the final patch with
testcase.

Thanks,
MuVen.

[aelias_OOO_until_Jul13, 2014-04-01 04:29:42]

On 2014/04/01 03:29:07, muven wrote:
> You dont want  const float kOverscrollEpsilon = 0.01f; to be changed to 
> move_threshold ?

It's cleaner to keep numerical fudging as small as possible.  So I'd rather not
increase the epsilon if we can avoid it.  I think move_threshold itself is too
high and it would be a cleanup to reduce it.

> 
> 
> Along with this i propose this safe gaurd check which removes unnecessary glow
> effects, 
> 
> bool did_overscroll =
>        !unused_root_delta.IsZero() && !(did_scroll_x && did_scroll_y);
> 
> i have verified in many scenearios (working usecases) , glow wont be initiated
> when 
> did_scroll_x = 1 and did_scroll_y =1. Kindly reconsider on this safe gaurd
> check. This gives
> OPTIMISES glow effect completely .

If the rest of the code is working correctly, then this safeguard shouldn't
change the behavior, right?  For this kind of invariant we usually prefer
DCHECK, that way bugs are highlighted rather than hidden.  I'd be fine if this
is changed to
DCHECK(!(did_scroll_x && did_scroll_y)) within the if() block.

[aelias_OOO_until_Jul13, 2014-04-01 04:31:11]

On 2014/04/01 04:29:42, aelias wrote:
> On 2014/04/01 03:29:07, muven wrote:
> > You dont want  const float kOverscrollEpsilon = 0.01f; to be changed to 
> > move_threshold ?
> 
> It's cleaner to keep numerical fudging as small as possible.  So I'd rather
not
> increase the epsilon if we can avoid it.  I think move_threshold itself is too
> high and it would be a cleanup to reduce it.
> 
> > 
> > 
> > Along with this i propose this safe gaurd check which removes unnecessary
glow
> > effects, 
> > 
> > bool did_overscroll =
> >        !unused_root_delta.IsZero() && !(did_scroll_x && did_scroll_y);
> > 
> > i have verified in many scenearios (working usecases) , glow wont be
initiated
> > when 
> > did_scroll_x = 1 and did_scroll_y =1. Kindly reconsider on this safe gaurd
> > check. This gives
> > OPTIMISES glow effect completely .
> 
> If the rest of the code is working correctly, then this safeguard shouldn't
> change the behavior, right?  For this kind of invariant we usually prefer
> DCHECK, that way bugs are highlighted rather than hidden.  I'd be fine if this
> is changed to
> DCHECK(!(did_scroll_x && did_scroll_y)) within the if() block.

On second thought, it's possible to have a large scroll delta that both scrolls
to the edge, then cause overscroll glow.  So I'm back to thinking we shouldn't
have this at all.

[MuVen, 2014-04-01 07:09:08]

Please take a look . I have update as per review comments.

Thanks,
MuVen.

[aelias_OOO_until_Jul13, 2014-04-01 19:30:32]

lgtm.  I updated the patch description for the latest approach.

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

https://codereview.chromium.org/218883005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:3165: TEST_F(LayerTreeHostImplTest,
RedundantGlowEffectCalls) {
nit: "Redundant" is not the right word for this because they're actually
incorrect.  I'd suggest renaming this to "NoOverscrollWhenNotAtEdge"

[MuVen, 2014-04-02 02:51:22]

Modified as per review comments.

Please take a look.

Thanks,
MuVen.

[aelias_OOO_until_Jul13, 2014-04-02 06:45:40]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-04-02 06:45:48]

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

[commit-bot: I haz the power, 2014-04-02 08:18:19]

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

[commit-bot: I haz the power, 2014-04-02 08:18:19]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

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

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

[commit-bot: I haz the power, 2014-04-02 09:16:21]

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

[commit-bot: I haz the power, 2014-04-02 16:40:19]

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

[commit-bot: I haz the power, 2014-04-02 16:40:19]

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

[MuVen, 2014-04-02 16:58:26]

Aelias,

Android_aosp has failed ... is it related to us ???

[aelias_OOO_until_Jul13, 2014-04-02 21:04:22]

No, it looks like flakiness.  Trying again.

[aelias_OOO_until_Jul13, 2014-04-02 21:04:26]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-04-02 21:05:22]

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

[commit-bot: I haz the power, 2014-04-03 00:35:53]

Change committed as 261241