Make TrustedWebActivityBrowserControlsVisibilityManager only observe active tab

This CL adds
CustomTabActivityTabProvider#registerTabObserver()
CustomTabActivityTabProvider#unregisterTabObserver()
These methods register (and unregister) a TabObserver for the
CustomTabActivity's active tab. The tab being observed is changed when the
CustomTabActivity's active tab changes. This differs from
TabObserverRegistrar#registerTabObserver() which registers the passed-in
TabObserver for all newly created tabs.

BUG=1020911
TEST=TabObserverRegistrarTest.testObserveActiveTab

Change-Id: Ie5c4b8f287209b672e89184a3e9f0710c69102ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1896140
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713554}

chrome/android/chrome_test_java_sources.gni

@@ -125,6 +125,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/customtabs/DetachedResourceRequestTest.java",
   "javatests/src/org/chromium/chrome/browser/customtabs/RequestThrottlerTest.java",
   "javatests/src/org/chromium/chrome/browser/customtabs/TrustedCdnPublisherUrlTest.java",
+  "javatests/src/org/chromium/chrome/browser/customtabs/content/TabObserverRegistrarTest.java",
   "javatests/src/org/chromium/chrome/browser/customtabs/dynamicmodule/CustomTabsDynamicModuleLoaderTest.java",
   "javatests/src/org/chromium/chrome/browser/customtabs/dynamicmodule/CustomTabsDynamicModuleNavigationTest.java",
   "javatests/src/org/chromium/chrome/browser/customtabs/dynamicmodule/CustomTabsDynamicModulePostMessageTest.java",

chrome/android/java/src/org/chromium/chrome/browser/browserservices/trustedwebactivityui/controller/TrustedWebActivityBrowserControlsVisibilityManager.java

