[Autofill Assistant] Removed dep from lite_service on service_impl

This makes it easier to control the amount of data we send in the RPC.
It's also cleaner this way, and furthermore, this is in preparation for
a new RPC, which lite_service will communicate with, but service_impl
will not.

Bug: b/171178550
Change-Id: Idbab2402ede4f4473575baf4254ed0cc13d0c7e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485254
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Auto-Submit: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Reviewed-by: Marian Fechete <marianfe@google.com>
Cr-Commit-Position: refs/heads/master@{#818965}

chrome/browser/android/autofill_assistant/lite_service_bridge.cc

@@ -11,7 +11,6 @@
 #include "components/autofill_assistant/browser/service/api_key_fetcher.h"
 #include "components/autofill_assistant/browser/service/lite_service.h"
 #include "components/autofill_assistant/browser/service/server_url_fetcher.h"
-#include "components/autofill_assistant/browser/service/service_impl.h"
 #include "content/public/browser/web_contents.h"
 
 namespace autofill_assistant {
@@ -48,17 +47,15 @@ jlong JNI_AutofillAssistantLiteService_CreateLiteService(
   }
 
   ServerUrlFetcher url_fetcher{ServerUrlFetcher::GetDefaultServerUrl()};
-  auto request_sender = std::make_unique<ServiceRequestSender>(
-      web_contents->GetBrowserContext(), /* access_token_fetcher = */ nullptr,
+  return reinterpret_cast<jlong>(new LiteService(
+      std::make_unique<ServiceRequestSender>(
+          web_contents->GetBrowserContext(),
+          /* access_token_fetcher = */ nullptr,
           std::make_unique<NativeURLLoaderFactory>(),
           ApiKeyFetcher().GetAPIKey(chrome::GetChannel()),
-      /* auth_enabled = */ false, /* disable_auth_if_no_access_token = */ true);
-
-  return reinterpret_cast<jlong>(new LiteService(
-      std::make_unique<ServiceImpl>(std::move(request_sender),
-                                    url_fetcher.GetSupportsScriptEndpoint(),
+          /* auth_enabled = */ false,
+          /* disable_auth_if_no_access_token = */ true),
       url_fetcher.GetNextActionsEndpoint(),
-                                    std::make_unique<EmptyClientContext>()),
       base::android::ConvertJavaStringToUTF8(env, jtrigger_script_path),
       base::BindOnce(&OnFinished, base::android::ScopedJavaGlobalRef<jobject>(
                                       java_lite_service)),

components/autofill_assistant/browser/service/lite_service.cc

@@ -12,17 +12,20 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "components/autofill_assistant/browser/protocol_utils.h"
 #include "net/http/http_status_code.h"
 
 namespace autofill_assistant {
 
 LiteService::LiteService(
-    std::unique_ptr<Service> service_impl,
+    std::unique_ptr<ServiceRequestSender> request_sender,
+    const GURL& get_actions_server_url,
     const std::string& trigger_script_path,
     base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
         notify_finished_callback,
     base::RepeatingCallback<void(bool)> notify_script_running_callback)
-    : service_impl_(std::move(service_impl)),
+    : request_sender_(std::move(request_sender)),
+      get_actions_server_url_(get_actions_server_url),
       trigger_script_path_(trigger_script_path),
       notify_finished_callback_(std::move(notify_finished_callback)),
       notify_script_running_callback_(
@@ -70,10 +73,12 @@ void LiteService::GetActions(const std::string& script_path,
 
   // Note: trigger context and payloads should not be sent to the backend for
   // privacy reasons.
-  service_impl_->GetActions(
-      trigger_script_path_, GURL(), TriggerContextImpl(),
-      /* global_payload = */ std::string(),
-      /* script_payload = */ std::string(),
+  request_sender_->SendRequest(
+      get_actions_server_url_,
+      ProtocolUtils::CreateInitialScriptActionsRequest(
+          trigger_script_path_, GURL(), /* global_payload = */ std::string(),
+          /* script_payload = */ std::string(), EmptyClientContext().AsProto(),
+          /* script_parameters = */ {}),
       base::BindOnce(&LiteService::OnGetActions, weak_ptr_factory_.GetWeakPtr(),
                      std::move(callback)));
 }

components/autofill_assistant/browser/service/lite_service.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "components/autofill_assistant/browser/metrics.h"
 #include "components/autofill_assistant/browser/service/service_impl.h"
+#include "components/autofill_assistant/browser/service/service_request_sender.h"
 #include "components/version_info/version_info.h"
 #include "url/gurl.h"
 
@@ -25,7 +26,8 @@ namespace autofill_assistant {
 class LiteService : public Service {
  public:
   explicit LiteService(
-      std::unique_ptr<Service> service_impl,
+      std::unique_ptr<ServiceRequestSender> request_sender,
+      const GURL& get_actions_server_url,
       const std::string& trigger_script_path,
       base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
           notify_finished_callback,
@@ -78,8 +80,12 @@ class LiteService : public Service {
   void StopWithoutErrorMessage(ResponseCallback callback,
                                Metrics::LiteScriptFinishedState state);
 
-  // The actual service that communicates with the backend.
-  std::unique_ptr<Service> service_impl_;
+  // The request sender that communicates with the backend.
+  std::unique_ptr<ServiceRequestSender> request_sender_;
+
+  // The GURL of the GetActions/GetNextActions rpc handler.
+  GURL get_actions_server_url_;
+
   // The script path to fetch actions from.
   std::string trigger_script_path_;
 

components/autofill_assistant/browser/service/lite_service_unittest.cc

@@ -5,6 +5,7 @@
 #include "components/autofill_assistant/browser/service/lite_service.h"
 #include "components/autofill_assistant/browser/service.pb.h"
 #include "components/autofill_assistant/browser/service/mock_service.h"
+#include "components/autofill_assistant/browser/service/mock_service_request_sender.h"
 #include "components/autofill_assistant/browser/service/service.h"
 #include "components/autofill_assistant/browser/test_util.h"
 #include "components/autofill_assistant/browser/trigger_context.h"
@@ -27,31 +28,31 @@ namespace autofill_assistant {
 namespace {
 const char kFakeScriptPath[] = "localhost/chrome/test";
 const char kFakeUrl[] = "https://www.example.com";
+const char kGetActionsServerUrl[] =
+    "https://www.fake.backend.com/action_server";
 }  // namespace
 
 using ::testing::_;
 using ::testing::AtMost;
 using ::testing::Eq;
+using ::testing::NiceMock;
 
 class LiteServiceTest : public testing::Test {
  protected:
   LiteServiceTest() {
-    auto service_impl = std::make_unique<MockService>();
-    mock_native_service_ = service_impl.get();
+    auto mock_request_sender =
+        std::make_unique<NiceMock<MockServiceRequestSender>>();
+    mock_request_sender_ = mock_request_sender.get();
 
     lite_service_ = std::make_unique<LiteService>(
-        std::move(service_impl), kFakeScriptPath, mock_finished_callback_.Get(),
+        std::move(mock_request_sender), GURL(kGetActionsServerUrl),
+        kFakeScriptPath, mock_finished_callback_.Get(),
         mock_script_running_callback_.Get());
-    EXPECT_CALL(*mock_native_service_, OnGetScriptsForUrl).Times(0);
-    EXPECT_CALL(*mock_native_service_, OnGetNextActions).Times(0);
 
-    EXPECT_CALL(*mock_native_service_,
-                OnGetActions(kFakeScriptPath, _, _, _, _, _))
+    EXPECT_CALL(*mock_request_sender_,
+                OnSendRequest(GURL(kGetActionsServerUrl), _, _))
         .Times(AtMost(1))
-        .WillOnce([&](const std::string& script_path, const GURL& url,
-                      const TriggerContext& trigger_context,
-                      const std::string& global_payload,
-                      const std::string& script_payload,
+        .WillOnce([&](const GURL& url, const std::string& request_body,
                       Service::ResponseCallback& callback) {
           std::string serialized_response;
           get_actions_response_.SerializeToString(&serialized_response);
@@ -78,13 +79,13 @@ class LiteServiceTest : public testing::Test {
     EXPECT_CALL(mock_finished_callback_, Run(state));
   }
 
+  NiceMock<MockServiceRequestSender>* mock_request_sender_;
   base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
       mock_finished_callback_;
   base::MockCallback<base::RepeatingCallback<void(bool)>>
       mock_script_running_callback_;
   base::MockCallback<base::OnceCallback<void(int, const std::string&)>>
       mock_response_callback_;
-  MockService* mock_native_service_ = nullptr;
   std::unique_ptr<LiteService> lite_service_;
   ActionsResponseProto get_actions_response_;
 };
@@ -98,9 +99,10 @@ TEST_F(LiteServiceTest, RunsNotificationOnDelete) {
       notification_callback,
       Run(Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED));
   {
-    LiteService lite_service(std::make_unique<MockService>(), kFakeScriptPath,
-                             notification_callback.Get(),
-                             script_running_callback.Get());
+    LiteService lite_service(
+        std::make_unique<NiceMock<MockServiceRequestSender>>(),
+        GURL(kGetActionsServerUrl), kFakeScriptPath,
+        notification_callback.Get(), script_running_callback.Get());
   }
 }
 
@@ -125,20 +127,20 @@ TEST_F(LiteServiceTest, GetActionsOnlySendsScriptPath) {
   TriggerContextImpl non_empty_context;
   non_empty_context.SetCallerAccountHash("something");
 
-  EXPECT_CALL(*mock_native_service_,
-              OnGetActions(kFakeScriptPath, _, _, _, _, _))
+  EXPECT_CALL(*mock_request_sender_,
+              OnSendRequest(GURL(kGetActionsServerUrl), _, _))
       .Times(1)
-      .WillOnce([&](const std::string& script_path, const GURL& url,
-                    const TriggerContext& trigger_context,
-                    const std::string& global_payload,
-                    const std::string& script_payload,
+      .WillOnce([&](const GURL& url, const std::string& request_body,
                     Service::ResponseCallback& callback) {
-        EXPECT_THAT(script_path, Eq(kFakeScriptPath));
+        ScriptActionRequestProto rpc_proto;
+        ASSERT_TRUE(rpc_proto.ParseFromString(request_body));
+
+        EXPECT_THAT(rpc_proto.initial_request().query().script_path(0),
+                    Eq(kFakeScriptPath));
         // All other information has been cleared.
-        EXPECT_THAT(url, Eq(GURL()));
-        EXPECT_THAT(trigger_context.get_caller_account_hash(), Eq(""));
-        EXPECT_THAT(global_payload, Eq(""));
-        EXPECT_THAT(script_payload, Eq(""));
+        EXPECT_THAT(rpc_proto.global_payload(), Eq(""));
+        EXPECT_THAT(rpc_proto.script_payload(), Eq(""));
+        EXPECT_THAT(rpc_proto.client_context(), Eq(ClientContextProto()));
       });
 
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl), non_empty_context,
@@ -157,13 +159,10 @@ TEST_F(LiteServiceTest, GetActionsOnlyFetchesFromScriptPath) {
 TEST_F(LiteServiceTest, StopsOnGetActionsFailed) {
   ExpectStopWithFinishedState(
       Metrics::LiteScriptFinishedState::LITE_SCRIPT_GET_ACTIONS_FAILED);
-  EXPECT_CALL(*mock_native_service_,
-              OnGetActions(kFakeScriptPath, _, _, _, _, _))
+  EXPECT_CALL(*mock_request_sender_,
+              OnSendRequest(GURL(kGetActionsServerUrl), _, _))
       .Times(1)
-      .WillOnce([&](const std::string& script_path, const GURL& url,
-                    const TriggerContext& trigger_context,
-                    const std::string& global_payload,
-                    const std::string& script_payload,
+      .WillOnce([&](const GURL& url, const std::string& request_body,
                     Service::ResponseCallback& callback) {
         std::move(callback).Run(net::HTTP_UNAUTHORIZED, std::string());
       });
@@ -177,13 +176,10 @@ TEST_F(LiteServiceTest, StopsOnGetActionsFailed) {
 TEST_F(LiteServiceTest, StopsOnGetActionsParsingError) {
   ExpectStopWithFinishedState(
       Metrics::LiteScriptFinishedState::LITE_SCRIPT_GET_ACTIONS_PARSE_ERROR);
-  EXPECT_CALL(*mock_native_service_,
-              OnGetActions(kFakeScriptPath, _, _, _, _, _))
+  EXPECT_CALL(*mock_request_sender_,
+              OnSendRequest(GURL(kGetActionsServerUrl), _, _))
       .Times(1)
-      .WillOnce([&](const std::string& script_path, const GURL& url,
-                    const TriggerContext& trigger_context,
-                    const std::string& global_payload,
-                    const std::string& script_payload,
+      .WillOnce([&](const GURL& url, const std::string& request_body,
                     Service::ResponseCallback& callback) {
         std::move(callback).Run(net::HTTP_OK, std::string("invalid proto"));
       });