[android] Remove unused scroll delta in TopControls

[android] Remove unused scroll delta in TopControls

In TopControlsManager, current_scroll_delta_ is consuming scroll delta
which is not been used by either top controls or the root layer.

So removing the unused scroll delta from the top controls so that top
controls will be shown before the glow effect.

BUG=424068
Committed: https://crrev.com/344435e974a2f4322735f04a0a3118aa262a6910
Cr-Commit-Position: refs/heads/master@{#303367}

[sujith, 2014-10-16 08:16:49]

sujiths.s@samsung.com changed reviewers:
+ sataya.m@samsung.com, siva.gunturi@samsung.com, sohan.jyoti@samsung.com

[sujith, 2014-10-16 08:54:02]

PTAL

[MuVen, 2014-10-16 11:01:28]

Sujith, 

Please add test for this, as this involves behaviour changes.

[sujith, 2014-10-16 15:13:20]

sujiths.s@samsung.com changed reviewers:
+ aelias@chromium.org, danakj@chromium.org, jdduke@chromium.org
- sataya.m@samsung.com, siva.gunturi@samsung.com, sohan.jyoti@samsung.com

[sujith, 2014-10-16 15:13:20]

PTAL

[jdduke (slow), 2014-10-16 21:29:17]

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

https://codereview.chromium.org/661643004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2665: if (consume_by_top_controls &&
unused_root_delta.y() > 0.0f)
Can you think of a solution that limits how much LTHI needs to know about the
TopControlsManager internals? For example, what if we avoid feeding the
TopControlsManager the |pending_delta| above if the root layer is at the max
limit of its scrollable area and the delta is positive? That could be folded
into the |ShouldTopControlsConsumeScroll| query.

[sujith, 2014-10-17 14:51:05]

PTAL

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

https://codereview.chromium.org/661643004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2665: if (consume_by_top_controls &&
unused_root_delta.y() > 0.0f)
On 2014/10/16 21:29:17, jdduke (OOO until Oct 21) wrote:
> Can you think of a solution that limits how much LTHI needs to know about the
> TopControlsManager internals? For example, what if we avoid feeding the
> TopControlsManager the |pending_delta| above if the root layer is at the max
> limit of its scrollable area and the delta is positive? That could be folded
> into the |ShouldTopControlsConsumeScroll| query.

Done.

[jdduke (slow), 2014-10-21 16:09:11]

Thanks, please add a test in LayerTreeHostImplWithTopControlsTest that ensures
the controls immediately start hiding when transitioning from a positive
overscroll to negative scrolling.

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

https://codereview.chromium.org/661643004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2588: if (scroll_delta.y() > 0 &&
top_controls_manager_->ContentTopOffset() == 0 &&
This is probably worth a comment along the lines of:

// Avoid confusing top controls bookkeeping with positive overscroll deltas.

[sujith, 2014-10-22 14:20:15]

PTAL

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

https://codereview.chromium.org/661643004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2588: if (scroll_delta.y() > 0 &&
top_controls_manager_->ContentTopOffset() == 0 &&
On 2014/10/21 16:09:10, jdduke wrote:
> This is probably worth a comment along the lines of:
> 
> // Avoid confusing top controls bookkeeping with positive overscroll deltas.

Done.

[aelias_OOO_until_Jul13, 2014-10-22 18:40:47]

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

