Commit a5406b62 authored by Mathias Carlen's avatar Mathias Carlen Committed by Commit Bot

[Autofill Assistant] Graceful shutdown for stop script.

Before this patch, we stopped autofill assistant with an error message
when the cvc step got cancelled. We are currently not able to
distinguish between a bad cvc and cancel.

This patch makes a first step towards a nicer flow where stop script
becomes a graceful shutdown with a status message.

Bug: 806868
Change-Id: I5d0e3f74d21f54df1276f994be2ae8c798fa2c0f
Reviewed-on: https://chromium-review.googlesource.com/c/1331394
Commit-Queue: Mathias Carlen <mcarlen@chromium.org>
Reviewed-by: default avatarStephane Zermatten <szermatt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607247}
parent 5421fc28
...@@ -142,8 +142,9 @@ class ActionDelegate { ...@@ -142,8 +142,9 @@ class ActionDelegate {
// state. // state.
virtual void Restart() = 0; virtual void Restart() = 0;
// Stop the current script and show |message| if it is not empty . // Stop the current script, shutdown autofill assistant and show |message| if
virtual void StopCurrentScript(const std::string& message) = 0; // it is not empty or a default message otherwise.
virtual void StopCurrentScriptAndShutdown(const std::string& message) = 0;
// Return the current ClientMemory. // Return the current ClientMemory.
virtual ClientMemory* GetClientMemory() = 0; virtual ClientMemory* GetClientMemory() = 0;
......
...@@ -77,6 +77,10 @@ class SelfDeleteFullCardRequester ...@@ -77,6 +77,10 @@ class SelfDeleteFullCardRequester
// payments::FullCardRequest::ResultDelegate: // payments::FullCardRequest::ResultDelegate:
void OnFullCardRequestFailed() override { void OnFullCardRequestFailed() override {
// Failed might because of cancel, so return nullptr to notice caller. // Failed might because of cancel, so return nullptr to notice caller.
//
// TODO(crbug.com/806868): Split the fail notification so that "cancel" and
// "wrong cvc" states can be handled differently. One should prompt a retry,
// the other a graceful shutdown - the current behavior.
std::move(callback_).Run(nullptr, base::string16()); std::move(callback_).Run(nullptr, base::string16());
delete this; delete this;
} }
...@@ -133,7 +137,7 @@ void AutofillAction::InternalProcessAction( ...@@ -133,7 +137,7 @@ void AutofillAction::InternalProcessAction(
delegate->GetClientMemory()->selected_address(name_)); delegate->GetClientMemory()->selected_address(name_));
if (!has_valid_data) { if (!has_valid_data) {
// User selected 'Fill manually'. // User selected 'Fill manually'.
delegate->StopCurrentScript(fill_form_message_); delegate->StopCurrentScriptAndShutdown(fill_form_message_);
EndAction(/* successful= */ true); EndAction(/* successful= */ true);
return; return;
} }
...@@ -199,7 +203,7 @@ void AutofillAction::OnDataSelected(ActionDelegate* delegate, ...@@ -199,7 +203,7 @@ void AutofillAction::OnDataSelected(ActionDelegate* delegate,
} }
if (guid.empty()) { if (guid.empty()) {
delegate->StopCurrentScript(fill_form_message_); delegate->StopCurrentScriptAndShutdown(fill_form_message_);
EndAction(/* successful= */ true); EndAction(/* successful= */ true);
return; return;
} }
...@@ -247,9 +251,10 @@ void AutofillAction::OnGetFullCard(ActionDelegate* delegate, ...@@ -247,9 +251,10 @@ void AutofillAction::OnGetFullCard(ActionDelegate* delegate,
std::unique_ptr<autofill::CreditCard> card, std::unique_ptr<autofill::CreditCard> card,
const base::string16& cvc) { const base::string16& cvc) {
if (!card) { if (!card) {
// TODO(crbug.com/806868): The failure might because of cancel, then ask to // Gracefully shutdown the script. The empty message forces the use of the
// choose a card again. // default message.
EndAction(false); delegate->StopCurrentScriptAndShutdown("");
EndAction(/* successful= */ true);
return; return;
} }
...@@ -333,7 +338,7 @@ void AutofillAction::OnCheckRequiredFieldsDone(ActionDelegate* delegate, ...@@ -333,7 +338,7 @@ void AutofillAction::OnCheckRequiredFieldsDone(ActionDelegate* delegate,
if (!allow_fallback) { if (!allow_fallback) {
// Validation failed and we don't want to try the fallback, so we stop // Validation failed and we don't want to try the fallback, so we stop
// the script. // the script.
delegate->StopCurrentScript(check_form_message_); delegate->StopCurrentScriptAndShutdown(check_form_message_);
EndAction(/* successful= */ true); EndAction(/* successful= */ true);
return; return;
} }
...@@ -353,7 +358,7 @@ void AutofillAction::OnCheckRequiredFieldsDone(ActionDelegate* delegate, ...@@ -353,7 +358,7 @@ void AutofillAction::OnCheckRequiredFieldsDone(ActionDelegate* delegate,
} }
} }
if (!has_fallbacks) { if (!has_fallbacks) {
delegate->StopCurrentScript(check_form_message_); delegate->StopCurrentScriptAndShutdown(check_form_message_);
EndAction(/* successful= */ true); EndAction(/* successful= */ true);
return; return;
} }
...@@ -410,7 +415,7 @@ void AutofillAction::OnSetFallbackFieldValue(ActionDelegate* delegate, ...@@ -410,7 +415,7 @@ void AutofillAction::OnSetFallbackFieldValue(ActionDelegate* delegate,
bool successful) { bool successful) {
if (!successful) { if (!successful) {
// Fallback failed: we stop the script without checking the fields. // Fallback failed: we stop the script without checking the fields.
delegate->StopCurrentScript(check_form_message_); delegate->StopCurrentScriptAndShutdown(check_form_message_);
EndAction(/* successful= */ true); EndAction(/* successful= */ true);
return; return;
} }
......
...@@ -167,7 +167,8 @@ class AutofillActionTest : public testing::Test { ...@@ -167,7 +167,8 @@ class AutofillActionTest : public testing::Test {
void ExpectActionToStopScript(const ActionProto& action_proto, void ExpectActionToStopScript(const ActionProto& action_proto,
const std::string& expected_message) { const std::string& expected_message) {
EXPECT_CALL(mock_action_delegate_, StopCurrentScript(expected_message)); EXPECT_CALL(mock_action_delegate_,
StopCurrentScriptAndShutdown(expected_message));
// The AutofillAction should finish successfully even when stopping the // The AutofillAction should finish successfully even when stopping the
// current script. // current script.
......
...@@ -126,7 +126,7 @@ class MockActionDelegate : public ActionDelegate { ...@@ -126,7 +126,7 @@ class MockActionDelegate : public ActionDelegate {
MOCK_METHOD0(GetClientMemory, ClientMemory*()); MOCK_METHOD0(GetClientMemory, ClientMemory*());
MOCK_METHOD0(GetPersonalDataManager, autofill::PersonalDataManager*()); MOCK_METHOD0(GetPersonalDataManager, autofill::PersonalDataManager*());
MOCK_METHOD0(GetWebContents, content::WebContents*()); MOCK_METHOD0(GetWebContents, content::WebContents*());
MOCK_METHOD1(StopCurrentScript, void(const std::string& message)); MOCK_METHOD1(StopCurrentScriptAndShutdown, void(const std::string& message));
MOCK_METHOD0(HideDetails, void()); MOCK_METHOD0(HideDetails, void());
MOCK_METHOD1(ShowDetails, void(const DetailsProto& details)); MOCK_METHOD1(ShowDetails, void(const DetailsProto& details));
MOCK_METHOD2(ShowProgressBar, void(int progress, const std::string& message)); MOCK_METHOD2(ShowProgressBar, void(int progress, const std::string& message));
......
...@@ -19,6 +19,8 @@ ...@@ -19,6 +19,8 @@
#include "components/autofill_assistant/browser/service.h" #include "components/autofill_assistant/browser/service.h"
#include "components/autofill_assistant/browser/ui_controller.h" #include "components/autofill_assistant/browser/ui_controller.h"
#include "components/autofill_assistant/browser/web_controller.h" #include "components/autofill_assistant/browser/web_controller.h"
#include "components/strings/grit/components_strings.h"
#include "ui/base/l10n/l10n_util.h"
namespace autofill_assistant { namespace autofill_assistant {
namespace { namespace {
...@@ -205,10 +207,12 @@ void ScriptExecutor::Restart() { ...@@ -205,10 +207,12 @@ void ScriptExecutor::Restart() {
at_end_ = RESTART; at_end_ = RESTART;
} }
void ScriptExecutor::StopCurrentScript(const std::string& message) { void ScriptExecutor::StopCurrentScriptAndShutdown(const std::string& message) {
if (!message.empty()) { // Use a default message when |message| is empty.
delegate_->GetUiController()->ShowStatusMessage(message); delegate_->GetUiController()->ShowStatusMessage(
} message.empty() ? l10n_util::GetStringUTF8(IDS_AUTOFILL_ASSISTANT_GIVE_UP)
: message);
at_end_ = SHUTDOWN_GRACEFULLY;
should_stop_script_ = true; should_stop_script_ = true;
} }
......
...@@ -119,7 +119,7 @@ class ScriptExecutor : public ActionDelegate { ...@@ -119,7 +119,7 @@ class ScriptExecutor : public ActionDelegate {
ClientMemory* GetClientMemory() override; ClientMemory* GetClientMemory() override;
autofill::PersonalDataManager* GetPersonalDataManager() override; autofill::PersonalDataManager* GetPersonalDataManager() override;
content::WebContents* GetWebContents() override; content::WebContents* GetWebContents() override;
void StopCurrentScript(const std::string& message) override; void StopCurrentScriptAndShutdown(const std::string& message) override;
void HideDetails() override; void HideDetails() override;
void ShowDetails(const DetailsProto& details) override; void ShowDetails(const DetailsProto& details) override;
void ShowProgressBar(int progress, const std::string& message) override; void ShowProgressBar(int progress, const std::string& message) override;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment