Clean up AssistantResponse processing.

This CL:
- Removes AssistantCardElement one-off logic from AssistantResponse so
  that it will scale to new AssisantUiElement types.
- Moves private classes into .cc to clean up .h

Bug: b:145003512
Change-Id: If6a9c79a7a6a49c10d903762ac9c1d82a8267c30
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006137
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733011}

ash/assistant/model/BUILD.gn

@@ -38,6 +38,7 @@ component("model") {
     "ui/assistant_card_element.h",
     "ui/assistant_text_element.cc",
     "ui/assistant_text_element.h",
+    "ui/assistant_ui_element.cc",
     "ui/assistant_ui_element.h",
   ]
 

ash/assistant/model/assistant_response.cc

@@ -4,13 +4,71 @@
 
 #include "ash/assistant/model/assistant_response.h"
 
-#include "ash/assistant/model/ui/assistant_card_element.h"
 #include "ash/assistant/model/ui/assistant_ui_element.h"
 #include "base/bind.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 
 namespace ash {
 
+// AssistantResponse::Processor ------------------------------------------------
+
+class AssistantResponse::Processor {
+ public:
+  Processor(AssistantResponse& response, ProcessingCallback callback)
+      : response_(response), callback_(std::move(callback)) {}
+
+  Processor(const Processor& copy) = delete;
+  Processor& operator=(const Processor& assign) = delete;
+
+  ~Processor() {
+    if (callback_)
+      std::move(callback_).Run(/*success=*/false);
+  }
+
+  void Process() {
+    // Responses should only be processed once.
+    DCHECK_EQ(ProcessingState::kUnprocessed, response_.processing_state());
+    response_.set_processing_state(ProcessingState::kProcessing);
+
+    for (const auto& ui_element : response_.GetUiElements()) {
+      // Start asynchronous processing of the UI element. Note that if the UI
+      // element does not require any pre-rendering processing the callback may
+      // be run synchronously.
+      ++processing_count_;
+      ui_element->Process(
+          base::BindOnce(&AssistantResponse::Processor::OnFinishedProcessing,
+                         base::Unretained(this)));
+    }
+
+    // If any elements are processing asynchronously this will no-op.
+    TryFinishing();
+  }
+
+ private:
+  void OnFinishedProcessing(bool success) {
+    // We handle success/failure cases the same because failures will skipped in
+    // view handling. We decrement our |processing_count_| and attempt to finish
+    // response processing. This will no-op if elements are still processing.
+    --processing_count_;
+    TryFinishing();
+  }
+
+  void TryFinishing() {
+    // No-op if we are already finished or if elements are still processing.
+    if (!callback_ || processing_count_ > 0)
+      return;
+
+    // Notify processing success.
+    response_.set_processing_state(ProcessingState::kProcessed);
+    std::move(callback_).Run(/*success=*/true);
+  }
+
+  AssistantResponse& response_;
+  ProcessingCallback callback_;
+
+  int processing_count_ = 0;
+};
+
 // AssistantResponse -----------------------------------------------------------
 
 AssistantResponse::AssistantResponse() = default;
@@ -60,60 +118,4 @@ void AssistantResponse::Process(ProcessingCallback callback) {
   processor_->Process();
 }
 
-// AssistantResponse::Processor ------------------------------------------------
-
-AssistantResponse::Processor::Processor(
-    AssistantResponse& response,
-    ProcessingCallback callback)
-    : response_(response),
-      callback_(std::move(callback)) {}
-
-AssistantResponse::Processor::~Processor() {
-  if (callback_)
-    std::move(callback_).Run(/*success=*/false);
-}
-
-void AssistantResponse::Processor::Process() {
-  // Responses should only be processed once.
-  DCHECK_EQ(ProcessingState::kUnprocessed, response_.processing_state());
-  response_.set_processing_state(ProcessingState::kProcessing);
-
-  for (const auto& ui_element : response_.GetUiElements()) {
-    switch (ui_element->type()) {
-      case AssistantUiElementType::kCard:
-        ++processing_count_;
-        // Start asynchronous processing of the card element.
-        static_cast<AssistantCardElement*>(ui_element.get())
-            ->Process(base::BindOnce(
-                &AssistantResponse::Processor::OnFinishedProcessing,
-                base::Unretained(this)));
-        break;
-      case AssistantUiElementType::kText:
-        // No processing necessary.
-        break;
-    }
-  }
-
-  // If any elements are processing asynchronously this will no-op.
-  TryFinishing();
-}
-
-void AssistantResponse::Processor::OnFinishedProcessing(bool success) {
-  // We handle success/failure cases the same because failures will skipped in
-  // view handling. We decrement our |processing_count_| and attempt to finish
-  // response processing. This will no-op if elements are still processing.
-  --processing_count_;
-  TryFinishing();
-}
-
-void AssistantResponse::Processor::TryFinishing() {
-  // No-op if we are already finished or if elements are still processing.
-  if (!callback_ || processing_count_ > 0)
-    return;
-
-  // Notify processing success.
-  response_.set_processing_state(ProcessingState::kProcessed);
-  std::move(callback_).Run(/*success=*/true);
-}
-
 }  // namespace ash

