Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user the…

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 58b58b1ecf6771307c3912d3827895a7bb237942..a475a02933141b8f62fb55d146228023c538bb06 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
@@ -45,6 +45,7 @@ import org.chromium.chrome.browser.ntp.LogoBridge.LogoObserver;
 import org.chromium.chrome.browser.ntp.MostVisitedItem.MostVisitedItemManager;
 import org.chromium.chrome.browser.ntp.NewTabPage.OnSearchBoxScrollListener;
 import org.chromium.chrome.browser.ntp.cards.NewTabPageAdapter;
+import org.chromium.chrome.browser.ntp.cards.NewTabPageListItem;
 import org.chromium.chrome.browser.ntp.cards.NewTabPageRecyclerView;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
 import org.chromium.chrome.browser.ntp.snippets.SnippetItemDecoration;
@@ -76,14 +77,14 @@ public class NewTabPageView extends FrameLayout
      * Indicates which UI mode we are using. Should be checked when manipulating some members, as
      * they may be unused or {@code null} depending on the mode.
      */
-    private boolean mUseCardsUi;
+    private static boolean USE_CARDS_UI;

[May, 2016/05/05 16:28:22]

ALL_CAPS only for static final, otherwise use sAbcXyz

http://source.android.com/source/code-style.html#follow-field-naming-conventions

[Bernhard Bauer, 2016/05/05 16:46:04]

SHOUTING_CASE is only used for constants (which are final). It also feels a bit
weird to assign a static variable from a constructor argument. We could keep it
as a member variable, but make it final?

[mcwilliams, 2016/05/05 17:39:47]

Done.

[mcwilliams, 2016/05/05 17:39:47]

Done.

 
     // Note: Only one of these will be valid at a time, depending on if we are using the old NTP
     // (NewTabPageScrollView) or the new NTP with cards (NewTabPageRecyclerView).
     private NewTabPageScrollView mScrollView;
     private NewTabPageRecyclerView mRecyclerView;
 
-    private NewTabPageLayout mContentView;
+    private NewTabPageLayout mNewTabPageLayout;
     private LogoView mSearchProviderLogoView;
     private View mSearchBoxView;
     private ImageView mVoiceSearchButton;
@@ -92,7 +93,7 @@ public class NewTabPageView extends FrameLayout
     private View mNoSearchLogoSpacer;
 
     /** Adapter for {@link #mRecyclerView}. Will be {@code null} when using the old UI */
-    private NewTabPageAdapter mNtpAdapter;
+    private NewTabPageAdapter mNewTabPageAdapter;
 
     private OnSearchBoxScrollListener mSearchBoxScrollListener;
 
@@ -119,6 +120,8 @@ public class NewTabPageView extends FrameLayout
     private int mSnapshotHeight;
     private int mSnapshotScrollY;
 
+    private int peekingCardVerticalScrollStart;

[May, 2016/05/05 16:28:22]

Unused variable?

[Bernhard Bauer, 2016/05/05 16:46:04]

Member variables should start with "m" (and continue in CamelCase, i.e.
mPeekingCardVerticalScrollStart).

[mcwilliams, 2016/05/05 17:39:46]

Done.

[mcwilliams, 2016/05/05 17:39:47]

Very much used

+
     /**
      * Manages the view interaction with the rest of the system.
      */
@@ -271,34 +274,36 @@ public class NewTabPageView extends FrameLayout
         mManager = manager;
         ViewStub stub = (ViewStub) findViewById(R.id.new_tab_page_layout_stub);
 
