Send ResponseInfo to callback handlers of network responses.

This also removes the redundant Service::ResponseCallback alias.

Unfortunately, this change ripples far and wide. I considered trying to
keep this localized to service_request_sender, but that would have
other, even less desirable side effects, such as having to indicate
which instances should record metrics and which should not. Also, it
would force us to make certain assumptions about the lifetime of that
class that we have not (and should not) make, such as whether a flow
may use the same instance throughout or not.

To avoid the same kind of ripple in the future, this CL introduces a
ResponseInfo struct that is passed along all network responses. For
now, it only contains what we need for the follow-up metrics, but
adding more fields to the struct should be trivial in the future.

Bug: b/201970915
Change-Id: I086bc75c712201978a31f8651707053cc88fba96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3540721
Reviewed-by: Florian Gauger <fga@google.com>
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Cr-Commit-Position: refs/heads/main@{#987891}

components/autofill_assistant/browser/autofill_assistant_impl.cc

@@ -28,7 +28,8 @@ const char kIntentScriptParameterKey[] = "INTENT";
 void OnCapabilitiesResponse(
     AutofillAssistant::GetCapabilitiesResponseCallback callback,
     int http_status,
-    const std::string& response_str) {
+    const std::string& response_str,
+    const ServiceRequestSender::ResponseInfo& response_info) {
   std::vector<AutofillAssistant::CapabilitiesInfo> infos;
   GetCapabilitiesByHashPrefixResponseProto resp;
 

components/autofill_assistant/browser/autofill_assistant_impl_unittest.cc

@@ -56,7 +56,8 @@ TEST_F(AutofillAssistantImpTest, GetCapabilitiesByHashPrefixEmptyRespose) {
   EXPECT_CALL(*mock_request_sender_,
               OnSendRequest(GURL(kScriptServerUrl), _, _,
                             RpcType::GET_CAPABILITIES_BY_HASH_PREFIX))
-      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, ""));
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, "",
+                                   ServiceRequestSender::ResponseInfo{}));
 
   EXPECT_CALL(
       mock_response_callback_,
@@ -70,7 +71,8 @@ TEST_F(AutofillAssistantImpTest, BackendRequestFailed) {
   EXPECT_CALL(*mock_request_sender_,
               OnSendRequest(GURL(kScriptServerUrl), _, _,
                             RpcType::GET_CAPABILITIES_BY_HASH_PREFIX))
-      .WillOnce(RunOnceCallback<2>(net::HTTP_FORBIDDEN, ""));
+      .WillOnce(RunOnceCallback<2>(net::HTTP_FORBIDDEN, "",
+                                   ServiceRequestSender::ResponseInfo{}));
 
   EXPECT_CALL(mock_response_callback_,
               Run(net::HTTP_FORBIDDEN,
@@ -84,7 +86,8 @@ TEST_F(AutofillAssistantImpTest, ParsingError) {
   EXPECT_CALL(*mock_request_sender_,
               OnSendRequest(GURL(kScriptServerUrl), _, _,
                             RpcType::GET_CAPABILITIES_BY_HASH_PREFIX))
-      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, "invalid"));
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, "invalid",
+                                   ServiceRequestSender::ResponseInfo{}));
 
   EXPECT_CALL(
       mock_response_callback_,
@@ -114,7 +117,8 @@ TEST_F(AutofillAssistantImpTest, GetCapabilitiesByHashPrefix) {
   EXPECT_CALL(*mock_request_sender_,
               OnSendRequest(GURL(kScriptServerUrl), _, _,
                             RpcType::GET_CAPABILITIES_BY_HASH_PREFIX))
-      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, serialized_proto));
+      .WillOnce(RunOnceCallback<2>(net::HTTP_OK, serialized_proto,
+                                   ServiceRequestSender::ResponseInfo{}));
 
   EXPECT_CALL(
       mock_response_callback_,

components/autofill_assistant/browser/controller.cc

@@ -465,9 +465,11 @@ void Controller::OnPeriodicScriptCheck() {
       settings_.periodic_script_check_interval);
 }
 
-void Controller::OnGetScripts(const GURL& url,
-                              int http_status,
-                              const std::string& response) {
+void Controller::OnGetScripts(
+    const GURL& url,
+    int http_status,
+    const std::string& response,
+    const ServiceRequestSender::ResponseInfo& response_info) {
   if (state_ == AutofillAssistantState::STOPPED)
     return;
 

components/autofill_assistant/browser/controller.h

@@ -200,7 +200,8 @@ class Controller : public ScriptExecutorDelegate,
 
   void OnGetScripts(const GURL& url,
                     int http_status,
-                    const std::string& response);
+                    const std::string& response,
+                    const ServiceRequestSender::ResponseInfo& response_info);
 
   // Execute |script_path| and, if execution succeeds, enter |end_state| and
   // call |on_success|.

components/autofill_assistant/browser/script_executor.cc

@@ -811,9 +811,11 @@ base::WeakPtr<ActionDelegate> ScriptExecutor::GetWeakPtr() const {
   return weak_ptr_factory_.GetWeakPtr();
 }
 
-void ScriptExecutor::OnGetActions(base::TimeTicks start_time,
-                                  int http_status,
-                                  const std::string& response) {
+void ScriptExecutor::OnGetActions(
+    base::TimeTicks start_time,
+    int http_status,
+    const std::string& response,
+    const ServiceRequestSender::ResponseInfo& response_info) {
   VLOG(2) << __func__ << " http-status=" << http_status;
   batch_start_time_ = base::TimeTicks::Now();
   const base::TimeDelta& roundtrip_duration = batch_start_time_ - start_time;
@@ -1082,7 +1084,8 @@ void ScriptExecutor::RequestUserData(
 void ScriptExecutor::OnRequestUserData(
     base::OnceCallback<void(bool, const GetUserDataResponseProto&)> callback,
     int http_status,
-    const std::string& response) {
+    const std::string& response,
+    const ServiceRequestSender::ResponseInfo& response_info) {
   if (http_status != net::HTTP_OK) {
     std::move(callback).Run(false, GetUserDataResponseProto());
     return;

components/autofill_assistant/browser/script_executor.h

@@ -28,6 +28,7 @@
 #include "components/autofill_assistant/browser/script_executor_delegate.h"
 #include "components/autofill_assistant/browser/script_executor_ui_delegate.h"
 #include "components/autofill_assistant/browser/service.pb.h"
+#include "components/autofill_assistant/browser/service/service_request_sender.h"
 #include "components/autofill_assistant/browser/top_padding.h"
 #include "components/autofill_assistant/browser/user_data.h"
 #include "components/autofill_assistant/browser/wait_for_dom_observer.h"
@@ -270,7 +271,8 @@ class ScriptExecutor : public ActionDelegate,
 
   void OnGetActions(base::TimeTicks start_time,
                     int http_status,
-                    const std::string& response);
+                    const std::string& response,
+                    const ServiceRequestSender::ResponseInfo& response_info);
   bool ProcessNextActionResponse(const std::string& response);
   void ReportPayloadsToListener();
   void ReportScriptsUpdateToListener(
@@ -329,7 +331,8 @@ class ScriptExecutor : public ActionDelegate,
   void OnRequestUserData(
       base::OnceCallback<void(bool, const GetUserDataResponseProto&)> callback,
       int http_status,
-      const std::string& response);
+      const std::string& response,
+      const ServiceRequestSender::ResponseInfo& response_info);
 
   // Maybe shows the message specified in a callout, depending on the current
   // state and client settings.