Add new NTP layout with snippet cards and hide it behind a flag

Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new NTP with cards.
The current NTP's content view will be preserved and added as a View in the new RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending on the runtime switches.

Mocks are here: https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromePlate
Spec is here: https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%2FSpec%20-%20zine.png%3Fz=width

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352
patch from issue 1812293002 at patchset 20001 (http://crrev.com/1812293002#ps20001)

Committed: https://crrev.com/d28fdf761d73abded2cacf31397a720da54ec509
Cr-Commit-Position: refs/heads/master@{#384532}

[dgn, 2016-03-28 21:35:24]

dgn@chromium.org changed reviewers:
+ newt@chromium.org, treib@chromium.org

[dgn, 2016-03-28 21:35:26]

PTAL

I'm looking into splitting SnippetsManager across SnippetsBridge and
NewTabPageAdapter. I have a couple of questions though:

https://codereview.chromium.org/1833763002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/1833763002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java:37:
protected void loadUrl(String url) {
Moved to the Snippets item, since it's the only where it makes sense to have the
method.

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

https://codereview.chromium.org/1833763002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:603:
mSnippetsManager = new SnippetsManager(mNewTabPageManager, mProfile);
How about checking here if MorningReads is enabled? We could skip creating the
manager here (and the bridge and native side as well). Also, we could switch
between the scrollview or recyclerview based layout without loading new_tab_page
first, then filling the stub.

https://codereview.chromium.org/1833763002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1833763002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:34:
SnippetsObserver wrappedObserver = new SnippetsObserver() {
@treib: Any reason why a separate object is passed instead of just implementing
the method on SnippetsBridge?

[Marc Treib, 2016-03-29 10:16:21]

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

https://codereview.chromium.org/1833763002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:603:
mSnippetsManager = new SnippetsManager(mNewTabPageManager, mProfile);
On 2016/03/28 21:35:25, dgn wrote:
> How about checking here if MorningReads is enabled? We could skip creating the
> manager here (and the bridge and native side as well). Also, we could switch
> between the scrollview or recyclerview based layout without loading
new_tab_page
> first, then filling the stub.

SGTM (but I know little about the UI stuff, newt should comment on that)

https://codereview.chromium.org/1833763002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1833763002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:34:
SnippetsObserver wrappedObserver = new SnippetsObserver() {
On 2016/03/28 21:35:25, dgn wrote:
> @treib: Any reason why a separate object is passed instead of just
implementing
> the method on SnippetsBridge?

You mean, SnippetsBridge could implement SnippetsObserver, and in
onSnippetsAvailable forward to the passed-in SnippetsObserver? I guess it's just
a different way to achieve the same thing. The "wrappedObserver" pattern is
common in the NTP code, but I don't really have a preference either way.

[dgn, 2016-03-30 11:51:29]

dgn@chromium.org changed reviewers:
+ bauerb@google.com

[dgn, 2016-03-30 11:51:30]

PTAL

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:248:
mRecyclerView = (NewTabPageRecyclerView) findViewById(R.id.ntp_recycler_view);
Possible change: NewTabPageRecyclerView and NewTabPageScrollView could implement
an interface that would be what NewTabPageView cares about, which would avoid
having two members here and having to choose all the time which one to use.

[Bernhard Bauer, 2016-03-30 13:55:02]

Description was changed from

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

patch from issue 1812293002 at patchset 20001
(http://crrev.com/1812293002#ps20001)
==========

to

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

patch from issue 1812293002 at patchset 20001
(http://crrev.com/1812293002#ps20001)
==========

[Bernhard Bauer, 2016-03-30 13:55:02]

bauerb@chromium.org changed reviewers:
+ bauerb@chromium.org
- bauerb@google.com

[newt (away), 2016-03-30 19:03:51]

Thanks for taking this over :)

Mostly just minor comments.

https://codereview.chromium.org/1833763002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(right):

https://codereview.chromium.org/1833763002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:24:
private static final String TAG = "SnippetsArticle";
s/Snippets/Snippet/

https://codereview.chromium.org/1833763002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:28:
public final String mTitle;
public member variables shouldn't have "m". Also, they should be documented.
Also, they're rarely appropriate, though maybe here.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page.xml (right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page.xml:14:
xmlns:android="http://schemas.android.com/apk/res/android"
remove xmlns

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_v2.xml (right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:2: <!-- Copyright 2015 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:8:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
xmlns:chrome not needed

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:14:
xmlns:android="http://schemas.android.com/apk/res/android"
remove xmlns

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:24: 
nit: remove extra newline

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560:
Log.d(TAG, "Snippets are enabled. Using the new NTP layout.");
Is this log message really helpful enough to check in? It'll only be printed out
in local builds, where it should be pretty clear whether the morning reads UI is
enabled or not.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:121:
// Make the search box and logo as wide as the most visited items
Why not use the old simpler logic?

    if (mMostVisitedLayout.getVisibility() != GONE) {
        ...
    }

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:134:
public static void recordSnippetAction(@SnippetsAction int action) {
Did you consider using user actions here instead of a histogram? A histogram has
the benefit that it's easy to see all the related events in a single view with
relative frequencies. A user action has the benefit that you can see a timeline
view, showing what actions users most often perform before or after, say,
clicking on a snippet. (SNIPPETS_SHOWN isn't really a user action though...)

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:79:
private NewTabPageAdapter mCardsManager;
update variable name

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:243:
boolean searchProviderHasLogo, SnippetsBridge snippetsBridge) {
add javadoc for this param, and mention that passing in null will revert to the
old NTP layout

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:248:
mRecyclerView = (NewTabPageRecyclerView) findViewById(R.id.ntp_recycler_view);
On 2016/03/30 11:51:30, dgn wrote:
> Possible change: NewTabPageRecyclerView and NewTabPageScrollView could
implement
> an interface that would be what NewTabPageView cares about, which would avoid
> having two members here and having to choose all the time which one to use. 

That would help in certain places, but I'd weigh the decision against the added
indirection and complexity when reading the code. In this case, NewTabPageView
still needs to be aware of the two different modes in a number of places (e.g.
when inflating the layouts, removing the toolbar, initializing the search box
scroll handling). The only benefit would be removing the if-else inside of
getVerticalScroll() and updateSearchBoxOnScroll(), but that's a pretty small
win.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:250:
.inflate(R.layout.new_tab_page_layout, null);
fix indentation. Also, it's better to pass in the parent view when inflating a
layout, as it allows the correct subclass of ViewGroup.LayoutParams to be used.
E.g.

    LayoutInflater.from(getContext()).inflate(R.layout.new_tab_page_layout,
mRecyclerView, false);

If the parent is, say, a LinearLayout and the inflated layout has
layout_weight="1", then the layout_weight will be lost unless you pass in the
parent view.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:571:
+ mContentView.getPaddingTop()));
nit: fix indentation. Should just be 8 spaces beyond the line above. (You could
wrap at a higher level, e.g. after *, to make this clearer too)

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:26:
public void onSnippetsReceived(List<SnippetArticle> snippets);
This is a much cleaner public interface :)

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:38:
public void destroy() {
javadoc

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:44:
public void setObserver(SnippetsObserver observer) {
javadoc. Worth mentioning that this should only be called once.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:63:
mObserver.onSnippetsReceived(newSnippets);
mObserver could be null here. (at least with the current public interface)

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:67:
private native void nativeDestroy(long nativeNTPSnippetsBridge);
nit: s/NTP/Ntp/

In Java code, even acronyms should be camelCased in names. "Ui", "Http", etc

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/AboveTheFoldListItem.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/AboveTheFoldListItem.java:10:
/** Item in the RecyclerView that will hold the above the fold contents for the
NTP */
This would be a great place to explain what in the world "above the fold
contents" are. Also, I'd avoid putting class header comments on a single line.
The class header comment is important; make it stand out a bit :)

    /**
     * Amazing class.
     */

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/AboveTheFoldListItem.java:23:
public AboveTheFoldListItem() {}
not needed

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:21:
public static class ViewHolder extends NewTabPageViewHolder {
This looks identical to SnippetHeaderViewHolder. Are they both needed / is one
unused?

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:27:
* Creates the TextView object for displaying the header for a group of snippets
I'd replace "TextView" with "View", since "TextView" is an implementation detail
(plus, the layout could change in the future).

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:41:
public HeaderListItem() {}
not needed

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java:40:
* @param snippetsManager the NewTabPageManager to use to start navigations
update params in javadoc

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java:53:
mSnippetsBridge.setObserver(this);
Shouldn't we remove this observer? Or are you counting on the SnippetBridge
lifetime being the same as the NewTabPageAdapter? (That seems like a slightly
dangerous hidden assumption that could lead to a memory leak.)

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageRecyclerView.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageRecyclerView.java:20:
public class NewTabPageRecyclerView extends RecyclerView {
Please copy over the focusableViewAvailable() method from NewTabPageScrollView.
See https://gerrit-int.chromium.org/44249 for why that code is needed.

[newt (away), 2016-03-30 19:04:45]

Also, be sure to test the new design when the default search engine is
non-Google, in which case we hide the logo.

[dgn, 2016-03-31 00:46:45]

PTAL. All seems ok with no logo.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page.xml (right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page.xml:14:
xmlns:android="http://schemas.android.com/apk/res/android"
On 2016/03/30 19:03:49, newt wrote:
> remove xmlns

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
File chrome/android/java/res/layout/new_tab_page_v2.xml (right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:1: <?xml version="1.0"
encoding="utf-8"?>
We might have to experiment with zine+toolbar. Restore the previous layout that
had the toolbar with the RecyclerView

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:2: <!-- Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/03/30 19:03:50, newt wrote:
> 2016

File removed.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:8:
xmlns:chrome="http://schemas.android.com/apk/res-auto"
On 2016/03/30 19:03:50, newt wrote:
> xmlns:chrome not needed

File removed.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:14:
xmlns:android="http://schemas.android.com/apk/res/android"
On 2016/03/30 19:03:49, newt wrote:
> remove xmlns

removed from new_tab_page.xml

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/res...
chrome/android/java/res/layout/new_tab_page_v2.xml:24: 
On 2016/03/30 19:03:49, newt wrote:
> nit: remove extra newline

File removed.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:560:
Log.d(TAG, "Snippets are enabled. Using the new NTP layout.");
On 2016/03/30 19:03:50, newt wrote:
> Is this log message really helpful enough to check in? It'll only be printed
out
> in local builds, where it should be pretty clear whether the morning reads UI
is
> enabled or not.

Removed. The flag itself is easy to check. What I was trying to debug here was
the cases where snippets are enabled but no snippets are loaded. This should
probably be logged somewhere else and with more details though, probably in
another CL.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:121:
// Make the search box and logo as wide as the most visited items
On 2016/03/30 19:03:50, newt wrote:
> Why not use the old simpler logic?
> 
>     if (mMostVisitedLayout.getVisibility() != GONE) {
>         ...
>     }

That's to address May's comment here:
https://codereview.chromium.org/1812293002/diff/20001/chrome/android/java/src...

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:134:
public static void recordSnippetAction(@SnippetsAction int action) {
On 2016/03/30 19:03:50, newt wrote:
> Did you consider using user actions here instead of a histogram? A histogram
has
> the benefit that it's easy to see all the related events in a single view with
> relative frequencies. A user action has the benefit that you can see a
timeline
> view, showing what actions users most often perform before or after, say,
> clicking on a snippet. (SNIPPETS_SHOWN isn't really a user action though...)

There is the issue that SNIPPETS_SHOWN isn't a user action, but the description
for that metric doesn't make it fit that well as histogram either. I'll ask
around about the expectations for this histogram, as I didn't change how it's
recorded, just moved it from SnippetManager

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:79:
private NewTabPageAdapter mCardsManager;
On 2016/03/30 19:03:50, newt wrote:
> update variable name

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:243:
boolean searchProviderHasLogo, SnippetsBridge snippetsBridge) {
On 2016/03/30 19:03:50, newt wrote:
> add javadoc for this param, and mention that passing in null will revert to
the
> old NTP layout

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:248:
mRecyclerView = (NewTabPageRecyclerView) findViewById(R.id.ntp_recycler_view);
On 2016/03/30 19:03:50, newt wrote:
> On 2016/03/30 11:51:30, dgn wrote:
> > Possible change: NewTabPageRecyclerView and NewTabPageScrollView could
> implement
> > an interface that would be what NewTabPageView cares about, which would
avoid
> > having two members here and having to choose all the time which one to use. 
> 
> That would help in certain places, but I'd weigh the decision against the
added
> indirection and complexity when reading the code. In this case, NewTabPageView
> still needs to be aware of the two different modes in a number of places (e.g.
> when inflating the layouts, removing the toolbar, initializing the search box
> scroll handling). The only benefit would be removing the if-else inside of
> getVerticalScroll() and updateSearchBoxOnScroll(), but that's a pretty small
> win.

I think it's very possible to make it aware of the difference only in the
constructor, and have the rest handled by the child view. But yeah, it wouldn't
be a very big win anyway. Can be reconsidered when we add new NTP experiments.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:250:
.inflate(R.layout.new_tab_page_layout, null);
On 2016/03/30 19:03:50, newt wrote:
> fix indentation. Also, it's better to pass in the parent view when inflating a
> layout, as it allows the correct subclass of ViewGroup.LayoutParams to be
used.
> E.g.
> 
>     LayoutInflater.from(getContext()).inflate(R.layout.new_tab_page_layout,
> mRecyclerView, false);
> 
> If the parent is, say, a LinearLayout and the inflated layout has
> layout_weight="1", then the layout_weight will be lost unless you pass in the
> parent view.

I think May did that on purpose. When setting the parent, the contentview does
like with ScrollView, and matches the parent. But with the new UI, instead of
being the only child, it's just the abovethefold item, and should not fill the
view. 
If I set the parent, I then have to adjust the height to match content. Is it
better to do that?

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:571:
+ mContentView.getPaddingTop()));
On 2016/03/30 19:03:50, newt wrote:
> nit: fix indentation. Should just be 8 spaces beyond the line above. (You
could
> wrap at a higher level, e.g. after *, to make this clearer too)

git cl format being weird again. Fixed.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageViewHolder.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/03/30 19:03:50, newt wrote:
> 2016

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:38:
public void destroy() {
On 2016/03/30 19:03:50, newt wrote:
> javadoc

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:44:
public void setObserver(SnippetsObserver observer) {
On 2016/03/30 19:03:50, newt wrote:
> javadoc. Worth mentioning that this should only be called once.

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:63:
mObserver.onSnippetsReceived(newSnippets);
On 2016/03/30 19:03:50, newt wrote:
> mObserver could be null here. (at least with the current public interface)

The method can be called from the native side only if the observer has been set
(NTPSnippetsServiceLoaded returns early when it's not) so it's safe from the
point of view of the initialization. For the destruction, mNativeSnippetsBridge
covers it. Mentioned that in the javadoc for setObserver.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:67:
private native void nativeDestroy(long nativeNTPSnippetsBridge);
On 2016/03/30 19:03:50, newt wrote:
> nit: s/NTP/Ntp/
> 
> In Java code, even acronyms should be camelCased in names. "Ui", "Http", etc

That refers to the c++ class name, can't be modified.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/AboveTheFoldListItem.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/AboveTheFoldListItem.java:10:
/** Item in the RecyclerView that will hold the above the fold contents for the
NTP */
On 2016/03/30 19:03:50, newt wrote:
> This would be a great place to explain what in the world "above the fold
> contents" are. Also, I'd avoid putting class header comments on a single line.
> The class header comment is important; make it stand out a bit :)
> 
>     /**
>      * Amazing class.
>      */

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/AboveTheFoldListItem.java:23:
public AboveTheFoldListItem() {}
On 2016/03/30 19:03:50, newt wrote:
> not needed

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:21:
public static class ViewHolder extends NewTabPageViewHolder {
On 2016/03/30 19:03:51, newt wrote:
> This looks identical to SnippetHeaderViewHolder. Are they both needed / is one
> unused?

made NTPViewHolder concrete and removed those two, as they don't bring anything
special currently.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:27:
* Creates the TextView object for displaying the header for a group of snippets
On 2016/03/30 19:03:50, newt wrote:
> I'd replace "TextView" with "View", since "TextView" is an implementation
detail
> (plus, the layout could change in the future).

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:41:
public HeaderListItem() {}
On 2016/03/30 19:03:50, newt wrote:
> not needed

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java:40:
* @param snippetsManager the NewTabPageManager to use to start navigations
On 2016/03/30 19:03:51, newt wrote:
> update params in javadoc

Done.

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java:53:
mSnippetsBridge.setObserver(this);
On 2016/03/30 19:03:51, newt wrote:
> Shouldn't we remove this observer? Or are you counting on the SnippetBridge
> lifetime being the same as the NewTabPageAdapter? (That seems like a slightly
> dangerous hidden assumption that could lead to a memory leak.)

Added methods to do it at NTP#destroy() time.

[newt (away), 2016-03-31 04:16:22]

last few comments, and a better suggestion re NewTabPageAdapter/SnippetsBridge
lifetimes :)

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:250:
.inflate(R.layout.new_tab_page_layout, null);
On 2016/03/31 00:46:45, dgn wrote:
> On 2016/03/30 19:03:50, newt wrote:
> > fix indentation. Also, it's better to pass in the parent view when inflating
a
> > layout, as it allows the correct subclass of ViewGroup.LayoutParams to be
> used.
> > E.g.
> > 
> >     LayoutInflater.from(getContext()).inflate(R.layout.new_tab_page_layout,
> > mRecyclerView, false);
> > 
> > If the parent is, say, a LinearLayout and the inflated layout has
> > layout_weight="1", then the layout_weight will be lost unless you pass in
the
> > parent view.
> 
> I think May did that on purpose. When setting the parent, the contentview does
> like with ScrollView, and matches the parent. But with the new UI, instead of
> being the only child, it's just the abovethefold item, and should not fill the
> view. 
> If I set the parent, I then have to adjust the height to match content. Is it
> better to do that?

I see. The current code is OK in that case, though it would increase clarity to
set the layout params explicitly, or at least comment that the default layout
params are what we want.

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/layout/new_tab_page_recycler_view.xml (right):

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/re...
chrome/android/java/res/layout/new_tab_page_recycler_view.xml:4: found in the
LICENSE file. -->
nit: empty line after header

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:716:
mNewTabPageView.onPageDestroyed();
Hmmm. In retrospect, I don't actually like the idea of involving NewTabPageView
in lifecycle events. So far there's been a clean separation between the page's
creation-destruction lifecycle and the all the view logic, and I think it makes
sense to keep it that way.

I'd suggest this:
 - NewTabPage owns the SnippetsBridge and doesn't expose it to any other class
 - Add a method "setSnippetsObserver()" to NewTabPageManager (similar to
setMostVisitedURLsObserver())
 - Instead of passing SnippetsBridge to NewTabPageAdapter, NewTabPageAdapter
will call NewTabPageManager.setSnippetsObserver() to register itself as an
observer.

This makes lifetime management simple, and parallel to the lifetime management
of all the bridge/helper classes.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:121:
// Make the search box and logo as wide as the most visited items
Ah, and May's comment was in response to my observation that splitting the
measuring logic across two files just added complexity. (The comment was not the
right fix in any case; unifying the measuring logic was the right fix.)

The original measuring logic was just fine. Let's restore it here.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:70:
private boolean mUseNewUi;
I think "mUseSnippetsUi" would be clearer

(There have been lots of "new" NTP designs, but only one snippets design.)

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:249:
*  will depend on whether {@code null},
fix content and indentation

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:78:
Log.w(TAG, "Snippets have been received while there is no observer to notify.");
Keep comments as short as reasonable. The log buffer is limited in bytes, not
lines. E.g. "Snippets received before observer set"

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:16:
public class HeaderListItem implements NewTabPageListItem {
Probably should be package-private, especially with such a generic name. Maybe
call this SnippetHeaderListItem?

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageRecyclerView.java:91:

extra newline

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/SnippetHeaderViewHolder.java:16:
public class SnippetHeaderViewHolder extends NewTabPageViewHolder {
Is this class used anywhere?

[dgn, 2016-03-31 13:38:32]

PTAL

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

https://codereview.chromium.org/1833763002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:250:
.inflate(R.layout.new_tab_page_layout, null);
On 2016/03/31 04:16:21, newt wrote:
> On 2016/03/31 00:46:45, dgn wrote:
> > On 2016/03/30 19:03:50, newt wrote:
> > > fix indentation. Also, it's better to pass in the parent view when
inflating
> a
> > > layout, as it allows the correct subclass of ViewGroup.LayoutParams to be
> > used.
> > > E.g.
> > > 
> > >    
LayoutInflater.from(getContext()).inflate(R.layout.new_tab_page_layout,
> > > mRecyclerView, false);
> > > 
> > > If the parent is, say, a LinearLayout and the inflated layout has
> > > layout_weight="1", then the layout_weight will be lost unless you pass in
> the
> > > parent view.
> > 
> > I think May did that on purpose. When setting the parent, the contentview
does
> > like with ScrollView, and matches the parent. But with the new UI, instead
of
> > being the only child, it's just the abovethefold item, and should not fill
the
> > view. 
> > If I set the parent, I then have to adjust the height to match content. Is
it
> > better to do that?
> 
> I see. The current code is OK in that case, though it would increase clarity
to
> set the layout params explicitly, or at least comment that the default layout
> params are what we want.

Did both.

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/re...
File chrome/android/java/res/layout/new_tab_page_recycler_view.xml (right):

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/re...
chrome/android/java/res/layout/new_tab_page_recycler_view.xml:4: found in the
LICENSE file. -->
On 2016/03/31 04:16:21, newt wrote:
> nit: empty line after header

Done.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:716:
mNewTabPageView.onPageDestroyed();
On 2016/03/31 04:16:22, newt wrote:
> Hmmm. In retrospect, I don't actually like the idea of involving
NewTabPageView
> in lifecycle events. So far there's been a clean separation between the page's
> creation-destruction lifecycle and the all the view logic, and I think it
makes
> sense to keep it that way.
> 
> I'd suggest this:
>  - NewTabPage owns the SnippetsBridge and doesn't expose it to any other class
>  - Add a method "setSnippetsObserver()" to NewTabPageManager (similar to
> setMostVisitedURLsObserver())
>  - Instead of passing SnippetsBridge to NewTabPageAdapter, NewTabPageAdapter
> will call NewTabPageManager.setSnippetsObserver() to register itself as an
> observer.
> 
> This makes lifetime management simple, and parallel to the lifetime management
> of all the bridge/helper classes.

Thanks, Done. The manager is always a valid reference so I removed the adapter's
unregistration. The bridge clears its reference in destroy() anyway.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:121:
// Make the search box and logo as wide as the most visited items
On 2016/03/31 04:16:22, newt wrote:
> Ah, and May's comment was in response to my observation that splitting the
> measuring logic across two files just added complexity. (The comment was not
the
> right fix in any case; unifying the measuring logic was the right fix.)
> 
> The original measuring logic was just fine. Let's restore it here.

Done.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:70:
private boolean mUseNewUi;
On 2016/03/31 04:16:22, newt wrote:
> I think "mUseSnippetsUi" would be clearer
> 
> (There have been lots of "new" NTP designs, but only one snippets design.)

Okay. There are going to be further experiments with the NTP but AFAIK all will
be based around cards and use that recyclerview. I was trying to capture that
with the name. I guess that's to be changed when we start needing it for more
than only snippets. What about that v2 package then? Should I just put
everything in the ntp package instead?

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:249:
*  will depend on whether {@code null},
On 2016/03/31 04:16:22, newt wrote:
> fix content and indentation

Done.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:78:
Log.w(TAG, "Snippets have been received while there is no observer to notify.");
On 2016/03/31 04:16:22, newt wrote:
> Keep comments as short as reasonable. The log buffer is limited in bytes, not
> lines. E.g. "Snippets received before observer set"

Removed the comment and just used an assert. The observer's lifetime is longer
than the one of the bridge, and the observer is now removed on destroy, so it's
safe.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/HeaderListItem.java:16:
public class HeaderListItem implements NewTabPageListItem {
On 2016/03/31 04:16:22, newt wrote:
> Probably should be package-private, especially with such a generic name. Maybe
> call this SnippetHeaderListItem?

Done.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageRecyclerView.java:91:

On 2016/03/31 04:16:22, newt wrote:
> extra newline

Done.

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/SnippetHeaderViewHolder.java:16:
public class SnippetHeaderViewHolder extends NewTabPageViewHolder {
On 2016/03/31 04:16:22, newt wrote:
> Is this class used anywhere?

Thanks, I forgot to remove it.

[dgn, 2016-03-31 16:32:51]

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

[dgn, 2016-03-31 16:32:52]

rkaplow@: PTAL. I didn't change UMA files since the previous CL, but I
refactored a bit how the metrics are recorded.

[newt (away), 2016-03-31 17:06:33]

lgtm after comments :)

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:70:
private boolean mUseNewUi;
On 2016/03/31 13:38:31, dgn wrote:
> On 2016/03/31 04:16:22, newt wrote:
> > I think "mUseSnippetsUi" would be clearer
> > 
> > (There have been lots of "new" NTP designs, but only one snippets design.)
> 
> Okay. There are going to be further experiments with the NTP but AFAIK all
will
> be based around cards and use that recyclerview. I was trying to capture that
> with the name. I guess that's to be changed when we start needing it for more
> than only snippets. What about that v2 package then? Should I just put
> everything in the ntp package instead?

In that case, maybe mUseCardsUi or mUseRecyclerViewUi. Whatever you think
balances accuracy, clarity, and simplicity :)

Re the package, I'm fine with using the ntp.v2 package (just be sure to move
everything back to the ntp package if/when this becomes the default UI); I'd
also be fine with not using it. Up to you.

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java:38:
* rest of the system.
nit: indent this line

    @param manager the NewTabPageManager ...
                   rest of the system.

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/SnippetHeaderListItem.java
(right):

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/SnippetHeaderListItem.java:5:
package org.chromium.chrome.browser.ntp.v2;
this should probably be in the ntp.snippets package along with
SnippetHeaderItemViewHolder.java and other related files.

[dgn, 2016-03-31 17:56:13]

done! Thank you very much!

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

https://codereview.chromium.org/1833763002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:70:
private boolean mUseNewUi;
On 2016/03/31 17:06:32, newt wrote:
> On 2016/03/31 13:38:31, dgn wrote:
> > On 2016/03/31 04:16:22, newt wrote:
> > > I think "mUseSnippetsUi" would be clearer
> > > 
> > > (There have been lots of "new" NTP designs, but only one snippets design.)
> > 
> > Okay. There are going to be further experiments with the NTP but AFAIK all
> will
> > be based around cards and use that recyclerview. I was trying to capture
that
> > with the name. I guess that's to be changed when we start needing it for
more
> > than only snippets. What about that v2 package then? Should I just put
> > everything in the ntp package instead?
> 
> In that case, maybe mUseCardsUi or mUseRecyclerViewUi. Whatever you think
> balances accuracy, clarity, and simplicity :)
> 
> Re the package, I'm fine with using the ntp.v2 package (just be sure to move
> everything back to the ntp package if/when this becomes the default UI); I'd
> also be fine with not using it. Up to you.

Done.

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java
(right):

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/NewTabPageAdapter.java:38:
* rest of the system.
On 2016/03/31 17:06:33, newt wrote:
> nit: indent this line
> 
>     @param manager the NewTabPageManager ...
>                    rest of the system.

Done.

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/SnippetHeaderListItem.java
(right):

https://codereview.chromium.org/1833763002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ntp/v2/SnippetHeaderListItem.java:5:
package org.chromium.chrome.browser.ntp.v2;
On 2016/03/31 17:06:33, newt wrote:
> this should probably be in the ntp.snippets package along with
> SnippetHeaderItemViewHolder.java and other related files.

It's true that the text is hardcoded to refer to snippets. Moved it there.

[rkaplow, 2016-04-01 02:18:41]

lgtm

[dgn, 2016-04-01 08:28:36]

The CQ bit was checked by dgn@chromium.org

[dgn, 2016-04-01 08:28:36]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1833763002/#ps180001
(title: "address more comments")

[commit-bot: I haz the power, 2016-04-01 08:28:54]

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

[dgn, 2016-04-01 08:28:59]

On 2016/04/01 02:18:41, rkaplow wrote:
> lgtm

Thanks!

[commit-bot: I haz the power, 2016-04-01 09:34:54]

Description was changed from

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

patch from issue 1812293002 at patchset 20001
(http://crrev.com/1812293002#ps20001)
==========

to

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

patch from issue 1812293002 at patchset 20001
(http://crrev.com/1812293002#ps20001)
==========

[commit-bot: I haz the power, 2016-04-01 09:34:55]

Committed patchset #10 (id:180001)

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

Description was changed from

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

patch from issue 1812293002 at patchset 20001
(http://crrev.com/1812293002#ps20001)
==========

to

==========
Add new NTP layout with snippet cards and hide it behind a flag

Swapping out the ScrollView in the current NTP for a RecyclerView for the new
NTP with cards.
The current NTP's content view will be preserved and added as a View in the new
RecyclerView.
The swapping will happen via a ViewStub that gets inflated at runtime, depending
on the runtime switches.

Mocks are here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/160225-ChromeP...
Spec is here:
https://folio.googleplex.com/chrome-ux/mocks/335-NTP-Zine/Zine/_UI%20Review#%...

Video demos:
Feature off: https://drive.google.com/open?id=0B1flmW1uZoFBWVZzVjZWRVotbjg
Feature on: https://drive.google.com/open?id=0B1flmW1uZoFBT0pNU2tFc3FlSDg

Known issues:
Missing favicon, publisher
Cards are not full width
NTP background is not the right gradient
Snap points are not yet implemented on new NTP
Cards are not shown as a hint yet
Search box jumps back down when scrolling between above the fold and the cards

BUG=584352

patch from issue 1812293002 at patchset 20001
(http://crrev.com/1812293002#ps20001)

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

[commit-bot: I haz the power, 2016-04-01 09:36:38]

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