https://codereview.chromium.org/661643004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2590:
(InnerViewportScrollLayer()->TotalScrollOffset().y() ==
This should check OuterViewportScrollLayer as well, and be merged with the other
MaxScrollOffset checks below.  I also don't see the reason to look at
scroll_delta again nor ContentTopOffset.  Please rewrite these three if
statements as:

if (active_tree()->TotalScrollOffset().y() <
active_tree()->TotalMaxScrollOffset().y())
  return true;

return false;

[sujith, 2014-10-23 06:57:11]

PTAL

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

https://codereview.chromium.org/661643004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2590:
(InnerViewportScrollLayer()->TotalScrollOffset().y() ==
On 2014/10/22 18:40:47, aelias wrote:
> This should check OuterViewportScrollLayer as well, and be merged with the
other
> MaxScrollOffset checks below.  I also don't see the reason to look at
> scroll_delta again nor ContentTopOffset.  Please rewrite these three if
> statements as:
> 
> if (active_tree()->TotalScrollOffset().y() <
> active_tree()->TotalMaxScrollOffset().y())
>   return true;
> 
> return false;

Done.

[aelias_OOO_until_Jul13, 2014-10-24 01:26:08]

lgtm

[sujith, 2014-10-24 02:38:33]

The CQ bit was checked by sujiths.s@samsung.com

[sujith, 2014-10-24 02:38:53]

The CQ bit was unchecked by sujiths.s@samsung.com

[sujith, 2014-10-24 08:08:22]

The CQ bit was checked by sujiths.s@samsung.com

[commit-bot: I haz the power, 2014-10-24 08:08:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661643004/80001

[commit-bot: I haz the power, 2014-10-24 08:57:17]

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

[commit-bot: I haz the power, 2014-10-24 08:57:18]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sujith, 2014-10-24 10:29:10]

The CQ bit was checked by sujiths.s@samsung.com

[commit-bot: I haz the power, 2014-10-24 10:30:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661643004/80001

[sujith, 2014-10-24 10:30:46]

The CQ bit was checked by sujiths.s@samsung.com

[commit-bot: I haz the power, 2014-10-24 10:30:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661643004/80001

[commit-bot: I haz the power, 2014-10-24 11:47:50]

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

[commit-bot: I haz the power, 2014-10-24 11:47:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sujith, 2014-10-28 10:55:24]

@aellas,@jdduke, thanks for reviewing the patch, i have updated the
ShouldTopControlsConsumeScroll to satisfy TopControlsViewportOffsetClamping
unittest. 
PTAL

[sujith, 2014-10-30 15:42:11]

friendly ping

[aelias_OOO_until_Jul13, 2014-10-30 20:40:59]

Could you explain what the problem was that caused the test to fail and why this
is the appropriate fix?  I'm having trouble figuring out the principle behind
it.

[sujith, 2014-10-31 09:11:43]

In TopControlsViewportOffsetClamping, its failed because when
TopControls are visible and TopMaxScrollOffset and TopScrollOffset are 
equal and trying to Scroll the content. So there it expects to Scroll
the topControls with the amount scrolled. 
But with our previous condition consume_by_top_controls will be true if
TotalScrollOffset().y() < TotalMaxScrollOffset().y(), so it will be 
false and the topControls won't hide. So that unittest is failed.

So i try to add an extra condition if the topControls is visible and 
TotalMaxScrollOffset().y() had some scrollable content, then make
consume_by_top_controls as true. So we can satisfy both the unittest.

[aelias_OOO_until_Jul13, 2014-10-31 19:16:54]

I think the test expectations should be changed instead of adding that extra if
statement.  The new behavior is not to be able to hide the top controls when
fully scrolled to the bottom, and that's fine.  You can change the expectations
to reflect that.

[sujith, 2014-11-01 13:03:54]

PTAL

[aelias_OOO_until_Jul13, 2014-11-04 06:07:55]

https://codereview.chromium.org/661643004/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/661643004/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2583: gfx::Vector2dF scroll_delta(0.f,
25.f);
You're making this test no longer try to scroll against the scroll limit?  Then
there isn't anything unique about it anymore.  Why not keep this positive like
it was before and check that top controls didn't move?

[sujith, 2014-11-06 09:28:00]

PTAL

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

https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2611: EXPECT_EQ(viewport_offset +
gfx::ScrollOffset(0.f, -25.f),
Since the +ve scroll will not reflect, if we scroll again with the -ve
value then the viewport_offset value also need to be updated. So i have 
modified the viewport_offset with the -ve scroll Offset.

[bokan, 2014-11-06 16:14:56]

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

[bokan, 2014-11-06 16:14:57]

Sorry to jump in late, but if I'm understanding this correctly, the current
changes to the unit test defeat the purpose of that test, which is to make sure
that changes to the viewport size due to top controls showing/hiding when the
viewport is fully scrolled don't cause "jumps" in the viewport scroll offset.
See comments on how to fix it.

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

https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2590: EXPECT_EQ(0.f,
This is fine, since scrolling down at the max extents no longer hides the top
controls; however, after this check we should still hide the top controls so the
rest of the test remains relevant. I would do that either by scrolling the top
controls directly (host_impl_->TopControlsManager()->ScrollBy()) or using the
TopControlsManager::UpdateTopControlsState method. Then perhaps do a sanity
check (as before) that the top controls are actually hidden.

https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2611: EXPECT_EQ(viewport_offset +
gfx::ScrollOffset(0.f, -25.f),
On 2014/11/06 09:28:00, sujith wrote:
> Since the +ve scroll will not reflect, if we scroll again with the -ve
> value then the viewport_offset value also need to be updated. So i have 
> modified the viewport_offset with the -ve scroll Offset.

If you hide the top controls as mentioned in my comment above, you should no
longer need to change this.

[sujith, 2014-11-07 05:06:40]

@aelias, @bokan, @jdduke thanks for the valuable inputs.
I have updated the unittest 
PTAL

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

https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2590: EXPECT_EQ(0.f,
On 2014/11/06 16:14:57, bokan wrote:
> This is fine, since scrolling down at the max extents no longer hides the top
> controls; however, after this check we should still hide the top controls so
the
> rest of the test remains relevant. I would do that either by scrolling the top
> controls directly (host_impl_->TopControlsManager()->ScrollBy()) or using the
> TopControlsManager::UpdateTopControlsState method. Then perhaps do a sanity
> check (as before) that the top controls are actually hidden. 

Done.

https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:2611: EXPECT_EQ(viewport_offset +
gfx::ScrollOffset(0.f, -25.f),
On 2014/11/06 16:14:57, bokan wrote:
> On 2014/11/06 09:28:00, sujith wrote:
> > Since the +ve scroll will not reflect, if we scroll again with the -ve
> > value then the viewport_offset value also need to be updated. So i have 
> > modified the viewport_offset with the -ve scroll Offset.
> 
> If you hide the top controls as mentioned in my comment above, you should no
> longer need to change this.

Done.

[bokan, 2014-11-07 05:19:19]

On 2014/11/07 05:06:40, sujith wrote:
> @aelias, @bokan, @jdduke thanks for the valuable inputs.
> I have updated the unittest 
> PTAL
> 
>
https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl_unittest.cc:2590: EXPECT_EQ(0.f,
> On 2014/11/06 16:14:57, bokan wrote:
> > This is fine, since scrolling down at the max extents no longer hides the
top
> > controls; however, after this check we should still hide the top controls so
> the
> > rest of the test remains relevant. I would do that either by scrolling the
top
> > controls directly (host_impl_->TopControlsManager()->ScrollBy()) or using
the
> > TopControlsManager::UpdateTopControlsState method. Then perhaps do a sanity
> > check (as before) that the top controls are actually hidden. 
> 
> Done.
> 
>
https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl_unittest.cc:2611: EXPECT_EQ(viewport_offset +
> gfx::ScrollOffset(0.f, -25.f),
> On 2014/11/06 16:14:57, bokan wrote:
> > On 2014/11/06 09:28:00, sujith wrote:
> > > Since the +ve scroll will not reflect, if we scroll again with the -ve
> > > value then the viewport_offset value also need to be updated. So i have 
> > > modified the viewport_offset with the -ve scroll Offset.
> > 
> > If you hide the top controls as mentioned in my comment above, you should no
> > longer need to change this.
> 
> Done.

Thanks, lgtm. (not an owner though)

[sujith, 2014-11-07 05:34:25]

The CQ bit was checked by sujiths.s@samsung.com

[commit-bot: I haz the power, 2014-11-07 05:35:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661643004/180001

[commit-bot: I haz the power, 2014-11-07 05:43:57]

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

[commit-bot: I haz the power, 2014-11-07 05:43:58]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sujith, 2014-11-07 09:40:54]

Rebase the source to avoid the build error.
PTAL

[aelias_OOO_until_Jul13, 2014-11-07 22:34:23]

lgtm

[sujith, 2014-11-08 02:12:14]

The CQ bit was checked by sujiths.s@samsung.com

[commit-bot: I haz the power, 2014-11-08 02:13:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661643004/200001

[commit-bot: I haz the power, 2014-11-08 03:04:15]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-11-08 03:05:04]

Patchset 11 (id:??) landed as
https://crrev.com/344435e974a2f4322735f04a0a3118aa262a6910
Cr-Commit-Position: refs/heads/master@{#303367}