Moving DSE preconnect to tabbed activity foreground/background

This replaces app state observing with tabbed activity observing. One
key difference is that tabbed activity does not include CCT. We only
want to preconnect DSE for non-CCT chrome foreground.

Bug: 1041118
Change-Id: I9e88e15c11cdfd7d13078a71c680be31c9b05932
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006478
Commit-Queue: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735128}

chrome/android/BUILD.gn

@@ -2675,6 +2675,7 @@ generate_jni("chrome_jni_headers") {
     "java/src/org/chromium/chrome/browser/metrics/UmaUtils.java",
     "java/src/org/chromium/chrome/browser/metrics/VariationsSession.java",
     "java/src/org/chromium/chrome/browser/mojo/ChromeInterfaceRegistrar.java",
+    "java/src/org/chromium/chrome/browser/navigation_predictor/NavigationPredictorBridge.java",
     "java/src/org/chromium/chrome/browser/net/nqe/NetworkQualityProvider.java",
     "java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java",
     "java/src/org/chromium/chrome/browser/notifications/ActionInfo.java",

chrome/android/chrome_java_sources.gni

@@ -938,6 +938,7 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/native_page/NativePageFactory.java",
   "java/src/org/chromium/chrome/browser/native_page/NativePageNavigationDelegate.java",
   "java/src/org/chromium/chrome/browser/native_page/NativePageNavigationDelegateImpl.java",
+  "java/src/org/chromium/chrome/browser/navigation_predictor/NavigationPredictorBridge.java",
   "java/src/org/chromium/chrome/browser/net/nqe/NetworkQualityObserver.java",
   "java/src/org/chromium/chrome/browser/net/nqe/NetworkQualityProvider.java",
   "java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java",

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

@@ -98,6 +98,7 @@ import org.chromium.chrome.browser.modaldialog.TabModalPresenter;
 import org.chromium.chrome.browser.multiwindow.MultiInstanceChromeTabbedActivity;
 import org.chromium.chrome.browser.multiwindow.MultiInstanceManager;
 import org.chromium.chrome.browser.native_page.NativePageAssassin;
+import org.chromium.chrome.browser.navigation_predictor.NavigationPredictorBridge;
 import org.chromium.chrome.browser.night_mode.WebContentsDarkModeController;
 import org.chromium.chrome.browser.ntp.NewTabPage;
 import org.chromium.chrome.browser.ntp.NewTabPageUma;
@@ -847,6 +848,12 @@ public class ChromeTabbedActivity extends ChromeActivity {
             }
         }
 
+        if (isWarmOnResume()) {
+            NavigationPredictorBridge.onActivityWarmResumed();
+        } else {
+            NavigationPredictorBridge.onColdStart();
+        }
+
         // This call is not guarded by a feature flag.
         SearchEngineChoiceNotification.handleSearchEngineChoice(this, getSnackbarManager());
 
@@ -863,6 +870,8 @@ public class ChromeTabbedActivity extends ChromeActivity {
         mLocaleManager.setSnackbarManager(null);
         mLocaleManager.stopObservingPhoneChanges();
 
+        NavigationPredictorBridge.onPause();
+
         super.onPauseWithNative();
     }
 

chrome/android/java/src/org/chromium/chrome/browser/navigation_predictor/NavigationPredictorBridge.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.navigation_predictor;
+
+import org.chromium.base.annotations.NativeMethods;
+
+/**
+ * Exposes methods to report tabs moving to foreground/background.
+ */
+public class NavigationPredictorBridge {
+    private NavigationPredictorBridge() {}
+
+    /**
+     * Reports to native that tabbed activity has resumed.
+     */
+    public static void onActivityWarmResumed() {
+        NavigationPredictorBridgeJni.get().onActivityWarmResumed();
+    }
+
+    /**
+     * Reports to native that tabbed activity has started.
+     */
+    public static void onColdStart() {
+        NavigationPredictorBridgeJni.get().onColdStart();
+    }
+
+    /**
+     * Reports to native that tabbed activity has paused (moved to background).
+     */
+    public static void onPause() {
+        NavigationPredictorBridgeJni.get().onPause();
+    }
+
+    @NativeMethods
+    interface Natives {
+        void onActivityWarmResumed();
+        void onColdStart();
+        void onPause();
+    }
+}

chrome/android/java/src/org/chromium/chrome/browser/navigation_predictor/OWNERS

+ryansturm@chromium.org
+robertogden@chromium.org
+
+# COMPONENT: Internals>Preload
\ No newline at end of file

