feedv2: Fix scroll restore

Scroll restore didn't work when:
- the feed is being restored in async, so scrolling must happen later,
- And the item at the top of the NTP was a header, present before
  the feed is loaded.

This CL stores the last visible item index in the scroll state,
and waits until the right number of items are added to the recycler
adapter.

Bug: 1115488
Change-Id: Iac489572da73659261231a002e2c2a66591234af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2360311Reviewed-by: Cathy Li <chili@chromium.org>
Commit-Queue: Cathy Li <chili@chromium.org>
Auto-Submit: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798871}

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

@@ -35,8 +35,7 @@ import java.util.List;
  */
 public class FeedStream implements Stream {
     private static final String TAG = "FeedStream";
-    private static final String SCROLL_POSITION = "scroll_pos";
-    private static final String SCROLL_OFFSET = "scroll_off";
+
     // How far the user has to scroll down in DP before attempting to load more content.
     static final int LOAD_MORE_TRIGGER_SCROLL_DISTANCE_DP = 100;
 
@@ -93,24 +92,19 @@ public class FeedStream implements Stream {
         if (layoutManager == null) {
             return "";
         }
-        int firstItemPosition = layoutManager.findFirstVisibleItemPosition();
-        if (firstItemPosition == RecyclerView.NO_POSITION) {
+        ScrollState state = new ScrollState();
+        state.position = layoutManager.findFirstVisibleItemPosition();
+        state.lastPosition = layoutManager.findLastVisibleItemPosition();
+        if (state.position == RecyclerView.NO_POSITION) {
             return "";
         }
-        View firstVisibleView = layoutManager.findViewByPosition(firstItemPosition);
+
+        View firstVisibleView = layoutManager.findViewByPosition(state.position);
         if (firstVisibleView == null) {
             return "";
         }
-        int firstVisibleTop = firstVisibleView.getTop();
-
-        JSONObject jsonSavedState = new JSONObject();
-        try {
-            jsonSavedState.put(SCROLL_POSITION, firstItemPosition);
-            jsonSavedState.put(SCROLL_OFFSET, firstVisibleTop);
-        } catch (JSONException e) {
-            Log.d(TAG, "Unable to write to a JSONObject.");
-        }
-        return jsonSavedState.toString();
+        state.offset = firstVisibleView.getTop();
+        return state.toJson();
     }
 
     @Override
@@ -250,27 +244,56 @@ public class FeedStream implements Stream {
      */
     private boolean restoreScrollState(String savedInstanceState) {
         assert (mRecyclerView != null);
-        int scrollPosition;
-        int scrollOffset;
-        try {
-            JSONObject jsonSavedState = new JSONObject(savedInstanceState);
-            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;
-        }
+        ScrollState state = ScrollState.fromJson(savedInstanceState);
+        if (state == null) return true;
 
         // If too few items exist, defer scrolling until later.
-        if (mRecyclerView.getAdapter().getItemCount() <= scrollPosition) return false;
+        if (mRecyclerView.getAdapter().getItemCount() <= state.lastPosition) return false;
 
         LinearLayoutManager layoutManager = (LinearLayoutManager) mRecyclerView.getLayoutManager();
         if (layoutManager != null) {
-            layoutManager.scrollToPositionWithOffset(scrollPosition, scrollOffset);
+            layoutManager.scrollToPositionWithOffset(state.position, state.offset);
         }
         return true;
     }
 
+    static class ScrollState {
+        private static final String SCROLL_POSITION = "pos";
+        private static final String SCROLL_LAST_POSITION = "lpos";
+        private static final String SCROLL_OFFSET = "off";
+
+        public int position;
+        public int lastPosition;
+        public int offset;
+
+        String toJson() {
+            JSONObject jsonSavedState = new JSONObject();
+            try {
+                jsonSavedState.put(SCROLL_POSITION, position);
+                jsonSavedState.put(SCROLL_LAST_POSITION, lastPosition);
+                jsonSavedState.put(SCROLL_OFFSET, offset);
+                return jsonSavedState.toString();
+            } catch (JSONException e) {
+                Log.d(TAG, "Unable to write to a JSONObject.");
+                return "";
+            }
+        }
+        @Nullable
+        static ScrollState fromJson(String json) {
+            ScrollState result = new ScrollState();
+            try {
+                JSONObject jsonSavedState = new JSONObject(json);
+                result.position = jsonSavedState.getInt(SCROLL_POSITION);
+                result.lastPosition = jsonSavedState.getInt(SCROLL_LAST_POSITION);
+                result.offset = jsonSavedState.getInt(SCROLL_OFFSET);
+            } catch (JSONException e) {
+                Log.d(TAG, "Unable to parse a JSONObject from a string.");
+                return null;
+            }
+            return result;
+        }
+    }
+
     // 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 {

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

@@ -30,6 +30,7 @@ import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.robolectric.Robolectric;
 import org.robolectric.annotation.Config;
+import org.robolectric.shadows.ShadowLog;
 
 import org.chromium.base.Callback;
 import org.chromium.base.test.BaseRobolectricTestRunner;
@@ -79,6 +80,9 @@ public class FeedStreamTest {
         mRecyclerView = (RecyclerView) mFeedStream.getView();
         mLayoutManager = new FakeLinearLayoutManager(mActivity);
         mRecyclerView.setLayoutManager(mLayoutManager);
+
+        // Print logs to stdout.
+        ShadowLog.stream = System.out;
     }
 
     @Test
@@ -222,6 +226,48 @@ public class FeedStreamTest {
                 .loadMore(anyLong(), any(FeedStreamSurface.class), any(Callback.class));
     }
 
+    @Test
+    public void testSerializeScrollState() {
+        FeedStream.ScrollState state = new FeedStream.ScrollState();
+        state.position = 2;
+        state.lastPosition = 4;
+        state.offset = 50;
+
+        FeedStream.ScrollState deserializedState = FeedStream.ScrollState.fromJson(state.toJson());
+
+        Assert.assertEquals(2, deserializedState.position);
+        Assert.assertEquals(4, deserializedState.lastPosition);
+        Assert.assertEquals(50, deserializedState.offset);
+        Assert.assertEquals(state.toJson(), deserializedState.toJson());
+    }
+
+    @Test
+    public void testGetSavedInstanceStateString() {
+        mFeedStream.onShow();
+        mFeedStream.setStreamContentVisibility(true);
+
+        View view1 = new FrameLayout(mActivity);
+        mLayoutManager.addChildToPosition(0, new FrameLayout(mActivity));
+        mLayoutManager.addChildToPosition(1, view1);
+        mLayoutManager.addChildToPosition(2, new FrameLayout(mActivity));
+        mLayoutManager.addChildToPosition(3, new FrameLayout(mActivity));
+
+        mLayoutManager.setFirstVisiblePosition(1);
+        mLayoutManager.setLastVisiblePosition(3);
+
+        String json = mFeedStream.getSavedInstanceStateString();
+        Assert.assertNotEquals("", json);
+
+        FeedStream.ScrollState state = FeedStream.ScrollState.fromJson(json);
+        Assert.assertEquals(1, state.position);
+        Assert.assertEquals(3, state.lastPosition);
+    }
+
+    @Test
+    public void testScrollStateFromInvalidJson() {
+        Assert.assertEquals(null, FeedStream.ScrollState.fromJson("{{=xcg"));
+    }
+
     private int getLoadMoreTriggerScrollDistance() {
         return (int) TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP,
                 FeedStream.LOAD_MORE_TRIGGER_SCROLL_DISTANCE_DP,