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

chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
index 16d74ade3a6fa20d24fc1f42f006bc3241f95486..99ad652b5b2e7dd8771ff74aac1e4e73c48368bb 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsManager.java
@@ -6,11 +6,13 @@ package org.chromium.chrome.browser.ntp.snippets;
 
 import android.graphics.drawable.Drawable;
 import android.os.AsyncTask;
-import android.support.v7.widget.RecyclerView;
+import android.view.View;
 import android.widget.ImageView;
 
 import org.chromium.base.Log;
 import org.chromium.base.StreamUtil;
+import org.chromium.chrome.browser.ntp.NewTabPageCardsManager;
+import org.chromium.chrome.browser.ntp.NewTabPageCardsManager.NewTabPageListItem;
 import org.chromium.chrome.browser.ntp.NewTabPageView.NewTabPageManager;
 import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge.SnippetsObserver;
 import org.chromium.chrome.browser.profiles.Profile;
@@ -21,22 +23,14 @@ import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Observable;
+import java.util.Observer;
 
 /**
  * A class that stores and manages the article snippets that will be shown on the NTP.
  */
-public class SnippetsManager {
+public class SnippetsManager extends Observable {

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

Observable/Observer aren't commonly used in our codebase. In fact, they aren't
used at all, besides here! They have some pretty serious downsides as I see it:
 - The method names are so generic that the code the calls and implements those
methods is hard to understand at a glance
 - The observed quantity isn't documented anywhere. One has to examine the
implementation to understand what is really being observed. Even the *type* of
the observed object isn't documented (since these interfaces predate generics),
so one must make a blind cast from Object to the hopefully correct type.
 - The generic and reused method names thwart grep and even use Eclipse's "find
all callsites" feature

I'd suggest defining and implementing an explicit SnippetsChanged interface.
However, if you follow my suggestion below about combining the Manager classes,
you won't need to implement any observer interface at all.

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

Changed to a custom implementation.

     private static final String TAG = "SnippetsManager";
-
-    /**
-     * Describes the header of a group of similar card snippets
-     */
-    public static final int SNIPPET_ITEM_TYPE_HEADER = 1;
-    /**
-     * Describes a single card snippet
-     */
-    public static final int SNIPPET_ITEM_TYPE_SNIPPET = 2;
-
     /**
      * Enum values for recording {@code SNIPPETS_STATE_HISTOGRAM} histogram.
      * Stored here as it is recorded in multiple places.
@@ -48,46 +42,30 @@ public class SnippetsManager {
     public static final String SNIPPETS_STATE_HISTOGRAM = "NewTabPage.Snippets.Interactions";
 
     private NewTabPageManager mNewTabPageManager;
-    private SnippetsAdapter mDataAdapter;
+    private List<NewTabPageListItem> mListSnippetItems = new ArrayList<NewTabPageListItem>();

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

nit: s/<NewTabPageListItem>/<>/

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

Done. 

     private SnippetsBridge mSnippetsBridge;
 
-    /** Base type for anything to add to the snippets view
-     */
-    public interface SnippetListItem {
-        /**
-         * Returns the type of this snippet item (SNIPPET_ITEM_TYPE_HEADER or
-         * SNIPPET_ITEM_TYPE_SNIPPET). This is so we can distinguish between different elements
-         * that are held in a single RecyclerView holder.
-         *
-         * @return the type of this list item.
-         */
-        public int getType();
-    }
-
     /** Represents the data for a header of a group of snippets
      */
-    public static class SnippetHeader implements SnippetListItem {
-        public final String mRecommendationBasis;
-
-        public SnippetHeader(String recommendationBasis) {
-            mRecommendationBasis = recommendationBasis;
-        }
+    public static class SnippetHeader implements NewTabPageListItem {
+        public SnippetHeader() {}
 
         @Override
         public int getType() {
-            return SNIPPET_ITEM_TYPE_HEADER;
+            return NewTabPageCardsManager.SNIPPET_ITEM_TYPE_HEADER;
         }
     }
 
     /**
      * Represents the data for a card snippet.
      */
-    public static class SnippetArticle implements SnippetListItem {
+    public static class SnippetArticle implements NewTabPageListItem {

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

SnippetArticle contains more code than SnippetsManager, which is almost empty
besides listening to the SnippetsBridge and dealing with Observable. I'd suggest
moving SnippetArticle into its own separate file.

Also, it seems your Manager classes are having a minor identity crisis.
SnippetsManager, NewTabPageCardsManager, and NewTabPageAdapter don't have
clearly separated roles, AFAICT. SnippetsManager listens for updates to the list
of snippets, then notifies NewTabPageCardsManager, which does nothing but
forward the update to NewTabPageAdapter.

I'd suggest merging all three of these classes into a single class
(NewTabPageAdapter). Splitting code into separate classes is useful only insofar
as there's a clear division of responsibility. In this case, the division seems
arbitrary (caused by organic growth and change of code -- I understand :), so it
makes sense to remove it. (Alternatively, perhaps there is a logical division
between these classes, in which case you could re-divide responsibilities along
those lines.)

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

Moved SnippetArticle into its own file. We still need to do a favicon fetching
and thumbnail caching so it's going to get more complex than it currently is.

I decided not to merge this into the other classes for the reasons mentioned
earlier in this CR. SnippetManager is intended to handle all snippets-related
data. NewTabPageCardsManager is intended to handle data coming from all
providers. At the moment, this is just the above the fold stuff and snippets,
but later we will be adding more data providers.

         public final String mTitle;
         public final String mPublisher;
         public final String mPreviewText;
         public final String mUrl;
         public final String mThumbnailPath;
+        public final long mTimestamp;
         public final int mPosition;
 
         private ThumbnailRenderingTask mThumbnailRenderingTask;
@@ -122,23 +100,29 @@ public class SnippetsManager {
 
             @Override
             protected void onPostExecute(Drawable thumbnail) {
-                mThumbnailView.setImageDrawable(thumbnail);
+                if (thumbnail == null) {
+                    mThumbnailView.setVisibility(View.GONE);
+                } else {
+                    mThumbnailView.setVisibility(View.VISIBLE);

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

We could fade in the images:

    mThumbnailView.setAlpha(0);
    mThumbnailView.animate().alpha(1).duration(...);

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

Fancy! :-D Added.

+                    mThumbnailView.setImageDrawable(thumbnail);
+                }
             }
         }
 
         public SnippetArticle(String title, String publisher, String previewText, String url,
-                String thumbnailPath, int position) {
+                String thumbnailPath, long timestamp, int position) {
             mTitle = title;
             mPublisher = publisher;
             mPreviewText = previewText;
             mUrl = url;
             mThumbnailPath = thumbnailPath;
+            mTimestamp = timestamp;
             mPosition = position;
         }
 
         @Override
         public int getType() {
-            return SNIPPET_ITEM_TYPE_SNIPPET;
+            return NewTabPageCardsManager.SNIPPET_ITEM_TYPE_SNIPPET;
         }
 
         /**
@@ -158,24 +142,27 @@ public class SnippetsManager {
 
     public SnippetsManager(NewTabPageManager tabManager, Profile profile) {
         mNewTabPageManager = tabManager;
-        mDataAdapter = new SnippetsAdapter(this);
         mSnippetsBridge = new SnippetsBridge(profile, new SnippetsObserver() {
             @Override
-            public void onSnippetsAvailable(
-                    String[] titles, String[] urls, String[] thumbnailUrls, String[] previewText) {
-                List<SnippetListItem> listItems = new ArrayList<SnippetListItem>();
+            public void onSnippetsAvailable(String[] titles, String[] urls, String[] thumbnailUrls,
+                    String[] previewText, long[] timestamps) {
+                mListSnippetItems = new ArrayList<NewTabPageListItem>();

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

Do you need to create a new ArrayList here? Seems like mListSnippetItems.clear()
would suffice.

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

Done.

+                if (titles.length > 0) {
+                    mListSnippetItems.add(new SnippetHeader());
+                }
                 for (int i = 0; i < titles.length; ++i) {
-                    SnippetArticle article = new SnippetArticle(
-                            titles[i], "", previewText[i], urls[i], thumbnailUrls[i], i);
-                    listItems.add(article);
+                    SnippetArticle article = new SnippetArticle(titles[i], "", previewText[i],
+                            urls[i], thumbnailUrls[i], timestamps[i], i);
+                    mListSnippetItems.add(article);
                 }
-                mDataAdapter.setSnippetListItems(listItems);
+                setChanged();
+                notifyObservers(mListSnippetItems);
             }
         });
     }
 
-    public void setSnippetsView(RecyclerView mSnippetsView) {
-        mSnippetsView.setAdapter(mDataAdapter);
+    public List<NewTabPageListItem> getSnippetItems() {
+        return mListSnippetItems;
     }
 
     public void loadUrl(String url) {
@@ -185,4 +172,10 @@ public class SnippetsManager {
     public void destroy() {
         mSnippetsBridge.destroy();
     }
+
+    @Override
+    public void addObserver(Observer observer) {
+        super.addObserver(observer);
+        observer.update(this, mListSnippetItems);
+    }
 }