-        mUseCardsUi = useCardsUi;
-        if (mUseCardsUi) {
+        USE_CARDS_UI = useCardsUi;
+        if (USE_CARDS_UI) {
             stub.setLayoutResource(R.layout.new_tab_page_recycler_view);
             mRecyclerView = (NewTabPageRecyclerView) stub.inflate();
 
             // Don't attach now, the recyclerView itself will determine when to do it.
-            mContentView = (NewTabPageLayout) LayoutInflater.from(getContext())
-                    .inflate(R.layout.new_tab_page_layout, mRecyclerView, false);
+            mNewTabPageLayout =
+                    (NewTabPageLayout) LayoutInflater.from(getContext())
+                            .inflate(R.layout.new_tab_page_layout, mRecyclerView, false);
 
             // Tailor the LayoutParams for the snippets UI, as the configuration in the XML is
             // made for the ScrollView UI.
-            ViewGroup.LayoutParams params = mContentView.getLayoutParams();
+            ViewGroup.LayoutParams params = mNewTabPageLayout.getLayoutParams();
             params.height = ViewGroup.LayoutParams.WRAP_CONTENT;
         } else {
             stub.setLayoutResource(R.layout.new_tab_page_scroll_view);
             mScrollView = (NewTabPageScrollView) stub.inflate();
             mScrollView.enableBottomShadow(SHADOW_COLOR);
-            mContentView = (NewTabPageLayout) findViewById(R.id.ntp_content);
+            mNewTabPageLayout = (NewTabPageLayout) findViewById(R.id.ntp_content);
         }
 
         mMostVisitedDesign = new MostVisitedDesign(getContext());
         mMostVisitedLayout =
-                (MostVisitedLayout) mContentView.findViewById(R.id.most_visited_layout);
+                (MostVisitedLayout) mNewTabPageLayout.findViewById(R.id.most_visited_layout);
         mMostVisitedDesign.initMostVisitedLayout(mMostVisitedLayout, searchProviderHasLogo);
 
-        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);
+        mSearchProviderLogoView =
+                (LogoView) mNewTabPageLayout.findViewById(R.id.search_provider_logo);
+        mSearchBoxView = mNewTabPageLayout.findViewById(R.id.search_box);
+        mNoSearchLogoSpacer = mNewTabPageLayout.findViewById(R.id.no_search_logo_spacer);
 
         final TextView searchBoxTextView = (TextView) mSearchBoxView
                 .findViewById(R.id.search_box_text);
@@ -332,7 +337,7 @@ public class NewTabPageView extends FrameLayout
             }
         });
 
