assistant: position assistant window on top in tablet mode.

Bug: b/112321739
Test: ash_unittests --gtest_fiter=AssistantContainerViewTest.TabletModeAnchoring.
Change-Id: I283000679241bf33f4c0411632090d0cb3799aac
Reviewed-on: https://chromium-review.googlesource.com/1175131
Commit-Queue: Muyuan Li <muyuanli@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583379}

ash/assistant/assistant_controller.cc

@@ -239,6 +239,11 @@ void AssistantController::OpenUrl(const GURL& url) {
   NotifyUrlOpened(url);
 }
 
+void AssistantController::ShutDown() {
+  // Controllers can be added to handle this event as needed.
+  assistant_ui_controller_->ShutDown();
+}
+
 void AssistantController::NotifyConstructed() {
   for (AssistantControllerObserver& observer : observers_)
     observer.OnAssistantControllerConstructed();

ash/assistant/assistant_controller.h

@@ -122,6 +122,9 @@ class ASH_EXPORT AssistantController
   // TODO(dmblack): Support opening specific URLs in the Assistant container.
   void OpenUrl(const GURL& url);
 
+  // Called before dtor to deregister services and avoid life cycle issues.
+  void ShutDown();
+
   AssistantInteractionController* interaction_controller() {
     DCHECK(assistant_interaction_controller_);
     return assistant_interaction_controller_.get();

ash/assistant/assistant_ui_controller.cc

@@ -14,6 +14,7 @@
 #include "ash/system/toast/toast_data.h"
 #include "ash/system/toast/toast_manager.h"
 #include "ash/voice_interaction/voice_interaction_controller.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/optional.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -44,6 +45,7 @@ AssistantUiController::AssistantUiController(
   AddModelObserver(this);
   assistant_controller_->AddObserver(this);
   Shell::Get()->highlighter_controller()->AddObserver(this);
+  Shell::Get()->tablet_mode_controller()->AddObserver(this);
 }
 
 AssistantUiController::~AssistantUiController() {
@@ -263,6 +265,20 @@ void AssistantUiController::ToggleUi(AssistantSource source) {
     ShowUi(source);
 }
 
+void AssistantUiController::OnTabletModeStarted() {
+  if (container_view_)
+    container_view_->OnTabletModeChanged();
+}
+
+void AssistantUiController::OnTabletModeEnded() {
+  if (container_view_)
+    container_view_->OnTabletModeChanged();
+}
+
+void AssistantUiController::ShutDown() {
+  Shell::Get()->tablet_mode_controller()->RemoveObserver(this);
+}
+
 void AssistantUiController::UpdateUiMode(
     base::Optional<AssistantUiMode> ui_mode) {
   // If a UI mode is provided, we will use it in lieu of updating UI mode on the

ash/assistant/assistant_ui_controller.h

@@ -15,6 +15,7 @@
 #include "ash/assistant/ui/caption_bar.h"
 #include "ash/assistant/ui/dialog_plate/dialog_plate.h"
 #include "ash/highlighter/highlighter_controller.h"
+#include "ash/wm/tablet_mode/tablet_mode_observer.h"
 #include "base/macros.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -44,7 +45,8 @@ class ASH_EXPORT AssistantUiController
       public AssistantMiniViewDelegate,
       public CaptionBarDelegate,
       public DialogPlateObserver,
-      public HighlighterController::Observer {
+      public HighlighterController::Observer,
+      public TabletModeObserver {
  public:
   explicit AssistantUiController(AssistantController* assistant_controller);
   ~AssistantUiController() override;
@@ -100,6 +102,13 @@ class ASH_EXPORT AssistantUiController
   void HideUi(AssistantSource source);
   void ToggleUi(AssistantSource source);
 
+  // TabletModeObserver:
+  void OnTabletModeStarted() override;
+  void OnTabletModeEnded() override;
+
+  // Called before dtor to clean up service dependencies.
+  void ShutDown();
+
   AssistantContainerView* GetViewForTest();
 
  private:

ash/assistant/ui/assistant_container_view.cc

@@ -4,6 +4,7 @@
 
 #include "ash/assistant/ui/assistant_container_view.h"
 
+#include <iostream>
 #include <memory>
 
 #include "ash/assistant/assistant_controller.h"
@@ -14,6 +15,7 @@
 #include "ash/assistant/ui/assistant_ui_constants.h"
 #include "ash/assistant/ui/assistant_web_view.h"
 #include "ash/shell.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
@@ -28,10 +30,12 @@ namespace ash {
 
 namespace {
 
+constexpr int kClamshellMarginBottomDip = 8;  // Margin in clamshell mode.
+constexpr int kTabletMarginTopDip = 24;       // Margin in tablet mode.
+
 // Appearance.
 constexpr SkColor kBackgroundColor = SK_ColorWHITE;
 constexpr int kCornerRadiusDip = 20;
-constexpr int kMarginDip = 8;
 
 // Animation.
 constexpr int kResizeAnimationDurationMs = 250;
@@ -245,13 +249,21 @@ void AssistantContainerView::SetAnchor(aura::Window* root_window) {
   display::Display display = display::Screen::GetScreen()->GetDisplayMatching(
       root_window->GetBoundsInScreen());
 
+  bool tablet_mode = Shell::Get()
+                         ->tablet_mode_controller()
+                         ->IsTabletModeWindowManagerEnabled();
+
   // Anchor to the bottom center of the work area.
   gfx::Rect work_area = display.work_area();
-  gfx::Rect anchor = gfx::Rect(work_area.x(), work_area.bottom() - kMarginDip,
-                               work_area.width(), 0);
+  gfx::Rect anchor =
+      gfx::Rect(work_area.x(),
+                tablet_mode ? kTabletMarginTopDip
+                            : work_area.bottom() - kClamshellMarginBottomDip,
+                work_area.width(), 0);
 
   SetAnchorRect(anchor);
-  set_arrow(views::BubbleBorder::Arrow::BOTTOM_CENTER);
+  set_arrow(tablet_mode ? views::BubbleBorder::Arrow::TOP_CENTER
+                        : views::BubbleBorder::Arrow::BOTTOM_CENTER);
 }
 
 void AssistantContainerView::OnUiModeChanged(AssistantUiMode ui_mode) {
@@ -297,13 +309,24 @@ void AssistantContainerView::AnimationProgressed(
   // Use our interpolated size.
   bounds.set_size(gfx::Size(size.width(), size.height()));
 
-  // Maintain our original |bottom| and |center_x| positions.
+  if (!Shell::Get()
+           ->tablet_mode_controller()
+           ->IsTabletModeWindowManagerEnabled()) {
+    // Maintain our original |bottom| positions in clamshell mode. In tablet
+    // mode, they should grow downwards, which is the default behavior.
+    bounds.set_y(bottom - bounds.height());
+  }
+  // Maintain |center_x| position.
   bounds.set_x(center_x - (bounds.width() / 2));
-  bounds.set_y(bottom - bounds.height());
 
   GetWidget()->SetBounds(bounds);
 }
 
+void AssistantContainerView::OnTabletModeChanged() {
+  DCHECK(GetWidget());
+  SetAnchor(GetWidget()->GetNativeWindow()->GetRootWindow());
+}
+
 void AssistantContainerView::OnDisplayMetricsChanged(
     const display::Display& display,
     uint32_t changed_metrics) {

ash/assistant/ui/assistant_container_view.h

@@ -58,6 +58,8 @@ class AssistantContainerView : public views::BubbleDialogDelegateView,
   void OnDisplayMetricsChanged(const display::Display& display,
                                uint32_t changed_metrics) override;
 
+  void OnTabletModeChanged();
+
  private:
   // Sets anchor rect to |root_window|. If it's null,
   // result of GetRootWindowForNewWindows() will be used.

ash/assistant/ui/assistant_container_view_unittest.cc

@@ -9,6 +9,7 @@
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/voice_interaction/voice_interaction_controller.h"
+#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
 #include "chromeos/chromeos_switches.h"
@@ -19,6 +20,9 @@ namespace ash {
 
 namespace {
 
+constexpr int kClamshellMarginBottomDip = 8;  // Margin in clamshell mode.
+constexpr int kTabletMarginTopDip = 24;       // Margin in tablet mode.
+
 class AssistantContainerViewTest : public AshTestBase {
  public:
   AssistantContainerViewTest() = default;
@@ -103,16 +107,41 @@ TEST_F(AssistantContainerViewTest, InitialAnchoring) {
               Shell::Get()->GetRootWindowForNewWindows()->GetBoundsInScreen())
           .work_area();
 
-  // We expect a bottom margin.
-  constexpr int bottom_margin = 8;
-
   // We expect the view to be horizontally centered and bottom aligned.
   gfx::Rect expected_bounds = gfx::Rect(expected_work_area);
   expected_bounds.ClampToCenteredSize(view->size());
   expected_bounds.set_y(expected_work_area.bottom() - view->height() -
-                        bottom_margin);
+                        kClamshellMarginBottomDip);
 
   ASSERT_EQ(expected_bounds, view->GetBoundsInScreen());
 }
 
+TEST_F(AssistantContainerViewTest, TabletModeAnchoring) {
+  // Guarantee short but non-zero duration for animations.
+  ui::ScopedAnimationDurationScaleMode scoped_animation_duration(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+
+  gfx::Rect expected_work_area =
+      display::Screen::GetScreen()
+          ->GetDisplayMatching(
+              Shell::Get()->GetRootWindowForNewWindows()->GetBoundsInScreen())
+          .work_area();
+
+  int clamshell_y = expected_work_area.bottom() - kClamshellMarginBottomDip;
+
+  ui_controller()->ShowUi(AssistantSource::kUnspecified);
+  AssistantContainerView* view = ui_controller()->GetViewForTest();
+
+  gfx::Rect bounds = view->GetBoundsInScreen();
+  ASSERT_EQ(bounds.bottom(), clamshell_y);
+
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
+  bounds = view->GetBoundsInScreen();
+  ASSERT_EQ(bounds.y(), kTabletMarginTopDip);
+
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(false);
+  bounds = view->GetBoundsInScreen();
+  ASSERT_EQ(bounds.bottom(), clamshell_y);
+}
+
 }  // namespace ash

ash/shell.cc

@@ -790,6 +790,11 @@ Shell::~Shell() {
   // the former may use the latter before destruction.
   app_list_controller_.reset();
 
+  // Shutdown |assistant_controller_| to properly remove observer on
+  // |tablet_mode_controller_|.
+  if (chromeos::switches::IsAssistantEnabled())
+    assistant_controller_->ShutDown();
+
   // Destroy tablet mode controller early on since it has some observers which
   // need to be removed.
   tablet_mode_controller_.reset();