Commander: refactor composite commands in backend

The original design for this was that when the user selected a
composite command, a specialized CommanderBackend object was
created, and CommanderController would delegate to it to allow
the user to enter details.

In practice the impedance was wrong. We need extra information
for the composite command (how to prompt the user), *don't*
need a lot of other things in the CommanderBackend interface,
need to duplicate logic re: command execution and risk lifetime
issues with nested composite commands.

In this change, composite commands are represented by a pair
of prompt text and a lambda with a signature similar to
CommandSource::GetCommands. All other logic is is handled by
the controller. For a worked example, see the dependent CL
https://chromium-review.googlesource.com/c/chromium/src/+/2535677

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

chrome/browser/ui/commander/command_source.cc

@@ -12,8 +12,7 @@ CommandItem::CommandItem(CommandItem&& other) = default;
 CommandItem& CommandItem::operator=(CommandItem&& other) = default;
 
 CommandItem::Type CommandItem::GetType() {
-  DCHECK(!command || !delegate_factory);
-  if (delegate_factory)
+  if (absl::get_if<CompositeCommand>(&command))
     return kComposite;
   return kOneShot;
 }

chrome/browser/ui/commander/command_source.h

@@ -6,14 +6,32 @@
 #define CHROME_BROWSER_UI_COMMANDER_COMMAND_SOURCE_H_
 
 #include "base/callback.h"
