[Autofill Assistant] Refactor dependencies of service.

This CL starts the effort to move dependencies of the service into
dedicated classes in order to facilitate unit testing and code reuse.

This CL is a refactoring only and should have no user-facing changes.
I plan to add dedicated unit tests in a follow-up.

Bug: b/158998456
Change-Id: Icd468ff98d9e1f5a028282b0e1eb42364aa46d16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2448496
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#814179}

chrome/browser/android/autofill_assistant/lite_service_bridge.cc

@@ -7,8 +7,10 @@
 #include "base/logging.h"
 #include "chrome/android/features/autofill_assistant/jni_headers/AutofillAssistantLiteService_jni.h"
 #include "chrome/common/channel_info.h"
+#include "components/autofill_assistant/browser/api_key_fetcher.h"
 #include "components/autofill_assistant/browser/lite_service.h"
 #include "components/autofill_assistant/browser/metrics.h"
+#include "components/autofill_assistant/browser/server_url_fetcher.h"
 #include "components/autofill_assistant/browser/service_impl.h"
 #include "content/public/browser/web_contents.h"
 
@@ -45,12 +47,16 @@ jlong JNI_AutofillAssistantLiteService_CreateLiteService(
     return 0;
   }
 
+  ServerUrlFetcher url_fetcher{ServerUrlFetcher::GetDefaultServerUrl()};
   return reinterpret_cast<jlong>(new LiteService(
-      std::make_unique<ServiceImpl>(web_contents->GetBrowserContext(),
-                                    chrome::GetChannel(),
-                                    std::make_unique<EmptyClientContext>(),
-                                    /* access_token_fetcher = */ nullptr,
-                                    /* auth_enabled = */ false),
+      std::make_unique<ServiceImpl>(
+          ApiKeyFetcher().GetAPIKey(chrome::GetChannel()),
+          url_fetcher.GetSupportsScriptEndpoint(),
+          url_fetcher.GetNextActionsEndpoint(),
+          web_contents->GetBrowserContext(),
+          std::make_unique<EmptyClientContext>(),
+          /* access_token_fetcher = */ nullptr,
+          /* auth_enabled = */ false),
       base::android::ConvertJavaStringToUTF8(env, jtrigger_script_path),
       base::BindOnce(&OnFinished, base::android::ScopedJavaGlobalRef<jobject>(
                                       java_lite_service)),

components/autofill_assistant/browser/BUILD.gn

@@ -89,6 +89,8 @@ static_library("browser") {
     "actions/wait_for_dom_action.h",
     "actions/wait_for_navigation_action.cc",
     "actions/wait_for_navigation_action.h",
+    "api_key_fetcher.cc",
+    "api_key_fetcher.h",
     "basic_interactions.cc",
     "basic_interactions.h",
     "batch_element_checker.cc",
@@ -157,6 +159,8 @@ static_library("browser") {
     "selector.h",
     "self_delete_full_card_requester.cc",
     "self_delete_full_card_requester.h",
+    "server_url_fetcher.cc",
+    "server_url_fetcher.h",
     "service.h",
     "service_impl.cc",
     "service_impl.h",

components/autofill_assistant/browser/api_key_fetcher.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 "components/autofill_assistant/browser/api_key_fetcher.h"
+
+#include "base/command_line.h"
+#include "components/autofill_assistant/browser/switches.h"
+#include "google_apis/google_api_keys.h"
+
+namespace autofill_assistant {
+
+std::string ApiKeyFetcher::GetAPIKey(version_info::Channel channel) {
+  const auto* command_line = base::CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switches::kAutofillAssistantServerKey)) {
+    return command_line->GetSwitchValueASCII(
+        switches::kAutofillAssistantServerKey);
+  }
+
+  if (google_apis::IsGoogleChromeAPIKeyUsed()) {
+    return channel == version_info::Channel::STABLE
+               ? google_apis::GetAPIKey()
+               : google_apis::GetNonStableAPIKey();
+  }
+  return "";
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/api_key_fetcher.h

+// 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.
+
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_API_KEY_FETCHER_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_API_KEY_FETCHER_H_
+
+#include <string>
+#include "components/version_info/version_info.h"
+
+namespace autofill_assistant {
+
+// Wrapper interface to allow mocking this in unit tests.
+struct ApiKeyFetcher {
+  virtual std::string GetAPIKey(version_info::Channel channel);
+};
+
+}  // namespace autofill_assistant
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_API_KEY_FETCHER_H_

components/autofill_assistant/browser/mock_service.cc

@@ -12,6 +12,7 @@ namespace autofill_assistant {
 
 MockService::MockService()
     : ServiceImpl(std::string("api_key"),
+                  GURL("http://fake"),
                   GURL("http://fake"),
                   nullptr,
                   nullptr,

components/autofill_assistant/browser/server_url_fetcher.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 "components/autofill_assistant/browser/server_url_fetcher.h"
+
+#include <string>
+
+#include "base/command_line.h"
+#include "components/autofill_assistant/browser/switches.h"
+#include "url/url_canon_stdstring.h"
+
+namespace {
+const char kDefaultAutofillAssistantServerUrl[] =
+    "https://automate-pa.googleapis.com";
+const char kScriptEndpoint[] = "/v1/supportsSite2";
+const char kActionEndpoint[] = "/v1/actions2";
+}  // namespace
+
+namespace autofill_assistant {
+
+ServerUrlFetcher::ServerUrlFetcher(const GURL& server_url)
+    : server_url_(server_url) {}
+ServerUrlFetcher::~ServerUrlFetcher() = default;
+
+// static
+GURL ServerUrlFetcher::GetDefaultServerUrl() {
+  std::string server_url =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kAutofillAssistantUrl);
+  if (server_url.empty()) {
+    return GURL(kDefaultAutofillAssistantServerUrl);
+  }
+  return GURL(server_url);
+}
+
+GURL ServerUrlFetcher::GetSupportsScriptEndpoint() {
+  url::StringPieceReplacements<std::string> script_replacements;
+  script_replacements.SetPathStr(kScriptEndpoint);
+  return server_url_.ReplaceComponents(script_replacements);
+}
+
+GURL ServerUrlFetcher::GetNextActionsEndpoint() {
+  url::StringPieceReplacements<std::string> action_replacements;
+  action_replacements.SetPathStr(kActionEndpoint);
+  return server_url_.ReplaceComponents(action_replacements);
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/server_url_fetcher.h

+// 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.
+
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_SERVER_URL_FETCHER_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_SERVER_URL_FETCHER_H_
+
+#include "url/gurl.h"
+
+namespace autofill_assistant {
+
+class ServerUrlFetcher {
+ public:
+  ServerUrlFetcher(const GURL& server_url);
+  virtual ~ServerUrlFetcher();
+
+  // Returns the default server url. This is either the hard-coded constant or,
+  // if applicable, the one provided via command-line argument.
+  static GURL GetDefaultServerUrl();
+
+  // Returns the endpoint to send the SupportsScript RPC to.
+  virtual GURL GetSupportsScriptEndpoint();
+  // Returns the endpoint to send the GetNextActions RPC to.
+  virtual GURL GetNextActionsEndpoint();
+
+ private:
+  GURL server_url_;
+};
+
+}  // namespace autofill_assistant
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_SERVER_URL_FETCHER_H_

components/autofill_assistant/browser/service_impl.cc

@@ -11,29 +11,22 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/strings/strcat.h"
-#include "base/strings/stringprintf.h"
+#include "components/autofill_assistant/browser/api_key_fetcher.h"
 #include "components/autofill_assistant/browser/client.h"
 #include "components/autofill_assistant/browser/protocol_utils.h"
+#include "components/autofill_assistant/browser/server_url_fetcher.h"
 #include "components/autofill_assistant/browser/switches.h"
 #include "components/autofill_assistant/browser/trigger_context.h"
-#include "components/version_info/version_info.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/storage_partition.h"
-#include "google_apis/google_api_keys.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "url/url_canon_stdstring.h"
 
 namespace autofill_assistant {
 namespace {
 
-const char* const kDefaultAutofillAssistantServerUrl =
-    "https://automate-pa.googleapis.com";
-const char* const kScriptEndpoint = "/v1/supportsSite2";
-const char* const kActionEndpoint = "/v1/actions2";
-
 net::NetworkTrafficAnnotationTag traffic_annotation =
     net::DefineNetworkTrafficAnnotation("autofill_service", R"(
         semantics {
@@ -53,42 +46,17 @@ net::NetworkTrafficAnnotationTag traffic_annotation =
           policy_exception_justification: "Not implemented."
         })");
 
-std::string GetAPIKey(version_info::Channel channel) {
-  const auto* command_line = base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kAutofillAssistantServerKey)) {
-    return command_line->GetSwitchValueASCII(
-        switches::kAutofillAssistantServerKey);
-  }
-
-  if (google_apis::IsGoogleChromeAPIKeyUsed()) {
-    return channel == version_info::Channel::STABLE
-               ? google_apis::GetAPIKey()
-               : google_apis::GetNonStableAPIKey();
-  }
-  return "";
-}
-
-std::string GetServerUrl() {
-  std::string server_url =
-      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-          switches::kAutofillAssistantUrl);
-  if (server_url.empty()) {
-    server_url = kDefaultAutofillAssistantServerUrl;
-  }
-  return server_url;
-}
-
 }  // namespace
 
 // static
 std::unique_ptr<ServiceImpl> ServiceImpl::Create(
     content::BrowserContext* context,
     Client* client) {
-  GURL server_url(GetServerUrl());
-  DCHECK(server_url.is_valid());
-
+  ServerUrlFetcher url_fetcher{ServerUrlFetcher::GetDefaultServerUrl()};
   return std::make_unique<ServiceImpl>(
-      GetAPIKey(client->GetChannel()), GURL(GetServerUrl()), context,
+      ApiKeyFetcher().GetAPIKey(client->GetChannel()),
+      url_fetcher.GetSupportsScriptEndpoint(),
+      url_fetcher.GetNextActionsEndpoint(), context,
       std::make_unique<ClientContextImpl>(client),
       client->GetAccessTokenFetcher(),
       /* auth_enabled = */ "false" !=
@@ -97,42 +65,23 @@ std::unique_ptr<ServiceImpl> ServiceImpl::Create(
 }
 
 ServiceImpl::ServiceImpl(const std::string& api_key,
-                         const GURL& server_url,
+                         const GURL& script_server_url,
+                         const GURL& action_server_url,
                          content::BrowserContext* context,
                          std::unique_ptr<ClientContext> client_context,
                          AccessTokenFetcher* access_token_fetcher,
                          bool auth_enabled)
     : context_(context),
+      script_server_url_(script_server_url),
+      script_action_server_url_(action_server_url),
       api_key_(api_key),
       client_context_(std::move(client_context)),
       access_token_fetcher_(access_token_fetcher),
-      fetching_token_(false),
-      auth_enabled_(auth_enabled),
-      weak_ptr_factory_(this) {
-  DCHECK(server_url.is_valid());
-
-  url::StringPieceReplacements<std::string> script_replacements;
-  script_replacements.SetPathStr(kScriptEndpoint);
-  script_server_url_ = server_url.ReplaceComponents(script_replacements);
-
-  url::StringPieceReplacements<std::string> action_replacements;
-  action_replacements.SetPathStr(kActionEndpoint);
-  script_action_server_url_ = server_url.ReplaceComponents(action_replacements);
-  VLOG(1) << "Using script domain " << script_action_server_url_.host();
+      auth_enabled_(auth_enabled) {
+  DCHECK(script_server_url.is_valid());
+  DCHECK(action_server_url.is_valid());
 }
 
-ServiceImpl::ServiceImpl(content::BrowserContext* context,
-                         version_info::Channel channel,
-                         std::unique_ptr<ClientContext> client_context,
-                         AccessTokenFetcher* access_token_fetcher,
-                         bool auth_enabled)
-    : ServiceImpl(GetAPIKey(channel),
-                  GURL(GetServerUrl()),
-                  context,
-                  std::move(client_context),
-                  access_token_fetcher,
-                  auth_enabled) {}
-
 ServiceImpl::~ServiceImpl() {}
 
 void ServiceImpl::GetScriptsForUrl(const GURL& url,

components/autofill_assistant/browser/service_impl.h

@@ -45,13 +45,9 @@ class ServiceImpl : public Service {
 
   // |context| and |access_token_fetcher| must remain valid for the lifetime of
   // the service instance.
-  ServiceImpl(content::BrowserContext* context,
-              version_info::Channel channel,
-              std::unique_ptr<ClientContext> client_context,
-              AccessTokenFetcher* access_token_fetcher,
-              bool auth_enabled);
   ServiceImpl(const std::string& api_key,
-              const GURL& server_url,
+              const GURL& script_server_url,
+              const GURL& action_server_url,
               content::BrowserContext* context,
               std::unique_ptr<ClientContext> client_context,
               AccessTokenFetcher* access_token_fetcher,
@@ -130,16 +126,16 @@ class ServiceImpl : public Service {
   AccessTokenFetcher* access_token_fetcher_;
 
   // True while waiting for a response from AccessTokenFetcher.
-  bool fetching_token_;
+  bool fetching_token_ = false;
 
   // Whether requests should be authenticated.
-  bool auth_enabled_;
+  bool auth_enabled_ = true;
 
   // An OAuth 2 token. Empty if not fetched yet or if the token has been
   // invalidated.
   std::string access_token_;
 
-  base::WeakPtrFactory<ServiceImpl> weak_ptr_factory_;
+  base::WeakPtrFactory<ServiceImpl> weak_ptr_factory_{this};
 
   FRIEND_TEST_ALL_PREFIXES(ServiceImplTestSignedInStatus, SetsSignedInStatus);