Avoid unnecessary gloweffect calls

Avoid unnecessary gloweffect calls

When there's some overscroll in the x axis (getting us past the rounding check) and
an epsilon of overscroll in the y axis due to numerical error, this later convinces
OverscrollGlow to trigger the fling glow in that direction.  The Rounded check only
protected us against epsilons in *both* directions.  Replace this with removing
the epsilon explicitly in unused_scroll_delta.

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

[MuVen, 2014-03-20 08:04:04]

Hello,

Kindly review the patch submitted and let me know your valuable comments on the
same

Thanks,
~MuVen

[danakj, 2014-03-20 17:01:30]

This LG from my end. +aelias to verify this is something android team would
expect/want.

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

https://codereview.chromium.org/206153002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2319: // glow effect
nit: missing period

[aelias_OOO_until_Jul13, 2014-03-21 00:16:11]

I can repro the bug in content shell (not Chrome).  When flinging back and
forth, there can be a glow in the direction of the latest fling, even though
it's not touching the edge of the document.

From logging, I can see the problem is that there's some overscroll in the x
axis (getting us past the rounding check) and an epsilon of overscroll in the y
axis due to numerical error which later convinces OverscrollGlow to trigger the
fling glow in that direction.  The Rounded check only protects us against
epsilons in *both* directions.

Wiping unused_root_delta in the orthogonal axis is the wrong fix though, it
would break the case where two glows are active at a corner.  Let me propose an
an alternative fix:

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

https://codereview.chromium.org/206153002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2275: if (layer_impl ==
InnerViewportScrollLayer())
Replace this with:

    if (layer_impl == InnerViewportScrollLayer()) {
      unused_root_delta.Subtract(applied_delta);
      const float kOverscrollEpsilon = 0.01f;
      if (fabs(unused_root_delta.x()) < kOverscrollEpsilon)
        unused_root_delta.set_x(0.0f);
      if (fabs(unused_root_delta.y()) < kOverscrollEpsilon)
        unused_root_delta.set_y(0.0f);
    }

(I observe the error values get as high as 0.002f so 0.01f seems like a
reasonable epsilon value.)

https://codereview.chromium.org/206153002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2325: bool did_overscroll =
!gfx::ToRoundedVector2d(unused_root_delta).IsZero();
And this can be simplified to:

bool did_overscroll = !unused_root_delta.IsZero();

[MuVen, 2014-03-21 07:27:08]

On 2014/03/21 00:16:11, aelias wrote:
> I can repro the bug in content shell (not Chrome).  When flinging back and
> forth, there can be a glow in the direction of the latest fling, even though
> it's not touching the edge of the document.
> 
> From logging, I can see the problem is that there's some overscroll in the x
> axis (getting us past the rounding check) and an epsilon of overscroll in the
y
> axis due to numerical error which later convinces OverscrollGlow to trigger
the
> fling glow in that direction.  The Rounded check only protects us against
> epsilons in *both* directions.
> 
> Wiping unused_root_delta in the orthogonal axis is the wrong fix though, it
> would break the case where two glows are active at a corner.  Let me propose
an
> an alternative fix:
> 
>
https://codereview.chromium.org/206153002/diff/1/cc/trees/layer_tree_host_imp...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/206153002/diff/1/cc/trees/layer_tree_host_imp...
> cc/trees/layer_tree_host_impl.cc:2275: if (layer_impl ==
> InnerViewportScrollLayer())
> Replace this with:
> 
>     if (layer_impl == InnerViewportScrollLayer()) {
>       unused_root_delta.Subtract(applied_delta);
>       const float kOverscrollEpsilon = 0.01f;
>       if (fabs(unused_root_delta.x()) < kOverscrollEpsilon)
>         unused_root_delta.set_x(0.0f);
>       if (fabs(unused_root_delta.y()) < kOverscrollEpsilon)
>         unused_root_delta.set_y(0.0f);
>     }
> 
> (I observe the error values get as high as 0.002f so 0.01f seems like a
> reasonable epsilon value.)
> 
>
https://codereview.chromium.org/206153002/diff/1/cc/trees/layer_tree_host_imp...
> cc/trees/layer_tree_host_impl.cc:2325: bool did_overscroll =
> !gfx::ToRoundedVector2d(unused_root_delta).IsZero();
> And this can be simplified to:
> 
> bool did_overscroll = !unused_root_delta.IsZero();


Thanks for the alternative patch. With my proposed patch i have verified all the
glow use cases, Top, Left, Right, Bottom, Top-Left, Top-Right, Bottom-Right,
Bottom-Left all the use cases are working fine.

In the TOP-LEFT glow both did_scroll_X and did_scroll_Y will be zero in that
case it wont enter my patch and unusedroot delta is uneffected at the point , so
my patch works in all the corner cases even.

Thanks,
MuVen

[aelias_OOO_until_Jul13, 2014-03-21 09:39:23]

OK, fair enough that overscrolling while in a corner will work so my criticism
wasn't quite accurate, but the fact remains that your patch can sometimes delete
perfectly good information for no reason.  For example, I believe it would
suppress the horizontal overscroll glow if you diagonally scrolling towards the
top left while in the vertical center of a document.  I'd ask you to switch to
my proposal unless you see a problem with it.

