Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2708653002:
[Android NTP] Allow overlapping snap scroll regions. (Closed)
|
Created:
3 years, 10 months ago by Bernhard Bauer Modified:
3 years, 10 months ago CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android NTP] Allow overlapping snap scroll regions.
Previously the code assumed that snap scroll regions would never overlap.
That assumption became false with the condensed NTP layout, with the result that
only the first snap would be applied and the NTP would come to rest in a
position it should have scrolled out of.
We now apply all snaps in order and have an assertion that the desired position
does not change after one pass.
BUG=693095
Review-Url: https://codereview.chromium.org/2708653002
Cr-Commit-Position: refs/heads/master@{#451767}
Committed: https://chromium.googlesource.com/chromium/src/+/6617e48c6e0733436a8fc430a17399038975926e
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 11
Patch Set 3 : test #
Total comments: 2
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bauerb@chromium.org changed reviewers: + mvanouwerkerk@chromium.org, peconn@chromium.org
Please review.
lgtm with nits https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:448: * @param flipPoint the threshold used to decide which bound of the region to scroll to. nit: flipPoint before regionEnd https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:451: private int scrollOutOfRegion( This method no longer performs a scroll, it calculates a new position (in pixels?). Maybe rename to something like "calculateSnapScrollPosition" https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:468: private int scrollOutOfRegion(int currentScroll, int regionStart, int regionEnd) { Please also update this method name and documentation. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:483: assert scrollTo == calculateSnapPosition(scrollTo, fakeBox, parentHeight); This assert is starting to feel more like a unit test. Generally snap scrolling has poor coverage. Would you mind filing a bug or adding a TODO for adding test coverage?
https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:451: private int scrollOutOfRegion( This can now be static. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:488: private int calculateSnapPosition(int scrollPosition, View fakeBox, int parentHeight) { Now that you've separated the function into "calculate result" (calculateSnapPosition" and "use result" (snapScroll) things should be more easily testable - could you add a few tests for calculateSnapPosition?
The CQ bit was checked by bauerb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tests added, PTAL. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:448: * @param flipPoint the threshold used to decide which bound of the region to scroll to. On 2017/02/20 13:44:28, Michael van Ouwerkerk wrote: > nit: flipPoint before regionEnd I switched the order of the parameters around, so the position stays the same in the overloads. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:451: private int scrollOutOfRegion( On 2017/02/21 09:55:03, PEConn wrote: > This can now be static. Done. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:451: private int scrollOutOfRegion( On 2017/02/20 13:44:28, Michael van Ouwerkerk wrote: > This method no longer performs a scroll, it calculates a new position (in > pixels?). Maybe rename to something like "calculateSnapScrollPosition" Done. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:483: assert scrollTo == calculateSnapPosition(scrollTo, fakeBox, parentHeight); On 2017/02/20 13:44:28, Michael van Ouwerkerk wrote: > This assert is starting to feel more like a unit test. Generally snap scrolling > has poor coverage. Would you mind filing a bug or adding a TODO for adding test > coverage? Done. https://codereview.chromium.org/2708653002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:488: private int calculateSnapPosition(int scrollPosition, View fakeBox, int parentHeight) { On 2017/02/21 09:55:03, PEConn wrote: > Now that you've separated the function into "calculate result" > (calculateSnapPosition" and "use result" (snapScroll) things should be more > easily testable - could you add a few tests for calculateSnapPosition? Done.
My comment isn't blocking - LGTM. https://codereview.chromium.org/2708653002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java (right): https://codereview.chromium.org/2708653002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:251: assertEquals(searchBoxTop - searchBoxTransitionLength - 1, It might increase readability to add convenience methods and change these lines to: assertNoSnapFrom(0) assertNoSnapFrom(toolbarHeight) assertSnapTo(toolbarHeight / 2 - 1, 0) But I'm not too fussed.
https://codereview.chromium.org/2708653002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java (right): https://codereview.chromium.org/2708653002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java:251: assertEquals(searchBoxTop - searchBoxTransitionLength - 1, On 2017/02/21 14:43:24, PEConn wrote: > It might increase readability to add convenience methods and change these lines > to: > > assertNoSnapFrom(0) > assertNoSnapFrom(toolbarHeight) > > assertSnapTo(toolbarHeight / 2 - 1, 0) > > But I'm not too fussed. The one thing I don't like about that is that it won't fail with a useful error message if the assertion happens in a different method -- assertEquals together with the line number gives the most information.
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2708653002/#ps40001 (title: "test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487691682050670,
"parent_rev": "e33ad59dcd763f847ce9152ba23578486b2f0ff4", "commit_rev":
"6617e48c6e0733436a8fc430a17399038975926e"}
Message was sent while issue was closed.
Description was changed from ========== [Android NTP] Allow overlapping snap scroll regions. Previously the code assumed that snap scroll regions would never overlap. That assumption became false with the condensed NTP layout, with the result that only the first snap would be applied and the NTP would come to rest in a position it should have scrolled out of. We now apply all snaps in order and have an assertion that the desired position does not change after one pass. BUG=693095 ========== to ========== [Android NTP] Allow overlapping snap scroll regions. Previously the code assumed that snap scroll regions would never overlap. That assumption became false with the condensed NTP layout, with the result that only the first snap would be applied and the NTP would come to rest in a position it should have scrolled out of. We now apply all snaps in order and have an assertion that the desired position does not change after one pass. BUG=693095 Review-Url: https://codereview.chromium.org/2708653002 Cr-Commit-Position: refs/heads/master@{#451767} Committed: https://chromium.googlesource.com/chromium/src/+/6617e48c6e0733436a8fc430a173... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6617e48c6e0733436a8fc430a173...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2705343003/ by aelias@chromium.org. The reason for reverting is: NewTabPageRecyclerViewTest#testSnapScrollCondensedLayout failing on tablets BUG=694851. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
