BraveActionAPI use-after-move fix and modernisation

This change corrects a use-after-move in
BraveActionAPI::NotifyObservers, when notifying more than one observer.
At the same time, all unecessary uses of std::unique_ptr<T> have been
removed in favour of stack-based wrapper types.

browser/brave_rewards/rewards_panel/rewards_panel_extension_handler.cc

@@ -78,10 +78,11 @@ void RewardsPanelExtensionHandler::OnRewardsPanelRequested(
       extension_service->component_loader())
       ->AddRewardsExtension();
 
-  std::string error;
-  extensions::BraveActionAPI::ShowActionUI(
-      browser_, brave_rewards_extension_id,
-      std::make_unique<std::string>(GetExtensionPath(args)), &error);
+  auto result = extensions::BraveActionAPI::ShowActionUI(
+      browser_, brave_rewards_extension_id, GetExtensionPath(args));
+  if (!result.has_value()) {
+    LOG(ERROR) << "Failure to show Action UI. error=" << result.error();
+  }
 }
 
 }  // namespace brave_rewards

browser/extensions/api/brave_action_api.cc

@@ -96,52 +96,44 @@ BraveActionAPI* BraveActionAPI::Get(Browser* context) {
 }
 
 // static
-bool BraveActionAPI::ShowActionUI(
-      ExtensionFunction* extension_function,
-      const std::string& extension_id,
-      std::unique_ptr<int> window_id_param,
-      std::unique_ptr<std::string> ui_relative_path_param,
-      std::string* error) {
+base::expected<bool, std::string> BraveActionAPI::ShowActionUI(
+    ExtensionFunction* extension_function,
+    const std::string& extension_id,
+    absl::optional<int> window_id_param,
+    absl::optional<std::string> ui_relative_path_param) {
   // Which browser should we send the action to
   Browser* browser = nullptr;
   // If the windowId is specified, find it. Otherwise get the active
   // window for the profile.
-  if (!window_id_param.get()) {
+  if (!window_id_param) {
     browser = ChromeExtensionFunctionDetails(extension_function)
         .GetCurrentBrowser();
     if (!browser) {
-      *error = tabs_constants::kNoCurrentWindowError;
-      return false;
+      return base::unexpected(tabs_constants::kNoCurrentWindowError);
     }
   } else {
-    int window_id = *window_id_param;
     std::string get_browser_error;
     if (!windows_util::GetBrowserFromWindowID(
-            extension_function,
-            window_id,
-            WindowController::GetAllWindowFilter(),
-            &browser,
+            extension_function, *window_id_param,
+            WindowController::GetAllWindowFilter(), &browser,
             &get_browser_error)) {
-      *error = get_browser_error;
-      return false;
+      return base::unexpected(get_browser_error);
     }
   }
-  return ShowActionUI(browser, extension_id, std::move(ui_relative_path_param),
-      error);
+  return ShowActionUI(browser, extension_id, std::move(ui_relative_path_param));
 }
 
 // static
-bool BraveActionAPI::ShowActionUI(
-        Browser* browser,
-        const std::string& extension_id,
-        std::unique_ptr<std::string> ui_relative_path_param,
-        std::string* error) {
+base::expected<bool, std::string> BraveActionAPI::ShowActionUI(
+    Browser* browser,
+    const std::string& extension_id,
+    absl::optional<std::string> ui_relative_path_param) {
   bool did_notify = BraveActionAPI::Get(browser)->NotifyObservers(extension_id,
       std::move(ui_relative_path_param));
   if (!did_notify) {
-    *error = "No toolbar is registered to observe BraveActionUI "
-              "calls for this window";
-    return false;
+    return base::unexpected(
+        "No toolbar is registered to observe BraveActionUI "
+        "calls for this window");
   }
   return true;
 }
@@ -158,12 +150,12 @@ void BraveActionAPI::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-bool BraveActionAPI::NotifyObservers(const std::string& extension_id,
-      std::unique_ptr<std::string> ui_relative_path_param) {
+bool BraveActionAPI::NotifyObservers(
+    const std::string& extension_id,
+    absl::optional<std::string> ui_relative_path_param) {
   bool did_notify = false;
   for (auto& observer : observers_) {
-    observer.OnBraveActionShouldTrigger(extension_id,
-        std::move(ui_relative_path_param));
+    observer.OnBraveActionShouldTrigger(extension_id, ui_relative_path_param);
     did_notify = true;
   }
   return did_notify;

browser/extensions/api/brave_action_api.h

@@ -6,13 +6,14 @@
 #ifndef BRAVE_BROWSER_EXTENSIONS_API_BRAVE_ACTION_API_H_
 #define BRAVE_BROWSER_EXTENSIONS_API_BRAVE_ACTION_API_H_
 
-#include <memory>
 #include <string>
 
 #include "base/observer_list.h"
+#include "base/types/expected.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "extensions/browser/extension_function.h"
 #include "extensions/common/extension.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 class Browser;
 
@@ -23,25 +24,23 @@ class BraveActionAPI : public KeyedService {
    public:
     Observer();
     virtual void OnBraveActionShouldTrigger(
-      const std::string& extension_id,
-      std::unique_ptr<std::string> ui_relative_path) = 0;
+        const std::string& extension_id,
+        const absl::optional<std::string>& ui_relative_path) = 0;
 
    protected:
     virtual ~Observer();
   };
 
   static BraveActionAPI* Get(Browser* context);
-  static bool ShowActionUI(
-        ExtensionFunction* extension_function,
-        const std::string& extension_id,
-        std::unique_ptr<int> window_id,
-        std::unique_ptr<std::string> ui_relative_path,
-        std::string* error);
-  static bool ShowActionUI(
-        Browser* browser,
-        const std::string& extension_id,
-        std::unique_ptr<std::string> ui_relative_path,
-        std::string* error);
+  static base::expected<bool, std::string> ShowActionUI(
+      ExtensionFunction* extension_function,
+      const std::string& extension_id,
+      absl::optional<int> window_id,
+      absl::optional<std::string> ui_relative_path);
+  static base::expected<bool, std::string> ShowActionUI(
+      Browser* browser,
+      const std::string& extension_id,
+      absl::optional<std::string> ui_relative_path);
   BraveActionAPI();
   BraveActionAPI(const BraveActionAPI&) = delete;
   BraveActionAPI& operator=(const BraveActionAPI&) = delete;
@@ -53,7 +52,7 @@ class BraveActionAPI : public KeyedService {
 
  protected:
   bool NotifyObservers(const std::string& extension_id,
-      std::unique_ptr<std::string> ui_relative_path_param);
+                       absl::optional<std::string> ui_relative_path_param);
 
  private:
   base::ObserverList<Observer>::Unchecked observers_;

browser/ui/views/brave_actions/brave_actions_container.cc

@@ -444,7 +444,7 @@ void BraveActionsContainer::OnExtensionActionUpdated(
 // BraveActionAPI::Observer
 void BraveActionsContainer::OnBraveActionShouldTrigger(
     const std::string& extension_id,
-    std::unique_ptr<std::string> ui_relative_path) {
+    const absl::optional<std::string>& ui_relative_path) {
   if (!IsContainerAction(extension_id)) {
     return;
   }

browser/ui/views/brave_actions/brave_actions_container.h

@@ -177,7 +177,7 @@ class BraveActionsContainer : public views::View,
   // BraveActionAPI::Observer
   void OnBraveActionShouldTrigger(
       const std::string& extension_id,
-      std::unique_ptr<std::string> ui_relative_path) override;
+      const absl::optional<std::string>& ui_relative_path) override;
 
   bool should_hide_ = false;