@@ -32,7 +32,7 @@ public class TrustedWebActivityBrowserControlsVisibilityManager {
     private final TabObserver mTabObserver = new EmptyTabObserver() {
         @Override
         public void onSSLStateUpdated(Tab tab) {
-            updateBrowserControlsState(tab);
+            updateBrowserControlsState();
         }
     };
 
@@ -40,7 +40,7 @@ public class TrustedWebActivityBrowserControlsVisibilityManager {
             new CustomTabActivityTabProvider.Observer() {
                 @Override
                 public void onTabSwapped(@NonNull Tab tab) {
-                    updateBrowserControlsState(tab);
+                    updateBrowserControlsState();
                 }
             };
 
@@ -60,20 +60,20 @@ public class TrustedWebActivityBrowserControlsVisibilityManager {
         if (mInTwaMode == inTwaMode) return;
 
         mInTwaMode = inTwaMode;
-        updateBrowserControlsState(mTabProvider.getTab());
+        updateBrowserControlsState();
 
         if (mInTwaMode) {
-            mTabObserverRegistrar.registerTabObserver(mTabObserver);
+            mTabObserverRegistrar.registerActivityTabObserver(mTabObserver);
             mTabProvider.addObserver(mActivityTabObserver);
         } else {
-            mTabObserverRegistrar.unregisterTabObserver(mTabObserver);
+            mTabObserverRegistrar.unregisterActivityTabObserver(mTabObserver);
             mTabProvider.removeObserver(mActivityTabObserver);
         }
     }
 
-    private void updateBrowserControlsState(Tab tab) {
+    private void updateBrowserControlsState() {
         @BrowserControlsState
-        int newBrowserControlsState = computeBrowserControlsState(tab);
+        int newBrowserControlsState = computeBrowserControlsState(mTabProvider.getTab());
         if (mBrowserControlsState == newBrowserControlsState) return;
 
         mBrowserControlsState = newBrowserControlsState;

chrome/android/java/src/org/chromium/chrome/browser/customtabs/content/TabObserverRegistrar.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.customtabs.content;
 
+import androidx.annotation.NonNull;
+
 import org.chromium.chrome.browser.dependency_injection.ActivityScope;
 import org.chromium.chrome.browser.lifecycle.ActivityLifecycleDispatcher;
 import org.chromium.chrome.browser.lifecycle.Destroyable;
@@ -25,9 +27,37 @@ import javax.inject.Inject;
  */
 @ActivityScope
 public class TabObserverRegistrar extends EmptyTabModelObserver implements Destroyable {
+    private CustomTabActivityTabProvider mTabProvider;
     private final Set<PageLoadMetrics.Observer> mPageLoadMetricsObservers = new HashSet<>();
     private final Set<TabObserver> mTabObservers = new HashSet<>();
 
+    /** Observers for active tab. */
+    private final Set<TabObserver> mActivityTabObservers = new HashSet<>();
+
+    /**
+     * Caches the {@link CustomTabActivityTabProvider}'s active tab so that TabObservers can be
+     * removed from the previous active tab when the active tab changes.
+     */
+    private Tab mTabProviderTab;
+
+    private final CustomTabActivityTabProvider.Observer mActivityTabProviderObserver =
+            new CustomTabActivityTabProvider.Observer() {
+                @Override
+                public void onInitialTabCreated(@NonNull Tab tab, @TabCreationMode int mode) {
+                    onTabProviderTabUpdated();
+                }
+
+                @Override
+                public void onTabSwapped(@NonNull Tab tab) {
+                    onTabProviderTabUpdated();
+                }
+
+                @Override
+                public void onAllTabsClosed() {
+                    onTabProviderTabUpdated();
+                }
+            };
+
     /**
      * Registers a {@link PageLoadMetrics.Observer} to be managed by this Registrar.
      */
@@ -49,8 +79,33 @@ public class TabObserverRegistrar extends EmptyTabModelObserver implements Destr
         mTabObservers.remove(observer);
     }
 
+    /**
+     * Registers a TabObserver for the CustomTabActivity's active tab. Changes the Tab that is
+     * being observed when the CustomTabActivity's active tab changes.
+     * Differs from {@link #registerTabObserver()} which observes all newly created tabs.
+     */
+    public void registerActivityTabObserver(TabObserver observer) {
+        mActivityTabObservers.add(observer);
+        Tab activeTab = mTabProvider.getTab();
+        if (activeTab != null) {
+            activeTab.addObserver(observer);
+        }
+    }
+
+    public void unregisterActivityTabObserver(TabObserver observer) {
+        mActivityTabObservers.remove(observer);
+        Tab activeTab = mTabProvider.getTab();
+        if (activeTab != null) {
+            activeTab.removeObserver(observer);
+        }
+    }
+
     @Inject
-    public TabObserverRegistrar(ActivityLifecycleDispatcher lifecycleDispatcher) {
+    public TabObserverRegistrar(ActivityLifecycleDispatcher lifecycleDispatcher,
+            CustomTabActivityTabProvider tabProvider) {
+        mTabProvider = tabProvider;
+        mTabProvider.addObserver(mActivityTabProviderObserver);
+
         lifecycleDispatcher.register(this);
     }
 
@@ -69,7 +124,7 @@ public class TabObserverRegistrar extends EmptyTabModelObserver implements Destr
     @Override
     public void tabRemoved(Tab tab) {
         removePageLoadMetricsObservers();
-        removeTabObservers(tab);
+        removeTabObservers(tab, mTabObservers);
     }
 
     /**
@@ -78,7 +133,7 @@ public class TabObserverRegistrar extends EmptyTabModelObserver implements Destr
      */
     public void addObserversForTab(Tab tab) {
         addPageLoadMetricsObservers();
-        addTabObservers(tab);
+        addTabObservers(tab, mTabObservers);
     }
 
     private void addPageLoadMetricsObservers() {
@@ -93,14 +148,27 @@ public class TabObserverRegistrar extends EmptyTabModelObserver implements Destr
         }
     }
 
-    private void addTabObservers(Tab tab) {
-        for (TabObserver observer : mTabObservers) {
+    /**
+     * Called when the {@link CustomTabActivityTabProvider}'s active tab has changed.
+     */
+    private void onTabProviderTabUpdated() {
+        if (mTabProviderTab != null) {
+            removeTabObservers(mTabProviderTab, mActivityTabObservers);
+        }
+        mTabProviderTab = mTabProvider.getTab();
+        if (mTabProviderTab != null) {
+            addTabObservers(mTabProviderTab, mActivityTabObservers);
+        }
+    }
+
+    private void addTabObservers(Tab tab, Set<TabObserver> tabObservers) {
+        for (TabObserver observer : tabObservers) {
             tab.addObserver(observer);
         }
     }
 
-    private void removeTabObservers(Tab tab) {
-        for (TabObserver observer : mTabObservers) {
+    private void removeTabObservers(Tab tab, Set<TabObserver> tabObservers) {
+        for (TabObserver observer : tabObservers) {
             tab.removeObserver(observer);
         }
     }

chrome/android/java/src/org/chromium/chrome/browser/customtabs/dependency_injection/CustomTabActivityComponent.java

@@ -20,6 +20,7 @@ import org.chromium.chrome.browser.customtabs.content.CustomTabActivityTabContro
 import org.chromium.chrome.browser.customtabs.content.CustomTabActivityTabFactory;
 import org.chromium.chrome.browser.customtabs.content.CustomTabActivityTabProvider;
 import org.chromium.chrome.browser.customtabs.content.CustomTabIntentHandler;
+import org.chromium.chrome.browser.customtabs.content.TabObserverRegistrar;
 import org.chromium.chrome.browser.customtabs.dynamicmodule.DynamicModuleCoordinator;
 import org.chromium.chrome.browser.customtabs.dynamicmodule.DynamicModuleToolbarController;
 import org.chromium.chrome.browser.customtabs.features.ImmersiveModeController;
@@ -56,6 +57,7 @@ public interface CustomTabActivityComponent extends ChromeActivityComponent {
     CustomTabCompositorContentInitializer resolveCompositorContentInitializer();
     CustomTabSessionHandler resolveSessionHandler();
     CustomTabActivityClientConnectionKeeper resolveConnectionKeeper();
+    TabObserverRegistrar resolveTabObserverRegistrar();
     TwaFinishHandler resolveTwaFinishHandler();
     ImmersiveModeController resolveImmersiveModeController();
 

chrome/android/java/src/org/chromium/chrome/browser/customtabs/features/toolbar/CustomTabToolbarColorController.java

@@ -4,8 +4,11 @@
 
 package org.chromium.chrome.browser.customtabs.features.toolbar;
 
+import androidx.annotation.NonNull;
+
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.browserservices.BrowserServicesIntentDataProvider;
+import org.chromium.chrome.browser.customtabs.content.CustomTabActivityTabProvider;
 import org.chromium.chrome.browser.customtabs.content.TabObserverRegistrar;
 import org.chromium.chrome.browser.dependency_injection.ActivityScope;
 import org.chromium.chrome.browser.lifecycle.ActivityLifecycleDispatcher;
@@ -28,16 +31,33 @@ public class CustomTabToolbarColorController implements InflationObserver {
     private final BrowserServicesIntentDataProvider mIntentDataProvider;
     private final ChromeActivity mActivity;
     private final TabObserverRegistrar mTabObserverRegistrar;
+    private final CustomTabActivityTabProvider mTabProvider;
+
+    /** Keeps track of the original color before the preview was shown. */
+    private int mOriginalColor;
+
+    /** True if a change to the toolbar color was made because of a preview. */
+    private boolean mTriggeredPreviewChange;
+
+    private final CustomTabActivityTabProvider.Observer mActivityTabObserver =
+            new CustomTabActivityTabProvider.Observer() {
+                @Override
+                public void onTabSwapped(@NonNull Tab tab) {
+                    updateColor(tab);
+                }
+            };
 
     @Inject
     public CustomTabToolbarColorController(ActivityLifecycleDispatcher lifecycleDispatcher,
             Lazy<ToolbarManager> toolbarManager,
             BrowserServicesIntentDataProvider intentDataProvider,
             ChromeActivity activity,
+            CustomTabActivityTabProvider tabProvider,
             TabObserverRegistrar tabObserverRegistrar) {
         mToolbarManager = toolbarManager;
         mIntentDataProvider = intentDataProvider;
         mActivity = activity;
+        mTabProvider = tabProvider;
         mTabObserverRegistrar = tabObserverRegistrar;
         lifecycleDispatcher.register(this);
     }
@@ -56,16 +76,11 @@ public class CustomTabToolbarColorController implements InflationObserver {
             manager.setShouldUpdateToolbarPrimaryColor(false);
         }
         observeTabToUpdateColor();
+        mTabProvider.addObserver(mActivityTabObserver);
     }
 
     private void observeTabToUpdateColor() {
-        mTabObserverRegistrar.registerTabObserver(new EmptyTabObserver() {
-            /** Keeps track of the original color before the preview was shown. */
-            private int mOriginalColor;
-
-            /** True if a change to the toolbar color was made because of a preview. */
-            private boolean mTriggeredPreviewChange;
-
+        mTabObserverRegistrar.registerActivityTabObserver(new EmptyTabObserver() {
             @Override
             public void onPageLoadFinished(Tab tab, String url) {
                 // Update the color when the page load finishes.
@@ -77,6 +92,8 @@ public class CustomTabToolbarColorController implements InflationObserver {
                 // Update the color on every new URL.
                 updateColor(tab);
             }
+        });
+    }
 
     /**
      * Updates the color of the Activity's CCT Toolbar. When a preview is shown, it should
@@ -108,6 +125,4 @@ public class CustomTabToolbarColorController implements InflationObserver {
 
         manager.setShouldUpdateToolbarPrimaryColor(shouldUpdateOriginal);
     }
-        });
-    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/content/TabObserverRegistrarTest.java

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.customtabs.content;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
+import android.support.test.InstrumentationRegistry;
+import android.support.test.filters.MediumTest;
+
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.test.util.CallbackHelper;
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.customtabs.CustomTabActivity;
+import org.chromium.chrome.browser.customtabs.CustomTabActivityTestRule;
+import org.chromium.chrome.browser.customtabs.CustomTabsTestUtils;
+import org.chromium.chrome.browser.tab.EmptyTabObserver;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tabmodel.EmptyTabModelObserver;
+import org.chromium.chrome.browser.tabmodel.TabLaunchType;
+import org.chromium.chrome.browser.tabmodel.TabModelSelector;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.content_public.browser.LoadUrlParams;
+import org.chromium.content_public.browser.test.util.DOMUtils;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
+import org.chromium.net.test.EmbeddedTestServer;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Tests for {@link TabObserverRegistrar}.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class TabObserverRegistrarTest {
+    private static class LoadUrlTabObserver extends EmptyTabObserver {
+        private List<String> mUrlLoadRequests = new ArrayList<>();
+
+        List<String> getLoadUrlRequests() {
+            return mUrlLoadRequests;
+        }
+
+        @Override
+        public void onLoadUrl(Tab tab, LoadUrlParams params, int loadType) {
+            mUrlLoadRequests.add(params.getUrl());
+        }
+    }
+
+    @Rule
+    public CustomTabActivityTestRule mCustomTabActivityTestRule = new CustomTabActivityTestRule();
+
+    /**
+     * Tests that the TabObserver registered by
+     * {@link TabObserverRegistrar#registerActivityTabObserver()} switches when the active tab is
+     * switched.
+     */
+    @Test
+    @MediumTest
+    public void testObserveActiveTab() throws Throwable {
+        EmbeddedTestServer testServer = mCustomTabActivityTestRule.getTestServer();
+        final String windowOpenUrl =
+                testServer.getURL("/chrome/test/data/android/customtabs/test_window_open.html");
+        final String url1 = testServer.getURL("/chrome/test/data/android/about.html");
+        final String url2 = testServer.getURL("/chrome/test/data/android/simple.html");
+
+        mCustomTabActivityTestRule.startCustomTabActivityWithIntent(
+                CustomTabsTestUtils.createMinimalCustomTabIntent(
+                        InstrumentationRegistry.getTargetContext(), windowOpenUrl));
+
+        // Register TabObserver via TabObserverRegistrar#registerActiveTabObserver()
+        CustomTabActivity customTabActivity = mCustomTabActivityTestRule.getActivity();
+        TabObserverRegistrar tabObserverRegistrar =
+                customTabActivity.getComponent().resolveTabObserverRegistrar();
+        LoadUrlTabObserver loadUrlTabObserver = new LoadUrlTabObserver();
+        tabObserverRegistrar.registerActivityTabObserver(loadUrlTabObserver);
+
+        final TabModelSelector tabSelector = customTabActivity.getTabModelSelector();
+        final Tab initialActiveTab = tabSelector.getCurrentTab();
+
+        // Open and wait for popup.
+        final CallbackHelper openTabHelper = new CallbackHelper();
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            tabSelector.getModel(false).addObserver(new EmptyTabModelObserver() {
+                @Override
+                public void didAddTab(Tab tab, @TabLaunchType int type) {
+                    openTabHelper.notifyCalled();
+                }
+            });
+        });
+        DOMUtils.clickNode(mCustomTabActivityTestRule.getWebContents(), "new_window");
+        openTabHelper.waitForCallback(0, 1);
+
+        assertEquals(2, tabSelector.getModel(false).getCount());
+        final Tab activeTab = tabSelector.getCurrentTab();
+        assertNotEquals(activeTab, initialActiveTab);
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            android.util.Log.e("ABCD", "loadUrl0");
+            initialActiveTab.loadUrl(new LoadUrlParams(url1));
+            activeTab.loadUrl(new LoadUrlParams(url2));
+        });
+
+        List<String> urlRequests = loadUrlTabObserver.getLoadUrlRequests();
+        assertEquals(1, urlRequests.size());
+        assertEquals(url2, urlRequests.get(0));
+    }
+}