Refactor Assistant proactive suggestions logic in ash.

Proactive suggestions controller logic in ash currently lives in
AssistantUiController. It makes use of some of the same logic as
standalone Assistant UI (e.g. for calculating usable work area).

Proactive suggestions logic has grown and will grow again soon as we
update triggering conditions (to only show UI as a result of user
scrolling activity). This is going to further muddy the
AssistantUiController if added to the code in its current state.

To address this, I've refactored Proactive suggestions controller logic
into AssistantProactiveSuggestionsController, a sub-controller owned by
the AssistantSuggestionsController. It is fairly self-contained and
copies some logic (e.g. for calculating usable work area) from the
AssistantUiController. This will make it much easier to clean up the
AssistantUiController when standalone UI is deprecated.

Bug: b:142144655
Change-Id: I10f4892ae166618a588505d75634c6c18a7cc6d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906935Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#715449}

ash/BUILD.gn

@@ -195,6 +195,8 @@ component("ash") {
     "assistant/assistant_notification_controller.h",
     "assistant/assistant_notification_expiry_monitor.cc",
     "assistant/assistant_notification_expiry_monitor.h",
+    "assistant/assistant_proactive_suggestions_controller.cc",
+    "assistant/assistant_proactive_suggestions_controller.h",
     "assistant/assistant_screen_context_controller.cc",
     "assistant/assistant_screen_context_controller.h",
     "assistant/assistant_settings.cc",

ash/assistant/assistant_proactive_suggestions_controller.cc

+// Copyright 2019 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/assistant_proactive_suggestions_controller.h"
+
+#include "ash/assistant/assistant_controller.h"
+#include "ash/assistant/assistant_suggestions_controller.h"
+#include "ash/assistant/assistant_ui_controller.h"
+#include "ash/assistant/ui/proactive_suggestions_view.h"
+#include "ash/public/cpp/assistant/proactive_suggestions.h"
+#include "ash/public/cpp/assistant/util/histogram_util.h"
+#include "chromeos/services/assistant/public/features.h"
+#include "ui/views/widget/widget.h"
+
+namespace ash {
+
+AssistantProactiveSuggestionsController::
+    AssistantProactiveSuggestionsController(
+        AssistantController* assistant_controller)
+    : assistant_controller_(assistant_controller) {
+  DCHECK(chromeos::assistant::features::IsProactiveSuggestionsEnabled());
+  assistant_controller_->AddObserver(this);
+}
+
+AssistantProactiveSuggestionsController::
+    ~AssistantProactiveSuggestionsController() {
+  auto* client = ProactiveSuggestionsClient::Get();
+  if (client)
+    client->SetDelegate(nullptr);
+
+  assistant_controller_->RemoveObserver(this);
+}
+
+void AssistantProactiveSuggestionsController::
+    OnAssistantControllerConstructed() {
+  assistant_controller_->suggestions_controller()->AddModelObserver(this);
+  assistant_controller_->view_delegate()->AddObserver(this);
+}
+
+void AssistantProactiveSuggestionsController::
+    OnAssistantControllerDestroying() {
+  assistant_controller_->view_delegate()->RemoveObserver(this);
+  assistant_controller_->suggestions_controller()->RemoveModelObserver(this);
+}
+
+void AssistantProactiveSuggestionsController::OnAssistantReady() {
+  // The proactive suggestions client initializes late so we need to wait for
+  // the ready signal before binding as its delegate.
+  auto* client = ProactiveSuggestionsClient::Get();
+  if (client)
+    client->SetDelegate(this);
+}
+
+void AssistantProactiveSuggestionsController::
+    OnProactiveSuggestionsClientDestroying() {
+  auto* client = ProactiveSuggestionsClient::Get();
+  if (client)
+    client->SetDelegate(nullptr);
+}
+
+void AssistantProactiveSuggestionsController::OnProactiveSuggestionsChanged(
+    scoped_refptr<const ProactiveSuggestions> proactive_suggestions) {
+  // This method is invoked from the ProactiveSuggestionsClient to inform us
+  // that the active set of proactive suggestions has changed. Because we have
+  // read-only access to the Assistant suggestions model, we forward this event
+  // to the Assistant suggestions controller to cache state. We will then react
+  // to the change in model state in OnProactiveSuggestionsChanged(new, old).
+  assistant_controller_->suggestions_controller()
+      ->OnProactiveSuggestionsChanged(std::move(proactive_suggestions));
+}
+
+void AssistantProactiveSuggestionsController::OnProactiveSuggestionsChanged(
+    scoped_refptr<const ProactiveSuggestions> proactive_suggestions,
+    scoped_refptr<const ProactiveSuggestions> old_proactive_suggestions) {
+  // If a set of earlier proactive suggestions had been shown, it's safe to
+  // assume they are now being closed due to a change in context. Note that this
+  // will no-op if the proactive suggestions view does not exist.
+  CloseUi(ProactiveSuggestionsShowResult::kCloseByContextChange);
+
+  if (!proactive_suggestions)
+    return;
+
+  // If this set of proactive suggestions is blacklisted, it should not be shown
+  // to the user. A set of proactive suggestions may be blacklisted as a result
+  // of duplicate suppression or as a result of the user explicitly closing the
+  // proactive suggestions view.
+  if (base::Contains(proactive_suggestions_blacklist_,
+                     proactive_suggestions->hash())) {
+    RecordProactiveSuggestionsShowAttempt(
+        proactive_suggestions->category(),
+        ProactiveSuggestionsShowAttempt::kAbortedByDuplicateSuppression);
+    return;
+  }
+
+  ShowUi();
+}
+
+void AssistantProactiveSuggestionsController::
+    OnProactiveSuggestionsCloseButtonPressed() {
+  // When the user explicitly closes the proactive suggestions view, that's a
+  // strong signal that they don't want to see the same suggestions again so we
+  // add it to our blacklist.
+  proactive_suggestions_blacklist_.emplace(
+      view_->proactive_suggestions()->hash());
+
+  CloseUi(ProactiveSuggestionsShowResult::kCloseByUser);
+}
+
+void AssistantProactiveSuggestionsController::
+    OnProactiveSuggestionsViewHoverChanged(bool is_hovering) {
+  // Hover changed events may occur during the proactive suggestion view's
+  // closing sequence. When this occurs, no further action is necessary.
+  if (!view_ || !view_->GetWidget() || view_->GetWidget()->IsClosed())
+    return;
+
+  if (!is_hovering) {
+    // When the user is no longer hovering over the proactive suggestions view
+    // we need to reset the timer so that it will auto-close appropriately.
+    auto_close_timer_.Reset();
+    return;
+  }
+
+  const base::TimeDelta remaining_time =
+      auto_close_timer_.desired_run_time() - base::TimeTicks::Now();
+
+  // The user is now hovering over the proactive suggestions view so we need to
+  // pause the auto-close timer until we are no longer in a hovering state. Once
+  // we leave hovering state, we will resume the auto-close timer with whatever
+  // |remaining_time| is left. To accomplish this, we schedule the auto-close
+  // timer to fire in the future...
+  auto_close_timer_.Start(FROM_HERE, remaining_time,
+                          auto_close_timer_.user_task());
+
+  // ...but immediately stop it so that when we reset the auto-close timer upon
+  // leaving hovering state, the timer will appropriately fire only after the
+  // |remaining_time| has elapsed.
+  auto_close_timer_.Stop();
+}
+
+void AssistantProactiveSuggestionsController::
+    OnProactiveSuggestionsViewPressed() {
+  CloseUi(ProactiveSuggestionsShowResult::kClick);
+
+  // Clicking on the proactive suggestions view causes the user to enter
+  // Assistant UI where a proactive suggestions interaction will be initiated.
+  assistant_controller_->ui_controller()->ShowUi(
+      AssistantEntryPoint::kProactiveSuggestions);
+}
+
+void AssistantProactiveSuggestionsController::ShowUi() {
+  DCHECK(!view_);
+  view_ = new ProactiveSuggestionsView(assistant_controller_->view_delegate());
+  view_->GetWidget()->ShowInactive();
+
+  RecordProactiveSuggestionsShowAttempt(
+      view_->proactive_suggestions()->category(),
+      ProactiveSuggestionsShowAttempt::kSuccess);
+
+  // If duplicate suppression is enabled, the user should not be presented with
+  // this set of proactive suggestions again so we add it to our blacklist.
+  if (chromeos::assistant::features::
+          IsProactiveSuggestionsSuppressDuplicatesEnabled()) {
+    proactive_suggestions_blacklist_.emplace(
+        view_->proactive_suggestions()->hash());
+  }
+
+  // The proactive suggestions view will automatically be closed if the user
+  // doesn't interact with it within a fixed time interval.
+  auto_close_timer_.Start(
+      FROM_HERE,
+      chromeos::assistant::features::GetProactiveSuggestionsTimeoutThreshold(),
+      base::BindRepeating(&AssistantProactiveSuggestionsController::CloseUi,
+                          weak_factory_.GetWeakPtr(),
+                          ProactiveSuggestionsShowResult::kCloseByTimeout));
+}
+
+void AssistantProactiveSuggestionsController::CloseUi(
+    ProactiveSuggestionsShowResult result) {
+  if (!view_)
+    return;
+
+  RecordProactiveSuggestionsShowResult(
+      view_->proactive_suggestions()->category(), result);
+
+  auto_close_timer_.Stop();
+
+  view_->GetWidget()->Close();
+  view_ = nullptr;
+}
+
+}  // namespace ash

ash/assistant/assistant_proactive_suggestions_controller.h

+// Copyright 2019 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_ASSISTANT_PROACTIVE_SUGGESTIONS_CONTROLLER_H_
+#define ASH_ASSISTANT_ASSISTANT_PROACTIVE_SUGGESTIONS_CONTROLLER_H_
+
+#include <memory>
+#include <set>
+
+#include "ash/assistant/assistant_controller_observer.h"
+#include "ash/assistant/model/assistant_suggestions_model_observer.h"
+#include "ash/assistant/ui/assistant_view_delegate.h"
+#include "ash/public/cpp/assistant/proactive_suggestions_client.h"
+#include "base/macros.h"
+#include "base/timer/timer.h"
+
+namespace ash {
+
+class AssistantController;
+class ProactiveSuggestions;
+class ProactiveSuggestionsView;
+
+namespace assistant {
+namespace metrics {
+enum class ProactiveSuggestionsShowAttempt;
+enum class ProactiveSuggestionsShowResult;
+}  // namespace metrics
+}  // namespace assistant
+
+// Controller for the Assistant proactive suggestions feature.
+class AssistantProactiveSuggestionsController
+    : public AssistantControllerObserver,
+      public ProactiveSuggestionsClient::Delegate,
+      public AssistantSuggestionsModelObserver,
+      public AssistantViewDelegateObserver {
+ public:
+  using ProactiveSuggestionsShowAttempt =
+      assistant::metrics::ProactiveSuggestionsShowAttempt;
+  using ProactiveSuggestionsShowResult =
+      assistant::metrics::ProactiveSuggestionsShowResult;
+
+  explicit AssistantProactiveSuggestionsController(
+      AssistantController* assistant_controller);
+  ~AssistantProactiveSuggestionsController() override;
+
+  // AssistantControllerObserver:
+  void OnAssistantControllerConstructed() override;
+  void OnAssistantControllerDestroying() override;
+  void OnAssistantReady() override;
+
+  // ProactiveSuggestionsClient::Delegate:
+  void OnProactiveSuggestionsClientDestroying() override;
+  void OnProactiveSuggestionsChanged(
+      scoped_refptr<const ProactiveSuggestions> proactive_suggestions) override;
+
+  // AssistantSuggestionsModelObserver:
+  void OnProactiveSuggestionsChanged(
+      scoped_refptr<const ProactiveSuggestions> proactive_suggestions,
+      scoped_refptr<const ProactiveSuggestions> old_proactive_suggestions)
+      override;
+
+  // AssistantViewDelegateObserver:
+  void OnProactiveSuggestionsCloseButtonPressed() override;
+  void OnProactiveSuggestionsViewHoverChanged(bool is_hovering) override;
+  void OnProactiveSuggestionsViewPressed() override;
+
+ private:
+  void ShowUi();
+  void CloseUi(ProactiveSuggestionsShowResult result);
+
+  AssistantController* const assistant_controller_;  // Owned by Shell.
+
+  ProactiveSuggestionsView* view_ = nullptr;  // Owned by view hierarchy.
+
+  // When shown, the proactive suggestions view will automatically be closed if
+  // the user doesn't interact with it within a fixed time interval.
+  base::RetainingOneShotTimer auto_close_timer_;
+
+  // If the hash for a set of proactive suggestions is found in this collection,
+  // they should not be shown to the user. A set of proactive suggestions may be
+  // added to the blacklist as a result of duplicate suppression or as a result
+  // of the user explicitly closing the proactive suggestions view.
+  std::set<size_t> proactive_suggestions_blacklist_;
+
+  base::WeakPtrFactory<AssistantProactiveSuggestionsController> weak_factory_{
+      this};
+
+  DISALLOW_COPY_AND_ASSIGN(AssistantProactiveSuggestionsController);
+};
+
+}  // namespace ash
+
+#endif  // ASH_ASSISTANT_ASSISTANT_PROACTIVE_SUGGESTIONS_CONTROLLER_H_

ash/assistant/assistant_suggestions_controller.cc

@@ -12,13 +12,13 @@
 #include "ash/assistant/util/assistant_util.h"
 #include "ash/assistant/util/deep_link_util.h"
 #include "ash/public/cpp/assistant/proactive_suggestions.h"
-#include "ash/public/cpp/assistant/proactive_suggestions_client.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/feature_list.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/rand_util.h"
 #include "chromeos/services/assistant/public/cpp/assistant_prefs.h"
+#include "chromeos/services/assistant/public/features.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -67,16 +67,18 @@ constexpr int kMaxNumOfConversationStarters = 3;
 AssistantSuggestionsController::AssistantSuggestionsController(
     AssistantController* assistant_controller)
     : assistant_controller_(assistant_controller) {
+  if (chromeos::assistant::features::IsProactiveSuggestionsEnabled()) {
+    proactive_suggestions_controller_ =
+        std::make_unique<AssistantProactiveSuggestionsController>(
+            assistant_controller_);
+  }
+
   UpdateConversationStarters();
   assistant_controller_->AddObserver(this);
   AssistantState::Get()->AddObserver(this);
 }
 
 AssistantSuggestionsController::~AssistantSuggestionsController() {
-  auto* client = ProactiveSuggestionsClient::Get();
-  if (client)
-    client->SetDelegate(nullptr);
-
   assistant_controller_->RemoveObserver(this);
   AssistantState::Get()->RemoveObserver(this);
 }
@@ -99,14 +101,6 @@ void AssistantSuggestionsController::OnAssistantControllerDestroying() {
   assistant_controller_->ui_controller()->RemoveModelObserver(this);
 }
 
-void AssistantSuggestionsController::OnAssistantReady() {
-  // The proactive suggestions client initializes late so we need to wait for
-  // the ready signal before binding as its delegate.
-  auto* client = ProactiveSuggestionsClient::Get();
-  if (client)
-    client->SetDelegate(this);
-}
-
 void AssistantSuggestionsController::OnUiVisibilityChanged(
     AssistantVisibility new_visibility,
     AssistantVisibility old_visibility,
@@ -118,21 +112,15 @@ void AssistantSuggestionsController::OnUiVisibilityChanged(
     UpdateConversationStarters();
 }
 
-void AssistantSuggestionsController::OnAssistantContextEnabled(bool enabled) {
-  UpdateConversationStarters();
-}
-
-void AssistantSuggestionsController::OnProactiveSuggestionsClientDestroying() {
-  auto* client = ProactiveSuggestionsClient::Get();
-  if (client)
-    client->SetDelegate(nullptr);
-}
-
 void AssistantSuggestionsController::OnProactiveSuggestionsChanged(
-    scoped_refptr<ProactiveSuggestions> proactive_suggestions) {
+    scoped_refptr<const ProactiveSuggestions> proactive_suggestions) {
   model_.SetProactiveSuggestions(std::move(proactive_suggestions));
 }
 
+void AssistantSuggestionsController::OnAssistantContextEnabled(bool enabled) {
+  UpdateConversationStarters();
+}
+
 // TODO(dmblack): The conversation starter cache should receive its contents
 // from the server. Hard-coding for the time being.
 void AssistantSuggestionsController::UpdateConversationStarters() {

ash/assistant/assistant_suggestions_controller.h

@@ -8,22 +8,21 @@
 #include <memory>
 
 #include "ash/assistant/assistant_controller_observer.h"
+#include "ash/assistant/assistant_proactive_suggestions_controller.h"
 #include "ash/assistant/model/assistant_suggestions_model.h"
 #include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/public/cpp/assistant/assistant_state.h"
-#include "ash/public/cpp/assistant/proactive_suggestions_client.h"
 #include "base/macros.h"
 
 namespace ash {
 
 class AssistantController;
 class AssistantSuggestionsModelObserver;
+class ProactiveSuggestions;
 
-class AssistantSuggestionsController
-    : public AssistantControllerObserver,
-      public AssistantUiModelObserver,
-      public AssistantStateObserver,
-      public ProactiveSuggestionsClient::Delegate {
+class AssistantSuggestionsController : public AssistantControllerObserver,
+                                       public AssistantUiModelObserver,
+                                       public AssistantStateObserver {
  public:
   explicit AssistantSuggestionsController(
       AssistantController* assistant_controller);
@@ -39,7 +38,6 @@ class AssistantSuggestionsController
   // AssistantControllerObserver:
   void OnAssistantControllerConstructed() override;
   void OnAssistantControllerDestroying() override;
-  void OnAssistantReady() override;
 
   // AssistantUiModelObserver:
   void OnUiVisibilityChanged(
@@ -48,10 +46,11 @@ class AssistantSuggestionsController
       base::Optional<AssistantEntryPoint> entry_point,
       base::Optional<AssistantExitPoint> exit_point) override;
 
-  // ProactiveSuggestionsClient::Delegate:
-  void OnProactiveSuggestionsClientDestroying() override;
+  // Invoked when the active set of |proactive_suggestions| has changed. Note
+  // that this method should only be called by the sub-controller for the
+  // proactive suggestions feature to update model state.
   void OnProactiveSuggestionsChanged(
-      scoped_refptr<ProactiveSuggestions> proactive_suggestions) override;
+      scoped_refptr<const ProactiveSuggestions> proactive_suggestions);
 
  private:
   // AssistantStateObserver:
@@ -61,6 +60,11 @@ class AssistantSuggestionsController
 
   AssistantController* const assistant_controller_;  // Owned by Shell.
 
+  // A sub-controller for the proactive suggestions feature. Note that this will
+  // only exist if the proactive suggestions feature is enabled.
+  std::unique_ptr<AssistantProactiveSuggestionsController>
+      proactive_suggestions_controller_;
+
   AssistantSuggestionsModel model_;
 
   DISALLOW_COPY_AND_ASSIGN(AssistantSuggestionsController);

ash/assistant/assistant_ui_controller.h

@@ -7,14 +7,12 @@
 
 #include <map>
 #include <memory>
-#include <set>
 #include <string>
 
 #include "ash/ash_export.h"
 #include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/model/assistant_interaction_model_observer.h"
 #include "ash/assistant/model/assistant_screen_context_model_observer.h"
-#include "ash/assistant/model/assistant_suggestions_model_observer.h"
 #include "ash/assistant/model/assistant_ui_model.h"
 #include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/assistant/ui/assistant_view_delegate.h"
@@ -46,20 +44,12 @@ namespace ash {
 
 class AssistantContainerView;
 class AssistantController;
-class ProactiveSuggestionsView;
-
-namespace assistant {
-namespace metrics {
-enum class ProactiveSuggestionsShowResult;
-}  // namespace metrics
-}  // namespace assistant
 
 class ASH_EXPORT AssistantUiController
     : public views::WidgetObserver,
       public AssistantControllerObserver,
       public AssistantInteractionModelObserver,
       public AssistantScreenContextModelObserver,
-      public AssistantSuggestionsModelObserver,
       public AssistantUiModelObserver,
       public AssistantViewDelegateObserver,
       public CaptionBarDelegate,
@@ -91,12 +81,6 @@ class ASH_EXPORT AssistantUiController
   void OnInteractionStateChanged(InteractionState interaction_state) override;
   void OnMicStateChanged(MicState mic_state) override;
 
-  // AssistantSuggestionsModelObserver:
-  void OnProactiveSuggestionsChanged(
-      scoped_refptr<const ProactiveSuggestions> proactive_suggestions,
-      scoped_refptr<const ProactiveSuggestions> old_proactive_suggestions)
-      override;
-
   // AssistantScreenContextModelObserver:
   void OnScreenContextRequestStateChanged(
       ScreenContextRequestState request_state) override;
@@ -107,9 +91,6 @@ class ASH_EXPORT AssistantUiController
   // AssistantViewDelegateObserver:
   void OnDialogPlateButtonPressed(AssistantButtonId id) override;
   void OnMiniViewPressed() override;
-  void OnProactiveSuggestionsCloseButtonPressed() override;
-  void OnProactiveSuggestionsViewHoverChanged(bool is_hovering) override;
-  void OnProactiveSuggestionsViewPressed() override;
 
   // HighlighterController::Observer:
   void OnHighlighterEnabledChanged(HighlighterEnabledState state) override;
@@ -163,17 +144,6 @@ class ASH_EXPORT AssistantUiController
   void CreateContainerView();
   void ResetContainerView();
 
-  // Constructs/resets |proactive_suggestions_view_|.
-  void CreateProactiveSuggestionsView();
-  void ResetProactiveSuggestionsView(
-      int category,
-      assistant::metrics::ProactiveSuggestionsShowResult result);
-
-  // Returns the root window for |container_view_| if it exists, otherwise
-  // |proactive_suggestions_view_|. Note that this method may only be called
-  // when one of these views exist.
-  aura::Window* GetRootWindow();
-
   // Adds/removes observers used for calculating usable work area as needed.
   void UpdateUsableWorkAreaObservers();
 
@@ -186,7 +156,6 @@ class ASH_EXPORT AssistantUiController
 
   // Owned by view hierarchy.
   AssistantContainerView* container_view_ = nullptr;
-  ProactiveSuggestionsView* proactive_suggestions_view_ = nullptr;
 
   std::unique_ptr<views::EventMonitor> event_monitor_;
 
@@ -196,19 +165,9 @@ class ASH_EXPORT AssistantUiController
   // session. We delay this behavior to allow the user an opportunity to resume.
   base::OneShotTimer auto_close_timer_;
 
-  // When shown, the proactive suggestions widget will automatically be closed
-  // if the user doesn't interact with it within a fixed interval.
-  base::RetainingOneShotTimer auto_close_proactive_suggestions_timer_;
-
   // Whether the UI controller is observing changes to the usable work area.
   bool is_observing_usable_work_area_ = false;
 
-  // When proactive suggestions duplicate suppression is enabled, we won't show
-  // proactive suggestions that have been shown before. To accomplish this, we
-  // must cache proactive suggestions that have already been shown to the user.
-  // We cache a hash representation of the proactive suggestions to save space.
-  std::set<size_t> shown_proactive_suggestions_;
-
   base::WeakPtrFactory<AssistantUiController> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(AssistantUiController);

ash/assistant/assistant_view_delegate_impl.cc

@@ -112,6 +112,11 @@ void AssistantViewDelegateImpl::GetNavigableContentsFactoryForView(
   assistant_controller_->GetNavigableContentsFactory(std::move(receiver));
 }
 
+aura::Window* AssistantViewDelegateImpl::GetRootWindowForDisplayId(
+    int64_t display_id) {
+  return Shell::Get()->GetRootWindowForDisplayId(display_id);
+}
+
 aura::Window* AssistantViewDelegateImpl::GetRootWindowForNewWindows() {
   return Shell::Get()->GetRootWindowForNewWindows();
 }

ash/assistant/assistant_view_delegate_impl.h

@@ -51,6 +51,7 @@ class AssistantViewDelegateImpl : public AssistantViewDelegate {
   void GetNavigableContentsFactoryForView(
       mojo::PendingReceiver<content::mojom::NavigableContentsFactory> receiver)
       override;
+  aura::Window* GetRootWindowForDisplayId(int64_t display_id) override;
   aura::Window* GetRootWindowForNewWindows() override;
   bool IsTabletMode() const override;
   void OnDialogPlateButtonPressed(AssistantButtonId id) override;

ash/assistant/model/assistant_suggestions_model.cc

@@ -53,14 +53,14 @@ AssistantSuggestionsModel::GetConversationStarters() const {
 }
 
 void AssistantSuggestionsModel::SetProactiveSuggestions(
-    scoped_refptr<ProactiveSuggestions> proactive_suggestions) {
+    scoped_refptr<const ProactiveSuggestions> proactive_suggestions) {
   if (ProactiveSuggestions::AreEqual(proactive_suggestions.get(),
                                      proactive_suggestions_.get())) {
     return;
   }
 
   auto old_proactive_suggestions = std::move(proactive_suggestions_);
-  proactive_suggestions_ = proactive_suggestions;
+  proactive_suggestions_ = std::move(proactive_suggestions);
   NotifyProactiveSuggestionsChanged(old_proactive_suggestions);
 }
 
@@ -78,7 +78,8 @@ void AssistantSuggestionsModel::NotifyConversationStartersChanged() {
 }
 
 void AssistantSuggestionsModel::NotifyProactiveSuggestionsChanged(
-    const scoped_refptr<ProactiveSuggestions>& old_proactive_suggestions) {
+    const scoped_refptr<const ProactiveSuggestions>&
+        old_proactive_suggestions) {
   for (AssistantSuggestionsModelObserver& observer : observers_) {
     observer.OnProactiveSuggestionsChanged(proactive_suggestions_,
                                            old_proactive_suggestions);

ash/assistant/model/assistant_suggestions_model.h

@@ -44,7 +44,7 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantSuggestionsModel {
 
   // Sets the cache of proactive suggestions.
   void SetProactiveSuggestions(
-      scoped_refptr<ProactiveSuggestions> proactive_suggestions);
+      scoped_refptr<const ProactiveSuggestions> proactive_suggestions);
 
   // Returns the cache of proactive suggestions.
   scoped_refptr<const ProactiveSuggestions> GetProactiveSuggestions() const;
@@ -52,10 +52,11 @@ class COMPONENT_EXPORT(ASSISTANT_MODEL) AssistantSuggestionsModel {
  private:
   void NotifyConversationStartersChanged();
   void NotifyProactiveSuggestionsChanged(
-      const scoped_refptr<ProactiveSuggestions>& old_proactive_suggestions);
+      const scoped_refptr<const ProactiveSuggestions>&
+          old_proactive_suggestions);
 
   std::vector<AssistantSuggestionPtr> conversation_starters_;
-  scoped_refptr<ProactiveSuggestions> proactive_suggestions_;
+  scoped_refptr<const ProactiveSuggestions> proactive_suggestions_;
 
   base::ObserverList<AssistantSuggestionsModelObserver> observers_;
 

ash/assistant/ui/BUILD.gn

@@ -104,6 +104,7 @@ component("ui") {
     "//ash/assistant/model",
     "//ash/assistant/ui:constants",
     "//ash/assistant/util",
+    "//ash/keyboard/ui",
     "//ash/public/cpp",
     "//ash/public/cpp/vector_icons",
     "//ash/resources/vector_icons",

ash/assistant/ui/DEPS

@@ -4,6 +4,7 @@ include_rules = [
   "+ash/assistant/model",
   "+ash/assistant/ui",
   "+ash/assistant/util",
+  "+ash/keyboard/ui",
   "+ash/public",
   "+ash/resources/vector_icons",
   "+ash/strings",

ash/assistant/ui/assistant_view_delegate.h

@@ -142,6 +142,9 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantViewDelegate {
       mojo::PendingReceiver<content::mojom::NavigableContentsFactory>
           receiver) = 0;
 
+  // Returns the root window for the specified |display_id|.
+  virtual aura::Window* GetRootWindowForDisplayId(int64_t display_id) = 0;
+
   // Returns the root window that newly created windows should be added to.
   virtual aura::Window* GetRootWindowForNewWindows() = 0;
 

ash/assistant/ui/proactive_suggestions_view.cc

@@ -8,6 +8,7 @@
 
 #include "ash/assistant/ui/assistant_ui_constants.h"
 #include "ash/assistant/ui/assistant_view_delegate.h"
+#include "ash/keyboard/ui/keyboard_ui_controller.h"
 #include "ash/public/cpp/assistant/proactive_suggestions.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "base/strings/utf_string_conversions.h"
@@ -42,16 +43,29 @@ constexpr int kPreferredHeightDip = 32;
 
 ProactiveSuggestionsView::ProactiveSuggestionsView(
     AssistantViewDelegate* delegate)
-    : views::Button(/*listener=*/this), delegate_(delegate) {
+    : views::Button(/*listener=*/this),
+      delegate_(delegate),
+      proactive_suggestions_(
+          delegate_->GetSuggestionsModel()->GetProactiveSuggestions()),
+      keyboard_workspace_occluded_bounds_(
+          keyboard::KeyboardUIController::Get()
+              ->GetWorkspaceOccludedBoundsInScreen()) {
+  DCHECK(proactive_suggestions_);
+
   InitLayout();
   InitWidget();
   InitWindow();
+  UpdateBounds();
 
-  delegate_->AddUiModelObserver(this);
+  // We need to observe both the screen and the keyboard states to allow us to
+  // dynamically re-bound in response to changes in the usable work area.
+  display::Screen::GetScreen()->AddObserver(this);
+  keyboard::KeyboardUIController::Get()->AddObserver(this);
 }
 
 ProactiveSuggestionsView::~ProactiveSuggestionsView() {
-  delegate_->RemoveUiModelObserver(this);
+  keyboard::KeyboardUIController::Get()->RemoveObserver(this);
+  display::Screen::GetScreen()->RemoveObserver(this);
 
   if (GetWidget() && GetWidget()->GetNativeWindow())
     GetWidget()->GetNativeWindow()->RemoveObserver(this);
@@ -123,11 +137,6 @@ void ProactiveSuggestionsView::ButtonPressed(views::Button* sender,
   delegate_->OnProactiveSuggestionsViewPressed();
 }
 
-void ProactiveSuggestionsView::OnUsableWorkAreaChanged(
-    const gfx::Rect& usable_work_area) {
-  UpdateBounds();
-}
-
 void ProactiveSuggestionsView::OnWindowDestroying(aura::Window* window) {
   window->RemoveObserver(this);
 }
@@ -158,6 +167,38 @@ void ProactiveSuggestionsView::OnWindowVisibilityChanging(aura::Window* window,
                   GetBoundsInScreen().y());
 }
 
+void ProactiveSuggestionsView::OnDisplayMetricsChanged(
+    const display::Display& display,
+    uint32_t changed_metrics) {
+  // When the keyboard is showing, display metric changes to the usable work
+  // area are handled by OnKeyboardOccludedBoundsChanged. This accounts for
+  // inconsistencies between the virtual keyboard and the A11Y keyboard.
+  if (!keyboard_workspace_occluded_bounds_.IsEmpty())
+    return;
+
+  // We only respond to events that occur in the same display as our view.
+  auto* root_window = GetWidget()->GetNativeWindow()->GetRootWindow();
+  if (root_window == delegate_->GetRootWindowForDisplayId(display.id()))
+    UpdateBounds();
+}
+
+void ProactiveSuggestionsView::OnKeyboardOccludedBoundsChanged(
+    const gfx::Rect& new_bounds_in_screen) {
+  if (!new_bounds_in_screen.IsEmpty()) {
+    // We only respond to events that occur in the same display as our view.
+    auto* root_window = GetWidget()->GetNativeWindow()->GetRootWindow();
+    if (root_window != delegate_->GetRootWindowForDisplayId(
+                           display::Screen::GetScreen()
+                               ->GetDisplayMatching(new_bounds_in_screen)
+                               .id())) {
+      return;
+    }
+  }
+
+  keyboard_workspace_occluded_bounds_ = new_bounds_in_screen;
+  UpdateBounds();
+}
+
 void ProactiveSuggestionsView::InitLayout() {
   views::BoxLayout* layout_manager =
       SetLayoutManager(std::make_unique<views::BoxLayout>(
@@ -197,10 +238,8 @@ void ProactiveSuggestionsView::InitLayout() {
   // The |description| string coming from the proactive suggestions server may
   // be HTML escaped so we need to unescape before displaying to avoid printing
   // HTML entities to the user.
-  label->SetText(
-      net::UnescapeForHTML(base::UTF8ToUTF16(delegate_->GetSuggestionsModel()
-                                                 ->GetProactiveSuggestions()
-                                                 ->description())));
+  label->SetText(net::UnescapeForHTML(
+      base::UTF8ToUTF16(proactive_suggestions_->description())));
 
   AddChildView(label);
 
@@ -232,8 +271,6 @@ void ProactiveSuggestionsView::InitWidget() {
   views::Widget* widget = new views::Widget();
   widget->Init(std::move(params));
   widget->SetContentsView(this);
-
-  UpdateBounds();
 }
 
 void ProactiveSuggestionsView::InitWindow() {
@@ -257,14 +294,33 @@ void ProactiveSuggestionsView::InitWindow() {
 }
 
 void ProactiveSuggestionsView::UpdateBounds() {
-  const gfx::Rect& usable_work_area =
-      delegate_->GetUiModel()->usable_work_area();
+  const gfx::Rect screen_bounds =
+      GetWidget()->GetNativeWindow()->GetRootWindow()->GetBoundsInScreen();
+
+  // Calculate the usable work area, accounting for keyboard state.
+  gfx::Rect usable_work_area;
+  if (keyboard_workspace_occluded_bounds_.height()) {
+    // The virtual keyboard, unlike the A11Y keyboard, doesn't affect display
+    // work area, so we need to manually calculate usable work area.
+    usable_work_area = gfx::Rect(
+        screen_bounds.x(), screen_bounds.y(), screen_bounds.width(),
+        screen_bounds.height() - keyboard_workspace_occluded_bounds_.height());
+  } else {
+    // When no keyboard is present, we can use the display's calculation of
+    // usable work area.
+    usable_work_area = display::Screen::GetScreen()
+                           ->GetDisplayMatching(screen_bounds)
+                           .work_area();
+  }
 
-  const gfx::Size size = CalculatePreferredSize();
+  // Inset the usable work area so that we have a margin between the our view
+  // and other system UI (e.g. the shelf).
+  usable_work_area.Inset(kMarginDip, kMarginDip);
 
+  // The view is bottom-left aligned.
+  const gfx::Size size = GetPreferredSize();
   const int left = usable_work_area.x();
   const int top = usable_work_area.bottom() - size.height();
-
   GetWidget()->SetBounds(gfx::Rect(left, top, size.width(), size.height()));
 }
 

ash/assistant/ui/proactive_suggestions_view.h

@@ -5,8 +5,9 @@
 #ifndef ASH_ASSISTANT_UI_PROACTIVE_SUGGESTIONS_VIEW_H_
 #define ASH_ASSISTANT_UI_PROACTIVE_SUGGESTIONS_VIEW_H_
 
-#include "ash/assistant/model/assistant_ui_model_observer.h"
+#include "ash/public/cpp/keyboard/keyboard_controller_observer.h"
 #include "ui/aura/window_observer.h"
+#include "ui/display/display_observer.h"
 #include "ui/views/controls/button/button.h"
 
 namespace views {
@@ -16,13 +17,15 @@ class ImageButton;
 namespace ash {
 
 class AssistantViewDelegate;
+class ProactiveSuggestions;
 
 // The view for proactive suggestions which serves as the feature's entry point.
 class COMPONENT_EXPORT(ASSISTANT_UI) ProactiveSuggestionsView
     : public views::Button,
       public views::ButtonListener,
-      public AssistantUiModelObserver,
-      public aura::WindowObserver {
+      public aura::WindowObserver,
+      public display::DisplayObserver,
+      public KeyboardControllerObserver {
  public:
   explicit ProactiveSuggestionsView(AssistantViewDelegate* delegate);
   ~ProactiveSuggestionsView() override;
@@ -39,13 +42,22 @@ class COMPONENT_EXPORT(ASSISTANT_UI) ProactiveSuggestionsView
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
-  // AssistantUiModelObserver:
-  void OnUsableWorkAreaChanged(const gfx::Rect& usable_work_area) override;
-
   // aura::WindowObserver:
   void OnWindowDestroying(aura::Window* window) override;
   void OnWindowVisibilityChanging(aura::Window* window, bool visible) override;
 
+  // display::DisplayObserver:
+  void OnDisplayMetricsChanged(const display::Display& display,
+                               uint32_t changed_metrics) override;
+
+  // KeyboardControllerObserver:
+  void OnKeyboardOccludedBoundsChanged(
+      const gfx::Rect& new_bounds_in_screen) override;
+
+  const ProactiveSuggestions* proactive_suggestions() const {
+    return proactive_suggestions_.get();
+  }
+
  private:
   void InitLayout();
   void InitWidget();
@@ -54,8 +66,15 @@ class COMPONENT_EXPORT(ASSISTANT_UI) ProactiveSuggestionsView
 
   AssistantViewDelegate* const delegate_;
 
+  // The set of proactive suggestions associated with this view.
+  scoped_refptr<const ProactiveSuggestions> proactive_suggestions_;
+
   views::ImageButton* close_button_;  // Owned by view hierarchy.
 
+  // Cached bounds of workspace occluded by the keyboard for determining usable
+  // work area in which to lay out this view's widget.
+  gfx::Rect keyboard_workspace_occluded_bounds_;
+
   DISALLOW_COPY_AND_ASSIGN(ProactiveSuggestionsView);
 };
 

ash/public/cpp/assistant/proactive_suggestions.cc

@@ -4,13 +4,33 @@
 
 #include "ash/public/cpp/assistant/proactive_suggestions.h"
 
+#include "base/hash/hash.h"
+
 namespace ash {
 
+namespace {
+
+// Helpers ---------------------------------------------------------------------
+
+size_t GetHash(int category,
+               const std::string& description,
+               const std::string& search_query,
+               const std::string& html) {
+  size_t hash = base::HashInts(category, base::FastHash(description));
+  hash = base::HashInts(hash, base::FastHash(search_query));
+  return base::HashInts(hash, base::FastHash(html));
+}
+
+}  // namespace
+
+// ProactiveSuggestions --------------------------------------------------------
+
 ProactiveSuggestions::ProactiveSuggestions(int category,
                                            std::string&& description,
                                            std::string&& search_query,
                                            std::string&& html)
-    : category_(category),
+    : hash_(GetHash(category, description, search_query, html)),
+      category_(category),
       description_(std::move(description)),
       search_query_(std::move(search_query)),
       html_(std::move(html)) {}
@@ -26,9 +46,8 @@ bool ProactiveSuggestions::AreEqual(const ProactiveSuggestions* a,
 // static
 size_t ProactiveSuggestions::ToHash(
     const ProactiveSuggestions* proactive_suggestions) {
-  return proactive_suggestions
-             ? std::hash<ProactiveSuggestions>{}(*proactive_suggestions)
-             : static_cast<size_t>(0);
+  return proactive_suggestions ? proactive_suggestions->hash()
+                               : static_cast<size_t>(0);
 }
 
 bool operator==(const ProactiveSuggestions& lhs,

ash/public/cpp/assistant/proactive_suggestions.h

@@ -8,7 +8,6 @@
 #include <string>
 
 #include "ash/public/cpp/ash_public_export.h"
-#include "base/hash/hash.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 
@@ -38,6 +37,7 @@ class ASH_PUBLIC_EXPORT ProactiveSuggestions
   // Returns a fast hash representation of the given |proactive_suggestions|.
   static size_t ToHash(const ProactiveSuggestions* proactive_suggestions);
 
+  size_t hash() const { return hash_; }
   int category() const { return category_; }
   const std::string& description() const { return description_; }
   const std::string& search_query() const { return search_query_; }
@@ -48,6 +48,7 @@ class ASH_PUBLIC_EXPORT ProactiveSuggestions
   friend class base::RefCounted<ProactiveSuggestions>;
   ~ProactiveSuggestions();
 
+  const size_t hash_;
   const int category_;
   const std::string description_;
   const std::string search_query_;
@@ -67,13 +68,7 @@ template <>
 struct hash<::ash::ProactiveSuggestions> {
   size_t operator()(
       const ::ash::ProactiveSuggestions& proactive_suggestions) const {
-    size_t description = base::FastHash(proactive_suggestions.description());
-    size_t search_query = base::FastHash(proactive_suggestions.search_query());
-    size_t html = base::FastHash(proactive_suggestions.html());
-
-    size_t hash = base::HashInts(proactive_suggestions.category(), description);
-    hash = base::HashInts(hash, search_query);
-    return base::HashInts(hash, html);
+    return proactive_suggestions.hash();
   }
 };
 

ash/public/cpp/assistant/proactive_suggestions_client.h

@@ -29,7 +29,7 @@ class ASH_PUBLIC_EXPORT ProactiveSuggestionsClient {
     // changed. Note that |proactive_suggestions| may be |nullptr| if none
     // exist.
     virtual void OnProactiveSuggestionsChanged(
-        scoped_refptr<ProactiveSuggestions> proactive_suggestions) {}
+        scoped_refptr<const ProactiveSuggestions> proactive_suggestions) {}
 
    protected:
     Delegate() = default;