[jdduke (slow), 2014-03-21 09:53:13]

On 2014/03/21 09:39:23, aelias wrote:
> OK, fair enough that overscrolling while in a corner will work so my criticism
> wasn't quite accurate, but the fact remains that your patch can sometimes
delete
> perfectly good information for no reason.  For example, I believe it would
> suppress the horizontal overscroll glow if you diagonally scrolling towards
the
> top left while in the vertical center of a document.  I'd ask you to switch to
> my proposal unless you see a problem with it.

Agreed, this introduces a difference in behavior from the stock Android
overscroll effect.  Scrolling and overscrolling axes should be more or less
independent, i.e., I should be able to continue overscrolling one axis while the
other axis is scrolling (as with the diagonal scroll aelias@ mentioned).

If possible, please switch to the solution proposed by aelias@. We might even
re-use the |move_threshold = 0.1f| constant to zero the unused_root_delta
values.

[MuVen, 2014-03-21 10:46:07]

Hi Aelias, jdduke,

Thank you for the suggestions on the proposed solution.

We can conclude that both the solutions are solving issue reported here.
Although, in particular, the diagonal case is not correctly handled with my
proposed patch. Thanks @aelias for referring to the use case and providing the
insights into the correct way of fixing it.

Although there are still some unnecessary calls to overscroll, these seem to be
required for a consistent user experience.

We could try to reduce those calls in another patch may be!

Thanks for for your valuable comments. I will post an updated patch with the
solution proposed by @aelias.

[MuVen, 2014-03-21 12:54:17]

Please take a look

[MuVen, 2014-03-21 13:01:59]

Please take a look at final patch submitted.

Thanks,

[MuVen, 2014-03-21 13:02:00]

Please take a look at final patch submitted.

Thanks,

[danakj, 2014-03-21 15:57:43]

Can we add a unit test for this change that would fail without it?

[danakj, 2014-03-21 15:58:27]

On Fri, Mar 21, 2014 at 11:57 AM, <danakj@chromium.org> wrote:

> Can we add a unit test for this change that would fail without it?
>

(Probably in layer_tree_host_impl_unittest.cc)


>
> https://codereview.chromium.org/206153002/
>

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

[MuVen, 2014-03-24 05:26:26]

Hi All,

As scrollby function returns wheather the scroll is done or not , if i write a
test case for scrollby in the working case and not working case(showing glow
during fast scroll in wiki page) scrollby return TRUE.

I have to write TESTCASE based on bool did_overscroll variable but this variable
is not returned.

Kindly let me know how to go on this.

Thanks,
~muven

[Sikugu_, 2014-03-24 06:12:46]

On 2014/03/24 05:26:26, muven wrote:
> Hi All,
> 
> As scrollby function returns wheather the scroll is done or not , if i write a
> test case for scrollby in the working case and not working case(showing glow
> during fast scroll in wiki page) scrollby return TRUE.
> 
> I have to write TESTCASE based on bool did_overscroll variable but this
variable
> is not returned.
> 
> Kindly let me know how to go on this.
> 
> Thanks,
> ~muven

Praveen,

I think , you don't have to write any separate test as you have just changed the
logic
in the api(scrollBy) which is already tested in
layer_tree_host_impl_unittest.cc.

Please make sure all the tests using this api are getting passed.

[MuVen, 2014-03-24 10:34:04]

Hi All,

Please find the attached cc_unittest report.

[==========] 2303 tests from 473 test cases ran. (14973 ms total)
[  PASSED  ] 2303 tests.

  YOU HAVE 4 DISABLED TESTS

C   39s Main 
********************************************************************************
C   39s Main  Summary
C   39s Main 
********************************************************************************
C   39s Main  ALL (2303 tests)
C   39s Main  PASS (2303 tests)
C   39s Main  FAIL (0 tests): []
C   39s Main  CRASH (0 tests): []
C   39s Main  TIMEOUT (0 tests): []
C   39s Main  UNKNOWN (0 tests): []
C   39s Main 
********************************************************************************


kindly let me know what needs to be done further.

Thanks,
muven.

[MuVen, 2014-03-25 05:03:42]

ping ?

[aelias_OOO_until_Jul13, 2014-03-25 05:04:52]

Dana's request was to write a new cc_unittest covering the bug just fixed.

[aelias_OOO_until_Jul13, 2014-03-25 05:08:12]

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

https://codereview.chromium.org/206153002/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2323: bool did_overscroll =
!gfx::ToRoundedVector2d(unused_root_delta).IsZero();
Please remove the ToRoundedVector2d call here as it's now redundant.

[MuVen, 2014-03-25 05:09:17]

On 2014/03/25 05:04:52, aelias wrote:
> Dana's request was to write a new cc_unittest covering the bug just fixed.

Thanks for the reply aelias,

But as i explained earlier for this issue the test case should check for
did_overscroll variable during  fast scroll? 

how to go on this ?

Thanks,

[MuVen, 2014-03-25 05:18:48]

