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

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
index 988e128e8d55e5796099b99217e2d8960f9fdba0..c33051b9e407e18a27891756343055f87b93d4d7 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
@@ -17,7 +17,6 @@ import android.net.Uri;
 import android.os.Build;
 import android.support.v4.graphics.drawable.RoundedBitmapDrawable;
 import android.support.v4.graphics.drawable.RoundedBitmapDrawableFactory;
-import android.support.v7.widget.LinearLayoutManager;
 import android.support.v7.widget.RecyclerView;
 import android.text.Editable;
 import android.text.TextUtils;
@@ -73,7 +72,7 @@ public class NewTabPageView extends FrameLayout
     private static final long SNAP_SCROLL_DELAY_MS = 30;
     private static final String TAG = "NewTabPageView";
 
-    private ViewGroup mContentView;
+    private NewTabPageLayout mContentView;
     private NewTabScrollView mScrollView;
     private LogoView mSearchProviderLogoView;
     private View mSearchBoxView;
@@ -83,7 +82,8 @@ public class NewTabPageView extends FrameLayout
     private View mMostVisitedPlaceholder;
     private View mOptOutView;
     private View mNoSearchLogoSpacer;
-    private RecyclerView mSnippetsView;
+    private NewTabRecyclerView mSnippetsView;

[newt (away), 2016/03/23 05:29:38]

I'd put this next to mScrollView and add a comment that only one of these two
variables will be valid.

[May, 2016/03/23 19:22:56]

Done.

+    private NewTabPageCardsManager mCardsManager;
 
     private OnSearchBoxScrollListener mSearchBoxScrollListener;
 
@@ -257,18 +257,28 @@ public class NewTabPageView extends FrameLayout
     public void initialize(NewTabPageManager manager, boolean isSingleUrlBarMode,
             boolean searchProviderHasLogo, SnippetsManager snippetsManager) {
         mManager = manager;
+        ViewStub stub = (ViewStub) findViewById(R.id.new_tab_page_layout_stub);
 
-        mScrollView = (NewTabScrollView) findViewById(R.id.ntp_scrollview);
-        mScrollView.enableBottomShadow(SHADOW_COLOR);
-        mContentView = (ViewGroup) findViewById(R.id.ntp_content);
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {

[newt (away), 2016/03/23 05:29:38]

Let's store this value in a member variable rather than rechecking it
everywhere.

[May, 2016/03/23 19:22:56]

Done.

+            stub.setLayoutResource(R.layout.new_tab_page_recyclerview);
+            mSnippetsView = (NewTabRecyclerView) stub.inflate();

[newt (away), 2016/03/23 05:29:38]

mRecyclerView is probably a better name for this, since it holds not only the
snippets but also the rest of the NTP content.

[May, 2016/03/23 19:22:56]

Done.

+            mContentView = (NewTabPageLayout) LayoutInflater.from(getContext())
+                                   .inflate(R.layout.new_tab_page_layout, null);
+        } else {
+            stub.setLayoutResource(R.layout.new_tab_page_scrollview);
+            mScrollView = (NewTabScrollView) stub.inflate();
+            mScrollView.enableBottomShadow(SHADOW_COLOR);
+            mContentView = (NewTabPageLayout) findViewById(R.id.ntp_content);
+        }
 
         mMostVisitedDesign = new MostVisitedDesign(getContext());
-        mMostVisitedLayout = (MostVisitedLayout) findViewById(R.id.most_visited_layout);
+        mMostVisitedLayout =
+                (MostVisitedLayout) mContentView.findViewById(R.id.most_visited_layout);
         mMostVisitedDesign.initMostVisitedLayout(mMostVisitedLayout, searchProviderHasLogo);
 
-        mSearchProviderLogoView = (LogoView) findViewById(R.id.search_provider_logo);
-        mSearchBoxView = findViewById(R.id.search_box);
-        mNoSearchLogoSpacer = findViewById(R.id.no_search_logo_spacer);
+        mSearchProviderLogoView = (LogoView) mContentView.findViewById(R.id.search_provider_logo);
+        mSearchBoxView = mContentView.findViewById(R.id.search_box);
+        mNoSearchLogoSpacer = mContentView.findViewById(R.id.no_search_logo_spacer);
 
         mSearchBoxTextView = (TextView) mSearchBoxView.findViewById(R.id.search_box_text);
         String hintText = getResources().getString(R.string.search_or_type_url);
@@ -300,7 +310,7 @@ public class NewTabPageView extends FrameLayout
             }
         });
 