-#include "base/optional.h"
+#include "third_party/abseil-cpp/absl/types/variant.h"
 #include "ui/gfx/range/range.h"
 
 class Browser;
 
 namespace commander {
 
-class CommanderBackend;
+struct CommandItem;
+
+// Provides and ranks available commands in response to user input. The
+// intention is for every system available through the commander to
+// provide its own source, which is responsible for tracking the state and
+// context necessary to provide appropriate commands from that system.
+class CommandSource {
+ public:
+  using CommandResults = std::vector<std::unique_ptr<CommandItem>>;
+  CommandSource() = default;
+  virtual ~CommandSource() = default;
+
+  // Returns a list of scored commands to return for |input|, or an empty
+  // list if none are appropriate. The commands are not guaranteed to be in
+  // any particular order. |browser| is the browser the active commander
+  // is attached to.
+  virtual CommandResults GetCommands(const base::string16& input,
+                                     Browser* browser) const = 0;
+};
 
 // Represents a single option that can be presented in the command palette.
 struct CommandItem {
@@ -36,6 +54,12 @@ struct CommandItem {
     kTab,
     kWindow,
   };
+
+  using CompositeCommandProvider =
+      base::RepeatingCallback<CommandSource::CommandResults(
+          const base::string16&)>;
+  using CompositeCommand = std::pair<base::string16, CompositeCommandProvider>;
+
   CommandItem();
   virtual ~CommandItem();
   CommandItem(const CommandItem& other) = delete;
@@ -51,13 +75,10 @@ struct CommandItem {
   // Optional secondary text for the command. Typically used to display a
   // hotkey.
   base::string16 annotation;
-  // The code to execute if the user selects this option, if it's a one-shot.
-  // Must be unset if |delegate_factory| is set.
-  base::Optional<base::OnceClosure> command;
-  // If the user selects this option and further input is required,
-  // this creates an item-specific backend to handle the rest of the command.
-  base::Optional<base::OnceCallback<std::unique_ptr<CommanderBackend>()>>
-      delegate_factory;
+  // If this command is a one-shot, executes the command. If this command is
+  // composite, provides the prompt text sent to the user, and a
+  // CompositeCommandProvider to handle additional user input.
+  absl::variant<base::OnceClosure, CompositeCommand> command;
   // How relevant the item is to user input. Expected range is (0,1], with 1
   // indicating a perfect match (in the absence of other criteria, this boils
   // down to an exact string match).
@@ -70,24 +91,6 @@ struct CommandItem {
   std::vector<gfx::Range> matched_ranges;
 };
 
-// Provides and ranks available commands in response to user input. The
-// intention is for every system available through the commander to
-// provide its own source, which is responsible for tracking the state and
-// context necessary to provide appropriate commands from that system.
-class CommandSource {
- public:
-  using CommandResults = std::vector<std::unique_ptr<CommandItem>>;
-  CommandSource() = default;
-  virtual ~CommandSource() = default;
-
-  // Returns a list of scored commands to return for |input|, or an empty
-  // list if none are appropriate. The commands are not guaranteed to be in
-  // any particular order. |browser| is the browser the active commander
-  // is attached to.
-  virtual CommandResults GetCommands(const base::string16& input,
-                                     Browser* browser) const = 0;
-};
-
 }  // namespace commander
 
 #endif  // CHROME_BROWSER_UI_COMMANDER_COMMAND_SOURCE_H_

chrome/browser/ui/commander/commander_controller.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/commander/apps_command_source.h"
 #include "chrome/browser/ui/commander/commander_view_model.h"
 #include "chrome/browser/ui/commander/simple_command_source.h"
+#include "third_party/abseil-cpp/absl/types/variant.h"
 
 namespace commander {
 
@@ -36,22 +37,17 @@ CommanderController::CommanderController(CommandSources sources)
 
 CommanderController::~CommanderController() = default;
 
-void CommanderController::OnDelegateViewModelCallback(
-    CommanderViewModel view_model) {
-  view_model.result_set_id = ++current_result_set_id_;
-  callback_.Run(view_model);
-}
-
 void CommanderController::OnTextChanged(const base::string16& text,
                                         Browser* browser) {
-  if (delegate_.get())
-    return delegate_->OnTextChanged(text, browser);
-
   std::vector<std::unique_ptr<CommandItem>> items;
-  for (auto& source : sources_) {
-    auto commands = source->GetCommands(text, browser);
-    items.insert(items.end(), std::make_move_iterator(commands.begin()),
-                 std::make_move_iterator(commands.end()));
+  if (composite_command_provider_) {
+    items = composite_command_provider_.Run(text);
+  } else {
+    for (auto& source : sources_) {
+      auto commands = source->GetCommands(text, browser);
+      items.insert(items.end(), std::make_move_iterator(commands.begin()),
+                   std::make_move_iterator(commands.end()));
+    }
   }
 
   // Just sort for now.
@@ -74,15 +70,14 @@ void CommanderController::OnTextChanged(const base::string16& text,
 
 void CommanderController::OnCommandSelected(size_t command_index,
                                             int result_set_id) {
-  if (delegate_.get())
-    return delegate_->OnCommandSelected(command_index, result_set_id);
   if (command_index >= current_items_.size() ||
       result_set_id != current_result_set_id_)
     return;
 
   CommandItem* item = current_items_[command_index].get();
   if (item->GetType() == CommandItem::Type::kOneShot) {
-    base::OnceClosure command = std::move(*(item->command));
+    base::OnceClosure command =
+        std::move(absl::get<base::OnceClosure>(item->command));
     // Dismiss the view.
     CommanderViewModel vm;
     vm.result_set_id = ++current_result_set_id_;
@@ -91,22 +86,21 @@ void CommanderController::OnCommandSelected(size_t command_index,
 
     std::move(command).Run();
   } else {
-    delegate_ = std::move(*(item->delegate_factory)).Run();
-    // base::Unretained is safe since we own |delegate_|.
-    delegate_->SetUpdateCallback(
-        base::BindRepeating(&CommanderController::OnDelegateViewModelCallback,
-                            base::Unretained(this)));
+    CommandItem::CompositeCommand command =
+        absl::get<CommandItem::CompositeCommand>(item->command);
+    composite_command_provider_ = command.second;
     // Tell the view to requery.
     CommanderViewModel vm;
     vm.result_set_id = ++current_result_set_id_;
     vm.action = CommanderViewModel::Action::kPrompt;
+    vm.prompt_text = command.first;
     callback_.Run(vm);
   }
 }
 
 void CommanderController::OnCompositeCommandCancelled() {
-  DCHECK(delegate_);
-  delegate_.reset();
+  DCHECK(composite_command_provider_);
+  composite_command_provider_.Reset();
 }
 
 void CommanderController::SetUpdateCallback(ViewModelUpdateCallback callback) {
@@ -115,6 +109,8 @@ void CommanderController::SetUpdateCallback(ViewModelUpdateCallback callback) {
 
 void CommanderController::Reset() {
   current_items_.clear();
+  if (composite_command_provider_)
+    composite_command_provider_.Reset();
 }
 
 // static

chrome/browser/ui/commander/commander_controller.h

@@ -37,16 +37,12 @@ class CommanderController : public CommanderBackend {
 
  private:
   explicit CommanderController(CommandSources sources);
-  // If a delegate is installed, this serves as the delegate's view model
-  // callback, allowing the controller to decorate or mutate the view model
-  // before passing it back to the view, if necessary.
-  void OnDelegateViewModelCallback(CommanderViewModel view_model);
 
   std::vector<std::unique_ptr<CommandItem>> current_items_;
   int current_result_set_id_;
   CommandSources sources_;
   ViewModelUpdateCallback callback_;
-  std::unique_ptr<CommanderBackend> delegate_;
+  CommandItem::CompositeCommandProvider composite_command_provider_;
 };
 
 }  // namespace commander