Eliminate list of values from JSCallsContainer::Event

Before this CL Oobe WebUI used marshaling/unmarshaling params to/from std::vector<Value> for the deferred js calls. With this CL we just use the existing callback options for this. Bug: 1298392, 1309022 Change-Id: Ib519a80797ec56dfdb465bab4c56ac7a5066de74 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3535861 Auto-Submit: Roman Sorokin <rsorokin@chromium.org> Reviewed-by: Renato Silva <rrsilva@google.com> Reviewed-by: Melissa Zhang <melzhang@chromium.org> Commit-Queue: Roman Sorokin <rsorokin@chromium.org> Cr-Commit-Position: refs/heads/main@{#984199}
chromium · Mar 23, 2022 · bdd5e4d · bdd5e4d
1 parent 305a07f
commit bdd5e4d
Show file tree

Hide file tree

Showing 28 changed files with 72 additions and 142 deletions.
diff --git a/chrome/browser/apps/app_discovery_service/recommended_arc_app_fetcher.cc b/chrome/browser/apps/app_discovery_service/recommended_arc_app_fetcher.cc
@@ -23,7 +23,7 @@ void RecommendedArcAppFetcher::GetApps(ResultCallback callback) {
   recommend_apps_fetcher_->Start();
 }
 
-void RecommendedArcAppFetcher::OnLoadSuccess(const base::Value& app_list) {
+void RecommendedArcAppFetcher::OnLoadSuccess(base::Value app_list) {
   if (!callback_)
     return;
   if (!app_list.is_dict()) {

diff --git a/chrome/browser/apps/app_discovery_service/recommended_arc_app_fetcher.h b/chrome/browser/apps/app_discovery_service/recommended_arc_app_fetcher.h
@@ -28,7 +28,7 @@ class RecommendedArcAppFetcher : public AppFetcher,
   void GetApps(ResultCallback callback) override;
 
   // RecommendAppsFetcherDelegate:
-  void OnLoadSuccess(const base::Value& app_list) override;
+  void OnLoadSuccess(base::Value app_list) override;
   void OnLoadError() override;
   void OnParseResponseError() override;
 

diff --git a/chrome/browser/ash/login/screens/offline_login_screen.cc b/chrome/browser/ash/login/screens/offline_login_screen.cc
@@ -118,7 +118,7 @@ void OfflineLoginScreen::LoadOffline() {
     params.SetStringKey("emailDomain", email_domain);
   }
   if (view_)
-    view_->LoadParams(params);
+    view_->LoadParams(std::move(params));
 }
 
 void OfflineLoginScreen::OnUserAction(const std::string& action_id) {

diff --git a/chrome/browser/ash/login/screens/recommend_apps/fake_recommend_apps_fetcher_delegate.cc b/chrome/browser/ash/login/screens/recommend_apps/fake_recommend_apps_fetcher_delegate.cc
@@ -34,9 +34,8 @@ void FakeRecommendAppsFetcherDelegate::OnParseResponseError() {
   SetResult(Result::PARSE_ERROR);
 }
 
-void FakeRecommendAppsFetcherDelegate::OnLoadSuccess(
-    const base::Value& app_list) {
-  loaded_apps_ = app_list.Clone();
+void FakeRecommendAppsFetcherDelegate::OnLoadSuccess(base::Value app_list) {
+  loaded_apps_ = std::move(app_list);
   SetResult(Result::SUCCESS);
 }
 

diff --git a/chrome/browser/ash/login/screens/recommend_apps/fake_recommend_apps_fetcher_delegate.h b/chrome/browser/ash/login/screens/recommend_apps/fake_recommend_apps_fetcher_delegate.h
@@ -40,7 +40,7 @@ class FakeRecommendAppsFetcherDelegate : public RecommendAppsFetcherDelegate {
   }
 
   // RecommendAppsFetcherDelegate:
-  void OnLoadSuccess(const base::Value& app_list) override;
+  void OnLoadSuccess(base::Value app_list) override;
   void OnLoadError() override;
   void OnParseResponseError() override;
 

diff --git a/chrome/browser/ash/login/screens/recommend_apps/recommend_apps_fetcher_delegate.h b/chrome/browser/ash/login/screens/recommend_apps/recommend_apps_fetcher_delegate.h
@@ -17,7 +17,7 @@ class RecommendAppsFetcherDelegate {
   virtual ~RecommendAppsFetcherDelegate() = default;
 
   // Called when the download of the recommend app list is successful.
-  virtual void OnLoadSuccess(const base::Value& app_list) = 0;
+  virtual void OnLoadSuccess(base::Value app_list) = 0;
 
   // Called when the download of the recommend app list fails.
   virtual void OnLoadError() = 0;

diff --git a/chrome/browser/ash/login/screens/recommend_apps_screen.cc b/chrome/browser/ash/login/screens/recommend_apps_screen.cc
@@ -87,9 +87,9 @@ void RecommendAppsScreen::HideImpl() {
   view_->Hide();
 }
 
-void RecommendAppsScreen::OnLoadSuccess(const base::Value& app_list) {
+void RecommendAppsScreen::OnLoadSuccess(base::Value app_list) {
   if (view_)
-    view_->OnLoadSuccess(app_list);
+    view_->OnLoadSuccess(std::move(app_list));
 }
 
 void RecommendAppsScreen::OnLoadError() {

diff --git a/chrome/browser/ash/login/screens/recommend_apps_screen.h b/chrome/browser/ash/login/screens/recommend_apps_screen.h
@@ -57,7 +57,7 @@ class RecommendAppsScreen : public BaseScreen,
   void SetSkipForTesting() { skip_for_testing_ = true; }
 
   // RecommendAppsFetcherDelegate:
-  void OnLoadSuccess(const base::Value& app_list) override;
+  void OnLoadSuccess(base::Value app_list) override;
   void OnLoadError() override;
   void OnParseResponseError() override;
 

diff --git a/chrome/browser/ash/login/screens/recommend_apps_screen_browsertest.cc b/chrome/browser/ash/login/screens/recommend_apps_screen_browsertest.cc
@@ -77,7 +77,7 @@ class StubRecommendAppsFetcher : public RecommendAppsFetcher {
     for (const auto& app : apps) {
       app_list.Append(app.ToValue());
     }
-    delegate_->OnLoadSuccess(app_list);
+    delegate_->OnLoadSuccess(std::move(app_list));
   }
 
   void SimulateParseError() {

diff --git a/chrome/browser/ui/webui/chromeos/login/assistant_optin_flow_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/assistant_optin_flow_screen_handler.cc
@@ -375,13 +375,14 @@ void AssistantOptInFlowScreenHandler::StopSpeakerIdEnrollment() {
   ResetReceiver();
 }
 
-void AssistantOptInFlowScreenHandler::ReloadContent(const base::Value& dict) {
-  CallJS("login.AssistantOptInFlowScreen.reloadContent", dict);
+void AssistantOptInFlowScreenHandler::ReloadContent(base::Value dict) {
+  CallJS("login.AssistantOptInFlowScreen.reloadContent", std::move(dict));
 }
 
 void AssistantOptInFlowScreenHandler::AddSettingZippy(const std::string& type,
-                                                      const base::Value& data) {
-  CallJS("login.AssistantOptInFlowScreen.addSettingZippy", type, data);
+                                                      base::Value data) {
+  CallJS("login.AssistantOptInFlowScreen.addSettingZippy", type,
+         std::move(data));
 }
 
 void AssistantOptInFlowScreenHandler::UpdateValuePropScreen() {
@@ -495,7 +496,7 @@ void AssistantOptInFlowScreenHandler::OnGetSettingsResponse(
         consented ? assistant::prefs::ConsentStatus::kActivityControlAccepted
                   : assistant::prefs::ConsentStatus::kUnknown);
   } else {
-    AddSettingZippy("settings", zippy_data);
+    AddSettingZippy("settings", std::move(zippy_data));
   }
 
   const bool is_oobe_in_progress =
@@ -512,7 +513,7 @@ void AssistantOptInFlowScreenHandler::OnGetSettingsResponse(
       "shouldSkipVoiceMatch",
       base::Value(!ash::AssistantState::Get()->HasAudioInputDevice()));
   dictionary.SetKey("childName", base::Value(GetGivenNameIfIsChild()));
-  ReloadContent(dictionary);
+  ReloadContent(std::move(dictionary));
 
   // Skip activity control and users will be in opted out mode.
   if (skip_activity_control)

diff --git a/chrome/browser/ui/webui/chromeos/login/assistant_optin_flow_screen_handler.h b/chrome/browser/ui/webui/chromeos/login/assistant_optin_flow_screen_handler.h
@@ -129,8 +129,8 @@ class AssistantOptInFlowScreenHandler
   void StopSpeakerIdEnrollment();
 
   // Send message and consent data to the page.
-  void ReloadContent(const base::Value& dict);
-  void AddSettingZippy(const std::string& type, const base::Value& data);
+  void ReloadContent(base::Value dict);
+  void AddSettingZippy(const std::string& type, base::Value data);
 
   // Update value prop screen to show the next settings.
   void UpdateValuePropScreen();

diff --git a/chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc b/chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc
@@ -47,7 +47,7 @@ void BaseWebUIHandler::ShowScreenWithData(OobeScreenId screen,
   if (data) {
     screen_params.SetKey("data", data->Clone());
   }
-  CallJS("cr.ui.Oobe.showScreen", screen_params);
+  CallJS("cr.ui.Oobe.showScreen", std::move(screen_params));
 }
 
 OobeUI* BaseWebUIHandler::GetOobeUI() {
@@ -65,8 +65,6 @@ void BaseWebUIHandler::OnJavascriptDisallowed() {
   javascript_disallowed_ = true;
 }
 
-void BaseWebUIHandler::InsertIntoList(std::vector<base::Value>*) {}
-
 void BaseWebUIHandler::OnRawCallback(
     const std::string& function_name,
     const content::WebUI::DeprecatedMessageCallback& callback,

diff --git a/chrome/browser/ui/webui/chromeos/login/base_webui_handler.h b/chrome/browser/ui/webui/chromeos/login/base_webui_handler.h
@@ -14,6 +14,7 @@
 #include "chrome/browser/ash/login/oobe_screen.h"
 #include "chrome/browser/ui/webui/chromeos/login/js_calls_container.h"
 #include "components/login/base_screen_handler_utils.h"
+#include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_message_handler.h"
 #include "ui/gfx/native_widget_types.h"
 
@@ -28,6 +29,18 @@ class LocalizedValuesBuilder;
 
 namespace chromeos {
 
+namespace {
+
+template <typename... Args>
+void PostponedJSCall(const std::string& function_name,
+                     Args... args,
+                     content::WebUI* web_ui) {
+  web_ui->CallJavascriptFunctionUnsafe(function_name,
+                                       base::Value(std::move(args))...);
+}
+
+}  // namespace
+
 class OobeUI;
 
 // Base class for all oobe/login WebUI handlers. These handlers are the binding
@@ -74,19 +87,18 @@ class BaseWebUIHandler : public content::WebUIMessageHandler {
   // All CallJS invocations can be recorded for tests if CallJS recording is
   // enabled.
   template <typename... Args>
-  void CallJS(const std::string& function_name, const Args&... args) {
+  void CallJS(const std::string& function_name, Args... args) {
     // Record the call if the WebUI is not loaded or if we are in a test.
     if (!js_calls_container_->is_initialized()) {
-      std::vector<base::Value> arguments;
-      InsertIntoList(&arguments, args...);
-      js_calls_container_->events()->emplace_back(
-          JSCallsContainer::Event(function_name, std::move(arguments)));
+      js_calls_container_->events()->push_back(base::BindOnce(
+          &PostponedJSCall<Args...>, function_name, std::move(args)...));
+      return;
     }
 
     // Make the call now if the WebUI is loaded.
     if (js_calls_container_->is_initialized() && !javascript_disallowed_)
-      web_ui()->CallJavascriptFunctionUnsafe(
-          function_name, ::login::MakeValue(args).Clone()...);
+      web_ui()->CallJavascriptFunctionUnsafe(function_name,
+                                             base::Value(std::move(args))...);
   }
 
   // TODO(crbug.com/1180291) - Remove once OOBE JS calls are fixed
@@ -146,25 +158,6 @@ class BaseWebUIHandler : public content::WebUIMessageHandler {
  private:
   friend class OobeUI;
 
-  // InsertIntoList takes a template parameter pack and expands into the
-  // following form:
-  //
-  //   for (auto arg : args)
-  //     stroage->emplace_back(::login::MakeValue(arg).Clone());
-  //
-  // This cannot be expressed with the current parameter pack expansion rules
-  // and the only way to do it is via compile-time recursion.
-  template <typename Head, typename... Tail>
-  void InsertIntoList(std::vector<base::Value>* storage,
-                      const Head& head,
-                      const Tail&... tail) {
-    storage->emplace_back(::login::MakeValue(head).Clone());
-    InsertIntoList(storage, tail...);
-  }
-  // Base condition for the recursion, when there are no more elements to
-  // insert. Does nothing.
-  void InsertIntoList(std::vector<base::Value>*);
-
   // These two functions wrap Add(Raw)Callback so that the incoming JavaScript
   // event can be recorded.
   void OnRawCallback(const std::string& function_name,

diff --git a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc
@@ -142,8 +142,8 @@ void CoreOobeHandler::FocusReturned(bool reverse) {
   CallJS("cr.ui.Oobe.focusReturned", reverse);
 }
 
-void CoreOobeHandler::ReloadContent(const base::DictionaryValue& dictionary) {
-  CallJS("cr.ui.Oobe.reloadContent", dictionary);
+void CoreOobeHandler::ReloadContent(base::DictionaryValue dictionary) {
+  CallJS("cr.ui.Oobe.reloadContent", std::move(dictionary));
 }
 
 void CoreOobeHandler::SetVirtualKeyboardShown(bool shown) {
@@ -322,7 +322,7 @@ void CoreOobeHandler::OnOobeConfigurationChanged() {
   configuration::FilterConfiguration(
       OobeConfiguration::Get()->GetConfiguration(),
       configuration::ConfigurationHandlerSide::HANDLER_JS, configuration);
-  CallJS("cr.ui.Oobe.updateOobeConfiguration", configuration);
+  CallJS("cr.ui.Oobe.updateOobeConfiguration", std::move(configuration));
 }
 
 void CoreOobeHandler::HandleLaunchHelpApp(double help_topic_id) {

diff --git a/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h b/chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h
@@ -38,7 +38,7 @@ class CoreOobeView {
  public:
   virtual ~CoreOobeView() = default;
 
-  virtual void ReloadContent(const base::DictionaryValue& dictionary) = 0;
+  virtual void ReloadContent(base::DictionaryValue dictionary) = 0;
   virtual void SetVirtualKeyboardShown(bool shown) = 0;
   virtual void SetShelfHeight(int height) = 0;
   virtual void UpdateKeyboardState() = 0;
@@ -101,7 +101,7 @@ class CoreOobeHandler : public BaseWebUIHandler,
 
  private:
   // CoreOobeView implementation:
-  void ReloadContent(const base::DictionaryValue& dictionary) override;
+  void ReloadContent(base::DictionaryValue dictionary) override;
   void SetVirtualKeyboardShown(bool displayed) override;
   void SetShelfHeight(int height) override;
   void FocusReturned(bool reverse) override;

diff --git a/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
@@ -739,7 +739,8 @@ void EnrollmentScreenHandler::OnAdConfigurationUnlocked(
   options->Append(std::move(custom));
   active_directory_join_type_ =
       ActiveDirectoryDomainJoinType::USING_CONFIGURATION;
-  CallJS("login.OAuthEnrollmentScreen.setAdJoinConfiguration", *options);
+  CallJS("login.OAuthEnrollmentScreen.setAdJoinConfiguration",
+         std::move(*options));
 }
 
 void EnrollmentScreenHandler::UpdateState(NetworkError::ErrorReason reason) {

diff --git a/chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc
@@ -570,7 +570,7 @@ void GaiaScreenHandler::LoadGaiaWithPartitionAndVersionAndConsent(
   was_security_token_pin_canceled_ = false;
 
   frame_state_ = FRAME_STATE_LOADING;
-  CallJS("login.GaiaSigninScreen.loadAuthExtension", params);
+  CallJS("login.GaiaSigninScreen.loadAuthExtension", std::move(params));
 }
 
 void GaiaScreenHandler::ReloadGaia(bool force_reload) {
@@ -1346,7 +1346,8 @@ void GaiaScreenHandler::ShowAllowlistCheckFailedError() {
                                   &family_link_allowed);
   params.SetBoolKey("familyLinkAllowed", family_link_allowed);
 
-  CallJS("login.GaiaSigninScreen.showAllowlistCheckFailedError", params);
+  CallJS("login.GaiaSigninScreen.showAllowlistCheckFailedError",
+         std::move(params));
 }
 
 void GaiaScreenHandler::LoadAuthExtension(bool force) {

diff --git a/chrome/browser/ui/webui/chromeos/login/js_calls_container.cc b/chrome/browser/ui/webui/chromeos/login/js_calls_container.cc
@@ -3,19 +3,12 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/chromeos/login/js_calls_container.h"
+#include <utility>
 
 #include "content/public/browser/web_ui.h"
 
 namespace chromeos {
 
-JSCallsContainer::Event::Event(const std::string& function_name,
-                               std::vector<base::Value>&& arguments)
-    : function_name(function_name), arguments(std::move(arguments)) {}
-
-JSCallsContainer::Event::~Event() = default;
-
-JSCallsContainer::Event::Event(Event&&) = default;
-
 JSCallsContainer::JSCallsContainer() = default;
 
 JSCallsContainer::~JSCallsContainer() = default;
@@ -24,20 +17,11 @@ void JSCallsContainer::ExecuteDeferredJSCalls(content::WebUI* web_ui) {
   DCHECK(!is_initialized());
   is_initialized_ = true;
 
+  auto events = std::exchange(events_, {});
   // Execute recorded outgoing events.
-  for (const auto& event : events_) {
-    // event.arguments is of type std::vector<base::Value>, but
-    // CallJavascriptFunctionUnsafe requires std::vector<const base::Value*>.
-    // Construct the new vector of pointers from the existing data.
-    std::vector<const base::Value*> args;
-    args.reserve(event.arguments.size());
-    auto* bp = event.arguments.data();
-    for (size_t i = 0; i < event.arguments.size(); ++i)
-      args.push_back(bp + i);
-    web_ui->CallJavascriptFunctionUnsafe(event.function_name, args);
+  for (auto& event : events) {
+    std::move(event).Run(web_ui);
   }
-
-  events_.clear();
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/ui/webui/chromeos/login/js_calls_container.h b/chrome/browser/ui/webui/chromeos/login/js_calls_container.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/callback_forward.h"
 #include "base/values.h"
 
 namespace content {
@@ -19,18 +20,7 @@ namespace chromeos {
 // BaseWebUIHandler.
 class JSCallsContainer {
  public:
-  // An event is a message/JS call to or from WebUI.
-  struct Event {
-    Event(const std::string& function_name,
-          std::vector<base::Value>&& arguments);
-    ~Event();
-    Event(Event&&);
-    Event(const Event&) = delete;
-    Event& operator=(const Event&) = delete;
-
-    std::string function_name;
-    std::vector<base::Value> arguments;
-  };
+  using Event = base::OnceCallback<void(content::WebUI* web_ui)>;
 
   JSCallsContainer();
   ~JSCallsContainer();

diff --git a/chrome/browser/ui/webui/chromeos/login/kiosk_autolaunch_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/kiosk_autolaunch_screen_handler.cc
@@ -75,7 +75,7 @@ void KioskAutolaunchScreenHandler::UpdateKioskApp() {
     icon_url = webui::GetBitmapDataUrl(*app.icon.bitmap());
 
   app_info.SetStringKey("appIconUrl", icon_url);
-  CallJS("login.AutolaunchScreen.updateApp", app_info);
+  CallJS("login.AutolaunchScreen.updateApp", std::move(app_info));
 }
 
 void KioskAutolaunchScreenHandler::DeclareLocalizedValues(

diff --git a/chrome/browser/ui/webui/chromeos/login/locale_switch_screen_handler.cc b/chrome/browser/ui/webui/chromeos/login/locale_switch_screen_handler.cc
@@ -40,7 +40,7 @@ void LocaleSwitchScreenHandler::Unbind() {
 void LocaleSwitchScreenHandler::UpdateStrings() {
   base::DictionaryValue localized_strings;
   GetOobeUI()->GetLocalizedStrings(&localized_strings);
-  core_oobe_view_->ReloadContent(localized_strings);
+  core_oobe_view_->ReloadContent(std::move(localized_strings));
 }
 
 void LocaleSwitchScreenHandler::DeclareLocalizedValues(