-        mVoiceSearchButton = (ImageView) mContentView.findViewById(R.id.voice_search_button);
+        mVoiceSearchButton = (ImageView) mNewTabPageLayout.findViewById(R.id.voice_search_button);
         mVoiceSearchButton.setOnClickListener(new View.OnClickListener() {
             @Override
             public void onClick(View v) {
@@ -368,7 +373,7 @@ public class NewTabPageView extends FrameLayout
             }
         } else {
             ((ViewGroup) toolbar.getParent()).removeView(toolbar);
-            if (!mUseCardsUi) {
+            if (!USE_CARDS_UI) {
                 // Only remove if we're using the old NTP view, the new one does not use a
                 // ScrollView
                 FrameLayout.LayoutParams params =
@@ -386,12 +391,14 @@ public class NewTabPageView extends FrameLayout
                 mMostVisitedDesign.getNumberOfTiles(searchProviderHasLogo));
 
         // Set up snippets
-        if (mUseCardsUi) {
-            mNtpAdapter = new NewTabPageAdapter(mManager, mContentView);
-            mRecyclerView.setAdapter(mNtpAdapter);
+        if (USE_CARDS_UI) {
+            mNewTabPageAdapter = new NewTabPageAdapter(mManager, mNewTabPageLayout);
+            mRecyclerView.setAdapter(mNewTabPageAdapter);
+            mRecyclerView.setNewTabPageLayout(mNewTabPageLayout);

[May, 2016/05/05 16:28:22]

I don't think this is necessary anymore

[mcwilliams, 2016/05/05 17:39:47]

Done.

 
             // Set up swipe-to-dismiss
-            ItemTouchHelper helper = new ItemTouchHelper(mNtpAdapter.getItemTouchCallbacks());
+            ItemTouchHelper helper =
+                    new ItemTouchHelper(mNewTabPageAdapter.getItemTouchCallbacks());
             helper.attachToRecyclerView(mRecyclerView);
 
             NewTabPageUma.recordSnippetAction(NewTabPageUma.SNIPPETS_ACTION_SHOWN);
@@ -408,6 +415,7 @@ public class NewTabPageView extends FrameLayout
             });
             initializeSearchBoxRecyclerViewScrollHandling();
             mRecyclerView.addItemDecoration(new SnippetItemDecoration());
+            updatePeekingCard();
         } else {
             initializeSearchBoxScrollHandling();
         }
@@ -419,12 +427,12 @@ public class NewTabPageView extends FrameLayout
         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.
-        View wrapperView = mUseCardsUi ? mRecyclerView : mScrollView;
+        View wrapperView = USE_CARDS_UI ? mRecyclerView : mScrollView;
         if (wrapperView.getHeight() != 0 && mSearchBoxView.getTop() != 0) {
             // getVerticalScroll is valid only for the RecyclerView if the first item is visible.
             // Luckily, if the first item is not visible, we know the toolbar transition should
             // be 100%.
-            if (mUseCardsUi && !mRecyclerView.isFirstItemVisible()) {
+            if (USE_CARDS_UI && !mRecyclerView.isFirstItemVisible()) {
                 percentage = 1f;
             } else {
                 int scrollY = getVerticalScroll();
@@ -439,6 +447,52 @@ public class NewTabPageView extends FrameLayout
         }
     }
 
+    /**
+     * Calculate the peeking card/first snippet bleed and padding when scrolling if it is visible.
+     */
+    private void updatePeekingCard() {

[Bernhard Bauer, 2016/05/05 16:46:04]

updatePeekingCardOnScroll()?

[mcwilliams, 2016/05/05 17:39:46]

It is not just on scroll but also on initialize

+        // Get the first snippet that could display to make the peeking card transition.
+        View firstSnippetView = null;
+        int recyclerViewSize = mNewTabPageAdapter.getItemCount();

[May, 2016/05/05 16:28:22]

Not really the recyclerViewSize, it's the adapterSize

[mcwilliams, 2016/05/05 17:39:46]

Done.

+        for (int i = 0; i < recyclerViewSize; i++) {
+            if (mNewTabPageAdapter.getItemViewType(i) == NewTabPageListItem.VIEW_TYPE_SNIPPET) {
+                firstSnippetView = mRecyclerView.getLayoutManager().findViewByPosition(i);
+                break;
+            }
+        }
+
+        // If first snippet exists change the parameters from peeking card with bleed to full page
+        // card when scrolling.
+        if (firstSnippetView != null && firstSnippetView.isShown()) {
+            // Value used for max peeking card height and padding.
+            int maxPadding = mNewTabPageLayout.getContext().getResources().getDimensionPixelSize(

[PEConn, 2016/05/05 16:31:28]

mNewTabPageLayout.getContext().getResources()

can be replaced with just:

getResources()

(the view has access to the Context and the Resources)

[mcwilliams, 2016/05/05 17:39:46]

Done.

+                    R.dimen.snippets_padding_and_peeking_card_height);
+
+            // As the user scrolls, the bleed increases or decreases.
+            int bleed = maxPadding - (getVerticalScroll() - peekingCardVerticalScrollStart);
+
+            // Never let the bleed go into negative digits.
+            bleed = Math.max(bleed, 0);
+            // Never let the bleed be greater than the maxPadding.
+            bleed = Math.min(bleed, maxPadding);
+
+            // Modify the padding so as the margin increases, the padding decreases so the cards
+            // content does not shift.
+            firstSnippetView.setPadding(
+                    maxPadding - bleed, maxPadding, maxPadding - bleed, maxPadding);

[PEConn, 2016/05/05 16:31:28]

Maybe add a comment that the 2nd and 4th arguments (the top and bottom padding)
happen to be equal to maxPadding, and we're not actually changing them.

[mcwilliams, 2016/05/05 17:39:46]

Done.

+
+            // Modify the margin to grow the card from bleed to full width.
+            RecyclerView.LayoutParams params =
+                    (RecyclerView.LayoutParams) firstSnippetView.getLayoutParams();
+            params.leftMargin = bleed;
+            params.rightMargin = bleed;
+        }
+    }
+
+    /**
+     * Sets up scrolling when snippets are enabled. It adds scroll listeners and touch listeners to

[PEConn, 2016/05/05 16:31:28]

I'd argue this comment is a bit too indepth - the first part is OK, but the
'adds scroll listeners and touch listeners' is a bit too close to
implementation. But this is probably just personal preference.

[mcwilliams, 2016/05/05 17:39:47]

Acknowledged.

+     * the RecyclerView.
+     */
     private void initializeSearchBoxRecyclerViewScrollHandling() {
         final Runnable mSnapScrollRunnable = new Runnable() {
             @Override
@@ -449,13 +503,14 @@ public class NewTabPageView extends FrameLayout
                 // computeVerticalScrollOffset only takes into account visible items).
                 // Luckily, we only need to perform the calculations if the first item is visible.
                 if (!mRecyclerView.isFirstItemVisible()) return;
+
                 final int currentScroll = mRecyclerView.computeVerticalScrollOffset();
 
                 // If snapping to Most Likely or to Articles, the omnibox will be at the top of the
                 // page, so offset the scroll so the scroll targets appear below it.
-                final int omniBoxHeight = mContentView.getPaddingTop();
+                final int omniBoxHeight = mNewTabPageLayout.getPaddingTop();
                 final int topOfMostLikelyScroll = mMostVisitedLayout.getTop() - omniBoxHeight;
-                final int topOfSnippetsScroll = mContentView.getHeight() - omniBoxHeight;
+                final int topOfSnippetsScroll = mNewTabPageLayout.getHeight() - omniBoxHeight;
 
                 assert currentScroll >= 0;
                 // Do not do any scrolling if the user is currently viewing articles.
@@ -468,10 +523,11 @@ public class NewTabPageView extends FrameLayout
                         mRecyclerView.getHeight() < mMostVisitedLayout.getBottom();
 
                 int targetScroll;
-                if (currentScroll < mContentView.getHeight() / 3) {
+                if (currentScroll < mNewTabPageLayout.getHeight() / 3) {
                     // In either case, if in the top 1/3 of the original NTP, snap to the top.
                     targetScroll = 0;
-                } else if (snapToMostLikely && currentScroll < mContentView.getHeight() * 2 / 3) {
+                } else if (
+                        snapToMostLikely && currentScroll < mNewTabPageLayout.getHeight() * 2 / 3) {

[May, 2016/05/05 16:28:22]

unnecessary line break

[mcwilliams, 2016/05/05 17:39:47]

If I don't line break it is over 100 chars?

[Bernhard Bauer, 2016/05/05 18:04:40]

I think what May meant was that breaking right after the opening parenthesis is
a bit weird. In particular as there are multiple conditions, you should be able
to break after |snapToMostLikely|, for example.

[mcwilliams, 2016/05/05 18:21:11]

Done.

                     // If in the middle 1/3 and we are snapping to Most Likely, snap to it.
                     targetScroll = topOfMostLikelyScroll;
                 } else {
@@ -480,6 +536,10 @@ public class NewTabPageView extends FrameLayout
                 }
 
                 mRecyclerView.smoothScrollBy(0, targetScroll - currentScroll);
+
+                // Set the peeking card vertical scroll start for the snapped view.
+                peekingCardVerticalScrollStart = targetScroll;
+
                 mPendingSnapScroll = false;
             }
         };
@@ -492,6 +552,7 @@ public class NewTabPageView extends FrameLayout
                     mRecyclerView.postDelayed(mSnapScrollRunnable, SNAP_SCROLL_DELAY_MS);
                 }
                 updateSearchBoxOnScroll();
+                updatePeekingCard();
             }
         });
 