-        mVoiceSearchButton = (ImageView) findViewById(R.id.voice_search_button);
+        mVoiceSearchButton = (ImageView) mContentView.findViewById(R.id.voice_search_button);
         mVoiceSearchButton.setOnClickListener(new View.OnClickListener() {
             @Override
             public void onClick(View v) {
@@ -342,7 +352,13 @@ public class NewTabPageView extends FrameLayout
             mScrollView.setLayoutParams(params);
         }
 
-        initializeSearchBoxScrollHandling();
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
+            toolbar.setVisibility(View.GONE);
+            initializeSearchBoxRecyclerViewScrollHandling();
+        } else {
+            initializeSearchBoxScrollHandling();
+        }
+
         addOnLayoutChangeListener(this);
         setSearchProviderHasLogo(searchProviderHasLogo);
 
@@ -354,11 +370,11 @@ public class NewTabPageView extends FrameLayout
 
         // Set up snippets
         if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
-            mSnippetsView = (RecyclerView) findViewById(R.id.snippets_card_list);
-            mSnippetsView.setVisibility(View.VISIBLE);
+            mCardsManager =
+                    new NewTabPageCardsManager(snippetsManager, mSnippetsView, mContentView);
+
             RecordHistogram.recordEnumeratedHistogram(SnippetsManager.SNIPPETS_STATE_HISTOGRAM,
                     SnippetsManager.SNIPPETS_SHOWN, SnippetsManager.NUM_SNIPPETS_ACTIONS);
-            mSnippetsView.setLayoutManager(new LinearLayoutManager(getContext()));
             mSnippetsView.addOnScrollListener(new RecyclerView.OnScrollListener() {
                 private boolean mScrolledOnce = false;
                 @Override
@@ -373,7 +389,6 @@ public class NewTabPageView extends FrameLayout
                             SnippetsManager.NUM_SNIPPETS_ACTIONS);
                 }
             });
-            snippetsManager.setSnippetsView(mSnippetsView);
         }
     }
 
@@ -442,12 +457,22 @@ public class NewTabPageView extends FrameLayout
         if (mDisableUrlFocusChangeAnimations) return;
 
         float percentage = 0;
-        // During startup the view may not be fully initialized, so we only calculate the current
-        // percentage if some basic view properties are sane.
-        if (mScrollView.getHeight() != 0 && mSearchBoxView.getTop() != 0) {
-            int scrollY = mScrollView.getScrollY();
-            percentage = Math.max(
-                    0f, Math.min(1f, scrollY / (float) mSearchBoxView.getTop()));
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
+            // During startup the view may not be fully initialized, so we only calculate the

[newt (away), 2016/03/23 05:29:38]

fix wrapping and move this comment above the if-else statement so it isn't
duplicated.

[May, 2016/03/23 19:22:56]

Done.

+            // current
+            // percentage if some basic view properties are sane.
+            if (mSnippetsView.getHeight() != 0 && mSearchBoxView.getTop() != 0) {
+                int scrollY = mSnippetsView.computeVerticalScrollOffset();
+                percentage = Math.max(0f, Math.min(1f, scrollY / (float) mSearchBoxView.getTop()));
+            }
+        } else {
+            // During startup the view may not be fully initialized, so we only calculate the
+            // current
+            // percentage if some basic view properties are sane.
+            if (mScrollView.getHeight() != 0 && mSearchBoxView.getTop() != 0) {
+                int scrollY = getVerticalScroll();
+                percentage = Math.max(0f, Math.min(1f, scrollY / (float) mSearchBoxView.getTop()));
+            }
         }
 
         updateVisualsForToolbarTransition(percentage);
@@ -457,12 +482,21 @@ public class NewTabPageView extends FrameLayout
         }
     }
 
