feedv2: make surface open conditions clear

This change ensures that the surface can't be opened more
than once, and only closes after opening.

Also verifies that all conditions are met prior to opening.

While testing this, I noticed scroll restore wasn't working.
I've fixed this by waiting until the recycler view adapter
has the right number of items before attempting to restore.

Bug: 1115213
Change-Id: I86bae811926fa4a6c1caf1f8dce8e32e80152613
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354656Reviewed-by: Vincent Boisselle <vincb@google.com>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798194}

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

@@ -45,16 +45,12 @@ public class FeedStream implements Stream {
     final FeedStreamSurface mFeedStreamSurface;
     private final ObserverList<ScrollListener> mScrollListeners =
             new ObserverList<ScrollListener>();
-    private boolean mShown;
     private RecyclerView mRecyclerView;
-    // setStreamContentVisibility() is always called once after onCreate(). So we can assume the
-    // stream content is hidden initially and it can be made visible later when
-    // setStreamContentVisibility() is called.
-    private boolean mIsStreamContentVisible;
     // For loading more content.
     private int mAccumulatedDySinceLastLoadMore;
 
-    private String mSavedScrollStateOnHide;
+    private String mScrollStateToRestore;
+    private RestoreScrollObserver mRestoreScrollObserver = new RestoreScrollObserver();
 
     public FeedStream(Activity activity, boolean isBackgroundDark, SnackbarManager snackbarManager,
             NativePageNavigationDelegate nativePageNavigationDelegate,
@@ -67,35 +63,27 @@ public class FeedStream implements Stream {
 
     @Override
     public void onCreate(@Nullable String savedInstanceState) {
+        mScrollStateToRestore = savedInstanceState;
         setupRecyclerView();
-        if (savedInstanceState != null && !savedInstanceState.isEmpty()) {
-            restoreScrollState(savedInstanceState);
-        }
     }
 
     @Override
     public void onShow() {
-        mShown = true;
-        if (mIsStreamContentVisible && !mFeedStreamSurface.isOpened()) {
-            mFeedStreamSurface.surfaceOpened();
-            if (mSavedScrollStateOnHide != null) {
-                restoreScrollState(mSavedScrollStateOnHide);
-                mSavedScrollStateOnHide = null;
-            }
-        }
+        mFeedStreamSurface.setStreamVisibility(true);
     }
 
     @Override
     public void onHide() {
+        mScrollStateToRestore = null;
         if (mFeedStreamSurface.isOpened()) {
-            mSavedScrollStateOnHide = getSavedInstanceStateString();
-            mFeedStreamSurface.surfaceClosed();
+            mScrollStateToRestore = getSavedInstanceStateString();
         }
-        mShown = false;
+        mFeedStreamSurface.setStreamVisibility(false);
     }
 
     @Override
     public void onDestroy() {
+        mScrollStateToRestore = null;
         mFeedStreamSurface.destroy();
     }
 
@@ -141,18 +129,7 @@ public class FeedStream implements Stream {
 
     @Override
     public void setStreamContentVisibility(boolean visible) {
-        if (visible == mIsStreamContentVisible) {
-            return;
-        }
-        mIsStreamContentVisible = visible;
-
-        if (visible) {
-            if (mShown) mFeedStreamSurface.surfaceOpened();
-        } else {
-            if (mFeedStreamSurface.isOpened()) {
-                mFeedStreamSurface.surfaceClosed();
-            }
-        }
+        mFeedStreamSurface.setStreamContentVisibility(visible);
     }
 
     @Override
@@ -225,8 +202,6 @@ public class FeedStream implements Stream {
     public void triggerRefresh() {}
 
     private void setupRecyclerView() {
-        assert (!mIsStreamContentVisible);
-
         mRecyclerView = (RecyclerView) mFeedStreamSurface.getView();
         mRecyclerView.setId(R.id.feed_stream_recycler_view);
         mRecyclerView.setClipToPadding(false);
@@ -247,13 +222,12 @@ public class FeedStream implements Stream {
                 }
             }
         });
+        mRecyclerView.getAdapter().registerAdapterDataObserver(mRestoreScrollObserver);
     }
 
     @VisibleForTesting
     void checkScrollingForLoadMore(int dy) {
-        if (!mIsStreamContentVisible) {
-            return;
-        }
+        if (!mFeedStreamSurface.isOpened()) return;
 
         mAccumulatedDySinceLastLoadMore += dy;
         if (mAccumulatedDySinceLastLoadMore < TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP,
@@ -268,17 +242,45 @@ public class FeedStream implements Stream {
         }
     }
 
-    private void restoreScrollState(String savedInstanceState) {
+    /**
+     * Restores the scroll state serialized to |savedInstanceState|.
+     * @return true if the scroll state was restored, or if the state could never be restored.
+     * false if we need to wait until more items are added to the recycler view to make it
+     * scrollable.
+     */
+    private boolean restoreScrollState(String savedInstanceState) {
+        assert (mRecyclerView != null);
+        int scrollPosition;
+        int scrollOffset;
         try {
             JSONObject jsonSavedState = new JSONObject(savedInstanceState);
-            LinearLayoutManager layoutManager =
-                    (LinearLayoutManager) mRecyclerView.getLayoutManager();
-            if (layoutManager != null) {
-                layoutManager.scrollToPositionWithOffset(jsonSavedState.getInt(SCROLL_POSITION),
-                        jsonSavedState.getInt(SCROLL_OFFSET));
-            }
+            scrollPosition = jsonSavedState.getInt(SCROLL_POSITION);
+            scrollOffset = jsonSavedState.getInt(SCROLL_OFFSET);
         } catch (JSONException e) {
             Log.d(TAG, "Unable to parse a JSONObject from a string.");
+            return true;
+        }
+
+        // If too few items exist, defer scrolling until later.
+        if (mRecyclerView.getAdapter().getItemCount() <= scrollPosition) return false;
+
+        LinearLayoutManager layoutManager = (LinearLayoutManager) mRecyclerView.getLayoutManager();
+        if (layoutManager != null) {
+            layoutManager.scrollToPositionWithOffset(scrollPosition, scrollOffset);
         }
+        return true;
     }
+
+    // Scroll state can't be restored until enough items are added to the recycler view adapter.
+    // Attempts to restore scroll state every time new items are added to the adapter.
+    class RestoreScrollObserver extends RecyclerView.AdapterDataObserver {
+        @Override
+        public void onItemRangeInserted(int positionStart, int itemCount) {
+            if (mScrollStateToRestore != null) {
+                if (restoreScrollState(mScrollStateToRestore)) {
+                    mScrollStateToRestore = null;
+                }
+            }
+        }
+    };
 }

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

@@ -102,8 +102,10 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
     private final NativePageNavigationDelegate mPageNavigationDelegate;
     private final HelpAndFeedback mHelpAndFeedback;
     private final ScrollReporter mScrollReporter = new ScrollReporter();
-
+    // True after onSurfaceOpened(), and before onSurfaceClosed().
     private boolean mOpened;
+    private boolean mStreamContentVisible;
+    private boolean mStreamVisible;
     private int mHeaderCount;
     private BottomSheetContent mBottomSheetContent;
     // If the bottom sheet was opened in response to an action on a slice, this is the slice ID.
@@ -130,13 +132,10 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
 
     // We avoid attaching surfaces until after |startup()| is called. This ensures that
     // the correct sign-in state is used if attaching the surface triggers a fetch.
-
     private static boolean sStartupCalled;
-    // Tracks all the surfaces that are waiting to be attached or already attached. When
-    // |sStartupCalled| is false, |startup()| has not been called and thus all the surfaces
-    // in this set are waiting to be attached. Otherwise, all the surfaces in this set are
-    // already attached.
-    private static HashSet<FeedStreamSurface> sSurfaces;
+    // Tracks all the instances of FeedStreamSurface.
+    @VisibleForTesting
+    static HashSet<FeedStreamSurface> sSurfaces;
 
     public static void startup() {
         if (sStartupCalled) return;
@@ -145,7 +144,7 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
         xSurfaceProcessScope();
         if (sSurfaces != null) {
             for (FeedStreamSurface surface : sSurfaces) {
-                surface.surfaceOpened();
+                surface.updateSurfaceOpenState();
             }
         }
     }
@@ -175,13 +174,12 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
      *  Clear all the data related to all surfaces.
      */
     public static void clearAll() {
-        FeedStreamSurface[] surfaces = null;
-        if (sSurfaces != null) {
-            surfaces = sSurfaces.toArray(new FeedStreamSurface[sSurfaces.size()]);
-            for (FeedStreamSurface surface : surfaces) {
-                surface.surfaceClosed();
-            }
-            sSurfaces = null;
+        ArrayList<FeedStreamSurface> openSurfaces = new ArrayList<FeedStreamSurface>();
+        for (FeedStreamSurface surface : sSurfaces) {
+            if (surface.isOpened()) openSurfaces.add(surface);
+        }
+        for (FeedStreamSurface surface : openSurfaces) {
+            surface.onSurfaceClosed();
         }
 
         ProcessScope processScope = xSurfaceProcessScope();
@@ -189,10 +187,8 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
             processScope.resetAccount();
         }
 
-        if (surfaces != null) {
-            for (FeedStreamSurface surface : surfaces) {
-                surface.surfaceOpened();
-            }
+        for (FeedStreamSurface surface : openSurfaces) {
+            surface.updateSurfaceOpenState();
         }
     }
 
@@ -337,15 +333,16 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
         } else {
             mRootView = null;
         }
+
+        trackSurface(this);
     }
 
     /**
      * Performs all necessary cleanups.
      */
     public void destroy() {
-        if (mOpened) {
-            surfaceClosed();
-        }
+        if (mOpened) onSurfaceClosed();
+        untrackSurface(this);
         if (mSliceViewTracker != null) {
             mSliceViewTracker.destroy();
             mSliceViewTracker = null;
@@ -429,6 +426,9 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
      */
     @CalledByNative
     void onStreamUpdated(byte[] data) {
+        // There should be no updates while the surface is closed. If the surface was recently
+        // closed, just ignore these.
+        if (!mOpened) return;
         StreamUpdate streamUpdate;
         try {
             streamUpdate = StreamUpdate.parseFrom(data);
@@ -781,41 +781,68 @@ public class FeedStreamSurface implements SurfaceActionsHandler, FeedActionsHand
     }
 
     /**
-     * Informs that the surface is opened. We can request the initial set of content now. Once
-     * the content is available, onStreamUpdated will be called.
+     * Informs whether or not feed content should be shown.
      */
-    public void surfaceOpened() {
-        mOpened = true;
-        trackSurface(this);
-        if (sStartupCalled) {
-            FeedStreamSurfaceJni.get().surfaceOpened(
-                    mNativeFeedStreamSurface, FeedStreamSurface.this);
-            mHybridListRenderer.onSurfaceOpened();
+    public void setStreamContentVisibility(boolean visible) {
+        if (mStreamContentVisible == visible) return;
+        mStreamContentVisible = visible;
+        updateSurfaceOpenState();
+    }
+
+    /**
+     * Informs FeedStreamSurface of the visibility of its parent stream.
+     */
+    public void setStreamVisibility(boolean visible) {
+        if (mStreamVisible == visible) return;
+        mStreamVisible = visible;
+        updateSurfaceOpenState();
+    }
+
+    private void updateSurfaceOpenState() {
+        boolean shouldOpen = sStartupCalled && mStreamContentVisible && mStreamVisible;
+        if (shouldOpen == mOpened) return;
+        if (shouldOpen) {
+            onSurfaceOpened();
+        } else {
+            onSurfaceClosed();
         }
     }
 
+    /**
+     * Called when the surface is considered opened. This happens when the feed should be visible
+     * and enabled on the screen.
+     */
+    private void onSurfaceOpened() {
+        assert (!mOpened);
+        assert (sStartupCalled);
+        assert (mStreamContentVisible);
+        // No feed content should exist.
+        assert (mContentManager.getItemCount() == mHeaderCount);
+        mOpened = true;
+        FeedStreamSurfaceJni.get().surfaceOpened(mNativeFeedStreamSurface, FeedStreamSurface.this);
+        mHybridListRenderer.onSurfaceOpened();
+    }
+
     /**
      * Informs that the surface is closed.
      */
-    public void surfaceClosed() {
+    private void onSurfaceClosed() {
+        assert (mOpened);
+        assert (sStartupCalled);
         // Let the hybrid list renderer know that the surface has closed, so it doesn't
         // interpret the removal of contents as related to actions otherwise initiated by
         // the user.
-        if (sStartupCalled) {
-            mHybridListRenderer.onSurfaceClosed();
-        }
+        mHybridListRenderer.onSurfaceClosed();
 
+        // Remove Feed content from the content manager.
         int feedCount = mContentManager.getItemCount() - mHeaderCount;
         if (feedCount > 0) {
             mContentManager.removeContents(mHeaderCount, feedCount);
         }
+
         mScrollReporter.onUnbind();
 
-        untrackSurface(this);
-        if (sStartupCalled) {
-            FeedStreamSurfaceJni.get().surfaceClosed(
-                    mNativeFeedStreamSurface, FeedStreamSurface.this);
-        }
+        FeedStreamSurfaceJni.get().surfaceClosed(mNativeFeedStreamSurface, FeedStreamSurface.this);
         mOpened = false;
     }
 

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

@@ -71,7 +71,8 @@ public class FeedStreamTest {
 
         when(mFeedServiceBridgeJniMock.getLoadMoreTriggerLookahead())
                 .thenReturn(LOAD_MORE_TRIGGER_LOOKAHEAD);
-
+        // Surfaces won't open until after startup.
+        FeedStreamSurface.startup();
         mFeedStream = new FeedStream(mActivity, false, mSnackbarManager, mPageNavigationDelegate,
                 mBottomSheetController);
         mFeedStream.onCreate(null);
@@ -196,6 +197,7 @@ public class FeedStreamTest {
 
     @Test
     public void testCheckScrollingForLoadMore_StreamContentVisible() {
+        mFeedStream.onShow();
         mFeedStream.setStreamContentVisibility(true);
         final int triggerDistance = getLoadMoreTriggerScrollDistance();
         final int itemCount = 10;