ash/assistant/model/assistant_response.h

@@ -72,38 +72,11 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantResponse
   void Process(ProcessingCallback callback);
 
  private:
+  class Processor;
+
   friend class base::RefCounted<AssistantResponse>;
   ~AssistantResponse();
 
-  // Handles processing for an AssistantResponse.
-  class Processor {
-   public:
-    Processor(
-        AssistantResponse& response,
-        ProcessingCallback callback);
-    ~Processor();
-
-    // Invoke to begin processing.
-    void Process();
-
-   private:
-    // Event fired upon completion of a UI element's asynchronous processing.
-    // Once all asynchronous processing of UI elements has completed, the
-    // response itself has finished processing.
-    void OnFinishedProcessing(bool success);
-
-    // Attempts to successfully complete response processing. This will no-op
-    // if we have already finished or if elements are still processing.
-    void TryFinishing();
-
-    AssistantResponse& response_;
-    ProcessingCallback callback_;
-
-    int processing_count_ = 0;
-
-    DISALLOW_COPY_AND_ASSIGN(Processor);
-  };
-
   std::vector<std::unique_ptr<AssistantUiElement>> ui_elements_;
   std::vector<AssistantSuggestionPtr> suggestions_;
   ProcessingState processing_state_ = ProcessingState::kUnprocessed;

ash/assistant/model/ui/assistant_card_element.cc

