[Autofill Assistant] Fail unknown actions instead of ignoring them.

Before this patch, unknown action would be skipped and not reported to
the server. This made it difficult to see what actions were unsupported.
Also, this made the client mistakenly think that response with only
unsupported action signalled the end of a script.

This patch introduces an unknown action type, which always fail. This
way, neither the client nor the developer will be confused by this case.

This patch also transition UpdateProcessedAction from taking a boolean
to taking a ProcessActionStatusProto, which lets us be more specific in
what failed. I also left TODOs for the cases where we should be more
specific and distinguish at least "element not found" from other errors.

Bug: 806868
Change-Id: I6efe1f2ac733f2048a5028c5ebb51fab2953868c
Reviewed-on: https://chromium-review.googlesource.com/c/1257901
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596653}

components/autofill_assistant/browser/BUILD.gn

@@ -32,6 +32,8 @@ jumbo_static_library("browser") {
     "actions/stop_action.h",
     "actions/tell_action.cc",
     "actions/tell_action.h",
+    "actions/unsupported_action.cc",
+    "actions/unsupported_action.h",
     "actions/upload_dom_action.cc",
     "actions/upload_dom_action.h",
     "actions/wait_for_dom_action.cc",

components/autofill_assistant/browser/actions/action.cc

@@ -10,12 +10,10 @@ Action::Action(const ActionProto& proto) : proto_(proto) {}
 
 Action::~Action() {}
 
-void Action::UpdateProcessedAction(bool status) {
+void Action::UpdateProcessedAction(ProcessedActionStatusProto status) {
   // Safety check in case process action is run twice.
   *processed_action_proto_->mutable_action() = proto_;
-  processed_action_proto_->set_status(
-      status ? ProcessedActionStatusProto::ACTION_APPLIED
-             : ProcessedActionStatusProto::OTHER_ACTION_STATUS);
+  processed_action_proto_->set_status(status);
 }
 
 }  // namespace autofill_assistant

components/autofill_assistant/browser/actions/action.h

@@ -31,7 +31,7 @@ class Action {
 
  protected:
   explicit Action(const ActionProto& proto);
-  void UpdateProcessedAction(bool status);
+  void UpdateProcessedAction(ProcessedActionStatusProto status);
 
   const ActionProto proto_;
 

components/autofill_assistant/browser/actions/autofill_action.cc

@@ -164,7 +164,7 @@ void AutofillAction::ProcessAction(ActionDelegate* delegate,
 }
 
 void AutofillAction::EndAction(bool successful) {
-  UpdateProcessedAction(successful);
+  UpdateProcessedAction(successful ? ACTION_APPLIED : OTHER_ACTION_STATUS);
   std::move(process_action_callback_).Run(std::move(processed_action_proto_));
 }
 
@@ -223,7 +223,7 @@ void AutofillAction::OnGetFullCard(ActionDelegate* delegate,
   if (!card) {
     // TODO(crbug.com/806868): The failure might because of cancel, then ask to
     // choose a card again.
-    UpdateProcessedAction(false);
+    UpdateProcessedAction(OTHER_ACTION_STATUS);
     std::move(process_action_callback_).Run(std::move(processed_action_proto_));
     return;
   }

components/autofill_assistant/browser/actions/click_action.cc

@@ -34,7 +34,9 @@ void ClickAction::ProcessAction(ActionDelegate* delegate,
 }
 
 void ClickAction::OnClick(ProcessActionCallback callback, bool status) {
-  UpdateProcessedAction(status);
+  // TODO(crbug.com/806868): Distinguish element not found from other error and
+  // report them as ELEMENT_RESOLUTION_FAILED.
+  UpdateProcessedAction(status ? ACTION_APPLIED : OTHER_ACTION_STATUS);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/focus_element_action.cc

@@ -39,7 +39,9 @@ void FocusElementAction::ProcessAction(ActionDelegate* delegate,
 
 void FocusElementAction::OnFocusElement(ProcessActionCallback callback,
                                         bool status) {
-  UpdateProcessedAction(status);
+  // TODO(crbug.com/806868): Distinguish element not found from other error and
+  // report them as ELEMENT_RESOLUTION_FAILED.
+  UpdateProcessedAction(status ? ACTION_APPLIED : OTHER_ACTION_STATUS);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/navigate_action.cc

@@ -24,7 +24,7 @@ void NavigateAction::ProcessAction(ActionDelegate* delegate,
   GURL url(proto_.navigate().url());
   delegate->LoadURL(url);
   processed_action_proto_ = std::make_unique<ProcessedActionProto>();
-  UpdateProcessedAction(/* status= */ true);
+  UpdateProcessedAction(ACTION_APPLIED);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/reset_action.cc

@@ -22,7 +22,7 @@ void ResetAction::ProcessAction(ActionDelegate* delegate,
                                 ProcessActionCallback callback) {
   delegate->Restart();
   processed_action_proto_ = std::make_unique<ProcessedActionProto>();
-  UpdateProcessedAction(true);
+  UpdateProcessedAction(ACTION_APPLIED);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/select_option_action.cc

@@ -40,7 +40,9 @@ void SelectOptionAction::ProcessAction(ActionDelegate* delegate,
 
 void SelectOptionAction::OnSelectOption(ProcessActionCallback callback,
                                         bool status) {
-  UpdateProcessedAction(status);
+  // TODO(crbug.com/806868): Distinguish element not found from other error and
+  // report them as ELEMENT_RESOLUTION_FAILED.
+  UpdateProcessedAction(status ? ACTION_APPLIED : OTHER_ACTION_STATUS);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/stop_action.cc

@@ -22,7 +22,7 @@ void StopAction::ProcessAction(ActionDelegate* delegate,
                                ProcessActionCallback callback) {
   delegate->Shutdown();
   processed_action_proto_ = std::make_unique<ProcessedActionProto>();
-  UpdateProcessedAction(true);
+  UpdateProcessedAction(ACTION_APPLIED);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/tell_action.cc

@@ -22,7 +22,7 @@ void TellAction::ProcessAction(ActionDelegate* delegate,
   processed_action_proto_ = std::make_unique<ProcessedActionProto>();
   // tell.message in the proto is localized.
   delegate->ShowStatusMessage(proto_.tell().message());
-  UpdateProcessedAction(true);
+  UpdateProcessedAction(ACTION_APPLIED);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/unsupported_action.cc

+// Copyright 2018 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/actions/unsupported_action.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/callback.h"
+
+namespace autofill_assistant {
+
+UnsupportedAction::UnsupportedAction(const ActionProto& proto)
+    : Action(proto) {}
+
+UnsupportedAction::~UnsupportedAction() {}
+
+void UnsupportedAction::ProcessAction(ActionDelegate* delegate,
+                                      ProcessActionCallback callback) {
+  processed_action_proto_ = std::make_unique<ProcessedActionProto>();
+  // TODO(crbug.com/806868): Add 'unsupported action' status to the protocol.
+  UpdateProcessedAction(UNKNOWN_ACTION_STATUS);
+  std::move(callback).Run(std::move(processed_action_proto_));
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/actions/unsupported_action.h

+// Copyright 2018 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_ACTIONS_UNSUPPORTED_ACTION_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_UNSUPPORTED_ACTION_H_
+
+#include "base/macros.h"
+#include "components/autofill_assistant/browser/actions/action.h"
+
+namespace autofill_assistant {
+// An unsupported action that always fails.
+class UnsupportedAction : public Action {
+ public:
+  explicit UnsupportedAction(const ActionProto& proto);
+  ~UnsupportedAction() override;
+
+  // Overrides Action:
+  void ProcessAction(ActionDelegate* delegate,
+                     ProcessActionCallback callback) override;
+
+ private:
+  void OnUnsupported(ProcessActionCallback callback, bool status);
+
+  DISALLOW_COPY_AND_ASSIGN(UnsupportedAction);
+};
+
+}  // namespace autofill_assistant
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_ACTIONS_UNSUPPORTED_ACTION_H_

components/autofill_assistant/browser/actions/upload_dom_action.cc

@@ -38,7 +38,9 @@ void UploadDomAction::ProcessAction(ActionDelegate* delegate,
 
 void UploadDomAction::OnBuildNodeTree(ProcessActionCallback callback,
                                       bool status) {
-  UpdateProcessedAction(status);
+  // TODO(crbug.com/806868): Distinguish element not found from other error and
+  // report them as ELEMENT_RESOLUTION_FAILED.
+  UpdateProcessedAction(status ? ACTION_APPLIED : OTHER_ACTION_STATUS);
   std::move(callback).Run(std::move(processed_action_proto_));
 }
 

components/autofill_assistant/browser/actions/wait_for_dom_action.cc

@@ -34,7 +34,7 @@ void WaitForDomAction::ProcessAction(ActionDelegate* delegate,
 
   // Fail the action if selectors is empty.
   if (proto_.wait_for_dom().selectors().empty()) {
-    UpdateProcessedAction(false);
+    UpdateProcessedAction(OTHER_ACTION_STATUS);
     DLOG(ERROR) << "Empty selector, failing action.";
     std::move(callback).Run(std::move(processed_action_proto_));
     return;
@@ -68,13 +68,13 @@ void WaitForDomAction::OnCheckElementExists(ActionDelegate* delegate,
                                             ProcessActionCallback callback,
                                             bool result) {
   if (result) {
-    UpdateProcessedAction(true);
+    UpdateProcessedAction(ACTION_APPLIED);
     std::move(callback).Run(std::move(processed_action_proto_));
     return;
   }
 
   if (rounds == 0) {
-    UpdateProcessedAction(false);
+    UpdateProcessedAction(ELEMENT_RESOLUTION_FAILED);
     std::move(callback).Run(std::move(processed_action_proto_));
     return;
   }

components/autofill_assistant/browser/protocol_utils.cc

@@ -15,6 +15,7 @@
 #include "components/autofill_assistant/browser/actions/select_option_action.h"
 #include "components/autofill_assistant/browser/actions/stop_action.h"
 #include "components/autofill_assistant/browser/actions/tell_action.h"
+#include "components/autofill_assistant/browser/actions/unsupported_action.h"
 #include "components/autofill_assistant/browser/actions/upload_dom_action.h"
 #include "components/autofill_assistant/browser/actions/wait_for_dom_action.h"
 #include "components/autofill_assistant/browser/service.pb.h"
@@ -193,6 +194,7 @@ bool ProtocolUtils::ParseActions(
       case ActionProto::ActionInfoCase::ACTION_INFO_NOT_SET: {
         DLOG(ERROR) << "Unknown or unsupported action with action_case="
                     << action.action_info_case();
+        actions->emplace_back(std::make_unique<UnsupportedAction>(action));
         break;
       }
     }

components/autofill_assistant/browser/script_executor_unittest.cc

@@ -162,6 +162,26 @@ TEST_F(ScriptExecutorTest, RunMultipleActions) {
   EXPECT_EQ(1u, processed_actions2_capture.size());
 }
 
+TEST_F(ScriptExecutorTest, UnsupportedAction) {
+  ActionsResponseProto actions_response;
+  actions_response.set_server_payload("payload");
+  actions_response.add_actions();  // action definition missing
+
+  EXPECT_CALL(mock_service_, OnGetActions(_, _, _))
+      .WillOnce(RunOnceCallback<2>(true, Serialize(actions_response)));
+
+  std::vector<ProcessedActionProto> processed_actions_capture;
+  EXPECT_CALL(mock_service_, OnGetNextActions(_, _, _))
+      .WillOnce(DoAll(SaveArg<1>(&processed_actions_capture),
+                      RunOnceCallback<2>(true, "")));
+  EXPECT_CALL(executor_callback_,
+              Run(Field(&ScriptExecutor::Result::success, true)));
+  executor_->Run(executor_callback_.Get());
+
+  ASSERT_EQ(1u, processed_actions_capture.size());
+  EXPECT_EQ(UNKNOWN_ACTION_STATUS, processed_actions_capture[0].status());
+}
+
 TEST_F(ScriptExecutorTest, StopAfterEnd) {
   ActionsResponseProto actions_response;
   actions_response.set_server_payload("payload");