Revert "Assistant: ensure single generic error message"

This reverts commit 3557d7dd.

Reason for revert: Caused failing on builder "linux-chromeos-chrome"
Bug: 1103100

Original change's description:
> Assistant: ensure single generic error message
> 
> Assistant processing v2 introduced a situation that allowed
> multiple error messages to flush to the screen due to
> AssistantInteractionControllerImpl::OnTtsStarted being
> called multiple times.
> 
> Add AssistantResponse::ContainsUiElement to check if duplicate
> error elements already exist before adding more.
> 
> Bug: b/158499400
> Change-Id: Ia90199ef8b733132fd5433ca47bb7905926b2f6e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2258196
> Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Reviewed-by: David Black <dmblack@google.com>
> Cr-Commit-Position: refs/heads/master@{#785935}

TBR=xiaohuic@chromium.org,dmblack@google.com,updowndota@chromium.org,wutao@chromium.org,meilinw@chromium.org,jeroendh@chromium.org,cowmoo@chromium.org

Change-Id: I596b799de09518816642de2c86398849939f5d99
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: b/158499400
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2286531Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786160}

ash/ambient/ui/assistant_response_container_view.cc

@@ -6,11 +6,9 @@
 
 #include "ash/assistant/model/assistant_response.h"
 #include "ash/assistant/model/ui/assistant_card_element.h"
-#include "ash/assistant/model/ui/assistant_error_element.h"
 #include "ash/assistant/model/ui/assistant_text_element.h"
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "ash/assistant/ui/assistant_view_delegate.h"
-#include "ash/assistant/ui/main_stage/assistant_error_element_view.h"
 #include "ash/assistant/ui/main_stage/assistant_text_element_view.h"
 #include "ash/assistant/ui/main_stage/element_animator.h"
 #include "ui/views/layout/box_layout.h"
@@ -61,10 +59,6 @@ AssistantResponseContainerView::HandleUiElement(
       AddTextElementView(new AssistantTextElement(
           static_cast<const AssistantCardElement*>(ui_element)->fallback()));
       break;
-    case AssistantUiElementType::kError:
-      AddErrorElementView(
-          static_cast<const AssistantErrorElement*>(ui_element));
-      break;
     case AssistantUiElementType::kText:
       AddTextElementView(static_cast<const AssistantTextElement*>(ui_element));
       break;
@@ -80,10 +74,4 @@ void AssistantResponseContainerView::AddTextElementView(
       std::make_unique<AssistantTextElementView>(text_element));
 }
 
-void AssistantResponseContainerView::AddErrorElementView(
-    const AssistantErrorElement* error_element) {
-  content_view()->AddChildView(
-      std::make_unique<AssistantErrorElementView>(error_element));
-}
-
 }  //  namespace ash

ash/ambient/ui/assistant_response_container_view.h