@@ -10,6 +10,71 @@
 
 namespace ash {
 
+// AssistantCardElement::Processor ---------------------------------------------
+
+class AssistantCardElement::Processor : public AssistantWebView2::Observer {
+ public:
+  Processor(AssistantCardElement& card_element, ProcessingCallback callback)
+      : card_element_(card_element), callback_(std::move(callback)) {}
+
+  Processor(const Processor& copy) = delete;
+  Processor& operator=(const Processor& assign) = delete;
+
+  ~Processor() override {
+    if (contents_view_)
+      contents_view_->RemoveObserver(this);
+
+    if (callback_)
+      std::move(callback_).Run(/*success=*/false);
+  }
+
+  void Process() {
+    // TODO(dmblack): Find a better way of determining desired card size.
+    const int width_dip =
+        kPreferredWidthDip - 2 * kUiElementHorizontalMarginDip;
+
+    // Configure parameters for the card.
+    AssistantWebView2::InitParams contents_params;
+    contents_params.enable_auto_resize = true;
+    contents_params.min_size = gfx::Size(width_dip, 1);
+    contents_params.max_size = gfx::Size(width_dip, INT_MAX);
+    contents_params.suppress_navigation = true;
+
+    // Create |contents_view_| and retain ownership so that is properly cleaned
+    // up in the case where it is never added to the view hierarchy.
+    contents_view_ = AssistantWebViewFactory::Get()->Create(contents_params);
+    contents_view_->set_owned_by_client();
+
+    // Observe |contents_view_| so that we are notified when loading is
+    // complete.
+    contents_view_->AddObserver(this);
+
+    // Encode the html string to be URL-safe.
+    std::string encoded_html;
+    base::Base64Encode(card_element_.html(), &encoded_html);
+
+    // Navigate to the data URL which represents the card.
+    constexpr char kDataUriPrefix[] = "data:text/html;base64,";
+    contents_view_->Navigate(GURL(kDataUriPrefix + encoded_html));
+  }
+
+ private:
+  // AssistantWebView2::Observer:
+  void DidStopLoading() override {
+    contents_view_->RemoveObserver(this);
+
+    // Pass ownership of |contents_view_| to the card element that was being
+    // processed and notify our |callback_| of success.
+    card_element_.set_contents_view(std::move(contents_view_));
+    std::move(callback_).Run(/*success=*/true);
+  }
+
+  AssistantCardElement& card_element_;
+  ProcessingCallback callback_;
+
+  std::unique_ptr<AssistantWebView2> contents_view_;
+};
+
 // AssistantCardElement --------------------------------------------------------
 
 AssistantCardElement::AssistantCardElement(const std::string& html,
@@ -25,57 +90,4 @@ void AssistantCardElement::Process(ProcessingCallback callback) {
   processor_->Process();
 }
 
-// AssistantCardElement::Processor ---------------------------------------------
-
-AssistantCardElement::Processor::Processor(
-    AssistantCardElement& card_element,
-    ProcessingCallback callback)
-    : card_element_(card_element),
-      callback_(std::move(callback)) {}
-
-AssistantCardElement::Processor::~Processor() {
-  if (contents_view_)
-    contents_view_->RemoveObserver(this);
-
-  if (callback_)
-    std::move(callback_).Run(/*success=*/false);
-}
-
-void AssistantCardElement::Processor::Process() {
-  // TODO(dmblack): Find a better way of determining desired card size.
-  const int width_dip = kPreferredWidthDip - 2 * kUiElementHorizontalMarginDip;
-
-  // Configure parameters for the card.
-  AssistantWebView2::InitParams contents_params;
-  contents_params.enable_auto_resize = true;
-  contents_params.min_size = gfx::Size(width_dip, 1);
-  contents_params.max_size = gfx::Size(width_dip, INT_MAX);
-  contents_params.suppress_navigation = true;
-
-  // Create |contents_view_| and retain ownership so that is properly cleaned up
-  // in the case where it is never added to the view hierarchy.
-  contents_view_ = AssistantWebViewFactory::Get()->Create(contents_params);
-  contents_view_->set_owned_by_client();
-
-  // Observe |contents_view_| so that we are notified when loading is complete.
-  contents_view_->AddObserver(this);
-
-  // Encode the html string to be URL-safe.
-  std::string encoded_html;
-  base::Base64Encode(card_element_.html(), &encoded_html);
-
-  // Navigate to the data URL which represents the card.
-  constexpr char kDataUriPrefix[] = "data:text/html;base64,";
-  contents_view_->Navigate(GURL(kDataUriPrefix + encoded_html));
-}
-
-void AssistantCardElement::Processor::DidStopLoading() {
-  contents_view_->RemoveObserver(this);
-
-  // Pass ownership of |contents_view_| to the card element that was being
-  // processed and notify our |callback_| of success.
-  card_element_.set_contents_view(std::move(contents_view_));
-  std::move(callback_).Run(/*success=*/true);
-}
-
 }  // namespace ash

ash/assistant/model/ui/assistant_card_element.h

@@ -21,12 +21,13 @@ namespace ash {
 class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantCardElement
     : public AssistantUiElement {
  public:
-  using ProcessingCallback = base::OnceCallback<void(bool)>;
-
   explicit AssistantCardElement(const std::string& html,
                                 const std::string& fallback);
   ~AssistantCardElement() override;
 
+  // AssistantUiElement:
+  void Process(ProcessingCallback callback) override;
+
   const std::string& html() const { return html_; }
   const std::string& fallback() const { return fallback_; }
 
@@ -38,31 +39,8 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantCardElement
     contents_view_ = std::move(contents_view);
   }
 
-  // Invoke to begin processing the card element. Upon completion, the specified
-  // |callback| will be run to indicate success or failure.
-  void Process(ProcessingCallback callback);
-
  private:
-  // Handles processing of an AssistantCardElement.
-  class Processor : public AssistantWebView2::Observer {
-   public:
-    Processor(AssistantCardElement& card_element, ProcessingCallback callback);
-    ~Processor() override;
-
-    // Invoke to begin processing.
-    void Process();
-
-    // AssistantWebView2::Observer:
-    void DidStopLoading() override;
-
-   private:
-    AssistantCardElement& card_element_;
-    ProcessingCallback callback_;
-
-    std::unique_ptr<AssistantWebView2> contents_view_;
-
-    DISALLOW_COPY_AND_ASSIGN(Processor);
-  };
+  class Processor;
 
   const std::string html_;
   const std::string fallback_;

ash/assistant/model/ui/assistant_ui_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_ui_element.h"
+
+namespace ash {
+
+AssistantUiElement::AssistantUiElement(AssistantUiElementType type)
+    : type_(type) {}
+
+AssistantUiElement::~AssistantUiElement() = default;
+
+void AssistantUiElement::Process(ProcessingCallback callback) {
+  // By default, Assistant UI elements do not require pre-rendering processing.
+  std::move(callback).Run(/*success=*/true);
+}
+
+}  // namespace ash

ash/assistant/model/ui/assistant_ui_element.h

@@ -5,6 +5,7 @@
 #ifndef ASH_ASSISTANT_MODEL_UI_ASSISTANT_UI_ELEMENT_H_
 #define ASH_ASSISTANT_MODEL_UI_ASSISTANT_UI_ELEMENT_H_
 
+#include "base/callback.h"
 #include "base/component_export.h"
 #include "base/macros.h"
 
@@ -23,12 +24,17 @@ enum class AssistantUiElementType {
 // Base class for a UI element that will be rendered inside of Assistant UI.
 class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantUiElement {
  public:
-  virtual ~AssistantUiElement() = default;
+  virtual ~AssistantUiElement();
 
   AssistantUiElementType type() const { return type_; }
 
+  // Invoke to being processing the UI element for rendering. Upon completion,
+  // the specified |callback| will be run to indicate success or failure.
+  using ProcessingCallback = base::OnceCallback<void(bool)>;
+  virtual void Process(ProcessingCallback callback);
+
  protected:
-  explicit AssistantUiElement(AssistantUiElementType type) : type_(type) {}
+  explicit AssistantUiElement(AssistantUiElementType type);
 
  private:
   const AssistantUiElementType type_;