chrome/browser/BUILD.gn

@@ -2549,6 +2549,7 @@ jumbo_static_library("browser") {
       "android/metrics/variations_session.cc",
       "android/mojo/chrome_interface_registrar_android.cc",
       "android/mojo/chrome_interface_registrar_android.h",
+      "android/navigation_predictor/navigation_predictor_bridge.cc",
       "android/net/nqe/network_quality_provider.cc",
       "android/net/nqe/network_quality_provider.h",
       "android/nfc_system_level_setting.h",

chrome/browser/android/navigation_predictor/OWNERS

+file://chrome/browser/navigation_predictor/OWNERS
+
+# COMPONENT: Internals>Preload
\ No newline at end of file

chrome/browser/android/navigation_predictor/navigation_predictor_bridge.cc

+// 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.
+
+#include <jni.h>
+#include <vector>
+
+#include "base/android/jni_android.h"
+#include "chrome/android/chrome_jni_headers/NavigationPredictorBridge_jni.h"
+#include "chrome/browser/navigation_predictor/navigation_predictor_keyed_service.h"
+#include "chrome/browser/navigation_predictor/navigation_predictor_keyed_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+
+using base::android::AttachCurrentThread;
+using base::android::JavaParamRef;
+
+static void JNI_NavigationPredictorBridge_OnColdStart(JNIEnv* env) {
+  NavigationPredictorKeyedService* navigation_predictor_service =
+      NavigationPredictorKeyedServiceFactory::GetForProfile(
+          ProfileManager::GetLastUsedProfile());
+  if (!navigation_predictor_service)
+    return;
+  navigation_predictor_service->search_engine_preconnector()
+      ->StartPreconnecting(
+          /*with_startup_delay=*/true);
+}
+
+static void JNI_NavigationPredictorBridge_OnActivityWarmResumed(JNIEnv* env) {
+  NavigationPredictorKeyedService* navigation_predictor_service =
+      NavigationPredictorKeyedServiceFactory::GetForProfile(
+          ProfileManager::GetLastUsedProfile());
+  if (!navigation_predictor_service)
+    return;
+  navigation_predictor_service->search_engine_preconnector()
+      ->StartPreconnecting(
+          /*with_startup_delay=*/false);
+}
+
+static void JNI_NavigationPredictorBridge_OnPause(JNIEnv* env) {
+  NavigationPredictorKeyedService* navigation_predictor_service =
+      NavigationPredictorKeyedServiceFactory::GetForProfile(
+          ProfileManager::GetLastUsedProfile());
+  if (!navigation_predictor_service)
+    return;
+  navigation_predictor_service->search_engine_preconnector()
+      ->StopPreconnecting();
+}

chrome/browser/navigation_predictor/navigation_predictor_keyed_service.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/navigation_predictor/navigation_predictor_keyed_service.h"
 
 #include "base/compiler_specific.h"
+#include "build/build_config.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
@@ -46,8 +47,10 @@ NavigationPredictorKeyedService::NavigationPredictorKeyedService(
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!browser_context->IsOffTheRecord());
 
+#if !defined(OS_ANDROID)
   // Start preconnecting to the search engine.
   search_engine_preconnector_.StartPreconnecting(/*with_startup_delay=*/true);
+#endif
 }
 
 NavigationPredictorKeyedService::~NavigationPredictorKeyedService() {
@@ -80,6 +83,7 @@ void NavigationPredictorKeyedService::RemoveObserver(Observer* observer) {
 }
 
 SearchEnginePreconnector*
-NavigationPredictorKeyedService::SearchEnginePreconnectorForTesting() {
+NavigationPredictorKeyedService::search_engine_preconnector() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return &search_engine_preconnector_;
 }

chrome/browser/navigation_predictor/navigation_predictor_keyed_service.h

@@ -71,7 +71,7 @@ class NavigationPredictorKeyedService : public KeyedService {
       content::BrowserContext* browser_context);
   ~NavigationPredictorKeyedService() override;
 
-  SearchEnginePreconnector* SearchEnginePreconnectorForTesting();
+  SearchEnginePreconnector* search_engine_preconnector();
 
   // |document_url| may be invalid. Called by navigation predictor.
   void OnPredictionUpdated(const content::WebContents* web_contents,

chrome/browser/navigation_predictor/search_engine_preconnector.cc

@@ -7,11 +7,11 @@
 #include "base/bind.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/time/time.h"
-#include "build/build_config.h"
 #include "chrome/browser/predictors/loading_predictor.h"
 #include "chrome/browser/predictors/loading_predictor_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "components/google/core/common/google_util.h"
 #include "components/search_engines/template_url_service.h"
 #include "content/public/browser/browser_context.h"
 #include "net/base/features.h"
@@ -24,74 +24,18 @@ const base::Feature kPreconnectToSearch{"PreconnectToSearch",
 
 SearchEnginePreconnector::SearchEnginePreconnector(
     content::BrowserContext* browser_context)
-    :
-#if defined(OS_ANDROID)
-      application_status_listener_(
-          base::android::ApplicationStatusListener::New(base::BindRepeating(
-              &SearchEnginePreconnector::OnApplicationStateChange,
-              // It's safe to use base::Unretained here since the application
-              // state listener is owned by |this|. So, no callbacks can
-              // arrive after |this| has been destroyed.
-              base::Unretained(this)))),
-#endif  // defined(OS_ANDROID)
-      browser_context_(browser_context),
-      currently_in_foreground_(true) {
+    : browser_context_(browser_context) {
   DCHECK(!browser_context_->IsOffTheRecord());
-
-#if defined(OS_ANDROID)
-  auto application_state = base::android::ApplicationStatusListener::GetState();
-
-  currently_in_foreground_ =
-      application_state ==
-          base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES ||
-      application_state ==
-          base::android::APPLICATION_STATE_HAS_PAUSED_ACTIVITIES;
-#endif  // defined(OS_ANDROID)
 }
 
 SearchEnginePreconnector::~SearchEnginePreconnector() = default;
 
-#if defined(OS_ANDROID)
-void SearchEnginePreconnector::OnApplicationStateChange(
-    base::android::ApplicationState application_state) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (!application_status_listener_)
-    return;
-
-  OnAppStateChanged(
-      application_state ==
-          base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES ||
-      application_state ==
-          base::android::APPLICATION_STATE_HAS_PAUSED_ACTIVITIES);
-}
-#endif  // defined(OS_ANDROID)
-
-void SearchEnginePreconnector::OnAppStateChangedForTesting(bool in_foreground) {
-  OnAppStateChanged(in_foreground);
-}
-
-void SearchEnginePreconnector::OnAppStateChanged(bool in_foreground) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (currently_in_foreground_ == in_foreground)
-    return;
-
-  currently_in_foreground_ = in_foreground;
-
-  if (!currently_in_foreground_) {
-    // Stop any future preconnects while in background.
-    timer_.Stop();
-    return;
-  }
-
-  StartPreconnecting(/*with_startup_delay=*/false);
+void SearchEnginePreconnector::StopPreconnecting() {
+  timer_.Stop();
 }
 
 void SearchEnginePreconnector::StartPreconnecting(bool with_startup_delay) {
-  if (!currently_in_foreground_)
-    return;
-
+  timer_.Stop();
   if (with_startup_delay) {
     timer_.Start(
         FROM_HERE,
@@ -108,7 +52,6 @@ void SearchEnginePreconnector::StartPreconnecting(bool with_startup_delay) {
 
 void SearchEnginePreconnector::PreconnectDSE() {
   DCHECK(!browser_context_->IsOffTheRecord());
-  DCHECK(currently_in_foreground_);
   DCHECK(!timer_.IsRunning());
 
   if (!base::FeatureList::IsEnabled(features::kPreconnectToSearch))

chrome/browser/navigation_predictor/search_engine_preconnector.h

@@ -8,13 +8,8 @@
 #include "base/feature_list.h"
 #include "base/sequence_checker.h"
 #include "base/timer/timer.h"
-#include "build/build_config.h"
 #include "url/origin.h"
 
-#if defined(OS_ANDROID)
-#include "base/android/application_status_listener.h"
-#endif
-
 namespace content {
 class BrowserContext;
 }  // namespace content
@@ -37,7 +32,8 @@ class SearchEnginePreconnector {
   // only during app start up.
   void StartPreconnecting(bool with_startup_delay);
 
-  void OnAppStateChangedForTesting(bool in_foreground);
+  // Stops preconnecting to the DSE. Called on app background.
+  void StopPreconnecting();
 
  private:
   // Preconnects to the default search engine synchronously. Preconnects in
@@ -47,32 +43,12 @@ class SearchEnginePreconnector {
   // Queries template service for the current DSE URL.
   GURL GetDefaultSearchEngineOriginURL() const;
 
-  // Called on Android when the app moves to or from the background/foreground.
-  // |in_foreground| is whether it is moving into the foreground or not.
-  void OnAppStateChanged(bool in_foreground);
-
-#if defined(OS_ANDROID)
-  // Called when application state changes. e.g., application is brought to the
-  // background or the foreground.
-  void OnApplicationStateChange(
-      base::android::ApplicationState application_state);
-
-  // Used to listen to the changes in the application state changes. e.g., when
-  // the application is brought to the background or the foreground.
-  std::unique_ptr<base::android::ApplicationStatusListener>
-      application_status_listener_;
-#endif  // defined(OS_ANDROID)
-
   // Used to get keyed services.
   content::BrowserContext* const browser_context_;
 
   // Used to preconnect regularly.
   base::OneShotTimer timer_;
 
-  // Always true on desktop, on Android only true when the app is the foreground
-  // app.
-  bool currently_in_foreground_;
-
   SEQUENCE_CHECKER(sequence_checker_);
 };
 

chrome/browser/navigation_predictor/search_engine_preconnector_browsertest.cc

@@ -28,10 +28,6 @@
 
 namespace {
 
-// Feature to control preconnect to search.
-const base::Feature kPreconnectToSearchTest{"PreconnectToSearch",
-                                            base::FEATURE_DISABLED_BY_DEFAULT};
-
 // TODO(https://crbug.com/1042727): Fix test GURL scoping and remove this getter
 // function.
 GURL FakeSearch() {
@@ -129,7 +125,7 @@ class SearchEnginePreconnectorNoDelaysBrowserTest
   SearchEnginePreconnectorNoDelaysBrowserTest() {
     {
       feature_list_.InitWithFeaturesAndParameters(
-          {{kPreconnectToSearchTest, {{"startup_delay_ms", "0"}}},
+          {{features::kPreconnectToSearch, {{"startup_delay_ms", "1000000"}}},
            {net::features::kNetUnusedIdleSocketTimeout,
             {{"unused_idle_socket_timeout_seconds", "0"}}}},
           {});
@@ -144,6 +140,11 @@ class SearchEnginePreconnectorNoDelaysBrowserTest
 
 IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorNoDelaysBrowserTest,
                        PreconnectSearch) {
+  // Put the fake search URL to be preconnected in foreground.
+  NavigationPredictorKeyedServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(browser()->profile()))
+      ->search_engine_preconnector()
+      ->StartPreconnecting(/*with_startup_delay=*/false);
   // Verifies that the default search is preconnected.
   static const char kShortName[] = "test";
   static const char kSearchURL[] =
@@ -155,8 +156,8 @@ IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorNoDelaysBrowserTest,
   ASSERT_TRUE(model->loaded());
 
   // Check default URL is being preconnected and test URL is not.
-  WaitForPreresolveCountForURL(GoogleSearch(), 1);
-  EXPECT_EQ(1, preresolve_counts_[GoogleSearch().GetOrigin()]);
+  WaitForPreresolveCountForURL(GoogleSearch(), 2);
+  EXPECT_EQ(2, preresolve_counts_[GoogleSearch().GetOrigin()]);
   EXPECT_EQ(0, preresolve_counts_[GetTestURL("/").GetOrigin()]);
 
   TemplateURLData data;
@@ -168,6 +169,12 @@ IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorNoDelaysBrowserTest,
   ASSERT_TRUE(template_url);
   model->SetUserSelectedDefaultSearchProvider(template_url);
 
+  // Put the fake search URL to be preconnected in foreground.
+  NavigationPredictorKeyedServiceFactory::GetForProfile(
+      Profile::FromBrowserContext(browser()->profile()))
+      ->search_engine_preconnector()
+      ->StartPreconnecting(/*with_startup_delay=*/false);
+
   // After switching search providers, the test URL should now start being
   // preconnected.
   WaitForPreresolveCountForURL(GetTestURL("/"), 2);
@@ -181,12 +188,6 @@ IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorNoDelaysBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorNoDelaysBrowserTest,
                        PreconnectOnlyInForeground) {
-  // Verifies that the default search is preconnected only on app foreground.
-  NavigationPredictorKeyedServiceFactory::GetForProfile(
-      Profile::FromBrowserContext(browser()->profile()))
-      ->SearchEnginePreconnectorForTesting()
-      ->OnAppStateChangedForTesting(false /* in_foreground */);
-
   static const char kShortName[] = "test";
   static const char kSearchURL[] =
       "/anchors_different_area.html?q={searchTerms}";
@@ -221,9 +222,8 @@ IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorNoDelaysBrowserTest,
   // Put the fake search URL to be preconnected in foreground.
   NavigationPredictorKeyedServiceFactory::GetForProfile(
       Profile::FromBrowserContext(browser()->profile()))
-      ->SearchEnginePreconnectorForTesting()
-      ->OnAppStateChangedForTesting(true /* in_foreground */);
-
+      ->search_engine_preconnector()
+      ->StartPreconnecting(/*with_startup_delay=*/false);
   WaitForPreresolveCountForURL(FakeSearch(), 2);
 
   // Preconnect should occur for fake search (2 since there are 2 NIKs).
@@ -239,7 +239,7 @@ class SearchEnginePreconnectorKeepSocketBrowserTest
   SearchEnginePreconnectorKeepSocketBrowserTest() {
     {
       feature_list_.InitWithFeaturesAndParameters(
-          {{kPreconnectToSearchTest, {{"startup_delay_ms", "0"}}},
+          {{features::kPreconnectToSearch, {{"startup_delay_ms", "1000000"}}},
            {net::features::kNetUnusedIdleSocketTimeout,
             {{"unused_idle_socket_timeout_seconds", "60"}}}},
           {});
@@ -255,11 +255,6 @@ class SearchEnginePreconnectorKeepSocketBrowserTest
 IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorKeepSocketBrowserTest,
                        SocketWarmForSearch) {
   // Verifies that a navigation to search will use a warm socket.
-  NavigationPredictorKeyedServiceFactory::GetForProfile(
-      Profile::FromBrowserContext(browser()->profile()))
-      ->SearchEnginePreconnectorForTesting()
-      ->OnAppStateChangedForTesting(false /* in_foreground */);
-
   static const char kShortName[] = "test";
   static const char kSearchURL[] =
       "/anchors_different_area.html?q={searchTerms}";
@@ -285,8 +280,8 @@ IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorKeepSocketBrowserTest,
   // Put the fake search URL to be preconnected in foreground.
   NavigationPredictorKeyedServiceFactory::GetForProfile(
       Profile::FromBrowserContext(browser()->profile()))
-      ->SearchEnginePreconnectorForTesting()
-      ->OnAppStateChangedForTesting(true /* in_foreground */);
+      ->search_engine_preconnector()
+      ->StartPreconnecting(/*with_startup_delay=*/false);
 
   WaitForPreresolveCountForURL(GetTestURL(kSearchURL), 1);
 
@@ -307,27 +302,29 @@ IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorKeepSocketBrowserTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorKeepSocketBrowserTest,
-                       SocketColdForNonSearch) {
-  // Verifies that a navigation to non search will not use a warm socket.
-  static const char kSearchURLWithQuery[] =
-      "/anchors_different_area.html?q=porgs";
-
-  ui_test_utils::NavigateToURL(browser(), GetTestURL(kSearchURLWithQuery));
-
-  auto ukm_recorder = std::make_unique<ukm::TestAutoSetUkmRecorder>();
+class SearchEnginePreconnectorDesktopAutoStartBrowserTest
+    : public SearchEnginePreconnectorBrowserTest {
+ public:
+  SearchEnginePreconnectorDesktopAutoStartBrowserTest() {
+    {
+      feature_list_.InitWithFeaturesAndParameters(
+          {{features::kPreconnectToSearch, {{"startup_delay_ms", "0"}}},
+           {net::features::kNetUnusedIdleSocketTimeout,
+            {{"unused_idle_socket_timeout_seconds", "0"}}}},
+          {});
+    }
+  }
 
-  ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
+  ~SearchEnginePreconnectorDesktopAutoStartBrowserTest() override = default;
 
-  const auto& entries =
-      ukm_recorder->GetMergedEntriesByName(ukm::builders::PageLoad::kEntryName);
-  EXPECT_EQ(1u, entries.size());
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SearchEnginePreconnectorDesktopAutoStartBrowserTest);
+};
 
-  for (const auto& kv : entries) {
-    EXPECT_TRUE(ukm_recorder->EntryHasMetric(
-        kv.second.get(),
-        ukm::builders::PageLoad::kMainFrameResource_SocketReusedName));
-  }
+IN_PROC_BROWSER_TEST_F(SearchEnginePreconnectorDesktopAutoStartBrowserTest,
+                       AutoStartDesktop) {
+  // Verifies that the default search is preconnected.
+  WaitForPreresolveCountForURL(GoogleSearch(), 2);
 }
 
 }  // namespace