[Home] Finish showing the NTP immediately if bottom sheet is opened

When toggling accessibility mode, if the overview mode is showing we
hide it and then open a new tab (see
ChromeTabbedActivity#onAccessibilityModeChanged()). In Chrome Home, if
the NTP UI is showing then the overview mode is technically showing.

Prior to this patch, when ChromeTabbedActivity tried to open a new tab, we
would "show" the bottom sheet new tab UI but since the bottom sheet was
already opened we never finished showing the new UI (which currently
consists of showing the regular toolbar). This patch updates the logic in
BottomSheetNewTabController to finishing showing the NTP UI immediately
if the bottom sheet is already open when
BottomSheetNewTabController#displayNewTabUi() is called.

BUG=731147

Change-Id: Ief2d8c738f2c96f34f293ea29e41d8a0ab69b275
Reviewed-on: https://chromium-review.googlesource.com/530077
Commit-Queue: Theresa <twellington@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478670}

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

@@ -75,14 +75,19 @@ public class BottomSheetNewTabController extends EmptyBottomSheetObserver {
         // to ensure the toolbar ends up in the correct state.
         if (!mLayoutManager.overviewVisible()) mLayoutManager.showOverview(true);
 
-        // Open the sheet.
-        mBottomSheet.setSheetState(mTabModelSelector.getTotalTabCount() == 0
-                        ? BottomSheet.SHEET_STATE_FULL
-                        : BottomSheet.SHEET_STATE_HALF,
-                true);
-
-        mBottomSheet.getBottomSheetMetrics().recordSheetOpenReason(
-                BottomSheetMetrics.OPENED_BY_NEW_TAB_CREATION);
+        // Open the sheet if it isn't open already. When toggling accessibility mode, this method
+        // may get called while the sheet is open. In that case, we should finish showing the new
+        // tab UI.
+        if (!mBottomSheet.isSheetOpen()) {
+            mBottomSheet.setSheetState(mTabModelSelector.getTotalTabCount() == 0
+                            ? BottomSheet.SHEET_STATE_FULL
+                            : BottomSheet.SHEET_STATE_HALF,
+                    true);
+            mBottomSheet.getBottomSheetMetrics().recordSheetOpenReason(
+                    BottomSheetMetrics.OPENED_BY_NEW_TAB_CREATION);
+        } else {
+            finishShowingNewTabUi();
+        }
     }
 
     /**
@@ -106,12 +111,7 @@ public class BottomSheetNewTabController extends EmptyBottomSheetObserver {
     public void onSheetOpened() {
         if (!mIsShowingNewTabUi) return;
 
-        // Transition from the tab switcher toolbar to the normal toolbar.
-        mToolbar.showNormalToolbar();
-
-        // ToolbarPhone makes the url bar unfocusable while in the tab switcher; make sure it is
-        // focusable when the sheet is open.
-        mLocationBar.setUrlBarFocusable(true);
+        finishShowingNewTabUi();
     }
 
     @Override
@@ -150,4 +150,13 @@ public class BottomSheetNewTabController extends EmptyBottomSheetObserver {
         mTabModelSelector.getModel(false).setIsPendingTabAdd(false);
         mTabModelSelector.getModel(true).setIsPendingTabAdd(false);
     }
+
+    private void finishShowingNewTabUi() {
+        // Transition from the tab switcher toolbar to the normal toolbar.
+        mToolbar.showNormalToolbar();
+
+        // ToolbarPhone makes the url bar unfocusable while in the tab switcher; make sure it is
+        // focusable when the sheet is open.
+        mLocationBar.setUrlBarFocusable(true);
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetNewTabControllerTest.java

@@ -75,11 +75,12 @@ public class BottomSheetNewTabControllerTest {
         mActivity.getTabModelSelector().getModel(false).addObserver(mTabModelObserver);
         mActivity.getTabModelSelector().getModel(true).addObserver(mTabModelObserver);
         mFadingBackgroundView = mActivity.getFadingBackgroundView();
+        mBottomSheetTestRule.setSheetState(BottomSheet.SHEET_STATE_PEEK, false);
     }
 
     @Test
     @SmallTest
-    public void testNTPOverTabSwitcher_Normal() throws InterruptedException, TimeoutException {
+    public void testNTPOverTabSwitcher_Normal() {
         assertEquals(1, mActivity.getTabModelSelector().getTotalTabCount());
         assertFalse("Overview mode should not be showing",
                 mActivity.getLayoutManager().overviewVisible());
@@ -114,7 +115,7 @@ public class BottomSheetNewTabControllerTest {
 
     @Test
     @SmallTest
-    public void testNTPOverTabSwitcher_Incognito() throws InterruptedException, TimeoutException {
+    public void testNTPOverTabSwitcher_Incognito() {
         assertEquals(1, mActivity.getTabModelSelector().getTotalTabCount());
         assertFalse("Overview mode should not be showing",
                 mActivity.getLayoutManager().overviewVisible());
@@ -157,7 +158,7 @@ public class BottomSheetNewTabControllerTest {
 
     @Test
     @SmallTest
-    public void testNTPOverTabSwitcher_NoTabs() throws InterruptedException, TimeoutException {
+    public void testNTPOverTabSwitcher_NoTabs() throws InterruptedException {
         // Close all tabs.
         ChromeTabUtils.closeAllTabs(InstrumentationRegistry.getInstrumentation(), mActivity);
         assertEquals(0, mActivity.getTabModelSelector().getTotalTabCount());
@@ -208,7 +209,7 @@ public class BottomSheetNewTabControllerTest {
         InstrumentationRegistry.getInstrumentation().waitForIdleSync();
     }
 
-    private void createNewBlankTab(final boolean incognito) throws InterruptedException {
+    private void createNewBlankTab(final boolean incognito) {
         MenuUtils.invokeCustomMenuActionSync(InstrumentationRegistry.getInstrumentation(),
                 mActivity, incognito ? R.id.new_incognito_tab_menu_id : R.id.new_tab_menu_id);
 
@@ -250,7 +251,7 @@ public class BottomSheetNewTabControllerTest {
 
         if (chromeHomeTabPageSelected) {
             assertFalse(mFadingBackgroundView.isEnabled());
-            assertEquals(0f, mFadingBackgroundView.getAlpha());
+            assertEquals(0f, mFadingBackgroundView.getAlpha(), 0);
         } else {
             assertTrue(mFadingBackgroundView.isEnabled());
         }