Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(349)

Issue 661643004: [android] Remove unused scroll delta in TopControls (Closed)

Created:
6 years, 2 months ago by sujith
Modified:
6 years, 1 month ago
CC:
cc-bugs_chromium.org, chromium-reviews, MuVen, sivag, sohanjg
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 #

Patch Set 2 : adding my name to authors lists #

Total comments: 2

Patch Set 3 : #

Total comments: 2

Patch Set 4 : Unittest Added #

Total comments: 2

Patch Set 5 : "updated ShouldTopControlsConsumeScroll" #

Patch Set 6 : Changed ShouldTopControlsConsumeScroll to satisfy TopControlsViewportOffsetClamping unittest #

Patch Set 7 : "Updated the unittest" #

Patch Set 8 : #

Total comments: 1

Patch Set 9 : "Updated the Unittest" #

Total comments: 5

Patch Set 10 : "Unittest updated" #

Patch Set 11 : "Rebase the source" #

Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -5 lines) Patch
M AUTHORS View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -5 lines 0 comments Download
M cc/trees/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +70 lines, -0 lines 0 comments Download

Messages

Total messages: 46 (13 generated)
sujith
PTAL
6 years, 2 months ago (2014-10-16 08:54:02 UTC) #2
MuVen
Sujith, Please add test for this, as this involves behaviour changes.
6 years, 2 months ago (2014-10-16 11:01:28 UTC) #3
sujith
PTAL
6 years, 2 months ago (2014-10-16 15:13:20 UTC) #5
jdduke (slow)
https://codereview.chromium.org/661643004/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/661643004/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode2665 cc/trees/layer_tree_host_impl.cc:2665: if (consume_by_top_controls && unused_root_delta.y() > 0.0f) Can you think ...
6 years, 2 months ago (2014-10-16 21:29:17 UTC) #6
sujith
PTAL https://codereview.chromium.org/661643004/diff/20001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/661643004/diff/20001/cc/trees/layer_tree_host_impl.cc#newcode2665 cc/trees/layer_tree_host_impl.cc:2665: if (consume_by_top_controls && unused_root_delta.y() > 0.0f) On 2014/10/16 ...
6 years, 2 months ago (2014-10-17 14:51:05 UTC) #7
jdduke (slow)
Thanks, please add a test in LayerTreeHostImplWithTopControlsTest that ensures the controls immediately start hiding when ...
6 years, 2 months ago (2014-10-21 16:09:11 UTC) #8
sujith
PTAL https://codereview.chromium.org/661643004/diff/40001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/661643004/diff/40001/cc/trees/layer_tree_host_impl.cc#newcode2588 cc/trees/layer_tree_host_impl.cc:2588: if (scroll_delta.y() > 0 && top_controls_manager_->ContentTopOffset() == 0 ...
6 years, 2 months ago (2014-10-22 14:20:15 UTC) #9
aelias_OOO_until_Jul13
https://codereview.chromium.org/661643004/diff/60001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/661643004/diff/60001/cc/trees/layer_tree_host_impl.cc#newcode2590 cc/trees/layer_tree_host_impl.cc:2590: (InnerViewportScrollLayer()->TotalScrollOffset().y() == This should check OuterViewportScrollLayer as well, and ...
6 years, 2 months ago (2014-10-22 18:40:47 UTC) #10
sujith
PTAL https://codereview.chromium.org/661643004/diff/60001/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/661643004/diff/60001/cc/trees/layer_tree_host_impl.cc#newcode2590 cc/trees/layer_tree_host_impl.cc:2590: (InnerViewportScrollLayer()->TotalScrollOffset().y() == On 2014/10/22 18:40:47, aelias wrote: > ...
6 years, 2 months ago (2014-10-23 06:57:11 UTC) #11
aelias_OOO_until_Jul13
lgtm
6 years, 2 months ago (2014-10-24 01:26:08 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661643004/80001
6 years, 2 months ago (2014-10-24 08:08:35 UTC) #16
commit-bot: I haz the power
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_chromeos_rel/builds/4735) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel/builds/8795)
6 years, 2 months ago (2014-10-24 08:57:18 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661643004/80001
6 years, 2 months ago (2014-10-24 10:30:01 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661643004/80001
6 years, 2 months ago (2014-10-24 10:30:59 UTC) #22
commit-bot: I haz the power
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_chromeos_rel/builds/4765)
6 years, 2 months ago (2014-10-24 11:47:55 UTC) #24
sujith
@aellas,@jdduke, thanks for reviewing the patch, i have updated the ShouldTopControlsConsumeScroll to satisfy TopControlsViewportOffsetClamping unittest. ...
6 years, 1 month ago (2014-10-28 10:55:24 UTC) #25
sujith
friendly ping
6 years, 1 month ago (2014-10-30 15:42:11 UTC) #26
aelias_OOO_until_Jul13
Could you explain what the problem was that caused the test to fail and why ...
6 years, 1 month ago (2014-10-30 20:40:59 UTC) #27
sujith
In TopControlsViewportOffsetClamping, its failed because when TopControls are visible and TopMaxScrollOffset and TopScrollOffset are equal ...
6 years, 1 month ago (2014-10-31 09:11:43 UTC) #28
aelias_OOO_until_Jul13
I think the test expectations should be changed instead of adding that extra if statement. ...
6 years, 1 month ago (2014-10-31 19:16:54 UTC) #29
sujith
PTAL
6 years, 1 month ago (2014-11-01 13:03:54 UTC) #30
aelias_OOO_until_Jul13
https://codereview.chromium.org/661643004/diff/140001/cc/trees/layer_tree_host_impl_unittest.cc File cc/trees/layer_tree_host_impl_unittest.cc (left): https://codereview.chromium.org/661643004/diff/140001/cc/trees/layer_tree_host_impl_unittest.cc#oldcode2583 cc/trees/layer_tree_host_impl_unittest.cc:2583: gfx::Vector2dF scroll_delta(0.f, 25.f); You're making this test no longer ...
6 years, 1 month ago (2014-11-04 06:07:55 UTC) #31
sujith
PTAL https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_host_impl_unittest.cc File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/661643004/diff/160001/cc/trees/layer_tree_host_impl_unittest.cc#newcode2611 cc/trees/layer_tree_host_impl_unittest.cc:2611: EXPECT_EQ(viewport_offset + gfx::ScrollOffset(0.f, -25.f), Since the +ve scroll ...
6 years, 1 month ago (2014-11-06 09:28:00 UTC) #32
bokan
Sorry to jump in late, but if I'm understanding this correctly, the current changes to ...
6 years, 1 month ago (2014-11-06 16:14:57 UTC) #34
sujith
@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_host_impl_unittest.cc ...
6 years, 1 month ago (2014-11-07 05:06:40 UTC) #35
bokan
On 2014/11/07 05:06:40, sujith wrote: > @aelias, @bokan, @jdduke thanks for the valuable inputs. > ...
6 years, 1 month ago (2014-11-07 05:19:19 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661643004/180001
6 years, 1 month ago (2014-11-07 05:35:48 UTC) #38
commit-bot: I haz the power
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_gn_rel/builds/32067)
6 years, 1 month ago (2014-11-07 05:43:58 UTC) #40
sujith
Rebase the source to avoid the build error. PTAL
6 years, 1 month ago (2014-11-07 09:40:54 UTC) #41
aelias_OOO_until_Jul13
lgtm
6 years, 1 month ago (2014-11-07 22:34:23 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661643004/200001
6 years, 1 month ago (2014-11-08 02:13:25 UTC) #44
commit-bot: I haz the power
Committed patchset #11 (id:200001)
6 years, 1 month ago (2014-11-08 03:04:15 UTC) #45
commit-bot: I haz the power
6 years, 1 month ago (2014-11-08 03:05:04 UTC) #46
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/344435e974a2f4322735f04a0a3118aa262a6910
Cr-Commit-Position: refs/heads/master@{#303367}

Powered by Google App Engine
This is Rietveld 408576698