@@ -12,7 +12,6 @@
 
 namespace ash {
 
-class AssistantErrorElement;
 class AssistantTextElement;
 class AssistantViewDelegate;
 
@@ -29,7 +28,6 @@ class AssistantResponseContainerView : public AnimatedContainerView {
  private:
   void InitLayout();
   void AddTextElementView(const AssistantTextElement* text_element);
-  void AddErrorElementView(const AssistantErrorElement* error_element);
 
   // AnimatedContainerView:
   std::unique_ptr<ElementAnimator> HandleUiElement(

ash/assistant/assistant_interaction_controller_impl.cc

@@ -14,7 +14,6 @@
 #include "ash/assistant/model/assistant_response.h"
 #include "ash/assistant/model/assistant_ui_model.h"
 #include "ash/assistant/model/ui/assistant_card_element.h"
-#include "ash/assistant/model/ui/assistant_error_element.h"
 #include "ash/assistant/model/ui/assistant_text_element.h"
 #include "ash/assistant/ui/assistant_ui_constants.h"
 #include "ash/assistant/util/assistant_util.h"
@@ -397,15 +396,16 @@ void AssistantInteractionControllerImpl::OnInteractionStarted(
 
 void AssistantInteractionControllerImpl::OnInteractionFinished(
     AssistantInteractionResolution resolution) {
-  model_.SetMicState(MicState::kClosed);
-
   // If we don't have an active interaction, that indicates that this
   // interaction was explicitly stopped outside of LibAssistant. In this case,
   // we ensure that the mic is closed but otherwise ignore this event.
-  if (IsResponseProcessingV2Enabled() && !HasActiveInteraction())
+  if (IsResponseProcessingV2Enabled() && !HasActiveInteraction()) {
+    model_.SetMicState(MicState::kClosed);
     return;
+  }
 
   model_.SetInteractionState(InteractionState::kInactive);
+  model_.SetMicState(MicState::kClosed);
 
   // The mic timeout resolution is delivered inconsistently by LibAssistant. To
   // account for this, we need to check if the interaction resolved normally
@@ -432,28 +432,23 @@ void AssistantInteractionControllerImpl::OnInteractionFinished(
   if (model_.pending_query().type() != AssistantQueryType::kNull)
     model_.CommitPendingQuery();
 
-  // It's possible that the pending response has already been committed. This
-  // occurs if the response contained TTS, as we flush the response to the UI
-  // when TTS is started to reduce latency.
-  if (!IsResponseProcessingV2Enabled() && !model_.pending_response())
-    return;
+  if (!IsResponseProcessingV2Enabled()) {
+    // It's possible that the pending response has already been committed. This
+    // occurs if the response contained TTS, as we flush the response to the UI
+    // when TTS is started to reduce latency.
+    if (!model_.pending_response())
+      return;
+  }
 
   AssistantResponse* response = GetResponseForActiveInteraction();
 
   // Some interaction resolutions require special handling.
   switch (resolution) {
-    case AssistantInteractionResolution::kError: {
-      // In the case of error, we show an appropriate message to the user. Do
-      // not show another error if an identical one already exists in the
-      // response.
-      auto err = std::make_unique<AssistantErrorElement>(
-          IDS_ASH_ASSISTANT_ERROR_GENERIC);
-
-      if (!response->ContainsUiElement(err.get()))
-        response->AddUiElement(std::move(err));
-
+    case AssistantInteractionResolution::kError:
+      // In the case of error, we show an appropriate message to the user.
+      response->AddUiElement(std::make_unique<AssistantTextElement>(
+          l10n_util::GetStringUTF8(IDS_ASH_ASSISTANT_ERROR_GENERIC)));
       break;
-    }
     case AssistantInteractionResolution::kMultiDeviceHotwordLoss:
       // In the case of hotword loss to another device, we show an appropriate
       // message to the user.
@@ -696,13 +691,9 @@ void AssistantInteractionControllerImpl::OnTtsStarted(bool due_to_error) {
       }
     }
 
-    // Create an error and add it to response. Do not add it if another
-    // identical error already exists in response.
-    auto err = std::make_unique<AssistantErrorElement>(
-        IDS_ASH_ASSISTANT_ERROR_GENERIC);
-
-    if (!response->ContainsUiElement(err.get()))
-      response->AddUiElement(std::move(err));
+    // Add an error message to the response.
+    response->AddUiElement(std::make_unique<AssistantTextElement>(
+        l10n_util::GetStringUTF8(IDS_ASH_ASSISTANT_ERROR_GENERIC)));
   }
 
   response->set_has_tts(true);
@@ -976,13 +967,12 @@ AssistantResponse*
 AssistantInteractionControllerImpl::GetResponseForActiveInteraction() {
   // Returns the response for the active interaction. In response processing v2,
   // this may be the pending response (if no client ops have yet been received)
-  // or else is the committed response.
-  if (IsResponseProcessingV2Enabled()) {
-    return model_.pending_response() ? model_.pending_response()
-                                     : model_.response();
-  }
-  // In response processing v1, this is always the pending response.
-  return model_.pending_response();
+  // or else is the committed response. In response processing v2, this is
+  // always the pending response.
+  return IsResponseProcessingV2Enabled() ? model_.pending_response()
+                                               ? model_.pending_response()
+                                               : model_.response()
+                                         : model_.pending_response();
 }
 
 AssistantVisibility AssistantInteractionControllerImpl::GetVisibility() const {

ash/assistant/assistant_interaction_controller_impl_unittest.cc

@@ -4,26 +4,15 @@
 
 #include "ash/assistant/assistant_interaction_controller_impl.h"
 
-#include <algorithm>
 #include <map>
 
 #include "ash/assistant/assistant_suggestions_controller_impl.h"
-#include "ash/assistant/model/assistant_interaction_model.h"
-#include "ash/assistant/model/assistant_interaction_model_observer.h"
-#include "ash/assistant/model/assistant_response.h"
-#include "ash/assistant/model/assistant_response_observer.h"
-#include "ash/assistant/model/ui/assistant_error_element.h"
-#include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "ash/assistant/test/assistant_ash_test_base.h"
-#include "ash/assistant/ui/main_stage/assistant_error_element_view.h"
 #include "ash/public/cpp/assistant/controller/assistant_interaction_controller.h"
 #include "ash/public/cpp/assistant/controller/assistant_suggestions_controller.h"
 #include "ash/test/fake_android_intent_helper.h"
 #include "base/bind.h"
-#include "base/scoped_observer.h"
-#include "base/test/scoped_feature_list.h"
 #include "chromeos/services/assistant/public/cpp/assistant_service.h"
-#include "chromeos/services/assistant/public/cpp/features.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace ash {
@@ -68,21 +57,9 @@ class AssistantInteractionSubscriberMock
       this};
 };
 
-// AssistantInteractionControllerImplTest --------------------------------------
-
-class AssistantInteractionControllerImplTest
-    : public AssistantAshTestBase,
-      public testing::WithParamInterface<bool> {
+class AssistantInteractionControllerImplTest : public AssistantAshTestBase {
  public:
-  AssistantInteractionControllerImplTest() {
-    if (GetParam()) {
-      feature_list_.InitAndEnableFeature(
-          chromeos::assistant::features::kAssistantResponseProcessingV2);
-    } else {
-      feature_list_.InitAndDisableFeature(
-          chromeos::assistant::features::kAssistantResponseProcessingV2);
-    }
-  }
+  AssistantInteractionControllerImplTest() = default;
 
   AssistantInteractionControllerImpl* interaction_controller() {
     return static_cast<AssistantInteractionControllerImpl*>(
@@ -108,14 +85,11 @@ class AssistantInteractionControllerImplTest
     result.localized_app_name = app_name;
     return result;
   }
-
- private:
-  base::test::ScopedFeatureList feature_list_;
 };
 
 }  // namespace
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldBecomeActiveWhenInteractionStarts) {
   EXPECT_EQ(interaction_model()->interaction_state(),
             InteractionState::kInactive);
@@ -127,7 +101,7 @@ TEST_P(AssistantInteractionControllerImplTest,
             InteractionState::kActive);
 }
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldReturnErrorWhenOpenAppIsCalledWhileInactive) {
   EXPECT_EQ(interaction_model()->interaction_state(),
             InteractionState::kInactive);
@@ -137,7 +111,7 @@ TEST_P(AssistantInteractionControllerImplTest,
   EXPECT_EQ(result, kErrorResult);
 }
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldReturnErrorWhenOpenAppIsCalledWithoutAnAndroidIntentHelper) {
   StartInteraction();
 
@@ -146,7 +120,7 @@ TEST_P(AssistantInteractionControllerImplTest,
   EXPECT_EQ(result, kErrorResult);
 }
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldReturnErrorWhenOpenAppIsCalledForUnknownAndroidApp) {
   StartInteraction();
   FakeAndroidIntentHelper fake_helper;
@@ -154,7 +128,7 @@ TEST_P(AssistantInteractionControllerImplTest,
                               CreateAndroidAppInfo("unknown-app-name")));
 }
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldLaunchAppAndReturnSuccessWhenOpenAppIsCalled) {
   const std::string app_name = "AppName";
   const std::string intent = "intent://AppName";
@@ -169,7 +143,7 @@ TEST_P(AssistantInteractionControllerImplTest,
   EXPECT_EQ(intent, fake_helper.last_launched_android_intent());
 }
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldAddSchemeToIntentWhenLaunchingAndroidApp) {
   const std::string app_name = "AppName";
   const std::string intent = "#Intent-without-a-scheme";
@@ -185,7 +159,7 @@ TEST_P(AssistantInteractionControllerImplTest,
   EXPECT_EQ(intent_with_scheme, fake_helper.last_launched_android_intent());
 }
 
-TEST_P(AssistantInteractionControllerImplTest,
+TEST_F(AssistantInteractionControllerImplTest,
        ShouldCorrectlyMapSuggestionTypeToQuerySource) {
   // Mock Assistant interaction subscriber.
   StrictMock<AssistantInteractionSubscriberMock> mock(assistant_service());
@@ -223,36 +197,4 @@ TEST_P(AssistantInteractionControllerImplTest,
   }
 }
 
-TEST_P(AssistantInteractionControllerImplTest, ShouldDisplayGenericErrorOnce) {
-  StartInteraction();
-
-  // Call OnTtsStarted twice to mimic the behavior of libassistant when network
-  // is disconnected.
-  interaction_controller()->OnTtsStarted(/*due_to_error=*/true);
-  interaction_controller()->OnTtsStarted(/*due_to_error=*/true);
-
-  base::RunLoop().RunUntilIdle();
-
-  auto& ui_elements =
-      interaction_controller()->GetModel()->response()->GetUiElements();
-
-  EXPECT_EQ(ui_elements.size(), 1ul);
-  EXPECT_EQ(ui_elements.front()->type(), AssistantUiElementType::kError);
-
-  base::RunLoop().RunUntilIdle();
-
-  interaction_controller()->OnInteractionFinished(
-      chromeos::assistant::AssistantInteractionResolution::kError);
-
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_EQ(ui_elements.size(), 1ul);
-  EXPECT_EQ(ui_elements.front()->type(), AssistantUiElementType::kError);
-}
-
-// We parameterize all AssistantInteractionControllerImplTests to verify that
-// they work for both response processing v1 as well as response processing v2.
-INSTANTIATE_TEST_SUITE_P(All,
-                         AssistantInteractionControllerImplTest,
-                         testing::Bool());
 }  // namespace ash

ash/assistant/model/BUILD.gn

@@ -37,8 +37,6 @@ component("model") {
     "assistant_ui_model_observer.h",
     "ui/assistant_card_element.cc",
     "ui/assistant_card_element.h",
-    "ui/assistant_error_element.cc",
-    "ui/assistant_error_element.h",
     "ui/assistant_text_element.cc",
     "ui/assistant_text_element.h",
     "ui/assistant_ui_element.cc",

ash/assistant/model/assistant_response.cc

@@ -4,11 +4,9 @@
 
 #include "ash/assistant/model/assistant_response.h"
 
-#include <algorithm>
 #include <utility>
 
 #include "ash/assistant/model/assistant_response_observer.h"
-#include "ash/assistant/model/ui/assistant_error_element.h"
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
@@ -210,26 +208,4 @@ void AssistantResponse::NotifySuggestionsAdded(
     observer.OnSuggestionsAdded(suggestions);
 }
 
-bool AssistantResponse::ContainsUiElement(
-    const AssistantUiElement* element) const {
-  DCHECK(element);
-
-  bool contains_element =
-      std::any_of(ui_elements_.cbegin(), ui_elements_.cend(),
-                  [element](const std::unique_ptr<AssistantUiElement>& other) {
-                    return *other == *element;
-                  });
-
-  return contains_element || ContainsPendingUiElement(element);
-}
-
-bool AssistantResponse::ContainsPendingUiElement(
-    const AssistantUiElement* element) const {
-  DCHECK(element);
-
-  return std::any_of(pending_ui_elements_.cbegin(), pending_ui_elements_.cend(),
-                     [element](const std::unique_ptr<PendingUiElement>& other) {
-                       return *other->ui_element == *element;
-                     });
-}
 }  // namespace ash

ash/assistant/model/assistant_response.h

@@ -88,16 +88,10 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantResponse
   // all UI elements in the response.
   void Process(ProcessingCallback callback);
 
-  // Return true if this response contains an identical ui element.
-  bool ContainsUiElement(const AssistantUiElement* element) const;
-
  private:
   void NotifyUiElementAdded(const AssistantUiElement* ui_element);
   void NotifySuggestionsAdded(const std::vector<const AssistantSuggestion*>&);
 
-  // Return true if the pending ui elements contain an identical ui element.
-  bool ContainsPendingUiElement(const AssistantUiElement* other) const;
-
   struct PendingUiElement;
   class Processor;
 

ash/assistant/model/ui/assistant_card_element.cc

@@ -95,9 +95,4 @@ void AssistantCardElement::Process(ProcessingCallback callback) {
   processor_->Process();
 }
 
-bool AssistantCardElement::Compare(const AssistantUiElement& other) const {
-  return other.type() == AssistantUiElementType::kCard &&
-         static_cast<const AssistantCardElement&>(other).html() == html_;
-}
-
 }  // namespace ash

ash/assistant/model/ui/assistant_card_element.h

@@ -7,7 +7,6 @@
 
 #include <memory>
 #include <string>
-#include <utility>
 
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "ash/public/cpp/assistant/assistant_web_view.h"
@@ -46,9 +45,6 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantCardElement
 
   std::unique_ptr<Processor> processor_;
 
-  // AssistantUiElement:
-  bool Compare(const AssistantUiElement& other) const override;
-
   DISALLOW_COPY_AND_ASSIGN(AssistantCardElement);
 };
 

