Android: Fix a bug of text selection not working in SERP

ChromeActionModeHandler adds an observer for init-web-contents event
of TabWebContentsObserver for each tab in order to register
ChromeActionModeCallback to its WebContents, therefore enabling
floating/toolbar action mode menu.

For newly created tab, there was a bug this registration of the callback
not working (ok for the tab already created and swapping into
foreground). It's because init-web-contents event occurs before
adding an observer(mInitWebContentsObserver) to TabWebContentsObserver,
missing the chance to register the callback.

This CL manually triggers the mInitWebContentsObserver for all the newly
created tab to fix the bug. Previously it was applied to only the very
first tab being added, but now extended to all those the newly created
and added.

Bug: 1067524
Change-Id: I511f15363e1e77f13347c5358aaf6eb3874a1a7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2136569Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758641}

chrome/android/chrome_test_java_sources.gni

@@ -8,6 +8,7 @@
 chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/ActivityTabProviderTest.java",
   "javatests/src/org/chromium/chrome/browser/AudioTest.java",
+  "javatests/src/org/chromium/chrome/browser/ChromeActionModeHandlerTest.java",
   "javatests/src/org/chromium/chrome/browser/ChromeActivityTest.java",
   "javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java",
   "javatests/src/org/chromium/chrome/browser/ContentViewFocusTest.java",

chrome/android/java/src/org/chromium/chrome/browser/ChromeActionModeHandler.java

