Changes for PR

brave · Feb 7, 2023 · 84cba7f · 84cba7f
1 parent 1f9437d
commit 84cba7f
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 18 deletions.
diff --git a/browser/ui/commander/command_centre.cc b/browser/ui/commander/command_centre.cc
@@ -19,7 +19,6 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
-#include "chrome/browser/ui/commander/command_source.h"
 #include "chrome/browser/ui/commander/commander.h"
 #include "chrome/browser/ui/commander/commander_view_model.h"
 #include "chrome/browser/ui/location_bar/location_bar.h"
@@ -33,14 +32,15 @@ CommandItemModel FromViewModel(const CommandItemViewModel& model) {
 }
 }  // namespace
 
-CommandCentre::CommandCentre(CommanderBackend* backend)
-    : backend_(backend) {
+CommandCentre::CommandCentre(CommanderBackend* backend) : backend_(backend) {
   SetInstance(this);
   backend_->SetUpdateCallback(base::BindRepeating(
       &CommandCentre::OnViewModelUpdated, weak_ptr_factory_.GetWeakPtr()));
 }
 
 CommandCentre::~CommandCentre() {
+  // As the |backend_| isn't checking the callback is valid, it's safer to set
+  // `base::DoNothing()` than `base::NullCallback()`.
   backend_->SetUpdateCallback(base::DoNothing());
 }
 
@@ -64,8 +64,8 @@ void CommandCentre::UpdateText(bool force) {
     return;
   }
 
-  std::u16string trimmed_text(
-      base::TrimWhitespace(text.substr(2), base::TRIM_LEADING));
+  std::u16string trimmed_text(base::TrimWhitespace(
+      text.substr(kCommandPrefixLength), base::TRIM_LEADING));
 
   if (trimmed_text == last_searched_ && browser == last_browser_ && !force) {
     return;
@@ -138,9 +138,8 @@ void CommandCentre::OnViewModelUpdated(CommanderViewModel vm) {
 
   items_.clear();
   items_.reserve(vm.items.size());
-  for (const auto& item : vm.items) {
-    items_.push_back(FromViewModel(item));
-  }
+  base::ranges::transform(vm.items, std::back_inserter(items_),
+                          &commander::FromViewModel);
 
   if (vm.action == CommanderViewModel::kPrompt) {
     prompt_ = vm.prompt_text;
@@ -158,8 +157,8 @@ void CommandCentre::OnViewModelUpdated(CommanderViewModel vm) {
 }
 
 // static
-std::unique_ptr<CommanderFrontend>
-CommanderFrontend::Create(CommanderBackend* backend) {
+std::unique_ptr<CommanderFrontend> CommanderFrontend::Create(
+    CommanderBackend* backend) {
   return std::make_unique<CommandCentre>(backend);
 }
 }  // namespace commander
diff --git a/browser/ui/commander/command_centre.h b/browser/ui/commander/command_centre.h
@@ -50,7 +50,7 @@ class CommandCentre : public CommanderFrontend,
   std::u16string last_searched_;
   std::u16string prompt_;
   std::vector<CommandItemModel> items_;
-  int result_set_id_;
+  int result_set_id_ = 0;
 
   raw_ptr<Browser> last_browser_;
   raw_ptr<CommanderBackend> backend_;

diff --git a/chromium_src/components/omnibox/browser/autocomplete_provider.h b/chromium_src/components/omnibox/browser/autocomplete_provider.h
@@ -6,7 +6,9 @@
 #ifndef BRAVE_CHROMIUM_SRC_COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_PROVIDER_H_
 #define BRAVE_CHROMIUM_SRC_COMPONENTS_OMNIBOX_BROWSER_AUTOCOMPLETE_PROVIDER_H_
 
-#define TYPE_BOOKMARK TYPE_BRAVE_COMMANDER = 1 << 19, TYPE_BOOKMARK
+// Note: We go negative with the BraveAutoCompleteTypes, so we don't conflict if
+// Chromium adds something new.
+#define TYPE_BOOKMARK TYPE_BRAVE_COMMANDER = -1 << 0, TYPE_BOOKMARK
 #include "src/components/omnibox/browser/autocomplete_provider.h"
 #undef TYPE_BOOKMARK
 

diff --git a/components/commander/common/commander_frontend_delegate.cc b/components/commander/common/commander_frontend_delegate.cc
@@ -5,13 +5,21 @@
 
 #include "brave/components/commander/common/commander_frontend_delegate.h"
 
+#include "base/check_is_test.h"
+#include "base/logging.h"
+
 namespace commander {
-CommanderFrontendDelegate* CommanderFrontendDelegate::instance_ = nullptr;
+raw_ptr<CommanderFrontendDelegate> CommanderFrontendDelegate::instance_ =
+    nullptr;
 CommanderFrontendDelegate* CommanderFrontendDelegate::Get() {
   return CommanderFrontendDelegate::instance_;
 }
 void CommanderFrontendDelegate::SetInstance(
     CommanderFrontendDelegate* instance) {
+  // The instance should only ever be set once, unless we're in tests.
+  if (instance_) {
+    CHECK_IS_TEST();
+  }
   instance_ = instance;
 }
 }  // namespace commander
diff --git a/components/commander/common/commander_frontend_delegate.h b/components/commander/common/commander_frontend_delegate.h
@@ -36,7 +36,7 @@ class CommanderFrontendDelegate {
   static void SetInstance(CommanderFrontendDelegate* instance);
 
  private:
-  static CommanderFrontendDelegate* instance_;
+  static raw_ptr<CommanderFrontendDelegate> instance_;
 };
 }  // namespace commander
 

diff --git a/components/commander/common/commander_model.cc b/components/commander/common/commander_model.cc
@@ -11,6 +11,8 @@ CommandItemModel::CommandItemModel(
     const std::vector<gfx::Range>& matched_ranges,
     const std::u16string& annotation)
     : title(title), matched_ranges(matched_ranges), annotation(annotation) {}
+
 CommandItemModel::CommandItemModel(const CommandItemModel& other) = default;
+
 CommandItemModel::~CommandItemModel() = default;
 }  // namespace commander
diff --git a/components/commander/common/commander_url.h b/components/commander/common/commander_url.h
@@ -10,9 +10,12 @@
 
 namespace commander {
 GURL GetCommandURL(uint32_t command_index, uint32_t result_set_id);
-// Tries to parse a brave-command://{command_index: uint}/{result_set_id: uint}
-// url into a |command_index| and |result_set_id|. Returns false if the parsing
-// failed.
+// Tries to parse a brave-command-{unguessable-token}://{command_index:
+// uint}/{result_set_id: uint} url into a |command_index| and |result_set_id|.
+// Returns false if the parsing failed.
+// Note: The scheme of |url| must contain the correct UnguessableToken - in
+// practice this means that the URL passed into this function should be
+// generated via |GetCommandURL|.
 [[nodiscard]] bool TryParseCommandURL(const GURL& url,
                                       uint32_t* command_index,
                                       uint32_t* result_set_id);

diff --git a/components/commander/common/constants.h b/components/commander/common/constants.h
@@ -6,8 +6,10 @@
 #ifndef BRAVE_COMPONENTS_COMMANDER_COMMON_CONSTANTS_H_
 #define BRAVE_COMPONENTS_COMMANDER_COMMON_CONSTANTS_H_
 
+#include <iterator>
 namespace commander {
 constexpr char16_t kCommandPrefix[] = u":>";
+constexpr uint16_t kCommandPrefixLength = std::size(kCommandPrefix);
 }
 
 #endif  // BRAVE_COMPONENTS_COMMANDER_COMMON_CONSTANTS_H_
diff --git a/components/omnibox/browser/commander_provider.cc b/components/omnibox/browser/commander_provider.cc
@@ -96,6 +96,6 @@ void CommanderProvider::OnCommanderUpdated() {
     matches_.push_back(match);
   }
 
-  NotifyListeners(true);
+  NotifyListeners(/* updated_matches= */ true);
 }
 }  // namespace commander