ash/assistant/model/ui/assistant_error_element.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 "ash/assistant/model/ui/assistant_error_element.h"
-
-#include "ash/assistant/ui/assistant_ui_constants.h"
-#include "ui/base/l10n/l10n_util.h"
-
-namespace ash {
-
-AssistantErrorElement::AssistantErrorElement(int message_id)
-    : AssistantUiElement(AssistantUiElementType::kError),
-      message_id_(message_id) {}
-
-AssistantErrorElement::~AssistantErrorElement() = default;
-
-bool AssistantErrorElement::Compare(const AssistantUiElement& other) const {
-  return other.type() == AssistantUiElementType::kError &&
-         static_cast<const AssistantErrorElement&>(other).message_id() ==
-             message_id_;
-}
-}  // namespace ash

ash/assistant/model/ui/assistant_error_element.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 ASH_ASSISTANT_MODEL_UI_ASSISTANT_ERROR_ELEMENT_H_
-#define ASH_ASSISTANT_MODEL_UI_ASSISTANT_ERROR_ELEMENT_H_
-
-#include <string>
-
-#include "ash/assistant/model/ui/assistant_ui_element.h"
-#include "base/component_export.h"
-
-namespace ash {
-
-// An Assistant UI error element that will be rendered as text.
-class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantErrorElement
-    : public AssistantUiElement {
- public:
-  explicit AssistantErrorElement(int message_id);
-
-  AssistantErrorElement(const AssistantErrorElement&) = delete;
-  AssistantErrorElement& operator=(const AssistantErrorElement&) = delete;
-
-  ~AssistantErrorElement() override;
-
-  int message_id() const { return message_id_; }
-
- private:
-  const int message_id_;
-
-  // AssistantUiElement:
-  bool Compare(const AssistantUiElement& other) const override;
-};
-
-}  // namespace ash
-
-#endif  // ASH_ASSISTANT_MODEL_UI_ASSISTANT_ERROR_ELEMENT_H_