@@ -45,15 +45,12 @@ import java.util.Set;
  */
 public class ChromeActionModeHandler {
     /** Observes the active WebContents being initialized into a Tab. */
-    private final Consumer<WebContents> mInitWebContentsObserver;
+    private final Callback<WebContents> mInitWebContentsObserver;
 
     private final ActivityTabProvider.ActivityTabTabObserver mActivityTabTabObserver;
 
     private Tab mActiveTab;
 
-    /** Set to {@code true} once the first Tab content is changed. */
-    private boolean mContentChanged;
-
     /**
      * @param activityTabProvider {@link ActivityTabProvider} instance.
      * @param actionBarObserver observer called when the contextual action bar's visibility
@@ -84,19 +81,6 @@ public class ChromeActionModeHandler {
                         TabWebContentsObserver.from(tab).addInitWebContentsObserver(
                                 mInitWebContentsObserver);
                         mActiveTab = tab;
-
-                        // For the very first tab being observed, we miss mInitWebContentsObserver
-                        // because TabObserver.onContentChanged ->
-                        // TabWebContentsObserver.initWebContents occurs before this activity tab
-                        // observer is ready. Manually triggers it here.
-                        if (!mContentChanged && tab.getWebContents() != null) {
-                            mInitWebContentsObserver.accept(tab.getWebContents());
-                        }
-                    }
-
-                    @Override
-                    public void onContentChanged(Tab tab) {
-                        mContentChanged = true;
                     }
                 };
     }

chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java

@@ -4,6 +4,7 @@
 
 package org.chromium.chrome.browser.tab;
 
+import android.os.Handler;
 import android.view.View;
 
 import androidx.annotation.IntDef;
@@ -12,9 +13,10 @@ import androidx.annotation.VisibleForTesting;
 import org.chromium.base.ActivityState;
 import org.chromium.base.ApplicationState;
 import org.chromium.base.ApplicationStatus;
-import org.chromium.base.Consumer;
+import org.chromium.base.Callback;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
+import org.chromium.base.ObserverList;
 import org.chromium.base.ObserverList.RewindableIterator;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.AppHooks;
@@ -32,8 +34,6 @@ import org.chromium.content_public.browser.WebContentsObserver;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
-import java.util.ArrayList;
-import java.util.List;
 
 /**
  * WebContentsObserver used by Tab.
@@ -73,7 +73,8 @@ public class TabWebContentsObserver extends TabWebContentsUserData {
     }
 
     private final TabImpl mTab;
-    private final List<Consumer<WebContents>> mInitObservers = new ArrayList<>();
+    private final ObserverList<Callback<WebContents>> mInitObservers = new ObserverList<>();
+    private final Handler mHandler = new Handler();
     private WebContentsObserver mObserver;
 
     public static TabWebContentsObserver from(Tab tab) {
@@ -97,16 +98,22 @@ public class TabWebContentsObserver extends TabWebContentsUserData {
 
     /**
      * Adds an observer triggered when |initWebContents| is invoked.
+     * <p>A newly created tab adding this observer misses the event because
+     * |TabObserver.onContentChanged| -&gt; |TabWebContentsObserver.initWebContents|
+     * occurs before the observer is added. Manually trigger it here.
+     * @param observer Observer to add.
      */
-    public void addInitWebContentsObserver(Consumer<WebContents> initObserver) {
-        mInitObservers.add(initObserver);
+    public void addInitWebContentsObserver(Callback<WebContents> observer) {
+        if (mInitObservers.addObserver(observer) && mTab.getWebContents() != null) {
+            observer.onResult(mTab.getWebContents());
+        }
     }
 
     /**
      * Remove the InitWebContents observer from the list.
      */
-    public void removeInitWebContentsObserver(Consumer<WebContents> initObserver) {
-        mInitObservers.remove(initObserver);
+    public void removeInitWebContentsObserver(Callback<WebContents> observer) {
+        mInitObservers.removeObserver(observer);
     }
 
     @Override
@@ -122,7 +129,7 @@ public class TabWebContentsObserver extends TabWebContentsUserData {
         // is not the default behavior for embedded web views.
         WebContentsAccessibility.fromWebContents(webContents).setShouldFocusOnPageLoad(true);
 
-        for (Consumer<WebContents> consumer : mInitObservers) consumer.accept(webContents);
+        for (Callback<WebContents> callback : mInitObservers) callback.onResult(webContents);
     }
 
     @Override

chrome/android/javatests/src/org/chromium/chrome/browser/ChromeActionModeHandlerTest.java

+// Copyright 2020 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;
+
+import android.support.test.InstrumentationRegistry;
+import android.support.test.filters.SmallTest;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.chrome.browser.flags.ChromeSwitches;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.TabLaunchType;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
+import org.chromium.components.embedder_support.util.UrlConstants;
+import org.chromium.content_public.browser.LoadUrlParams;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
+import org.chromium.content_public.browser.test.util.WebContentsUtils;
+import org.chromium.content_public.common.ContentUrlConstants;
+import org.chromium.net.test.EmbeddedTestServer;
+
+/**
+ * Tests {@link ChromeActionModeHandler} operation.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class ChromeActionModeHandlerTest {
+    @Rule
+    public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
+
+    private void assertActionModeIsReady() {
+        // clang-format off
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> Assert.assertTrue(WebContentsUtils.isActionModeSupported(
+                        mActivityTestRule.getWebContents())));
+        // clang-format on
+    }
+
+    @Test
+    @SmallTest
+    public void testActionModeSetForNewTab() {
+        EmbeddedTestServer testServer = EmbeddedTestServer.createAndStartServer(
+                InstrumentationRegistry.getInstrumentation().getContext());
+
+        mActivityTestRule.startMainActivityWithURL(UrlConstants.NTP_URL);
+        mActivityTestRule.loadUrl(ContentUrlConstants.ABOUT_BLANK_DISPLAY_URL);
+        assertActionModeIsReady();
+
+        LoadUrlParams urlParams = new LoadUrlParams(ContentUrlConstants.ABOUT_BLANK_DISPLAY_URL);
+        Tab tab = mActivityTestRule.getActivity().getActivityTabProvider().get();
+
+        // Assert that a new tab has an action mode callback set as expected.
+        // clang-format off
+        TestThreadUtils.runOnUiThreadBlockingNoException(
+                () -> mActivityTestRule.getActivity().getTabModelSelector().openNewTab(
+                        urlParams, TabLaunchType.FROM_LONGPRESS_FOREGROUND, tab, true));
+        // clang-format on
+        assertActionModeIsReady();
+        testServer.stopAndDestroyServer();
+    }
+}

content/public/android/java/src/org/chromium/content/browser/selection/SelectionPopupControllerImpl.java

@@ -352,7 +352,8 @@ public class SelectionPopupControllerImpl extends ActionModeCallbackHelper
     }
 
     // True if action mode is initialized to a working (not a no-op) mode.
-    private boolean isActionModeSupported() {
+    @VisibleForTesting
+    public boolean isActionModeSupported() {
         return mCallback != EMPTY_CALLBACK;
     }
 

content/public/test/android/javatests/src/org/chromium/content_public/browser/test/util/WebContentsUtils.java

@@ -122,6 +122,18 @@ public class WebContentsUtils {
         return SelectionPopupControllerImpl.createForTesting(webContents);
     }
 
+    /**
+     * Checks if the given WebContents has a valid {@link ActionMode.Callback} set in place.
+     * @return {@code true} if WebContents (its SelectionPopupController) has a valid
+     *         action mode callback object.
+     */
+    public static boolean isActionModeSupported(WebContents webContents) {
+        SelectionPopupControllerImpl controller =
+                ((SelectionPopupControllerImpl) SelectionPopupController.fromWebContents(
+                        webContents));
+        return controller.isActionModeSupported();
+    }
+
     private static native void nativeReportAllFrameSubmissions(
             WebContents webContents, boolean enabled);
     private static native RenderFrameHost nativeGetFocusedFrame(WebContents webContents);