As per review comments i have removed the redundant roundedvector2d call.

Please take a look.

Thanks,
muven

[MuVen, 2014-03-25 05:25:47]

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

[MuVen, 2014-03-25 05:25:50]

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

[aelias_OOO_until_Jul13, 2014-03-25 05:47:36]

On 2014/03/25 05:09:17, muven wrote:
> On 2014/03/25 05:04:52, aelias wrote:
> > Dana's request was to write a new cc_unittest covering the bug just fixed.
> 
> Thanks for the reply aelias,
> 
> But as i explained earlier for this issue the test case should check for
> did_overscroll variable during  fast scroll? 
> 
> how to go on this ?
> 
> Thanks,

I'd suggest using a subclass of InputHandlerClient counting calls to
DidOverscroll.  cc/trees/layer_tree_host_unittest_scroll.cc 's
ThreadCheckingInputHandlerClient has an example of that.

[MuVen, 2014-03-25 14:44:49]

Hi,

Test Case has been added.

Please take a look.

Thanks,
muven

[danakj, 2014-03-25 15:36:57]

Thanks for the test. LGTM w/ a couple comments.

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

https://codereview.chromium.org/206153002/diff/90001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:3166: // When we scroll down and
scroll up unnecessary GlowEffect calls are called.
"are called" -> "should not be called"

https://codereview.chromium.org/206153002/diff/90001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:3190: EXPECT_EQ(gfx::Vector2dF(),
host_impl_->accumulated_root_overscroll());
can you compare EXPECT_EQ(gfx::Vector2dF().ToString(),
host_impl_->accumulated_root_overscroll().ToString()) instead? This way when it
fails we get nice text output.

https://codereview.chromium.org/206153002/diff/90001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:3192: EXPECT_EQ(gfx::Vector2dF(),
host_impl_->accumulated_root_overscroll());
same

[aelias_OOO_until_Jul13, 2014-03-25 18:22:23]

lgtm.  I updated your patch description to reflect the latest approach.

[MuVen, 2014-03-26 05:07:27]

Hi danakj, aelias,

I have added the proper testcase name & comments.

And added changes as per review comments.

Thanks,
muven.

[aelias_OOO_until_Jul13, 2014-03-26 05:10:17]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-03-26 05:10:38]

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

[commit-bot: I haz the power, 2014-03-26 05:44:04]

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

[commit-bot: I haz the power, 2014-03-26 05:44:04]

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

[aelias_OOO_until_Jul13, 2014-03-26 06:08:05]

https://codereview.chromium.org/206153002/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/206153002/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3194:
host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0, -2.30));
You need to change this to -2.30f to fix a Windows compile warning.

[MuVen, 2014-03-26 06:30:14]

On 2014/03/26 06:08:05, aelias wrote:
>
https://codereview.chromium.org/206153002/diff/160001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/206153002/diff/160001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl_unittest.cc:3194:
> host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0, -2.30));
> You need to change this to -2.30f to fix a Windows compile warning.

Done !!!

[aelias_OOO_until_Jul13, 2014-03-26 06:31:35]

OK, you can click the "Commit" button yourself after receiving lgtm.

[MuVen, 2014-03-26 06:42:08]

On 2014/03/26 06:31:35, aelias wrote:
> OK, you can click the "Commit" button yourself after receiving lgtm.

OK, you can click the "Commit" button yourself "after receiving lgtm" ? Will i
need to get another LGTM ?

[MuVen, 2014-03-26 06:42:10]

On 2014/03/26 06:31:35, aelias wrote:
> OK, you can click the "Commit" button yourself after receiving lgtm.

OK, you can click the "Commit" button yourself "after receiving lgtm" ? Will i
need to get another LGTM ?

[aelias_OOO_until_Jul13, 2014-03-26 06:45:16]

You don't need any further approval, see the process a
http://dev.chromium.org/developers/testing/commit-queue

[MuVen, 2014-03-26 06:50:20]

On 2014/03/26 06:45:16, aelias wrote:
> You don't need any further approval, see the process a
> http://dev.chromium.org/developers/testing/commit-queue

ok !!!

Thanks(aelias & danakj) for your patient support & valuable suggestions which
helped me to submit my first opensource patch.
 
Looking forward to work with you.

Thanks,
muven.

[danakj, 2014-03-26 13:46:10]

On Wed, Mar 26, 2014 at 2:50 AM, <sataya.m@samsung.com> wrote:

> On 2014/03/26 06:45:16, aelias wrote:
>
>> You don't need any further approval, see the process a
>> http://dev.chromium.org/developers/testing/commit-queue
>>
>
> ok !!!
>
> Thanks(aelias & danakj) for your patient support & valuable suggestions
> which
> helped me to submit my first opensource patch.
>
> Looking forward to work with you.
>

cheers :)


>
> Thanks,
> muven.
>
> https://codereview.chromium.org/206153002/
>

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

[aelias_OOO_until_Jul13, 2014-03-26 18:06:00]

The CQ bit was checked by aelias@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:06:54]

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

[commit-bot: I haz the power, 2014-03-26 22:44:46]

Change committed as 259729