ash/assistant/model/ui/assistant_text_element.cc

@@ -13,9 +13,4 @@ AssistantTextElement::AssistantTextElement(const std::string& text)
 
 AssistantTextElement::~AssistantTextElement() = default;
 
-bool AssistantTextElement::Compare(const AssistantUiElement& other) const {
-  return other.type() == AssistantUiElementType::kText &&
-         static_cast<const AssistantTextElement&>(other).text() == text_;
-}
-
 }  // namespace ash

ash/assistant/model/ui/assistant_text_element.h

@@ -25,9 +25,6 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantTextElement
  private:
   const std::string text_;
 
-  // AssistantUiElement:
-  bool Compare(const AssistantUiElement& other) const override;
-
   DISALLOW_COPY_AND_ASSIGN(AssistantTextElement);
 };
 

ash/assistant/model/ui/assistant_ui_element.h

@@ -15,9 +15,8 @@ namespace ash {
 
 // Defines possible types of Assistant UI elements.
 enum class AssistantUiElementType {
-  kCard,   // See AssistantCardElement.
-  kError,  // See AssistantErrorElement.
-  kText,   // See AssistantTextElement.
+  kCard,    // See AssistantCardElement.
+  kText,    // See AssistantTextElement.
 };
 
 // AssistantUiElement ----------------------------------------------------------
@@ -27,10 +26,6 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantUiElement {
  public:
   virtual ~AssistantUiElement();
 
-  bool operator==(const AssistantUiElement& other) const {
-    return this->Compare(other);
-  }
-
   AssistantUiElementType type() const { return type_; }
 
   // Invoke to being processing the UI element for rendering. The specified
@@ -47,8 +42,6 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantUiElement {
  private:
   const AssistantUiElementType type_;
 
-  virtual bool Compare(const AssistantUiElement& other) const = 0;
-
   DISALLOW_COPY_AND_ASSIGN(AssistantUiElement);
 };
 

ash/assistant/ui/BUILD.gn

@@ -52,8 +52,6 @@ component("ui") {
     "main_stage/animated_container_view.h",
     "main_stage/assistant_card_element_view.cc",
     "main_stage/assistant_card_element_view.h",
-    "main_stage/assistant_error_element_view.cc",
-    "main_stage/assistant_error_element_view.h",
     "main_stage/assistant_footer_view.cc",
     "main_stage/assistant_footer_view.h",
     "main_stage/assistant_onboarding_view.cc",

ash/assistant/ui/main_stage/assistant_error_element_view.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 "ash/assistant/ui/main_stage/assistant_error_element_view.h"
-
-#include "ash/assistant/model/ui/assistant_error_element.h"
-#include "ui/base/l10n/l10n_util.h"
-
-namespace ash {
-
-AssistantErrorElementView::AssistantErrorElementView(
-    const AssistantErrorElement* error_element)
-    : AssistantTextElementView(
-          l10n_util::GetStringUTF8(error_element->message_id())) {}
-
-const char* AssistantErrorElementView::GetClassName() const {
-  return "AssistantErrorElementView";
-}
-
-}  // namespace ash

ash/assistant/ui/main_stage/assistant_error_element_view.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 ASH_ASSISTANT_UI_MAIN_STAGE_ASSISTANT_ERROR_ELEMENT_VIEW_H_
-#define ASH_ASSISTANT_UI_MAIN_STAGE_ASSISTANT_ERROR_ELEMENT_VIEW_H_
-
-#include "ash/assistant/model/ui/assistant_error_element.h"
-#include "ash/assistant/ui/main_stage/assistant_text_element_view.h"
-#include "base/component_export.h"
-
-namespace ash {
-
-// AssistantErrorElementView is the visual representation of an
-// AssistantErrorElement. AssistantErrorElementView uses the same rendering
-// logic as AssistantTextElementView.
-class COMPONENT_EXPORT(ASSISTANT_UI) AssistantErrorElementView
-    : public AssistantTextElementView {
- public:
-  explicit AssistantErrorElementView(
-      const AssistantErrorElement* error_element);
-
-  // AssistantTextElementView:
-  const char* GetClassName() const override;
-};
-
-}  // namespace ash
-
-#endif  // ASH_ASSISTANT_UI_MAIN_STAGE_ASSISTANT_ERROR_ELEMENT_VIEW_H_

ash/assistant/ui/main_stage/assistant_text_element_view.cc

@@ -18,11 +18,8 @@ namespace ash {
 // AssistantTextElementView ----------------------------------------------------
 
 AssistantTextElementView::AssistantTextElementView(
-    const AssistantTextElement* text_element)
-    : AssistantTextElementView(text_element->text()) {}
-
-AssistantTextElementView::AssistantTextElementView(const std::string& text) {
-  InitLayout(text);
+    const AssistantTextElement* text_element) {
+  InitLayout(text_element);
 }
 
 AssistantTextElementView::~AssistantTextElementView() = default;
@@ -49,12 +46,13 @@ void AssistantTextElementView::ChildPreferredSizeChanged(views::View* child) {
   PreferredSizeChanged();
 }
 
-void AssistantTextElementView::InitLayout(const std::string& text) {
+void AssistantTextElementView::InitLayout(
+    const AssistantTextElement* text_element) {
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
   // Label.
-  label_ =
-      AddChildView(std::make_unique<views::Label>(base::UTF8ToUTF16(text)));
+  label_ = AddChildView(
+      std::make_unique<views::Label>(base::UTF8ToUTF16(text_element->text())));
   label_->SetAutoColorReadabilityEnabled(false);
   label_->SetBackground(views::CreateSolidBackground(SK_ColorWHITE));
   label_->SetEnabledColor(kTextColorPrimary);

ash/assistant/ui/main_stage/assistant_text_element_view.h

@@ -25,9 +25,6 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantTextElementView
     : public AssistantUiElementView {
  public:
   explicit AssistantTextElementView(const AssistantTextElement* text_element);
-
-  explicit AssistantTextElementView(const std::string& text);
-
   ~AssistantTextElementView() override;
 
   // AssistantUiElementView:
@@ -37,7 +34,7 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantTextElementView
   void ChildPreferredSizeChanged(views::View* child) override;
 
  private:
-  void InitLayout(const std::string& text);
+  void InitLayout(const AssistantTextElement* text_element);
 
   views::Label* label_;  // Owned by view hierarchy.
 

ash/assistant/ui/main_stage/assistant_ui_element_view_factory.cc

@@ -5,12 +5,10 @@
 #include "ash/assistant/ui/main_stage/assistant_ui_element_view_factory.h"
 
 #include "ash/assistant/model/ui/assistant_card_element.h"
-#include "ash/assistant/model/ui/assistant_error_element.h"
 #include "ash/assistant/model/ui/assistant_text_element.h"
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "ash/assistant/ui/assistant_view_delegate.h"
 #include "ash/assistant/ui/main_stage/assistant_card_element_view.h"
-#include "ash/assistant/ui/main_stage/assistant_error_element_view.h"
 #include "ash/assistant/ui/main_stage/assistant_text_element_view.h"
 #include "ash/assistant/ui/main_stage/assistant_ui_element_view.h"
 
@@ -28,9 +26,6 @@ std::unique_ptr<AssistantUiElementView> AssistantUiElementViewFactory::Create(
     case AssistantUiElementType::kCard:
       return std::make_unique<AssistantCardElementView>(
           delegate_, static_cast<const AssistantCardElement*>(ui_element));
-    case AssistantUiElementType::kError:
-      return std::make_unique<AssistantErrorElementView>(
-          static_cast<const AssistantErrorElement*>(ui_element));
     case AssistantUiElementType::kText:
       return std::make_unique<AssistantTextElementView>(
           static_cast<const AssistantTextElement*>(ui_element));

chrome/browser/ui/ash/assistant/assistant_browsertest.cc

@@ -2,15 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/strings/string_util.h"
 #include "base/test/bind_test_util.h"
-#include "base/test/scoped_run_loop_timeout.h"
-#include "base/time/time.h"
 #include "chrome/browser/ui/ash/assistant/assistant_test_mixin.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
 #include "chromeos/audio/cras_audio_handler.h"
+#include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/dbus/power_manager/backlight.pb.h"
 #include "chromeos/services/assistant/public/cpp/features.h"
-#include "chromeos/services/assistant/service.h"
 #include "content/public/test/browser_test.h"
 
 namespace chromeos {
@@ -50,17 +49,12 @@ class AssistantBrowserTest : public MixinBasedInProcessBrowserTest,
 
   ~AssistantBrowserTest() override = default;
 
-  AssistantTestMixin* tester() { return &tester_; }
-
   void ShowAssistantUi() {
     if (!tester()->IsVisible())
       tester()->PressAssistantKey();
   }
 
-  void CloseAssistantUi() {
-    if (tester()->IsVisible())
-      tester()->PressAssistantKey();
-  }
+  AssistantTestMixin* tester() { return &tester_; }
 
   void InitializeBrightness() {
     auto* power_manager = chromeos::PowerManagerClient::Get();
@@ -230,30 +224,6 @@ IN_PROC_BROWSER_TEST_P(AssistantBrowserTest, ShouldTurnDownBrightness) {
   ExpectBrightnessDown();
 }
 
-IN_PROC_BROWSER_TEST_P(AssistantBrowserTest,
-                       ShouldShowSingleErrorOnNetworkDown) {
-  tester()->StartAssistantAndWaitForReady();
-
-  ShowAssistantUi();
-
-  EXPECT_TRUE(tester()->IsVisible());
-
-  tester()->DisableFakeS3Server();
-
-  base::RunLoop().RunUntilIdle();
-
-  tester()->SendTextQuery("Is this thing on?");
-
-  tester()->ExpectErrorResponse(
-      "Something went wrong. Try again in a few seconds");
-
-  tester()->ExpectNoChange(base::TimeDelta::FromSeconds(1));
-
-  // This is necessary to prevent a UserInitiatedVoicelessActivity from blocking
-  // test harness teardown while we wait on assistant to finish the interaction.
-  CloseAssistantUi();
-}
-
 // We parameterize all AssistantBrowserTests to verify that they work for both
 // response processing v1 as well as response processing v2.
 INSTANTIATE_TEST_SUITE_P(All, AssistantBrowserTest, testing::Bool());

chrome/browser/ui/ash/assistant/assistant_test_mixin.cc

@@ -21,7 +21,7 @@
 #include "chrome/browser/chromeos/login/test/fake_gaia_mixin.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/ui/ash/assistant/test_support/fake_s3_server.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/login/auth/user_context.h"
@@ -44,6 +44,10 @@ LoginManagerMixin::TestUserInfo GetTestUserInfo() {
       AccountId::FromUserEmailGaiaId(kTestUser, kTestUserGaiaId));
 }
 
+bool Equals(const char* left, const char* right) {
+  return strcmp(left, right) == 0;
+}
+
 // Waiter that blocks in the |Wait| method until a given |AssistantStatus|
 // is reached, or until a timeout is hit.
 // On timeout this will abort the test with a useful error message.
@@ -220,7 +224,7 @@ class TypedResponseWaiter : public ResponseWaiter {
   // ResponseWaiter overrides:
   base::Optional<std::string> GetResponseTextOfView(
       views::View* view) const override {
-    if (view->GetClassName() == class_name_) {
+    if (Equals(view->GetClassName(), class_name_.c_str())) {
       return static_cast<ash::AssistantUiElementView*>(view)
           ->ToStringForTesting();
     }
@@ -248,7 +252,7 @@ class TypedExpectedResponseWaiter : public ExpectedResponseWaiter {
   // ExpectedResponseWaiter overrides:
   base::Optional<std::string> GetResponseTextOfView(
       views::View* view) const override {
-    if (view->GetClassName() == class_name_) {
+    if (Equals(view->GetClassName(), class_name_.c_str())) {
       return static_cast<ash::AssistantUiElementView*>(view)
           ->ToStringForTesting();
     }
@@ -275,44 +279,6 @@ void CheckResult(base::OnceClosure quit,
       base::TimeDelta::FromMilliseconds(10));
 }
 
-// Calls a callback when the view hierarchy changes.
-class CallbackViewHierarchyChangedObserver : views::ViewObserver {
- public:
-  explicit CallbackViewHierarchyChangedObserver(
-      views::View* parent_view,
-      base::RepeatingCallback<void(const views::ViewHierarchyChangedDetails&)>
-          callback)
-      : callback_(callback), parent_view_(parent_view) {
-    parent_view_->AddObserver(this);
-  }
-
-  ~CallbackViewHierarchyChangedObserver() override {
-    if (parent_view_)
-      parent_view_->RemoveObserver(this);
-  }
-
-  // ViewObserver:
-  void OnViewHierarchyChanged(
-      views::View* observed_view,
-      const views::ViewHierarchyChangedDetails& details) override {
-    std::move(callback_).Run(details);
-  }
-
-  void OnViewIsDeleting(views::View* view) override {
-    DCHECK_EQ(view, parent_view_);
-
-    if (parent_view_)
-      parent_view_->RemoveObserver(this);
-
-    parent_view_ = nullptr;
-  }
-
- private:
-  base::RepeatingCallback<void(const views::ViewHierarchyChangedDetails&)>
-      callback_;
-  views::View* parent_view_;
-};
-
 }  // namespace
 
 // Test mixin for the browser tests that logs in the given user and issues
@@ -416,10 +382,6 @@ void AssistantTestMixin::SetUpOnMainThread() {
 
 void AssistantTestMixin::TearDownOnMainThread() {
   DisableAssistant();
-  DisableFakeS3Server();
-}
-
-void AssistantTestMixin::DisableFakeS3Server() {
   fake_s3_server_.Teardown();
 }
 
@@ -533,17 +495,6 @@ void AssistantTestMixin::ExpectAnyOfTheseTextResponses(
   waiter.RunUntilResponseReceived();
 }
 
-void AssistantTestMixin::ExpectErrorResponse(
-    const std::string& expected_response,
-    base::TimeDelta wait_timeout) {
-  const base::test::ScopedRunLoopTimeout run_timeout(FROM_HERE, wait_timeout);
-  TypedExpectedResponseWaiter waiter("AssistantErrorElementView",
-                                     test_api_->ui_element_container(),
-                                     {expected_response});
-
-  waiter.RunUntilResponseReceived();
-}
-
 void AssistantTestMixin::ExpectTimersResponse(
     const std::vector<base::TimeDelta>& timers,
     base::TimeDelta wait_timeout) {
@@ -596,30 +547,6 @@ bool AssistantTestMixin::IsVisible() {
   return test_api_->IsVisible();
 }
 
-void AssistantTestMixin::ExpectNoChange(base::TimeDelta wait_timeout) {
-  base::test::ScopedDisableRunLoopTimeout disable_timeout;
-
-  base::RunLoop run_loop;
-
-  // Exit the runloop after wait_timeout.
-  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindRepeating(
-          [](base::RepeatingClosure quit) { std::move(quit).Run(); },
-          run_loop.QuitClosure()),
-      wait_timeout);
-
-  // Fail the runloop when the view hierarchy changes.
-  auto callback = base::BindRepeating(
-      [](const views::ViewHierarchyChangedDetails& change) { FAIL(); });
-
-  CallbackViewHierarchyChangedObserver observer(
-      test_api_->ui_element_container(), std::move(callback));
-
-  EXPECT_NO_FATAL_FAILURE(run_loop.Run())
-      << "View hierarchy changed during ExpectNoChange.";
-}
-
 PrefService* AssistantTestMixin::GetUserPreferences() {
   return ProfileManager::GetPrimaryUserProfile()->GetPrefs();
 }

chrome/browser/ui/ash/assistant/assistant_test_mixin.h

@@ -12,7 +12,6 @@
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "chrome/browser/ui/ash/assistant/test_support/fake_s3_server.h"
-#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/test/base/mixin_based_in_process_browser_test.h"
 #include "ui/events/keycodes/keyboard_codes_posix.h"
 
@@ -57,8 +56,6 @@ class AssistantTestMixin : public InProcessBrowserTestMixin {
   void SetUpOnMainThread() override;
   void TearDownOnMainThread() override;
 
-  void DisableFakeS3Server();
-
   // Starts the Assistant service and wait until it is ready to process
   // queries. Should be called as the first action in every test.
   void StartAssistantAndWaitForReady(
@@ -101,12 +98,6 @@ class AssistantTestMixin : public InProcessBrowserTestMixin {
   void ExpectCardResponse(const std::string& expected_response,
                           base::TimeDelta wait_timeout = kDefaultWaitTimeout);
 
-  // Waits until an error response is rendered that contains the given text. If
-  // |expected_response| is not received in |wait_timeout|, this will fail the
-  // test.
-  void ExpectErrorResponse(const std::string& expected_response,
-                           base::TimeDelta wait_timeout = kDefaultWaitTimeout);
-
   // Waits until a text response is rendered that contains the given text.
   // If |expected_response| is not received in |wait_timeout|, this will fail
   // the test.
@@ -136,10 +127,6 @@ class AssistantTestMixin : public InProcessBrowserTestMixin {
   // Returns true if the Assistant UI is currently visible.
   bool IsVisible();
 
-  // Watches the view hierarchy for change and fails if
-  // a view is updated / deleted / added before wait_timeout time elapses.
-  void ExpectNoChange(base::TimeDelta wait_timeout = kDefaultWaitTimeout);
-
  private:
   PrefService* GetUserPreferences();
   void SendKeyPress(ui::KeyboardCode key);

chrome/browser/ui/ash/assistant/test_support/fake_s3_server.cc

@@ -183,12 +183,6 @@ void FakeS3Server::UnsetFakeS3ServerURI() {
 }
 
 void FakeS3Server::StartS3ServerProcess(FakeS3Mode mode) {
-  if (process_running_) {
-    LOG(WARNING)
-        << "Called FakeS3Server::StartS3ServerProcess when already running.";
-    return;
-  }
-
   base::FilePath fake_s3_server_main =
       GetExecutableDir().Append(FILE_PATH_LITERAL(kFakeS3ServerBinary));
 
@@ -199,17 +193,10 @@ void FakeS3Server::StartS3ServerProcess(FakeS3Mode mode) {
   AppendArgument(&command_line, "--test_data_file", GetTestDataFileName());
 
   fake_s3_server_ = base::LaunchProcess(command_line, base::LaunchOptions{});
-  process_running_ = true;
 }
 
 void FakeS3Server::StopS3ServerProcess() {
-  if (!process_running_) {
-    LOG(WARNING)
-        << "Called FakeS3Server::StopS3ServerProcess when already stopped.";
-    return;
-  }
   fake_s3_server_.Terminate(/*exit_code=*/0, /*wait=*/true);
-  process_running_ = false;
 }
 
 std::string FakeS3Server::GetTestDataFileName() {

chrome/browser/ui/ash/assistant/test_support/fake_s3_server.h

@@ -65,7 +65,6 @@ class FakeS3Server {
   std::string access_token_{"FAKE_ACCESS_TOKEN"};
   std::string fake_s3_server_uri_;
   int data_file_version_;
-  bool process_running_ = false;
 
   std::unique_ptr<PortSelector> port_selector_;