+    private void initializeSearchBoxRecyclerViewScrollHandling() {
+        mSnippetsView.addOnScrollListener(new RecyclerView.OnScrollListener() {
+            @Override
+            public void onScrolled(RecyclerView recyclerView, int dx, int dy) {
+                updateSearchBoxOnScroll();
+            }
+        });
+    }
+
     private void initializeSearchBoxScrollHandling() {
         final Runnable mSnapScrollRunnable = new Runnable() {
             @Override
             public void run() {
                 if (!mPendingSnapScroll) return;
-                int scrollY = mScrollView.getScrollY();
+                int scrollY = getVerticalScroll();

[newt (away), 2016/03/23 05:29:38]

why change this? You're using mScrollView explicitly down below, so I'd just
keep this as mScrollView.getScrollY()

[May, 2016/03/23 19:22:56]

Reverted.

                 int dividerTop = mMostVisitedLayout.getTop() - mContentView.getPaddingTop();
                 if (scrollY > 0 && scrollY < dividerTop) {
                     mScrollView.smoothScrollTo(0, scrollY < (dividerTop / 2) ? 0 : dividerTop);
@@ -629,8 +663,15 @@ public class NewTabPageView extends FrameLayout
      * @see #setUrlFocusChangeAnimationPercent
      */
     private void setUrlFocusChangeAnimationPercentInternal(float percent) {
-        mContentView.setTranslationY(percent * (-mMostVisitedLayout.getTop()
-                + mScrollView.getScrollY() + mContentView.getPaddingTop()));
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {

[newt (away), 2016/03/23 05:29:38]

Extract the common logic here, e.g.

    int scrollOffset = mAreSnippetsEnabled ? ... : ...;
    mContentView.setTranslationY(...);

[May, 2016/03/23 19:22:56]

Done.

+            mContentView.setTranslationY(percent
+                    * (-mMostVisitedLayout.getTop() + mSnippetsView.computeVerticalScrollOffset()
+                                                 + mContentView.getPaddingTop()));
+        } else {
+            mContentView.setTranslationY(
+                    percent * (-mMostVisitedLayout.getTop() + getVerticalScroll()
+                                      + mContentView.getPaddingTop()));
+        }
         updateVisualsForToolbarTransition(percent);
     }
 
@@ -666,12 +707,18 @@ public class NewTabPageView extends FrameLayout
                 searchBoxY + mSearchBoxView.getHeight() - mSearchBoxView.getPaddingBottom());
 
         transformedBounds.set(originalBounds);
-        View view = (View) mSearchBoxView.getParent();
-        while (view != null) {
-            transformedBounds.offset(-view.getScrollX(), -view.getScrollY());
-            if (view == this) break;
-            transformedBounds.offset((int) view.getX(), (int) view.getY());
-            view = (View) view.getParent();
+
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
+            transformedBounds.offset(-getHorizontalScroll(), -getVerticalScroll());

[newt (away), 2016/03/23 05:29:38]

I'd suggest always using the looping logic below -- not just for the no-snippets
case. It appears that the only adjustment you need to make is in computing the
scroll offset when handling the RecyclerView (use computeVerticalScrollOffset()
instead of getScrollY()). In other words, you could keep the old code, but
replace line 671 with this:

    if (view instanceof RecyclerView) {
        transformedBounds.offset(-view.computeHorizontalScrollOffset(),
-view.computeVerticalScrollOffset());
    } else {
        transformedBounds.offset(-view.getScrollX(), -view.getScrollY());
    }

[May, 2016/03/23 19:22:56]

Done.

+            transformedBounds.offset((int) mSnippetsView.getX(), (int) mSnippetsView.getY());
+        } else {
+            View view = (View) mSearchBoxView.getParent();
+            while (view != null) {
+                transformedBounds.offset(-getHorizontalScroll(), -getVerticalScroll());

[newt (away), 2016/03/23 05:29:38]

This isn't right. We shouldn't be offsetting by the ScrollView's scroll each
time we go up a parent level; we should be offsetting by view.getScrollX() each
time (as in the old code).

[May, 2016/03/23 19:22:56]

Good catch. Fixed.

+                if (view == this) break;
+                transformedBounds.offset((int) view.getX(), (int) view.getY());
+                view = (View) view.getParent();
+            }
         }
     }
 
@@ -732,10 +779,9 @@ public class NewTabPageView extends FrameLayout
                     - mMostVisitedDesign.getMostVisitedLayoutBleed(), MeasureSpec.EXACTLY);
             int searchBoxHeight = MeasureSpec.makeMeasureSpec(
                     mSearchBoxView.getMeasuredHeight(), MeasureSpec.EXACTLY);
-            int logoHeight = MeasureSpec.makeMeasureSpec(
-                    mSearchProviderLogoView.getMeasuredHeight(), MeasureSpec.EXACTLY);
             mSearchBoxView.measure(mostVisitedWidth, searchBoxHeight);
-            mSearchProviderLogoView.measure(mostVisitedWidth, logoHeight);
+            mContentView.setMostVisitedWidth(mMostVisitedLayout.getMeasuredWidth()
+                    - mMostVisitedDesign.getMostVisitedLayoutBleed());
         }
     }
 
@@ -746,10 +792,8 @@ public class NewTabPageView extends FrameLayout
     boolean shouldCaptureThumbnail() {
         if (getWidth() == 0 || getHeight() == 0) return false;
 
-        return mSnapshotMostVisitedChanged
-                || getWidth() != mSnapshotWidth
-                || getHeight() != mSnapshotHeight
-                || mScrollView.getScrollY() != mSnapshotScrollY;
+        return mSnapshotMostVisitedChanged || getWidth() != mSnapshotWidth
+                || getHeight() != mSnapshotHeight || getVerticalScroll() != mSnapshotScrollY;
     }
 
     /**
@@ -761,7 +805,7 @@ public class NewTabPageView extends FrameLayout
         ViewUtils.captureBitmap(this, canvas);
         mSnapshotWidth = getWidth();
         mSnapshotHeight = getHeight();
-        mSnapshotScrollY = mScrollView.getScrollY();
+        mSnapshotScrollY = getVerticalScroll();
         mSnapshotMostVisitedChanged = false;
     }
 
@@ -1025,4 +1069,20 @@ public class NewTabPageView extends FrameLayout
             }
         }
     }
+
+    private int getVerticalScroll() {
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
+            return mSnippetsView.computeVerticalScrollOffset();
+        } else {
+            return mScrollView.getScrollY();
+        }
+    }
+
+    private int getHorizontalScroll() {

[newt (away), 2016/03/23 05:29:38]

I don't think this method needs to exist (see comments above).

[May, 2016/03/23 19:22:56]

Done.

+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.NTP_SNIPPETS)) {
+            return mSnippetsView.computeHorizontalScrollOffset();
+        } else {
+            return mScrollView.getScrollX();
+        }
+    }
 }