[Android, Crash] Hide accessory sheet during tab overview

This CL makes the ManualFillingMediator consider various cases of a tab
being hidden. Most importantly, it's now aware to when the tab overview
is opened which resolves a crash that happened in combination with the
HorizontalTabSwitcherAndroid feature.

Bug: 856523, 853742, 856033
Change-Id: If09a3ac3551e2ebed1cfac6bb4870ff98d6c1ec1
Reviewed-on: https://chromium-review.googlesource.com/1118384Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571480}

chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/KeyboardAccessoryView.java

@@ -69,7 +69,7 @@ class KeyboardAccessoryView extends LinearLayout {
 
         mSuggestionsView = findViewById(R.id.suggestions_view);
 
-        // Apply RTL layout changes to the views childen:
+        // Apply RTL layout changes to the views children:
         ApiCompatibilityUtils.setLayoutDirection(mSuggestionsView,
                 isLayoutRtl() ? View.LAYOUT_DIRECTION_RTL : View.LAYOUT_DIRECTION_LTR);
     }
@@ -99,25 +99,26 @@ class KeyboardAccessoryView extends LinearLayout {
      * @param contentDescription The contentDescription to be used for the tab icon.
      */
     void addTabAt(int position, Drawable icon, CharSequence contentDescription) {
-        this.post(() -> { // Using |post| ensures this is cannot happen before inflation finishes.
-            TabLayout.Tab tab = mTabLayout.newTab();
-            tab.setIcon(icon.mutate()); // mutate() needed to change the active tint.
-            tab.setContentDescription(contentDescription);
-            mTabLayout.addTab(tab, position, false);
-        });
+        if (mTabLayout == null) return; // Inflation not done yet. Will be invoked again afterwards.
+        TabLayout.Tab tab = mTabLayout.newTab();
+        tab.setIcon(icon.mutate()); // mutate() needed to change the active tint.
+        tab.setContentDescription(contentDescription);
+        mTabLayout.addTab(tab, position, false);
     }
 
     void removeTabAt(int position) {
-        this.post(() -> mTabLayout.removeTabAt(position));
+        if (mTabLayout == null) return; // Inflation not done yet. Will be invoked again afterwards.
+        TabLayout.Tab tab = mTabLayout.getTabAt(position);
+        if (tab == null) return; // The tab was already removed.
+        mTabLayout.removeTab(tab);
     }
 
     /**
      * Removes all tabs.
      */
     void clearTabs() {
-        this.post(() -> {
-            if (mTabLayout != null) mTabLayout.removeAllTabs();
-        });
+        if (mTabLayout == null) return; // Inflation not done yet. Will be invoked again afterwards.
+        mTabLayout.removeAllTabs();
     }
 
     ViewPager.OnPageChangeListener getPageChangeListener() {

chrome/android/java/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingMediator.java

@@ -9,7 +9,10 @@ import android.support.annotation.Nullable;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeFeatureList;
+import org.chromium.chrome.browser.ChromeTabbedActivity;
 import org.chromium.chrome.browser.autofill.keyboard_accessory.KeyboardAccessoryData.Provider;
+import org.chromium.chrome.browser.compositor.layouts.Layout;
+import org.chromium.chrome.browser.compositor.layouts.SceneChangeObserver;
 import org.chromium.chrome.browser.fullscreen.FullscreenOptions;
 import org.chromium.chrome.browser.tab.EmptyTabObserver;
 import org.chromium.chrome.browser.tab.Tab;
@@ -87,23 +90,38 @@ class ManualFillingMediator
     private TabModelSelectorTabModelObserver mTabModelObserver;
     private Tab mActiveBrowserTab;
 
+    private final SceneChangeObserver mTabSwitcherObserver = new SceneChangeObserver() {
+        @Override
+        public void onTabSelectionHinted(int tabId) {}
+
+        @Override
+        public void onSceneChange(Layout layout) {
+            // Includes events like side-swiping between tabs and triggering contextual search.
+            mAccessorySheet.hide();
+        }
+    };
+
     private final TabObserver mTabObserver = new EmptyTabObserver() {
         @Override
         public void onHidden(Tab tab) {
             // TODO(fhorschig): Test that the accessory is reset and close if a tab changes.
             // TODO(fhorschig): Test that this hides everything.
-            resetAccessory(tab);
+            mAccessorySheet.hide();
+            resetAccessory();
         }
 
         @Override
         public void onDestroyed(Tab tab) {
             // TODO(fhorschig): Test that this hides everything.
-            // TODO(fhorschig): Clear caches and remove tab. What to select next?
+            mAccessorySheet.hide();
+            resetAccessory();
+            mModel.remove(tab); // Clears tab if still present.
         }
 
         @Override
         public void onEnterFullscreenMode(Tab tab, FullscreenOptions options) {
             // TODO(fhorschig): Test that this hides everything.
+            mAccessorySheet.hide();
         }
     };
 
@@ -112,6 +130,13 @@ class ManualFillingMediator
         mKeyboardAccessory = keyboardAccessory;
         mAccessorySheet = accessorySheet;
         mActivity = activity;
+        if (activity instanceof ChromeTabbedActivity) {
+            // This object typically lives as long as the layout manager, so there is no need to
+            // unsubscribe which would occasionally use an invalidated object.
+            ((ChromeTabbedActivity) activity)
+                    .getLayoutManager()
+                    .addSceneChangeObserver(mTabSwitcherObserver);
+        }
         mTabModelObserver = new TabModelSelectorTabModelObserver(mActivity.getTabModelSelector()) {
             @Override
             public void didSelectTab(Tab tab, @TabModel.TabSelectionType int type, int lastId) {
@@ -121,6 +146,8 @@ class ManualFillingMediator
 
             @Override
             public void willCloseTab(Tab tab, boolean animate) {
+                mAccessorySheet.hide();
+                resetAccessory();
                 mModel.remove(tab);
             }
         };
@@ -187,17 +214,15 @@ class ManualFillingMediator
 
     private void restoreCachedState(Tab browserTab) {
         AccessoryState state = getOrCreateAccessoryState(browserTab);
+        resetAccessory();
         if (state.mPasswordAccessorySheet != null) {
             addTab(state.mPasswordAccessorySheet.getTab());
         }
         if (state.mActionsProvider != null) state.mActionsProvider.notifyAboutCachedItems();
     }
 
-    private void resetAccessory(Tab browserTab) {
-        AccessoryState state = getOrCreateAccessoryState(browserTab);
-        if (state.mPasswordAccessorySheet != null) {
-            setTabs(new KeyboardAccessoryData.Tab[0]);
-        }
+    private void resetAccessory() {
+        setTabs(new KeyboardAccessoryData.Tab[0]);
     }
 
     private void setTabs(KeyboardAccessoryData.Tab[] tabs) {
@@ -207,6 +232,8 @@ class ManualFillingMediator
 
     @VisibleForTesting
     void addTab(KeyboardAccessoryData.Tab tab) {
+        // TODO(fhorschig): This should add the tab only to the state. Sheet and accessory should be
+        // using a |set| method or even observe the state.
         mKeyboardAccessory.addTab(tab);
         mAccessorySheet.addTab(tab);
     }

chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingIntegrationTest.java

@@ -20,12 +20,12 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeSwitches;
-import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
 
 import java.util.concurrent.TimeoutException;
 /**
@@ -37,8 +37,8 @@ import java.util.concurrent.TimeoutException;
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
 public class ManualFillingIntegrationTest {
     @Rule
-    public final ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
-            new ChromeActivityTestRule<>(ChromeActivity.class);
+    public final ChromeTabbedActivityTestRule mActivityTestRule =
+            new ChromeTabbedActivityTestRule();
 
     private final ManualFillingTestHelper mHelper = new ManualFillingTestHelper(mActivityTestRule);
 
@@ -138,5 +138,29 @@ public class ManualFillingIntegrationTest {
         mHelper.waitToBeHidden(withId(R.id.keyboard_accessory_sheet));
     }
 
+    @Test
+    @SmallTest
+    public void testInvokingTabSwitcherHidesAccessory()
+            throws InterruptedException, TimeoutException {
+        mHelper.loadTestPage(false);
+        mHelper.createTestTab();
+
+        // Focus the field to bring up the accessory.
+        mHelper.clickPasswordField();
+        mHelper.waitForKeyboard();
+
+        // Click the tab to show the sheet and hide the keyboard.
+        whenDisplayed(withId(R.id.tabs)).perform(selectTabAtPosition(0));
+        mHelper.waitForKeyboardToDisappear();
+        whenDisplayed(withId(R.id.keyboard_accessory_sheet));
+
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            mActivityTestRule.getActivity().getLayoutManager().showOverview(false);
+            mActivityTestRule.getActivity().getLayoutManager().hideOverview(false);
+        });
+
+        mHelper.waitToBeHidden(withId(R.id.keyboard_accessory_sheet));
+    }
+
     // TODO(fhorschig): Check that it overlays info bars.
 }

chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/ManualFillingTestHelper.java

@@ -23,6 +23,7 @@ import android.support.test.espresso.PerformException;
 import android.support.test.espresso.UiController;
 import android.support.test.espresso.ViewAction;
 import android.support.test.espresso.ViewInteraction;
+import android.support.v7.content.res.AppCompatResources;
 import android.view.View;
 import android.view.ViewGroup;
 
@@ -31,8 +32,7 @@ import org.hamcrest.Matcher;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.UrlUtils;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeActivity;
-import org.chromium.chrome.test.ChromeActivityTestRule;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
 import org.chromium.content.browser.test.util.CriteriaHelper;
 import org.chromium.content.browser.test.util.DOMUtils;
 import org.chromium.content_public.browser.WebContents;
@@ -45,10 +45,10 @@ import java.util.concurrent.atomic.AtomicReference;
  * Helpers in this class simplify interactions with the Keyboard Accessory and the sheet below it.
  */
 public class ManualFillingTestHelper {
-    private final ChromeActivityTestRule<ChromeActivity> mActivityTestRule;
+    private final ChromeTabbedActivityTestRule mActivityTestRule;
     private final AtomicReference<WebContents> mWebContentsRef = new AtomicReference<>();
 
-    ManualFillingTestHelper(ChromeActivityTestRule<ChromeActivity> activityTestRule) {
+    ManualFillingTestHelper(ChromeTabbedActivityTestRule activityTestRule) {
         mActivityTestRule = activityTestRule;
     }
 
@@ -102,7 +102,7 @@ public class ManualFillingTestHelper {
     public void createTestTab() {
         mActivityTestRule.getActivity().getManualFillingController().getMediatorForTesting().addTab(
                 new KeyboardAccessoryData.Tab(
-                        InstrumentationRegistry.getContext().getResources().getDrawable(
+                        AppCompatResources.getDrawable(InstrumentationRegistry.getContext(),
                                 android.R.drawable.ic_lock_lock),
                         "TestTabDescription", R.layout.empty_accessory_sheet, null));
     }

chrome/android/javatests/src/org/chromium/chrome/browser/autofill/keyboard_accessory/PasswordAccessoryIntegrationTest.java

@@ -36,12 +36,11 @@ import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.FlakyTest;
 import org.chromium.base.test.util.RetryOnFailure;
 import org.chromium.chrome.R;
-import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.autofill.keyboard_accessory.KeyboardAccessoryData.Item;
-import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
 import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
 import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
 
@@ -57,8 +56,8 @@ import java.util.concurrent.atomic.AtomicReference;
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
 public class PasswordAccessoryIntegrationTest {
     @Rule
-    public final ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
-            new ChromeActivityTestRule<>(ChromeActivity.class);
+    public final ChromeTabbedActivityTestRule mActivityTestRule =
+            new ChromeTabbedActivityTestRule();
 
     private final ManualFillingTestHelper mHelper = new ManualFillingTestHelper(mActivityTestRule);