Refine snap scrolling on the Cards New Tab Page.

Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

In addition, it disables the search logo fading out on scroll.

Because the scrolling behaviour has changed, the following user actions are obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
Committed: https://crrev.com/bb72610a2c82054430648c6c15aa9ff7159cdb55
Cr-Commit-Position: refs/heads/master@{#403144}

[PEConn, 2016-06-28 12:58:54]

Patchset #1 (id:1) has been deleted

[PEConn, 2016-06-28 13:00:17]

peconn@chromium.org changed reviewers:
+ bauerb@chromium.org, mastiz@chromium.org

[PEConn, 2016-06-28 13:00:17]

PTAL.

[PEConn, 2016-06-28 13:04:38]

Description was changed from

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

Because the scrolling behaviour has changed, it will effect the following user
actions:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

to

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

[PEConn, 2016-06-28 13:07:16]

peconn@chromium.org changed reviewers:
+ asvitkine@chromium.org

[PEConn, 2016-06-28 13:07:16]

@asvitkine, please review the actions.xml changes.

[mastiz, 2016-06-28 13:13:57]

Please update histograms.xml and mark label 6 of SnippetsInteractions as
obsolete.

https://codereview.chromium.org/2106753002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2106753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:89:
public static final int SNIPPETS_ACTION_SCROLLED_BELOW_THE_FOLD_ONCE = 6;
Should we rename all relevant constants above and perhaps prefix with OBSOLETE_
or DEPRECATED_? Not sure what the convention is in other places.

[PEConn, 2016-06-28 13:26:44]

https://codereview.chromium.org/2106753002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java
(right):

https://codereview.chromium.org/2106753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:89:
public static final int SNIPPETS_ACTION_SCROLLED_BELOW_THE_FOLD_ONCE = 6;
On 2016/06/28 13:13:56, mastiz wrote:
> Should we rename all relevant constants above and perhaps prefix with
OBSOLETE_
> or DEPRECATED_? Not sure what the convention is in other places.

Acknowledged.

[PEConn, 2016-06-28 13:26:54]

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

[commit-bot: I haz the power, 2016-06-28 13:27:09]

Dry run: 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, 2016-06-28 13:29:15]

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

[commit-bot: I haz the power, 2016-06-28 13:29:16]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[PEConn, 2016-06-28 13:35:08]

@asvitkine, could you please review both the histograms.xml and actions.xml
changes.

