Scroll to articles when peeking card is tapped.

Scroll to articles when peeking card is tapped.

Additionally, move selected layout related functions out of NewTabPageView
and into CardsLayoutOperations class.

BUG=612543
Committed: https://crrev.com/e1521d2e17d9c472a2794e29d3c1b482151e430a
Cr-Commit-Position: refs/heads/master@{#395868}

[PEConn, 2016-05-23 14:42:40]

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

[PEConn, 2016-05-23 14:42:41]

The new functionality is in |updatePeekingCard|, starting from line 69, the
other code changes are just extracting functions to a static class.

[Bernhard Bauer, 2016-05-24 12:07:24]

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java
(right):

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:51:
padding = Math.min(Math.max(padding, 0), maxPadding);
There is MathUtils.clamp() for this, BTW.

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:71:
final int finalPadding = padding;
You could just keep using |padding| here.

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:76:
NewTabPageLayout ntpLayout = (NewTabPageLayout) getFirstViewMatchingViewType(
Pass this in as well?

[PEConn, 2016-05-24 13:06:04]

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java
(right):

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:51:
padding = Math.min(Math.max(padding, 0), maxPadding);
On 2016/05/24 12:07:24, Bernhard Bauer wrote:
> There is MathUtils.clamp() for this, BTW.

Done.

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:71:
final int finalPadding = padding;
On 2016/05/24 12:07:24, Bernhard Bauer wrote:
> You could just keep using |padding| here.

It was final so it could be used in the callback below, however with your above
comment I can make |padding| final.

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:76:
NewTabPageLayout ntpLayout = (NewTabPageLayout) getFirstViewMatchingViewType(
On 2016/05/24 12:07:24, Bernhard Bauer wrote:
> Pass this in as well?

I think this function is called fairly frequently (whenever the peeking card
changes size), so I was trying to push any processing into the onClick method,
so it would only be run if the button was actually clicked.

[PEConn, 2016-05-24 13:24:47]

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

[commit-bot: I haz the power, 2016-05-24 13:25:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2004893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2004893002/20001

[Bernhard Bauer, 2016-05-25 08:34:11]

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java
(right):

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:76:
NewTabPageLayout ntpLayout = (NewTabPageLayout) getFirstViewMatchingViewType(
On 2016/05/24 13:06:04, PEConn1 wrote:
> On 2016/05/24 12:07:24, Bernhard Bauer wrote:
> > Pass this in as well?
> 
> I think this function is called fairly frequently (whenever the peeking card
> changes size), so I was trying to push any processing into the onClick method,
> so it would only be run if the button was actually clicked.

Yes, but you have the NewTabPageLayout as a member in NewTabPageView, and
passing in one additional parameter is not going to make a huge difference in
terms of performance.

https://codereview.chromium.org/2004893002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java
(right):

https://codereview.chromium.org/2004893002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:72:
firstSnippet.setOnClickListener(new OnClickListener(){
Nit: space before the opening brace.

[PEConn, 2016-05-25 10:14:39]

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java
(right):

https://codereview.chromium.org/2004893002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:76:
NewTabPageLayout ntpLayout = (NewTabPageLayout) getFirstViewMatchingViewType(
On 2016/05/25 08:34:10, Bernhard Bauer wrote:
> On 2016/05/24 13:06:04, PEConn1 wrote:
> > On 2016/05/24 12:07:24, Bernhard Bauer wrote:
> > > Pass this in as well?
> > 
> > I think this function is called fairly frequently (whenever the peeking card
> > changes size), so I was trying to push any processing into the onClick
method,
> > so it would only be run if the button was actually clicked.
> 
> Yes, but you have the NewTabPageLayout as a member in NewTabPageView, and
> passing in one additional parameter is not going to make a huge difference in
> terms of performance.

Ah, sorry, I thought you'd mean move the |getFirstViewMatchingViewType| up to
line 71 and pass it into the OnClickListener, not pass the NewTabPageLayout into
the function.

Done.

https://codereview.chromium.org/2004893002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java
(right):

https://codereview.chromium.org/2004893002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardsLayoutOperations.java:72:
firstSnippet.setOnClickListener(new OnClickListener(){
On 2016/05/25 08:34:10, Bernhard Bauer wrote:
> Nit: space before the opening brace.

Done.

[Bernhard Bauer, 2016-05-25 10:29:14]

lgtm

[PEConn, 2016-05-25 10:35:03]

The CQ bit was checked by peconn@chromium.org

[commit-bot: I haz the power, 2016-05-25 10:35:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2004893002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2004893002/60001

[commit-bot: I haz the power, 2016-05-25 11:55:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-25 11:57:27]

Description was changed from

==========
Scroll to articles when peeking card is tapped.

Additionally, move selected layout related functions out of NewTabPageView
and into CardsLayoutOperations class.

BUG=612543
==========

to

==========
Scroll to articles when peeking card is tapped.

Additionally, move selected layout related functions out of NewTabPageView
and into CardsLayoutOperations class.

BUG=612543

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

[commit-bot: I haz the power, 2016-05-25 11:57:28]

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