@@ -513,13 +574,17 @@ public class NewTabPageView extends FrameLayout
         });
     }
 
+    /**
+     * Sets up scrolling when snippets are disabled. It ads scroll and touch listeners to the scroll

[PEConn, 2016/05/05 16:31:28]

*adds

[Bernhard Bauer, 2016/05/05 16:46:04]

Nit: "adds"

[mcwilliams, 2016/05/05 17:39:47]

Done.

[mcwilliams, 2016/05/05 17:39:47]

Done.

+     * view.
+     */
     private void initializeSearchBoxScrollHandling() {
         final Runnable mSnapScrollRunnable = new Runnable() {
             @Override
             public void run() {
                 if (!mPendingSnapScroll) return;
                 int scrollY = mScrollView.getScrollY();
-                int dividerTop = mMostVisitedLayout.getTop() - mContentView.getPaddingTop();
+                int dividerTop = mMostVisitedLayout.getTop() - mNewTabPageLayout.getPaddingTop();
                 if (scrollY > 0 && scrollY < dividerTop) {
                     mScrollView.smoothScrollTo(0, scrollY < (dividerTop / 2) ? 0 : dividerTop);
                 }
@@ -603,9 +668,9 @@ public class NewTabPageView extends FrameLayout
 
         // Hide or show all the views above the Most Visited items.
         int visibility = hasLogo ? View.VISIBLE : View.GONE;
-        int childCount = mContentView.getChildCount();
+        int childCount = mNewTabPageLayout.getChildCount();
         for (int i = 0; i < childCount; i++) {
-            View child = mContentView.getChildAt(i);
+            View child = mNewTabPageLayout.getChildAt(i);
             if (child == mMostVisitedLayout) break;
             // Don't change the visibility of a ViewStub as that will automagically inflate it.
             if (child instanceof ViewStub) continue;
@@ -687,12 +752,16 @@ public class NewTabPageView extends FrameLayout
         // Only apply the scrolling offset when not using the cards UI, as there we will either snap
         // to the top of the page (with scrolling offset 0), or snap to below the fold, where Most
         // Likely items are not visible anymore, so they will stay out of sight.
-        int scrollOffset = mUseCardsUi ? 0 : mScrollView.getScrollY();
-        mContentView.setTranslationY(percent
-                * (-mMostVisitedLayout.getTop() + scrollOffset + mContentView.getPaddingTop()));
+        int scrollOffset = USE_CARDS_UI ? 0 : mScrollView.getScrollY();
+        mNewTabPageLayout.setTranslationY(percent * (-mMostVisitedLayout.getTop() + scrollOffset
+                                                            + mNewTabPageLayout.getPaddingTop()));
         updateVisualsForToolbarTransition(percent);
     }
 
+    /**
+     * Updates the opacity of the fake omnibox and Google logo when scrolling.
+     * @param transitionPercentage
+     */
     private void updateVisualsForToolbarTransition(float transitionPercentage) {
         // Complete the full alpha transition in the first 40% of the animation.
         float searchUiAlpha =
@@ -893,7 +962,7 @@ public class NewTabPageView extends FrameLayout
             // The page contents are initially hidden; otherwise they'll be drawn centered on the
             // page before the most visited sites are available and then jump upwards to make space
             // once the most visited sites are available.
-            mContentView.setVisibility(View.VISIBLE);
+            mNewTabPageLayout.setVisibility(View.VISIBLE);
         }
         mSnapshotMostVisitedChanged = true;
     }
@@ -1074,11 +1143,28 @@ public class NewTabPageView extends FrameLayout
         }
     }
 
