feed v2: fix native view handling bug in recycler

This is the chromium/src side of the change.
It introduces a getViewType() API in ListContentManager,
and implements it.

This CL fixes a secondary bug: if a native view is added, removed,
and then added again to the FeedListContentManager, we would previously
try to wrap the view multiple times, which throws an exception. Now,
we wrap a view at most once.

Bug: 1119516, b/165317997
Change-Id: I2163c0e159c6d434329ea34f0449356a561a0c96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2364965Reviewed-by: Dan H <harringtond@chromium.org>
Reviewed-by: Cathy Li <chili@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800247}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedListContentManager.java

@@ -8,8 +8,11 @@ import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 import android.view.ViewGroup.LayoutParams;
+import android.view.ViewParent;
 import android.widget.FrameLayout;
 
+import androidx.annotation.Nullable;
+
 import org.chromium.chrome.browser.xsurface.FeedActionsHandler;
 import org.chromium.chrome.browser.xsurface.ListContentManager;
 import org.chromium.chrome.browser.xsurface.ListContentManagerObserver;
@@ -79,9 +82,11 @@ public class FeedListContentManager implements ListContentManager {
      * For the content that is supported by the native view.
      */
     public static class NativeViewContent extends FeedContent {
-        private FrameLayout mEnclosingLayout;
         private View mNativeView;
         private int mResId;
+        // An unique ID for this NativeViewContent. This is initially 0, and assigned by
+        // FeedListContentManager when needed.
+        private int mViewType;
 
         /** Holds an inflated native view. */
         public NativeViewContent(String key, View nativeView) {
@@ -104,13 +109,32 @@ public class FeedListContentManager implements ListContentManager {
                 mNativeView =
                         LayoutInflater.from(parent.getContext()).inflate(mResId, parent, false);
             }
-            if (mEnclosingLayout == null) {
-                mEnclosingLayout = new FrameLayout(parent.getContext());
-                mEnclosingLayout.setLayoutParams(
-                        new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT));
-                mEnclosingLayout.addView(mNativeView);
+
+            // If there's already a parent, we have already enclosed this view previously.
+            // This can happen if a native view is added, removed, and added again.
+            ViewParent nativeViewParent = mNativeView.getParent();
+            if (nativeViewParent != null) {
+                assert nativeViewParent instanceof View;
+                return (View) nativeViewParent;
             }
-            return mEnclosingLayout;
+
+            FrameLayout enclosingLayout = new FrameLayout(parent.getContext());
+            enclosingLayout.setLayoutParams(
+                    new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.WRAP_CONTENT));
+            enclosingLayout.addView(mNativeView);
+            return enclosingLayout;
+        }
+
+        int getViewType() {
+            return mViewType;
+        }
+
+        void setViewType(int viewType) {
+            mViewType = viewType;
+        }
+
+        public View getNestedView() {
+            return mNativeView;
         }
 
         @Override
@@ -122,6 +146,7 @@ public class FeedListContentManager implements ListContentManager {
     private ArrayList<FeedContent> mFeedContentList;
     private ArrayList<ListContentManagerObserver> mObservers;
     private final Map<String, Object> mHandlers;
+    private int mPreviousViewType;
 
     FeedListContentManager(
             SurfaceActionsHandler surfaceActionsHandler, FeedActionsHandler feedActionsHandler) {
@@ -251,10 +276,18 @@ public class FeedListContentManager implements ListContentManager {
     }
 
     @Override
-    public View getNativeView(int index, ViewGroup parent) {
-        assert mFeedContentList.get(index).isNativeView();
-        NativeViewContent nativeViewContent = (NativeViewContent) mFeedContentList.get(index);
-        return nativeViewContent.getNativeView(parent);
+    public int getViewType(int position) {
+        assert mFeedContentList.get(position).isNativeView();
+        NativeViewContent content = (NativeViewContent) mFeedContentList.get(position);
+        if (content.getViewType() == 0) content.setViewType(++mPreviousViewType);
+        return content.getViewType();
+    }
+
+    @Override
+    public View getNativeView(int viewType, ViewGroup parent) {
+        NativeViewContent viewContent = findNativeViewByType(viewType);
+        assert viewContent != null;
+        return viewContent.getNativeView(parent);
     }
 
     @Override
@@ -276,4 +309,17 @@ public class FeedListContentManager implements ListContentManager {
     public void removeObserver(ListContentManagerObserver observer) {
         mObservers.remove(observer);
     }
+
+    @Nullable
+    private NativeViewContent findNativeViewByType(int viewType) {
+        // Note: since there's relatively few native views, they're mostly at the front, a linear
+        // search isn't terrible. This function is also called infrequently.
+        for (int i = 0; i < mFeedContentList.size(); i++) {
+            FeedContent item = mFeedContentList.get(i);
+            if (!item.isNativeView()) continue;
+            NativeViewContent nativeContent = (NativeViewContent) item;
+            if (nativeContent.getViewType() == viewType) return nativeContent;
+        }
+        return null;
+    }
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2/NativeViewListRenderer.java

@@ -55,11 +55,9 @@ public class NativeViewListRenderer extends RecyclerView.Adapter<NativeViewListR
 
     @Override
     public ViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
-        // viewType is same as position.
-        int position = viewType;
         View v;
-        if (mManager.isNativeView(position)) {
-            v = mManager.getNativeView(position, parent);
+        if (viewType >= 0) {
+            v = mManager.getNativeView(viewType, parent);
         } else {
             TextView textView = new TextView(ContextUtils.getApplicationContext());
             String message = "Unable to render external view";
@@ -71,7 +69,8 @@ public class NativeViewListRenderer extends RecyclerView.Adapter<NativeViewListR
 
     @Override
     public int getItemViewType(int position) {
-        return position;
+        if (!mManager.isNativeView(position)) return -1;
+        return mManager.getViewType(position);
     }
 
     /* HybridListRenderer methods */

chrome/android/junit/src/org/chromium/chrome/browser/feed/v2/FeedListContentManagerTest.java

@@ -14,6 +14,7 @@ import android.app.Activity;
 import android.content.Context;
 import android.support.test.filters.SmallTest;
 import android.view.View;
+import android.view.ViewParent;
 import android.widget.FrameLayout;
 import android.widget.LinearLayout;
 
@@ -196,10 +197,52 @@ public class FeedListContentManagerTest implements ListContentManagerObserver {
         assertTrue(mManager.isNativeView(4));
 
         assertArrayEquals("foo".getBytes(), mManager.getExternalViewBytes(0));
-        assertEquals(v2, getNativeView(1));
-        assertEquals(v3, getNativeView(2));
+        assertEquals(v2, getNativeView(mManager.getViewType(1)));
+        assertEquals(v3, getNativeView(mManager.getViewType(2)));
         assertArrayEquals("hello".getBytes(), mManager.getExternalViewBytes(3));
-        assertEquals(v5, getNativeView(4));
+        assertEquals(v5, getNativeView(mManager.getViewType(4)));
+    }
+
+    @Test
+    @SmallTest
+    public void testGetViewDataCreatesEnclosingViewOnce() {
+        View v = new View(mContext);
+        FeedListContentManager.FeedContent c = createNativeViewContent(v);
+
+        addContents(0, ImmutableList.of(c));
+
+        assertEquals(v, getNativeView(mManager.getViewType(0)));
+        ViewParent p = v.getParent();
+
+        // Remove and re-add the same view, but with a new NativeViewContent.
+        // This time, getNativeView() does not need to create the enclosing parent view.
+        removeContents(0, 1);
+        c = createNativeViewContent(v);
+        addContents(0, ImmutableList.of(c));
+        assertEquals(v, getNativeView(mManager.getViewType(0)));
+        assertEquals(p, v.getParent());
+    }
+
+    @Test
+    @SmallTest
+    public void testGetNativeViewAfterMove() {
+        View v1 = new View(mContext);
+        FeedListContentManager.FeedContent c1 = createNativeViewContent(v1);
+        View v0 = new View(mContext);
+        FeedListContentManager.FeedContent c0 = createNativeViewContent(v0);
+
+        addContents(0, ImmutableList.of(c1));
+        int t1 = mManager.getViewType(0);
+        addContents(0, ImmutableList.of(c0));
+        int t0 = mManager.getViewType(0);
+
+        assertEquals(1, t1);
+        assertEquals(2, t0);
+
+        assertEquals(t0, mManager.getViewType(0));
+        assertEquals(t1, mManager.getViewType(1));
+        assertEquals(v0, getNativeView(t0));
+        assertEquals(v1, getNativeView(t1));
     }
 
     @Override
@@ -278,8 +321,8 @@ public class FeedListContentManagerTest implements ListContentManagerObserver {
         return new FeedListContentManager.NativeViewContent(v.toString(), v);
     }
 
-    private View getNativeView(int index) {
-        View view = mManager.getNativeView(index, mParent);
+    private View getNativeView(int viewType) {
+        View view = mManager.getNativeView(viewType, mParent);
         assertNotNull(view);
         assertTrue(view instanceof FrameLayout);
         return ((FrameLayout) view).getChildAt(0);

chrome/browser/xsurface/android/java/src/org/chromium/chrome/browser/xsurface/ListContentManager.java

@@ -40,10 +40,21 @@ public interface ListContentManager {
      * View should not be attached to parent. {@link bindNativeView} will
      * be called later to attach more information to the view.
      */
-    default View getNativeView(int index, ViewGroup parent) {
+    default View getNativeView(int viewType, ViewGroup parent) {
         return null;
     }
 
+    /**
+     * Returns the view type for item at position.
+     *
+     * The view type is later passed to getNativeView to attach the view.
+     * This will only be called when isNativeView(position) is true.
+     */
+    default int getViewType(int position) {
+        // This is an incorrect, but backwards compatible implementation.
+        return position;
+    }
+
     /**
      * Binds the data at the specified location.
      */