[Bernhard Bauer, 2016-06-28 14:00:17]

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

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482:
private void snapScroll() {
Should this whole thing move to the RecyclerView?

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:486:
scrollOutOfRegion(mSearchBoxView.getTop() - searchBoxTransitionLength,
Do we actually want to have two calls to scrollOutOfRegion?

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:500:
// Finally, we get |A + B - C + B| because the transition starts from the
|A + B - C + D|?

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:753:
// mSearchProviderLogoView.setAlpha(searchUiAlpha);
?

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:200:
return getChildAt(2).getTop();
Okay, I know all of that stuff is only temporarily hardcoded until we support
multiple sections, but could we at least pull this value out to a constant?

Actually, I don't think getChildAt() is the right method to use, as that's going
to be the third current child of the RecyclerView, regardless of what view that
is. Couldn't we just use findFirstCard().itemView?

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:881:
final float growthPercent = Math.min(1f, 1f - mUrlExpansionPercent);
|mUrlExpansionPercent| is always going to be between 1 and 0, so you don't need
the extra check.

I'm somewhat worried about doing this even in the pre-cards UI though, because
on its own this will look weird. Should we put that behind the snippets flag? We
could access it through NewTabPage so we wouldn't have to directly check the
flags here.

[PEConn, 2016-06-28 14:08:20]

Description was changed from

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

to

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

In addition, it disables the search logo fading out on scroll.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

[PEConn, 2016-06-28 14:32:51]

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

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482:
private void snapScroll() {
On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> Should this whole thing move to the RecyclerView?

I think scrollOutOfRegion should be moved into the RecyclerView. I'm not too
sure about snapScroll since it depends on a few things (the position of the
search box, the ntp height and the ntp current scroll).

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:486:
scrollOutOfRegion(mSearchBoxView.getTop() - searchBoxTransitionLength,
On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> Do we actually want to have two calls to scrollOutOfRegion?

I've ensured that only one should be called.

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:500:
// Finally, we get |A + B - C + B| because the transition starts from the
On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> |A + B - C + D|?

Done.

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:753:
// mSearchProviderLogoView.setAlpha(searchUiAlpha);
On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> ?

Ah, sorry.

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:200:
return getChildAt(2).getTop();
On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> Okay, I know all of that stuff is only temporarily hardcoded until we support
> multiple sections, but could we at least pull this value out to a constant?
> 
> Actually, I don't think getChildAt() is the right method to use, as that's
going
> to be the third current child of the RecyclerView, regardless of what view
that
> is. Couldn't we just use findFirstCard().itemView?

Done.

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:881:
final float growthPercent = Math.min(1f, 1f - mUrlExpansionPercent);
On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> |mUrlExpansionPercent| is always going to be between 1 and 0, so you don't
need
> the extra check.
> 
> I'm somewhat worried about doing this even in the pre-cards UI though, because
> on its own this will look weird. Should we put that behind the snippets flag?
We
> could access it through NewTabPage so we wouldn't have to directly check the
> flags here.

Done.

[PEConn, 2016-06-28 14:33:00]

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

[commit-bot: I haz the power, 2016-06-28 14:33:41]

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

[Bernhard Bauer, 2016-06-28 14:44:00]

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

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482:
private void snapScroll() {
On 2016/06/28 14:32:51, PEConn1 wrote:
> On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> > Should this whole thing move to the RecyclerView?
> 
> I think scrollOutOfRegion should be moved into the RecyclerView. I'm not too
> sure about snapScroll since it depends on a few things (the position of the
> search box, the ntp height and the ntp current scroll).

Dunno... we could just pass them in. I think overall moving the code out of this
class outweighs the overhead of having to pass these as parameters.

https://codereview.chromium.org/2106753002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(left):

https://codereview.chromium.org/2106753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:692:
mSearchProviderLogoView.setAlpha(searchUiAlpha);
Can we also guard this with the cardsUI check?

[Alexei Svitkine (slow), 2016-06-28 14:47:57]

lgtm

[PEConn, 2016-06-28 15:39:04]

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

https://codereview.chromium.org/2106753002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482:
private void snapScroll() {
On 2016/06/28 14:44:00, Bernhard Bauer wrote:
> On 2016/06/28 14:32:51, PEConn1 wrote:
> > On 2016/06/28 14:00:17, Bernhard Bauer wrote:
> > > Should this whole thing move to the RecyclerView?
> > 
> > I think scrollOutOfRegion should be moved into the RecyclerView. I'm not too
> > sure about snapScroll since it depends on a few things (the position of the
> > search box, the ntp height and the ntp current scroll).
> 
> Dunno... we could just pass them in. I think overall moving the code out of
this
> class outweighs the overhead of having to pass these as parameters.

Done.

https://codereview.chromium.org/2106753002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(left):

https://codereview.chromium.org/2106753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:692:
mSearchProviderLogoView.setAlpha(searchUiAlpha);
On 2016/06/28 14:44:00, Bernhard Bauer wrote:
> Can we also guard this with the cardsUI check?

Done.

[PEConn, 2016-06-28 15:50:59]

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

[commit-bot: I haz the power, 2016-06-28 15:51:21]

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

[Bernhard Bauer, 2016-06-28 17:02:34]

bauerb@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Bernhard Bauer, 2016-06-28 17:02:35]

Nice! LGTM, but we might still want to get Ted's approval for the Toolbar
changes.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:413:
} else if (mUseCardsUi && !mRecyclerView.isFirstItemVisible()) {
Should we merge the mUseCardsUI check? I'm thinking we could put the
!mUseCardsUI case first and then these two.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:265:
public boolean scrollOutOfRegion(int regionStart, int flipPoint, int regionEnd)
{
These could now be made private.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:313:
int start =  getPeekingCardTop()  // A.
Nit: One superfluous space before getPeekingCardTop().

[commit-bot: I haz the power, 2016-06-28 18:43:13]

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

[commit-bot: I haz the power, 2016-06-28 18:43:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ted C, 2016-06-29 04:13:30]

code lgtm

Personally, I find landscape transition to be quite jarring in the video.  What
is the reasoning behind the limiting of the growth?  It isn't described in the
bug other than "we want to do X", but it'd be nice to know why. I do think
having the starting point of the growth be determined by the amount it needs to
grow would make this look nicer (portrait wouldn't be affected but landscape
would increase earlier).

[mastiz, 2016-06-29 09:15:16]

lgtm

[PEConn, 2016-06-29 10:17:35]

On 2016/06/29 04:13:30, Ted C wrote:
> code lgtm
> 
> Personally, I find landscape transition to be quite jarring in the video. 
What
> is the reasoning behind the limiting of the growth?  It isn't described in the
> bug other than "we want to do X", but it'd be nice to know why. I do think
> having the starting point of the growth be determined by the amount it needs
to
> grow would make this look nicer (portrait wouldn't be affected but landscape
> would increase earlier).

I added a comment to the bug to fill in a bit of the logic.

I don't mind changing the transition start point in a future CL, but I'd want to
get an OK from the UI guys for it.

[mcwilliams, 2016-06-29 11:04:29]

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

[mcwilliams, 2016-06-29 11:04:32]

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:408:
float percentage;
Can we rename this variable to something a little more descriptive in what it is
used for

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:460:

Remove new lines

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:205:
}
Any chance we can get these from the CardViewHolder

[PEConn, 2016-06-29 11:07:36]

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:413:
} else if (mUseCardsUi && !mRecyclerView.isFirstItemVisible()) {
On 2016/06/28 17:02:35, Bernhard Bauer wrote:
> Should we merge the mUseCardsUI check? I'm thinking we could put the
> !mUseCardsUI case first and then these two.

Done.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:265:
public boolean scrollOutOfRegion(int regionStart, int flipPoint, int regionEnd)
{
On 2016/06/28 17:02:35, Bernhard Bauer wrote:
> These could now be made private.

Done.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:313:
int start =  getPeekingCardTop()  // A.
On 2016/06/28 17:02:35, Bernhard Bauer wrote:
> Nit: One superfluous space before getPeekingCardTop().

Done.

[Bernhard Bauer, 2016-06-29 11:28:03]

Found some more nits, sorry for not catching them earlier:

https://codereview.chromium.org/2106753002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:195:
public boolean isPeekingCardVisible() {
Oh, these could be made private now, BTW (and maybe even inlined?).

https://codereview.chromium.org/2106753002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:212:
int firstCardIndex = 2; // 0 => above-the-fold, 1 => header, 2 => card
Should we extract these to constants? We also use the value 1 below in
findHeaderView().

[PEConn, 2016-06-29 17:27:16]

@bauerb - could you look over the change to ToolbarPhone.java?

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:408:
float percentage;
On 2016/06/29 11:04:32, mcwilliams wrote:
> Can we rename this variable to something a little more descriptive in what it
is
> used for

Done.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:460:

On 2016/06/29 11:04:32, mcwilliams wrote:
> Remove new lines

Done.

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:205:
}
On 2016/06/29 11:04:32, mcwilliams wrote:
> Any chance we can get these from the CardViewHolder

We could add getTop and getHeight to the CardViewHolder, but I'm not sure why we
would want to do this?

https://codereview.chromium.org/2106753002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/2106753002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:195:
public boolean isPeekingCardVisible() {
On 2016/06/29 11:28:03, Bernhard Bauer wrote:
> Oh, these could be made private now, BTW (and maybe even inlined?).

Done.

https://codereview.chromium.org/2106753002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:212:
int firstCardIndex = 2; // 0 => above-the-fold, 1 => header, 2 => card
On 2016/06/29 11:28:03, Bernhard Bauer wrote:
> Should we extract these to constants? We also use the value 1 below in
> findHeaderView().

Done.

[Bernhard Bauer, 2016-06-29 17:31:41]

lgtm

https://codereview.chromium.org/2106753002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2106753002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:728:
// depend on the scroll value (such as the Toolbar location) are separate from
the parts
Nit: maybe mention _absolute_ scroll value to make it clear what the difference
is.

[PEConn, 2016-06-29 17:52:47]

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

[commit-bot: I haz the power, 2016-06-29 17:54:38]

Dry run: 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, 2016-06-29 19:07:15]

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

[commit-bot: I haz the power, 2016-06-29 19:07:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[PEConn, 2016-06-30 08:10:36]

https://codereview.chromium.org/2106753002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java
(right):

https://codereview.chromium.org/2106753002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:728:
// depend on the scroll value (such as the Toolbar location) are separate from
the parts
On 2016/06/29 17:31:41, Bernhard Bauer wrote:
> Nit: maybe mention _absolute_ scroll value to make it clear what the
difference
> is.

Done.

[PEConn, 2016-06-30 08:11:00]

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

[commit-bot: I haz the power, 2016-06-30 08:11:18]

Dry run: 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, 2016-06-30 09:09:05]

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

[commit-bot: I haz the power, 2016-06-30 09:09:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcwilliams, 2016-06-30 09:35:07]

lgtm

[PEConn, 2016-06-30 09:35:49]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-06-30 09:35:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, mastiz@chromium.org, tedchoc@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2106753002/#ps180001
(title: "Update comment.")

[commit-bot: I haz the power, 2016-06-30 09:36:01]

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

[PEConn, 2016-06-30 09:36:02]

The CQ bit was checked by peconn@chromium.org

[commit-bot: I haz the power, 2016-06-30 09:36:20]

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, 2016-06-30 11:53:41]

Description was changed from

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

In addition, it disables the search logo fading out on scroll.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

to

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

In addition, it disables the search logo fading out on scroll.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

[commit-bot: I haz the power, 2016-06-30 11:53:43]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-06-30 11:55:35]

Description was changed from

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

In addition, it disables the search logo fading out on scroll.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930
==========

to

==========
Refine snap scrolling on the Cards New Tab Page.

This greatly reduces the regions where the Cards New Tab Page snaps to scroll.
- Scrolling will snap to ensure the page does not come to rest in the middle
  of either the fakebox or peeking card transition.
- Scrolling will snap back if only the top of the peeking card is visible,
  making it properly peek.

In addition, it disables the search logo fading out on scroll.

Because the scrolling behaviour has changed, the following user actions are
obsolete:
- MobileNTP.Snippets.ScrolledAboveTheFold
- MobileNTP.Snippets.ScrolledBelowTheFold

BUG=623930

Committed: https://crrev.com/bb72610a2c82054430648c6c15aa9ff7159cdb55
Cr-Commit-Position: refs/heads/master@{#403144}
==========

[commit-bot: I haz the power, 2016-06-30 11:55:36]

Patchset 9 (id:??) landed as
https://crrev.com/bb72610a2c82054430648c6c15aa9ff7159cdb55
Cr-Commit-Position: refs/heads/master@{#403144}