Add the snippet header back into the code and show the snippet header when the user scrolls the sni…

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 939c2d38d5920012c76e1bffebc705787f1c40c4..2c83922c77532e1369e53b3e4d6486e223994b6d 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
@@ -122,6 +122,8 @@ public class NewTabPageView extends FrameLayout
 
     private int mPeekingCardVerticalScrollStart;
 
+    private int mViewPortHeight;

[May, 2016/05/09 16:15:19]

We don't seem to use this in this file.

[mcwilliams, 2016/05/10 09:26:05]

Done.

+
     /**
      * Manages the view interaction with the rest of the system.
      */
@@ -410,6 +412,7 @@ public class NewTabPageView extends FrameLayout
             initializeSearchBoxRecyclerViewScrollHandling();
             mRecyclerView.addItemDecoration(new SnippetItemDecoration());
             updatePeekingCard();
+            updateSnippetsHeaderDisplay();
         } else {
             initializeSearchBoxScrollHandling();
         }
@@ -487,6 +490,75 @@ public class NewTabPageView extends FrameLayout
     }
 
     /**
+     * Show the snippets header when the user scrolls down and snippet articles starts reaching the
+     * top of the screen.
+     */
+    private void updateSnippetsHeaderDisplay() {
+        // Get the snippet header view.
+        View snippetHeader = null;
+        int adapterSize = mNewTabPageAdapter.getItemCount();
+        for (int i = 0; i < adapterSize; i++) {

[May, 2016/05/09 16:15:19]

Can you pull out this section here into a private helper function and change
updatePeekingCard() to use that?

[mcwilliams, 2016/05/10 09:26:04]

Done.

+            if (mNewTabPageAdapter.getItemViewType(i) == NewTabPageListItem.VIEW_TYPE_HEADER) {
+                snippetHeader = mRecyclerView.getLayoutManager().findViewByPosition(i);
+                break;
+            }
+        }
+
+        // Start doing the calculations if the snippet header is currently shown on screen.
+        if (snippetHeader != null && snippetHeader.isShown()) {

[Bernhard Bauer, 2016/05/09 17:47:41]

Return early otherwise?

[mcwilliams, 2016/05/10 09:26:05]

Seems a little verbose to return when this is all the function does and will
exit now naturally

[Bernhard Bauer, 2016/05/10 14:14:19]

Early returns are the idiomatic style in Chrome/Android code, as it prevents
indentation "pyramids", and in particular with the Android style four-space
indent, that can happen pretty fast.

[mcwilliams, 2016/05/10 14:25:37]

Makes sense :) thanks

+            // Get the max snippet header height.
+            int maxSnippetHeaderHeight =
+                    getResources().getDimensionPixelSize(R.dimen.snippets_article_header_height);
+            // We need a measurement of when to start modifying the display of article header.
+            int numberHeaderHeight = 2;

[May, 2016/05/09 16:15:19]

I have no idea what this is. Can you clarify? Also if it's never going to
change, make it final.

[mcwilliams, 2016/05/10 09:26:05]

Done.

[mcwilliams, 2016/05/10 09:26:05]

Done.

+            // Used to indicate when to start modifying the snippet header.
+            int heightToStartChangingHeader = maxSnippetHeaderHeight * numberHeaderHeight;
+            int snippetHeaderTop = snippetHeader.getTop();
+            int heightOmnibox = mNewTabPageLayout.getPaddingTop();

[May, 2016/05/09 16:15:19]

Can you get the omnibox height directly rather than using the fact that the
padding happens to be the same height?

[Bernhard Bauer, 2016/05/09 17:47:41]

Nit: |omniboxHeight|?

[mcwilliams, 2016/05/10 09:26:05]

1. This is already being done here:
initializeSearchBoxRecyclerViewScrollHandling line 574
2. Unfortunately I do not have access to the ToolbarPhone from NTPLayout. The
padding is set for the layout in the xml as:
android:paddingTop="@dimen/toolbar_height_no_shadow"

Any suggestions?

[mcwilliams, 2016/05/10 09:26:05]

Done.

[May, 2016/05/10 13:46:09]

Yeah, seems like there's precedent for using this. I don't see any clear
alternatives, let's leave it as is.

[mcwilliams, 2016/05/10 13:59:09]

Acknowledged.

+
+            // Check if snippet header top - minus the height of the omnibox is less than value to

[May, 2016/05/09 16:15:19]

The first line of the comment is confusing to me. It's describing the
calculations rather than saying what you're trying to achieve (e.g., "Check if
the top of the snippets is getting to within x of the omnibox, and if to start
showing the header")

[mcwilliams, 2016/05/10 09:26:05]

Done.

+            // start changing the header. If this is true, change the snippets
+            // header display otherwise hide the snippet header.
+            if (snippetHeaderTop < heightOmnibox + heightToStartChangingHeader) {
+                // The amount of space the article header has scrolled into the
+                // heightToStartChangingHeader.

[Bernhard Bauer, 2016/05/09 17:47:41]

Use |pipe| symbols for variable names.

[mcwilliams, 2016/05/10 09:26:04]

Done.

+                int amountScrolledIntoHeaderSpace =
+                        heightToStartChangingHeader - (snippetHeaderTop - heightOmnibox);
+
+                // Remove the numberHeaderHeight to get the actual header height we want to
+                // display. Never let the height be more than the maxSnippetHeaderHeight.
+                int headerHeight = Math.min(
+                        amountScrolledIntoHeaderSpace / numberHeaderHeight, maxSnippetHeaderHeight);
+
+                // Get the alpha for the snippet header.
+                float alpha = (float) headerHeight / maxSnippetHeaderHeight;
+
+                // Set the snippet header height and alpha.
+                setSnippetHeaderDisplay(snippetHeader, headerHeight, alpha);
+            } else {
+                // Don't show the snippet header.
+                setSnippetHeaderDisplay(snippetHeader, 0, 0);
+            }
+        }
+    }
+
+    /**
+     * Set the snippet header display. Set the params with height and the alpha of the header.
+     * @param snippetHeader

[Bernhard Bauer, 2016/05/09 17:47:41]

Parameters don't match.

[mcwilliams, 2016/05/10 09:26:05]

Done.

+     * @param height
+     */
+    private void setSnippetHeaderDisplay(View snippetHeader, int height, float alpha) {

[May, 2016/05/09 16:15:19]

This can be a static function.

[Bernhard Bauer, 2016/05/09 17:47:40]

Or just inline this and move the call sites out of the if-statement?

[mcwilliams, 2016/05/10 09:26:04]

Agree, it as more complex when messing with margins but figured I could simplify
and never removed this

[mcwilliams, 2016/05/10 09:26:05]

Acknowledged.

+        // Set the alpha of the header.
+        snippetHeader.setAlpha(alpha);
+
+        // Set the params of the header.
+        RecyclerView.LayoutParams params =
+                (RecyclerView.LayoutParams) snippetHeader.getLayoutParams();
+        params.height = height;
+        snippetHeader.setLayoutParams(params);
+    }
+
+    /**
      * Sets up scrolling when snippets are enabled. It adds scroll listeners and touch listeners to
      * the RecyclerView.
      */
@@ -501,7 +573,7 @@ public class NewTabPageView extends FrameLayout
                 // Luckily, we only need to perform the calculations if the first item is visible.
                 if (!mRecyclerView.isFirstItemVisible()) return;
 
-                final int currentScroll = mRecyclerView.computeVerticalScrollOffset();
+                final int currentScroll = getVerticalScroll();
 
                 // 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.
@@ -549,6 +621,7 @@ public class NewTabPageView extends FrameLayout
                 }
                 updateSearchBoxOnScroll();
                 updatePeekingCard();
+                updateSnippetsHeaderDisplay();
             }
         });
 
@@ -1143,7 +1216,8 @@ public class NewTabPageView extends FrameLayout
     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));
+            mViewPortHeight = MeasureSpec.getSize(heightMeasureSpec);
+            mNewTabPageLayout.setParentScrollViewportHeight(mViewPortHeight);
         }
         super.onMeasure(widthMeasureSpec, heightMeasureSpec);
     }