Remove preload from BottomSheetController

The preload functionality causes more problems than it solves. Content
that wants to delay loading until the sheet is opened can do so using
events provided by the BottomSheetObserver. This fixes a race
condition in EoC that would cause the sheet to peek before being
requested.

Bug: 834923,843267
Change-Id: If5c35e22a502e42f6bc591784b4fb8343e713eff
Reviewed-on: https://chromium-review.googlesource.com/1060353
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559173}

chrome/android/java/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsCoordinator.java

@@ -80,33 +80,18 @@ public class ContextualSuggestionsCoordinator {
     }
 
     /**
-     * Preload the contextual suggestions content in the {@link BottomSheet}; the sheet won't
-     * actually be shown until {@link #showContentInSheet()} is called.
+     * Show the contextual suggestions content in the {@link BottomSheet}.
+     * Only the views needed for peeking the bottom sheet will be constructed. Another
+     * call to {@link #displaySuggestions()} is needed to finish inflating views for the
+     * suggestions cards.
      */
-    void preloadContentInSheet() {
+    void showContentInSheet() {
         mToolbarCoordinator =
                 new ToolbarCoordinator(mActivity, mBottomSheetController.getBottomSheet(), mModel);
         mContentCoordinator =
                 new ContentCoordinator(mActivity, mBottomSheetController.getBottomSheet());
         mBottomSheetContent = new ContextualSuggestionsBottomSheetContent(
                 mContentCoordinator, mToolbarCoordinator);
-        // TODO(twellington): Handle the case where preload returns false.
-        mBottomSheetController.requestContentPreload(mBottomSheetContent);
-    }
-
-    /**
-     * Show the contextual suggestions content in the {@link BottomSheet}.
-     * {@link #preloadContentInSheet()} must be called prior to calling this method.
-     *
-     * Only the views needed for peeking the bottom sheet will be constructed. Another
-     * call to {@link #displaySuggestions()} is needed to finish inflating views for the
-     * suggestions cards.
-     */
-    void showContentInSheet() {
-        // #preloadContentInSheet will always be called before this method, regardless of
-        // whether content was actually put in the sheet (meaning mBottomSheetContent should never
-        // be null). If content is not successfully preloaded
-        // BottomSheetController#requestContentPreload will return false.
         assert mBottomSheetContent != null;
         mBottomSheetController.requestShowContent(mBottomSheetContent, false);
     }

chrome/android/java/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsMediator.java

@@ -256,8 +256,7 @@ class ContextualSuggestionsMediator
             }
 
             if (clusters.size() > 0 && clusters.get(0).getSuggestions().size() > 0) {
-                preloadContentInSheet(
-                        generateClusterList(clusters), suggestionsResult.getPeekText());
+                prepareModel(generateClusterList(clusters), suggestionsResult.getPeekText());
                 // If the controls are already off-screen, show the suggestions immediately so they
                 // are available on reverse scroll.
                 maybeShowContentInSheet();
@@ -343,7 +342,7 @@ class ContextualSuggestionsMediator
         }
     }
 
-    private void preloadContentInSheet(ClusterList clusters, String title) {
+    private void prepareModel(ClusterList clusters, String title) {
         if (mSuggestionsSource == null) return;
 
         mModel.setClusterList(clusters);
@@ -362,7 +361,6 @@ class ContextualSuggestionsMediator
         mModel.setMenuButtonDelegate(this);
         mModel.setDefaultToolbarClickListener(view -> mCoordinator.expandBottomSheet());
         mModel.setTitle(title);
-        mCoordinator.preloadContentInSheet();
     }
 
     private void maybeShowContentInSheet() {

chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java

@@ -282,7 +282,7 @@ public class BottomSheetController implements ApplicationStatus.ActivityStateLis
      */
     public boolean requestShowContent(BottomSheetContent content, boolean animate) {
         // If pre-load failed, do nothing. The content will automatically be queued.
-        if (!requestContentPreload(content)) return false;
+        if (!loadInternal(content)) return false;
         if (!mBottomSheet.isSheetOpen() && !isOtherUIObscuring()) {
             mBottomSheet.setSheetState(BottomSheet.SHEET_STATE_PEEK, animate);
         }
@@ -291,13 +291,11 @@ public class BottomSheetController implements ApplicationStatus.ActivityStateLis
     }
 
     /**
-     * Request content start loading in the bottom sheet without actually showing the
-     * {@link BottomSheet}. Another call to {@link #requestShowContent(BottomSheetContent, boolean)}
-     * is required to actually make the sheet appear.
-     * @param content The content to pre-load.
+     * Handles loading or suppressing of content based on priority.
+     * @param content The content to load.
      * @return True if the content started loading.
      */
-    public boolean requestContentPreload(BottomSheetContent content) {
+    private boolean loadInternal(BottomSheetContent content) {
         if (content == mBottomSheet.getCurrentSheetContent()) return true;
         if (!canShowInLayout(mLayoutManager.getActiveLayout())) return false;
 

chrome/android/javatests/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsTest.java

@@ -17,6 +17,7 @@ import android.support.test.filters.LargeTest;
 import android.support.test.filters.MediumTest;
 import android.support.v7.widget.RecyclerView;
 import android.view.View;
+import android.view.View.OnLayoutChangeListener;
 
 import org.junit.After;
 import org.junit.Assert;
@@ -158,11 +159,14 @@ public class ContextualSuggestionsTest {
     @MediumTest
     @Feature({"ContextualSuggestions"})
     public void testOpenContextualSuggestionsBottomSheet() {
-        assertTrue("Bottom sheet should contain suggestions content",
-                mBottomSheet.getCurrentSheetContent()
-                                instanceof ContextualSuggestionsBottomSheetContent);
         assertEquals("Sheet should still be hidden.", BottomSheet.SHEET_STATE_HIDDEN,
                 mBottomSheet.getSheetState());
+
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            mMediator.showContentInSheetForTesting(true, true);
+            mBottomSheet.endAnimations();
+        });
+
         assertEquals("Title text should be set.",
                 FakeContextualSuggestionsSource.TEST_TOOLBAR_TITLE, mModel.getTitle());
 
@@ -175,11 +179,6 @@ public class ContextualSuggestionsTest {
         RecyclerView recyclerView = (RecyclerView) content.getContentView();
         assertEquals("RecyclerView should be empty.", 0, recyclerView.getChildCount());
 
-        ThreadUtils.runOnUiThreadBlocking(() -> {
-            mMediator.showContentInSheetForTesting(true, true);
-            mBottomSheet.endAnimations();
-        });
-
         assertEquals("Sheet should be peeked.", BottomSheet.SHEET_STATE_PEEK,
                 mBottomSheet.getSheetState());
         assertNull("RecyclerView should still be empty.", recyclerView.getAdapter());
@@ -328,6 +327,16 @@ public class ContextualSuggestionsTest {
         View menuButton =
                 mBottomSheet.getCurrentSheetContent().getToolbarView().findViewById(R.id.more);
 
+        CallbackHelper layoutHelper = new CallbackHelper();
+        title.addOnLayoutChangeListener(new OnLayoutChangeListener() {
+            @Override
+            public void onLayoutChange(View v, int left, int top, int right, int bottom,
+                    int oldLeft, int oldTop, int oldRight, int oldBottom) {
+                layoutHelper.notifyCalled();
+            }
+        });
+        layoutHelper.waitForCallback(0);
+
         int titleWidth = title.getWidth();
         // Menu button is not visible on peek state.
         assertEquals(View.GONE, menuButton.getVisibility());