[CodeHealth] Introduce new WebUI::MessageCallback

WebUI::DeprecatedMessageCallback2 is taking base::Value::ConstListView,
with changed plans around base::Value refactoring it should be
const base::Value::List& instead.

This CL adds new MessageCallback and corresponding
RegisterMessageCallback() unblocking actual refactoring of callers.

This CL also includes an examples of how the refactoring will look like.

Bug: 1300095
Change-Id: Ibaf0b075b4fd60f99f393535cebce154965f4bce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483819
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Mohamad Ahmadi <mahmadi@chromium.org>
Commit-Queue: Mohamad Ahmadi <mahmadi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#975287}

chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc

@@ -37,12 +37,12 @@ CryptohomeWebUIHandler::CryptohomeWebUIHandler() {}
 CryptohomeWebUIHandler::~CryptohomeWebUIHandler() {}
 
 void CryptohomeWebUIHandler::RegisterMessages() {
-  web_ui()->RegisterDeprecatedMessageCallback2(
+  web_ui()->RegisterMessageCallback(
       "pageLoaded", base::BindRepeating(&CryptohomeWebUIHandler::OnPageLoaded,
                                         weak_ptr_factory_.GetWeakPtr()));
 }
 
-void CryptohomeWebUIHandler::OnPageLoaded(base::Value::ConstListView args) {
+void CryptohomeWebUIHandler::OnPageLoaded(const base::Value::List& args) {
   UserDataAuthClient* userdataauth_client = UserDataAuthClient::Get();
   CryptohomePkcs11Client* cryptohome_pkcs11_client =
       CryptohomePkcs11Client::Get();

chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.h

@@ -37,7 +37,7 @@ class CryptohomeWebUIHandler : public content::WebUIMessageHandler {
 
  private:
   // This method is called from JavaScript.
-  void OnPageLoaded(base::Value::ConstListView args);
+  void OnPageLoaded(const base::Value::List& args);
 
   void GotIsTPMTokenEnabledOnUIThread(bool is_tpm_token_enabled);
 

content/browser/webui/web_ui_impl.cc

@@ -258,6 +258,11 @@ void WebUIImpl::CallJavascriptFunctionUnsafe(
   ExecuteJavascript(GetJavascriptCall(function_name, args));
 }
 
+void WebUIImpl::RegisterMessageCallback(base::StringPiece message,
+                                        MessageCallback callback) {
+  message_callbacks_.emplace(message, std::move(callback));
+}
+
 void WebUIImpl::RegisterDeprecatedMessageCallback2(
     base::StringPiece message,
     DeprecatedMessageCallback2 callback) {
@@ -276,11 +281,19 @@ void WebUIImpl::ProcessWebUIMessage(const GURL& source_url,
   if (controller_->OverrideHandleWebUIMessage(source_url, message, args))
     return;
 
-  // Look up the callback for this message.
-  auto callback_pair = deprecated_message_callbacks_2_.find(message);
-  if (callback_pair != deprecated_message_callbacks_2_.end()) {
+  auto callback_pair = message_callbacks_.find(message);
+  if (callback_pair != message_callbacks_.end()) {
     // Forward this message and content on.
-    callback_pair->second.Run(args.GetListDeprecated());
+    callback_pair->second.Run(args.GetList());
+    return;
+  }
+
+  // Look up the deprecated callback for this message.
+  auto deprecated_callback_2_pair =
+      deprecated_message_callbacks_2_.find(message);
+  if (deprecated_callback_2_pair != deprecated_message_callbacks_2_.end()) {
+    // Forward this message and content on.
+    deprecated_callback_2_pair->second.Run(args.GetListDeprecated());
     return;
   }
 

content/browser/webui/web_ui_impl.h

@@ -73,6 +73,8 @@ class CONTENT_EXPORT WebUIImpl : public WebUI,
   const std::vector<std::string>& GetRequestableSchemes() override;
   void AddRequestableScheme(const char* scheme) override;
   void AddMessageHandler(std::unique_ptr<WebUIMessageHandler> handler) override;
+  void RegisterMessageCallback(base::StringPiece message,
+                               MessageCallback callback) override;
   void RegisterDeprecatedMessageCallback2(
       base::StringPiece message,
       DeprecatedMessageCallback2 callback) override;
@@ -126,10 +128,17 @@ class CONTENT_EXPORT WebUIImpl : public WebUI,
   void DisallowJavascriptOnAllHandlers();
 
   // A map of message name -> message handling callback.
+  std::map<std::string, MessageCallback> message_callbacks_;
+
+  // A map of message name -> message handling callback.
+  // TODO(crbug.com/1300095): Remove once RegisterDeprecatedMessageCallback2()
+  // instances are migrated to RegisterMessageCallback().
   std::map<std::string, DeprecatedMessageCallback2>
       deprecated_message_callbacks_2_;
 
   // A map of message name -> message handling callback.
+  // TODO(crbug.com/1300095): Remove once RegisterDeprecatedMessageCallback()
+  // instances are migrated to RegisterMessageCallback().
   std::map<std::string, DeprecatedMessageCallback>
       deprecated_message_callbacks_;
 

content/public/browser/web_ui.h

@@ -73,27 +73,29 @@ class CONTENT_EXPORT WebUI {
   virtual void AddMessageHandler(
       std::unique_ptr<WebUIMessageHandler> handler) = 0;
 
-  // TODO(crbug.com/1300095): new version of DeprecatedMessageCallback2 that
-  // takes base::Value::List as a parameter needs to be introduced. Afterwards
-  // existing callers of RegisterDeprecatedMessageCallback2() should be migrated
-  // to the new RegisterMessageCallback() (not yet introduced) version.
-  //
   // Used by WebUIMessageHandlers. If the given message is already registered,
   // the call has no effect.
+  using MessageCallback =
+      base::RepeatingCallback<void(const base::Value::List&)>;
+  virtual void RegisterMessageCallback(base::StringPiece message,
+                                       MessageCallback callback) = 0;
+
+  // TODO(crbug.com/1300095): Instances of RegisterDeprecatedMessageCallback2()
+  // should be migrated to RegisterMessageCallback() above if possible.
+  //
+  // Used by WebUIMessageHandlers. If the given message is already registered,
+  // the call has no effect. Use RegisterMessageCallback() above in new code.
   using DeprecatedMessageCallback2 =
       base::RepeatingCallback<void(base::Value::ConstListView)>;
   virtual void RegisterDeprecatedMessageCallback2(
       base::StringPiece message,
       DeprecatedMessageCallback2 callback) = 0;
 
-  // TODO(crbug.com/1300095): new version of DeprecatedMessageCallback that
-  // takes base::Value::List as a parameter needs to be introduced. Afterwards
-  // existing callers of RegisterDeprecatedMessageCallback() should be migrated
-  // to the new RegisterMessageCallback() (not yet introduced) version if
-  // possible.
+  // TODO(crbug.com/1300095): Instances of RegisterDeprecatedMessageCallback()
+  // should be migrated to RegisterMessageCallback() above if possible.
   //
   // Used by WebUIMessageHandlers. If the given message is already registered,
-  // the call has no effect.
+  // the call has no effect. Use RegisterMessageCallback() above in new code.
   using DeprecatedMessageCallback =
       base::RepeatingCallback<void(const base::ListValue*)>;
   virtual void RegisterDeprecatedMessageCallback(

content/public/test/test_web_ui.cc

@@ -27,14 +27,25 @@ void TestWebUI::ClearTrackedCalls() {
 
 void TestWebUI::HandleReceivedMessage(const std::string& handler_name,
                                       const base::ListValue* args) {
-  const auto callbacks_map_it =
+  const auto callbacks_map_it = message_callbacks_.find(handler_name);
+  if (callbacks_map_it != message_callbacks_.end()) {
+    // Create a copy of the callbacks before running them. Without this, it
+    // could be possible for the callback's handler to register a new message
+    // handler during iteration of the vector, resulting in undefined behavior.
+    std::vector<MessageCallback> callbacks_to_run = callbacks_map_it->second;
+    for (auto& callback : callbacks_to_run)
+      callback.Run(args->GetList());
+    return;
+  }
+
+  const auto deprecated_callbacks_2_map_it =
       deprecated_message_callbacks_2_.find(handler_name);
-  if (callbacks_map_it != deprecated_message_callbacks_2_.end()) {
+  if (deprecated_callbacks_2_map_it != deprecated_message_callbacks_2_.end()) {
     // Create a copy of the callbacks before running them. Without this, it
     // could be possible for the callback's handler to register a new message
     // handler during iteration of the vector, resulting in undefined behavior.
     std::vector<DeprecatedMessageCallback2> callbacks_to_run =
-        callbacks_map_it->second;
+        deprecated_callbacks_2_map_it->second;
     for (auto& callback : callbacks_to_run)
       callback.Run(args->GetListDeprecated());
     return;
@@ -98,6 +109,12 @@ void TestWebUI::AddMessageHandler(
   handlers_.push_back(std::move(handler));
 }
 
+void TestWebUI::RegisterMessageCallback(base::StringPiece message,
+                                        MessageCallback callback) {
+  message_callbacks_[static_cast<std::string>(message)].push_back(
+      std::move(callback));
+}
+
 void TestWebUI::RegisterDeprecatedMessageCallback2(
     base::StringPiece message,
     DeprecatedMessageCallback2 callback) {

content/public/test/test_web_ui.h

@@ -47,6 +47,8 @@ class TestWebUI : public WebUI {
   const std::vector<std::string>& GetRequestableSchemes() override;
   void AddRequestableScheme(const char* scheme) override;
   void AddMessageHandler(std::unique_ptr<WebUIMessageHandler> handler) override;
+  void RegisterMessageCallback(base::StringPiece message,
+                               MessageCallback callback) override;
   void RegisterDeprecatedMessageCallback2(
       base::StringPiece message,
       DeprecatedMessageCallback2 callback) override;
@@ -123,6 +125,7 @@ class TestWebUI : public WebUI {
  private:
   void OnJavascriptCall(const CallData& call_data);
 
+  base::flat_map<std::string, std::vector<MessageCallback>> message_callbacks_;
   base::flat_map<std::string, std::vector<DeprecatedMessageCallback2>>
       deprecated_message_callbacks_2_;
   base::flat_map<std::string, std::vector<DeprecatedMessageCallback>>

docs/webui_explainer.md

@@ -258,7 +258,7 @@ void OvenHandler::RegisterMessages() {
                           base::Unretained(this)));
 }
 
-void OvenHandler::HandleBakeDonuts(base::Value::ConstListView args) {
+void OvenHandler::HandleBakeDonuts(const base::Value::List& args) {
   AllowJavascript();
 
   // IMPORTANT: Fully validate `args`.