Commander views layer

This is responsible for:
- Mediating between the controller and WebUI layers.
- Managing the widget the WebUI interface lives in.
- Holding the web view the WebUI interface lives in. This is necessary
  because creating a renderer on demand caused unacceptable latency in
  the commander prototype.

Reviewer note: Please note that there are 5 separate paths to close
- Dismissed by controller via ViewModel
- Dismissed by browser via Hide
- Dismissed by WebUI via OnDismiss
- Window closed (possibly from parent window closing)
- Object destroyed
I'm pretty sure they're all safe, but this part definitely needs a
second pair of eyes.

Bug: 1014639
Change-Id: I422754923a0be3ce3e2b7b6a500add76f7e5ecd6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2364225Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800609}

chrome/browser/ui/BUILD.gn

@@ -920,6 +920,7 @@ static_library("ui") {
       "commander/commander_backend.h",
       "commander/commander_controller.cc",
       "commander/commander_controller.h",
+      "commander/commander_frontend.h",
       "commander/commander_view_model.cc",
       "commander/commander_view_model.h",
       "commander/fuzzy_finder.cc",
@@ -3359,6 +3360,8 @@ static_library("ui") {
       "views/chrome_web_dialog_view.h",
       "views/collected_cookies_views.cc",
       "views/collected_cookies_views.h",
+      "views/commander_frontend_views.cc",
+      "views/commander_frontend_views.h",
       "views/confirm_bubble_views.cc",
       "views/confirm_bubble_views.h",
       "views/constrained_web_dialog_delegate_views.cc",

chrome/browser/ui/commander/commander_backend.h

@@ -37,6 +37,9 @@ class CommanderBackend {
   // synchronous with user input, since some command sources may be async or
   // provide incremental results.
   virtual void SetUpdateCallback(ViewModelUpdateCallback callback) = 0;
+  // Called when the UI layer is closed. This is a hook to allow the backend
+  // to release any bound state.
+  virtual void Reset() {}
 };
 
 }  // namespace commander

chrome/browser/ui/commander/commander_controller.cc

@@ -71,13 +71,14 @@ void CommanderController::OnCommandSelected(size_t command_index,
 
   CommandItem* item = current_items_[command_index].get();
   if (item->GetType() == CommandItem::Type::kOneShot) {
+    base::OnceClosure command = std::move(*(item->command));
     // Dismiss the view.
     CommanderViewModel vm;
     vm.result_set_id = ++current_result_set_id_;
     vm.action = CommanderViewModel::Action::kClose;
     callback_.Run(vm);
 
-    std::move(*(item->command)).Run();
+    std::move(command).Run();
   } else {
     delegate_ = std::move(*(item->delegate_factory)).Run();
     // base::Unretained is safe since we own |delegate_|.
@@ -96,6 +97,10 @@ void CommanderController::SetUpdateCallback(ViewModelUpdateCallback callback) {
   callback_ = std::move(callback);
 }
 
+void CommanderController::Reset() {
+  current_items_.clear();
+}
+
 // static
 std::unique_ptr<CommanderController>
 CommanderController::CreateWithSourcesForTesting(CommandSources sources) {

chrome/browser/ui/commander/commander_controller.h

@@ -29,6 +29,7 @@ class CommanderController : public CommanderBackend {
   void OnTextChanged(const base::string16& text, Browser* browser) override;
   void OnCommandSelected(size_t command_index, int result_set_id) override;
   void SetUpdateCallback(ViewModelUpdateCallback callback) override;
+  void Reset() override;
 
   static std::unique_ptr<CommanderController> CreateWithSourcesForTesting(
       CommandSources sources);

chrome/browser/ui/commander/commander_frontend.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 CHROME_BROWSER_UI_COMMANDER_COMMANDER_FRONTEND_H_
+#define CHROME_BROWSER_UI_COMMANDER_COMMANDER_FRONTEND_H_
+
+#include <memory>
+
+#include "base/strings/string16.h"
+
+class Browser;
+
+namespace commander {
+
+class CommanderBackend;
+
+// Abstract interface for the commander UI.
+class CommanderFrontend {
+ public:
+  CommanderFrontend() = default;
+  virtual ~CommanderFrontend() = default;
+
+  // Show the UI, anchored to |browser|'s window.
+  virtual void Show(Browser* browser) = 0;
+  // Hide the UI, if showing.
+  virtual void Hide() = 0;
+
+  static std::unique_ptr<CommanderFrontend> Create(CommanderBackend* backend);
+
+  // Disallow copy and assign
+  CommanderFrontend(const CommanderFrontend& other) = delete;
+  CommanderFrontend& operator=(const CommanderFrontend& other) = delete;
+};
+
+}  // namespace commander
+
+#endif  // CHROME_BROWSER_UI_COMMANDER_COMMANDER_FRONTEND_H_

chrome/browser/ui/views/commander_frontend_views.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 "chrome/browser/ui/views/commander_frontend_views.h"
+
+#include "base/bind.h"
+#include "build/build_config.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/ui/commander/commander_backend.h"
+#include "chrome/browser/ui/commander/commander_view_model.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/theme_copying_widget.h"
+#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
+#include "ui/views/controls/webview/webview.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_delegate.h"
+
+#include "base/debug/stack_trace.h"
+
+namespace {
+// TODO(lgrey): Temporary
+constexpr gfx::Size kDefaultSize(400, 30);
+}  // namespace
+
+// A small shim to handle passing keyboard events back up to the browser.
+// Required for hotkeys to work.
+class CommanderWebView : public views::WebView {
+ public:
+  explicit CommanderWebView(content::BrowserContext* context)
+      : views::WebView(context) {}
+  bool HandleKeyboardEvent(
+      content::WebContents* source,
+      const content::NativeWebKeyboardEvent& event) override {
+    CHECK(owner_);
+    return event_handler_.HandleKeyboardEvent(event, owner_->GetFocusManager());
+  }
+
+  void set_owner(views::View* owner) { owner_ = owner; }
+
+ private:
+  views::UnhandledKeyboardEventHandler event_handler_;
+  views::View* owner_;
+};
+
+CommanderFrontendViews::CommanderFrontendViews(
+    commander::CommanderBackend* backend)
+    : backend_(backend) {
+  widget_delegate_ = std::make_unique<views::WidgetDelegate>();
+  widget_delegate_->SetCanActivate(true);
+  widget_delegate_->RegisterWindowClosingCallback(
+      base::BindRepeating(&CommanderFrontendViews::OnWindowClosing,
+                          weak_ptr_factory_.GetWeakPtr()));
+
+  backend_->SetUpdateCallback(
+      base::BindRepeating(&CommanderFrontendViews::OnViewModelUpdated,
+                          weak_ptr_factory_.GetWeakPtr()));
+
+#if !defined(OS_CHROMEOS)
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  profile_manager->CreateProfileAsync(
+      ProfileManager::GetSystemProfilePath(),
+      base::BindRepeating(&CommanderFrontendViews::OnSystemProfileAvailable,
+                          weak_ptr_factory_.GetWeakPtr()),
+      base::string16(), std::string());
+#else
+  // TODO(lgrey): ChromeOS doesn't have a system profile. Need to find
+  // a better way to do this before Commander is hooked up, but doing
+  // this for now to unblock.
+  CreateWebView(ProfileManager::GetPrimaryUserProfile());
+#endif
+}
+
+CommanderFrontendViews::~CommanderFrontendViews() {
+  backend_->SetUpdateCallback(base::DoNothing());
+  if (widget_)
+    widget_->CloseNow();
+}
+
+void CommanderFrontendViews::Show(Browser* browser) {
+  if (!is_web_view_created()) {
+    browser_ = browser;
+    show_requested_ = true;
+    return;
+  }
+  DCHECK(!is_showing());
+  show_requested_ = false;
+  browser_ = browser;
+  views::View* parent = BrowserView::GetBrowserViewForBrowser(browser_);
+
+  widget_ = new ThemeCopyingWidget(parent->GetWidget());
+  views::Widget::InitParams params(
+      views::Widget::InitParams::TYPE_WINDOW_FRAMELESS);
+  params.delegate = widget_delegate_.get();
+  params.name = "Commander";
+  params.parent = parent->GetWidget()->GetNativeView();
+  widget_->Init(std::move(params));
+
+  web_view_->set_owner(parent);
+  web_view_->SetSize(kDefaultSize);
+  web_view_ptr_ = widget_->SetContentsView(std::move(web_view_));
+
+  widget_->CenterWindow(kDefaultSize);
+  widget_->Show();
+
+  web_view_ptr_->RequestFocus();
+  web_view_ptr_->GetWebContents()->Focus();
+}
+
+void CommanderFrontendViews::Hide() {
+  DCHECK(is_showing());
+  widget_->Close();
+}
+
+void CommanderFrontendViews::OnWindowClosing() {
+  DCHECK(is_showing());
+  backend_->Reset();
+  web_view_ = widget_->GetRootView()->RemoveChildViewT(web_view_ptr_);
+  web_view_->set_owner(nullptr);
+  show_requested_ = false;
+  browser_ = nullptr;
+  widget_ = nullptr;
+}
+
+void CommanderFrontendViews::OnTextChanged(const base::string16& text) {
+  DCHECK(is_showing());
+  backend_->OnTextChanged(text, browser_);
+}
+
+void CommanderFrontendViews::OnOptionSelected(size_t option_index,
+                                              int result_set_id) {
+  DCHECK(is_showing());
+  backend_->OnCommandSelected(option_index, result_set_id);
+}
+
+void CommanderFrontendViews::OnDismiss() {
+  Hide();
+}
+
+void CommanderFrontendViews::OnHeightChanged(int new_height) {
+  DCHECK(is_showing());
+  gfx::Size size = kDefaultSize;
+  size.set_height(new_height);
+  widget_->SetSize(size);
+  web_view_ptr_->SetSize(size);
+}
+
+void CommanderFrontendViews::OnViewModelUpdated(
+    commander::CommanderViewModel view_model) {
+  DCHECK(is_showing());
+  if (view_model.action == commander::CommanderViewModel::Action::kClose) {
+    Hide();
+    return;
+  }
+  // TODO(lgrey): Pass view model to WebUI.
+}
+
+void CommanderFrontendViews::OnSystemProfileAvailable(
+    Profile* profile,
+    Profile::CreateStatus status) {
+  if (status == Profile::CreateStatus::CREATE_STATUS_CREATED && !is_showing())
+    CreateWebView(profile);
+}
+
+void CommanderFrontendViews::CreateWebView(Profile* profile) {
+  DCHECK(!is_web_view_created());
+
+  web_view_ = std::make_unique<CommanderWebView>(profile);
+  web_view_->set_allow_accelerators(true);
+  if (show_requested_)
+    Show(browser_);
+}
+
+// static
+std::unique_ptr<commander::CommanderFrontend>
+commander::CommanderFrontend::Create(commander::CommanderBackend* backend) {
+  return std::make_unique<CommanderFrontendViews>(backend);
+}

chrome/browser/ui/views/commander_frontend_views.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 CHROME_BROWSER_UI_VIEWS_COMMANDER_FRONTEND_VIEWS_H_
+#define CHROME_BROWSER_UI_VIEWS_COMMANDER_FRONTEND_VIEWS_H_
+
+#include "chrome/browser/ui/commander/commander_frontend.h"
+
+#include <memory>
+
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/profiles/profile_manager.h"
+
+class CommanderWebView;
+
+namespace views {
+class WidgetDelegate;
+class Widget;
+}  // namespace views
+
+namespace commander {
+class CommanderBackend;
+struct CommanderViewModel;
+}  // namespace commander
+
+// Views implementation of commander::CommanderFrontend. The actual UI is WebUI;
+// this class is responsible for setting up the infrastructure to host the
+// WebUI in its own widget and mediating between the WebUI implementation and
+// the controller.
+class CommanderFrontendViews : public commander::CommanderFrontend {
+ public:
+  explicit CommanderFrontendViews(commander::CommanderBackend* backend);
+  ~CommanderFrontendViews() override;
+
+  // commander::CommanderFrontend overrides
+  void Show(Browser* browser) override;
+  void Hide() override;
+
+  // TODO(lgrey): When the WebUI layer is added, these declarations should
+  // be moved to the CommanderHandler::Delegate interface.
+  // Called when the text is changed in the WebUI interface.
+  void OnTextChanged(const base::string16& text);
+  // Called when an option is selected (clicked or enter pressed) in the WebUI
+  // interface.
+  void OnOptionSelected(size_t option_index, int result_set_id);
+  // Called when the WebUI interface wants to dismiss the UI.
+  void OnDismiss();
+  // Called when the WebUI interface's content height has changed.
+  void OnHeightChanged(int new_height);
+
+ private:
+  // Receives view model updates from |backend_|.
+  void OnViewModelUpdated(commander::CommanderViewModel view_model);
+  // Receives system profile from CreateProfileAsync()
+  void OnSystemProfileAvailable(Profile* profile, Profile::CreateStatus status);
+
+  // Creates a web_view_ to host the WebUI interface for |profile|. Should only
+  // be called once when the system profile becomes available.
+  void CreateWebView(Profile* profile);
+
+  // Called by widget delegate when widget closes.
+  void OnWindowClosing();
+
+  bool is_showing() { return widget_ != nullptr; }
+  bool is_web_view_created() {
+    return is_showing() || web_view_.get() != nullptr;
+  }
+
+  // The controller. Must outlive this object.
+  commander::CommanderBackend* backend_;
+  // True if Show() was called before the system profile is available.
+  // If this is set, CreateWebView() will call Show().
+  bool show_requested_ = false;
+  // The widget that contains |web_view_ptr_|
+  views::Widget* widget_ = nullptr;
+  // |widget_|'s delegate
+  std::unique_ptr<views::WidgetDelegate> widget_delegate_;
+  // The web_view_ that hosts the WebUI instance. Populated when the view
+  // is showing and null otherwise.
+  CommanderWebView* web_view_ptr_ = nullptr;
+  // |web_view_ptr_| is held here when the widget is *not* showing.
+  std::unique_ptr<CommanderWebView> web_view_;
+  // The browser |widget_| is attached to.
+  Browser* browser_;
+
+  base::WeakPtrFactory<CommanderFrontendViews> weak_ptr_factory_{this};
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_COMMANDER_FRONTEND_VIEWS_H_

chrome/browser/ui/views/commander_frontend_views_browsertest.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 "chrome/browser/ui/views/commander_frontend_views.h"
+
+#include "base/macros.h"
+#include "chrome/browser/ui/commander/commander_backend.h"
+#include "chrome/browser/ui/commander/commander_view_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/test/browser_test.h"
+#include "ui/views/test/widget_test.h"
+#include "ui/views/widget/any_widget_observer.h"
+#include "ui/views/widget/widget.h"
+
+class CommanderFrontendViewsTest : public InProcessBrowserTest {
+ public:
+  // TODO(lgrey): This is copied over from CommanderControllerUnittest, with
+  // modifications. If we need it one more time, extract to a common file.
+  class TestBackend : public commander::CommanderBackend {
+   public:
+    void OnTextChanged(const base::string16& text, Browser* browser) override {
+      text_changed_invocations_.push_back(text);
+    }
+    void OnCommandSelected(size_t command_index, int result_set_id) override {
+      command_selected_invocations_.push_back(command_index);
+    }
+    void SetUpdateCallback(commander::CommanderBackend::ViewModelUpdateCallback
+                               callback) override {
+      callback_ = std::move(callback);
+    }
+    void Reset() override { reset_invocation_count_++; }
+
+    void CallCallback() {
+      commander::CommanderViewModel vm;
+      CallCallback(vm);
+    }
+
+    void CallCallback(commander::CommanderViewModel vm) { callback_.Run(vm); }
+
+    const std::vector<base::string16> text_changed_invocations() {
+      return text_changed_invocations_;
+    }
+    const std::vector<size_t> command_selected_invocations() {
+      return command_selected_invocations_;
+    }
+
+    int reset_invocation_count() { return reset_invocation_count_; }
+
+   private:
+    commander::CommanderBackend::ViewModelUpdateCallback callback_;
+    std::vector<base::string16> text_changed_invocations_;
+    std::vector<size_t> command_selected_invocations_;
+    int reset_invocation_count_ = 0;
+  };
+
+ protected:
+  std::unique_ptr<TestBackend> backend_;
+
+ private:
+  void SetUpOnMainThread() override {
+    backend_ = std::make_unique<TestBackend>();
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, ShowShowsWidget) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+  frontend->Show(browser());
+  views::Widget* commander_widget = waiter.WaitIfNeededAndGet();
+  EXPECT_TRUE(commander_widget);
+}
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, HideHidesWidget) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+  frontend->Show(browser());
+  views::Widget* commander_widget = waiter.WaitIfNeededAndGet();
+  EXPECT_EQ(backend_->reset_invocation_count(), 0);
+
+  views::test::WidgetDestroyedWaiter destroyed_waiter(commander_widget);
+  frontend->Hide();
+  destroyed_waiter.Wait();
+  EXPECT_EQ(backend_->reset_invocation_count(), 1);
+}
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, DismissHidesWidget) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+  frontend->Show(browser());
+  views::Widget* commander_widget = waiter.WaitIfNeededAndGet();
+  EXPECT_EQ(backend_->reset_invocation_count(), 0);
+
+  views::test::WidgetDestroyedWaiter destroyed_waiter(commander_widget);
+  frontend->OnDismiss();
+  destroyed_waiter.Wait();
+  EXPECT_EQ(backend_->reset_invocation_count(), 1);
+}
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, ViewModelCloseHidesWidget) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+  frontend->Show(browser());
+  views::Widget* commander_widget = waiter.WaitIfNeededAndGet();
+  EXPECT_EQ(backend_->reset_invocation_count(), 0);
+
+  views::test::WidgetDestroyedWaiter destroyed_waiter(commander_widget);
+  commander::CommanderViewModel vm;
+  vm.action = commander::CommanderViewModel::Action::kClose;
+  backend_->CallCallback(vm);
+  destroyed_waiter.Wait();
+  EXPECT_EQ(backend_->reset_invocation_count(), 1);
+}
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, OnHeightChangedSizesWidget) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+  frontend->Show(browser());
+  views::Widget* commander_widget = waiter.WaitIfNeededAndGet();
+  int old_height = commander_widget->GetRootView()->height();
+  int new_height = 200;
+  // Ensure changing height isn't a no-op.
+  EXPECT_NE(old_height, new_height);
+
+  frontend->OnHeightChanged(200);
+  EXPECT_EQ(new_height, commander_widget->GetRootView()->height());
+}
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, PassesOnOptionSelected) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+  frontend->Show(browser());
+  ignore_result(waiter.WaitIfNeededAndGet());
+
+  frontend->OnOptionSelected(8, 13);
+  ASSERT_EQ(backend_->command_selected_invocations().size(), 1u);
+  EXPECT_EQ(backend_->command_selected_invocations().back(), 8u);
+}
+
+IN_PROC_BROWSER_TEST_F(CommanderFrontendViewsTest, PassesOnTextChanged) {
+  auto frontend = std::make_unique<CommanderFrontendViews>(backend_.get());
+  const base::string16 input = base::ASCIIToUTF16("orange");
+  views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey(),
+                                       "Commander");
+
+  frontend->Show(browser());
+  ignore_result(waiter.WaitIfNeededAndGet());
+
+  frontend->OnTextChanged(input);
+  ASSERT_EQ(backend_->text_changed_invocations().size(), 1u);
+  EXPECT_EQ(backend_->text_changed_invocations().back(), input);
+}

chrome/test/BUILD.gn

@@ -6207,6 +6207,7 @@ if (!is_android) {
         "../browser/ui/views/bookmarks/bookmark_bar_view_test.cc",
         "../browser/ui/views/bookmarks/bookmark_bar_view_test_helper.h",
         "../browser/ui/views/certificate_selector_browsertest.cc",
+        "../browser/ui/views/commander_frontend_views_browsertest.cc",
         "../browser/ui/views/constrained_window_views_browsertest.cc",
         "../browser/ui/views/exclusive_access_bubble_views_interactive_uitest.cc",
         "../browser/ui/views/extensions/extension_dialog_interactive_uitest.cc",