NTP: handle scroll position changes that are due to layout.

NTP: handle scroll position changes that are due to layout.

When the device is rotated the RecyclerView may change its scroll
position during layout without broadcasting a scroll event. Naughty.

BUG=701269
Review-Url: https://codereview.chromium.org/2758833002
Cr-Commit-Position: refs/heads/master@{#457821}
Committed: https://chromium.googlesource.com/chromium/src/+/e42e82d24eb1e150007055d0ba8ec641367114a9

[Michael van Ouwerkerk, 2017-03-17 16:23:33]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 16:23:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael van Ouwerkerk, 2017-03-17 16:23:53]

mvanouwerkerk@chromium.org changed reviewers:
+ dgn@chromium.org

[Michael van Ouwerkerk, 2017-03-17 16:23:53]

Nicolas, could you take a look please?

[dgn, 2017-03-17 16:49:28]

lgtm

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:357:
private void initializeLayoutChangeListeners() {
nit: should that have TraceEvent like the other intializeFoo methods?

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:387:
int scrollY = mRecyclerView.computeVerticalScrollOffset();
nit: can't the whole thing move into handleScroll? I imagine that if the
recyclerview notifies of scroll event mLastScrollY != scrollY will be true
anyway

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:390:
handleScroll();
nit: setupScrollhandling runs after initializeLayoutChangeListeners, so the snap
scroll runnable might be initialised yet, if for some reason (tbh I don't see
that happening, but just sayin') the listener is called synchronously. 

Initialise the runnable in the constructor and make it final? It could be an
inner class to keep the constructor clean.

[commit-bot: I haz the power, 2017-03-17 16:55:45]

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

[commit-bot: I haz the power, 2017-03-17 16:55:46]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2017-03-17 17:46:56]

Thanks for the quick review!

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:357:
private void initializeLayoutChangeListeners() {
On 2017/03/17 16:49:27, dgn wrote:
> nit: should that have TraceEvent like the other intializeFoo methods?

Done.

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:387:
int scrollY = mRecyclerView.computeVerticalScrollOffset();
On 2017/03/17 16:49:27, dgn wrote:
> nit: can't the whole thing move into handleScroll? I imagine that if the
> recyclerview notifies of scroll event mLastScrollY != scrollY will be true
> anyway

This avoids calling handleScroll unnecessarily, otherwise it would run each time
the layout changes. It's called fairly often as is, during scrolling, so I'd
like to keep it small.

https://codereview.chromium.org/2758833002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:390:
handleScroll();
On 2017/03/17 16:49:27, dgn wrote:
> nit: setupScrollhandling runs after initializeLayoutChangeListeners, so the
snap
> scroll runnable might be initialised yet, if for some reason (tbh I don't see
> that happening, but just sayin') the listener is called synchronously. 
> 
> Initialise the runnable in the constructor and make it final? It could be an
> inner class to keep the constructor clean.

Done.

[Michael van Ouwerkerk, 2017-03-17 17:47:01]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2017-03-17 17:47:02]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org
Link to the patchset: https://codereview.chromium.org/2758833002/#ps20001
(title: "Address review comments from dgn.")

[commit-bot: I haz the power, 2017-03-17 17:47:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 18:26:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489772821911510,
"parent_rev": "00d67ef0786f6a28b6557e6224ddce2104ce8207", "commit_rev":
"e42e82d24eb1e150007055d0ba8ec641367114a9"}

[commit-bot: I haz the power, 2017-03-17 18:27:03]

Description was changed from

==========
NTP: handle scroll position changes that are due to layout.

When the device is rotated the RecyclerView may change its scroll
position during layout without broadcasting a scroll event. Naughty.

BUG=701269
==========

to

==========
NTP: handle scroll position changes that are due to layout.

When the device is rotated the RecyclerView may change its scroll
position during layout without broadcasting a scroll event. Naughty.

BUG=701269

Review-Url: https://codereview.chromium.org/2758833002
Cr-Commit-Position: refs/heads/master@{#457821}
Committed:
https://chromium.googlesource.com/chromium/src/+/e42e82d24eb1e150007055d0ba8e...
==========

[commit-bot: I haz the power, 2017-03-17 18:27:04]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e42e82d24eb1e150007055d0ba8e...