+    @Override
+    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
+        // Set the parent scroll view port height in the layout.
+        if (mNewTabPageLayout != null) {
+            mNewTabPageLayout.setParentScrollViewportHeight(MeasureSpec.getSize(heightMeasureSpec));

[May, 2016/05/05 16:28:22]

If we're doing this here, do we still need the
mNewTabPageLayout.setParentScrollViewportHeight() in NewTabPageScrollView?

[mcwilliams, 2016/05/05 17:39:47]

No, removed

+        }
+        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
+    }
+
     private int getVerticalScroll() {
-        if (mUseCardsUi) {
+        if (USE_CARDS_UI) {
             return mRecyclerView.computeVerticalScrollOffset();
         } else {
             return mScrollView.getScrollY();
         }
     }
+
+    /**
+     * Are snippets currently enabled.
+     * @return
+     */
+    public static boolean isUseCardsUiEnabled() {
+        return USE_CARDS_UI;

[PEConn, 2016/05/05 16:31:28]

What if the NewTabPageView hasn't been initialized?

I don't think we'd come across this in this CL, but 'isUseCardsUiEnabled' sounds
pretty generic and the fact its a static function implies it should work
regardless of whether an instance of NewTabPageView has had a certain function
run on it.

[mcwilliams, 2016/05/05 17:39:47]

Done.

+    }
 }