Refactor VersionHandler to interop with JS via promises

This CL reworks the interop mechanism for obtaining version info
from VersionHandler in about:version.  Instead of relying on
chrome.send to result in several asynchronous callbacks, the
parts are requested using cr.sendWithPromise and handlers are
explicitly called by JS.  The original chrome.send mechanism
is left intact for use by platform-specific derived classes.

Bug: 964528

Change-Id: Ia68e95beb68b38201d29d5d51448ee989bdf333f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1717483Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682332}

chrome/browser/ui/webui/version_handler.cc

@@ -26,6 +26,7 @@
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
+#include "content/public/browser/web_ui_message_handler.h"
 #include "content/public/common/content_constants.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -66,17 +67,73 @@ void VersionHandler::RegisterMessages() {
       version_ui::kRequestVersionInfo,
       base::BindRepeating(&VersionHandler::HandleRequestVersionInfo,
                           base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      version_ui::kRequestVariationInfo,
+      base::BindRepeating(&VersionHandler::HandleRequestVariationInfo,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      version_ui::kRequestPluginInfo,
+      base::BindRepeating(&VersionHandler::HandleRequestPluginInfo,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      version_ui::kRequestPathInfo,
+      base::BindRepeating(&VersionHandler::HandleRequestPathInfo,
+                          base::Unretained(this)));
 }
 
 void VersionHandler::HandleRequestVersionInfo(const base::ListValue* args) {
+  // This method is overridden by platform-specific handlers which may still
+  // use |CallJavascriptFunction|. Main version info is returned by promise
+  // using handlers below.
+  // TODO(orinj): To fully eliminate chrome.send usage in JS, derived classes
+  // could be made to work more like this base class, using
+  // |ResolveJavascriptCallback| instead of |CallJavascriptFunction|.
+  AllowJavascript();
+}
+
+void VersionHandler::HandleRequestVariationInfo(const base::ListValue* args) {
+  AllowJavascript();
+
+  std::string callback_id;
+  bool include_variations_cmd;
+  CHECK_EQ(2U, args->GetSize());
+  CHECK(args->GetString(0, &callback_id));
+  CHECK(args->GetBoolean(1, &include_variations_cmd));
+
+  base::Value response(base::Value::Type::DICTIONARY);
+  response.SetKey(version_ui::kKeyVariationsList,
+                  std::move(*version_ui::GetVariationsList()));
+  if (include_variations_cmd) {
+    response.SetKey(version_ui::kKeyVariationsCmd,
+                    version_ui::GetVariationsCommandLineAsValue());
+  }
+  ResolveJavascriptCallback(base::Value(callback_id), response);
+}
+
+void VersionHandler::HandleRequestPluginInfo(const base::ListValue* args) {
   AllowJavascript();
+
+  std::string callback_id;
+  CHECK_EQ(1U, args->GetSize());
+  CHECK(args->GetString(0, &callback_id));
+
 #if BUILDFLAG(ENABLE_PLUGINS)
   // The Flash version information is needed in the response, so make sure
   // the plugins are loaded.
   content::PluginService::GetInstance()->GetPlugins(
-      base::Bind(&VersionHandler::OnGotPlugins,
-          weak_ptr_factory_.GetWeakPtr()));
+      base::Bind(&VersionHandler::OnGotPlugins, weak_ptr_factory_.GetWeakPtr(),
+                 callback_id));
+#else
+  RejectJavascriptCallback(base::Value(callback_id), base::Value());
 #endif
+}
+
+void VersionHandler::HandleRequestPathInfo(const base::ListValue* args) {
+  AllowJavascript();
+
+  std::string callback_id;
+  CHECK_EQ(1U, args->GetSize());
+  CHECK(args->GetString(0, &callback_id));
 
   // Grab the executable path on the FILE thread. It is returned in
   // OnGotFilePaths.
@@ -87,32 +144,25 @@ void VersionHandler::HandleRequestVersionInfo(const base::ListValue* args) {
       base::BindOnce(&GetFilePaths, Profile::FromWebUI(web_ui())->GetPath(),
                      base::Unretained(exec_path_buffer),
                      base::Unretained(profile_path_buffer)),
-      base::BindOnce(
-          &VersionHandler::OnGotFilePaths, weak_ptr_factory_.GetWeakPtr(),
-          base::Owned(exec_path_buffer), base::Owned(profile_path_buffer)));
-
-  // Respond with the variations info immediately.
-  CallJavascriptFunction(version_ui::kReturnVariationInfo,
-                         *version_ui::GetVariationsList());
-  GURL current_url = web_ui()->GetWebContents()->GetVisibleURL();
-  if (current_url.query().find(version_ui::kVariationsShowCmdQuery) !=
-      std::string::npos) {
-    CallJavascriptFunction(version_ui::kReturnVariationCmd,
-                           version_ui::GetVariationsCommandLineAsValue());
-  }
+      base::BindOnce(&VersionHandler::OnGotFilePaths,
+                     weak_ptr_factory_.GetWeakPtr(), callback_id,
+                     base::Owned(exec_path_buffer),
+                     base::Owned(profile_path_buffer)));
 }
 
-void VersionHandler::OnGotFilePaths(base::string16* executable_path_data,
+void VersionHandler::OnGotFilePaths(std::string callback_id,
+                                    base::string16* executable_path_data,
                                     base::string16* profile_path_data) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  base::Value exec_path(*executable_path_data);
-  base::Value profile_path(*profile_path_data);
-  CallJavascriptFunction(version_ui::kReturnFilePaths, exec_path, profile_path);
+  base::Value response(base::Value::Type::DICTIONARY);
+  response.SetKey(version_ui::kKeyExecPath, base::Value(*executable_path_data));
+  response.SetKey(version_ui::kKeyProfilePath, base::Value(*profile_path_data));
+  ResolveJavascriptCallback(base::Value(callback_id), response);
 }
 
 #if BUILDFLAG(ENABLE_PLUGINS)
 void VersionHandler::OnGotPlugins(
+    std::string callback_id,
     const std::vector<content::WebPluginInfo>& plugins) {
   // Obtain the version of the first enabled Flash plugin.
   std::vector<content::WebPluginInfo> info_array;
@@ -132,9 +182,7 @@ void VersionHandler::OnGotPlugins(
       }
     }
   }
-
-  base::Value arg(flash_version_and_path);
-
-  CallJavascriptFunction(version_ui::kReturnFlashVersion, arg);
+  ResolveJavascriptCallback(base::Value(callback_id),
+                            base::Value(flash_version_and_path));
 }
 #endif  // BUILDFLAG(ENABLE_PLUGINS)

chrome/browser/ui/webui/version_handler.h

@@ -22,20 +22,36 @@ class VersionHandler : public content::WebUIMessageHandler {
   // content::WebUIMessageHandler implementation.
   void RegisterMessages() override;
 
-  // Callback for the "requestVersionInfo" message. This asynchronously requests
-  // the flash version and eventually returns it to the front end along with the
-  // list of variations using OnGotPlugins.
+  // Callback for the "requestVersionInfo" message sent by |chrome.send| in JS.
+  // This is still supported for platform-specific asynchronous calls (see
+  // derived classes) but the main version information is now retrieved with
+  // below messages using |cr.sendWithPromise|.
   virtual void HandleRequestVersionInfo(const base::ListValue* args);
 
+  // Callback for the "requestVariationInfo" message. This resolves immediately
+  // with variations list as well as command variations if requested.
+  virtual void HandleRequestVariationInfo(const base::ListValue* args);
+
+  // Callback for the "requestPluginInfo" message. This asynchronously requests
+  // the flash version and eventually returns it to the front end along with the
+  // list of variations using |OnGotPlugins|.
+  virtual void HandleRequestPluginInfo(const base::ListValue* args);
+
+  // Callback for the "requestPathInfo" message. This resolves asynchronously
+  // with |OnGotFilePaths|.
+  virtual void HandleRequestPathInfo(const base::ListValue* args);
+
  private:
   // Callback which handles returning the executable and profile paths to the
   // front end.
-  void OnGotFilePaths(base::string16* executable_path_data,
+  void OnGotFilePaths(std::string callback_id,
+                      base::string16* executable_path_data,
                       base::string16* profile_path_data);
 
   // Callback for GetPlugins which responds to the page with the Flash version.
   // This also initiates the OS Version load on ChromeOS.
-  void OnGotPlugins(const std::vector<content::WebPluginInfo>& plugins);
+  void OnGotPlugins(std::string callback_id,
+                    const std::vector<content::WebPluginInfo>& plugins);
 
   // Factory for the creating refs in callbacks.
   base::WeakPtrFactory<VersionHandler> weak_ptr_factory_{this};

chrome/browser/ui/webui/version_handler_chromeos.cc

@@ -16,6 +16,8 @@ VersionHandlerChromeOS::~VersionHandlerChromeOS() {}
 
 void VersionHandlerChromeOS::HandleRequestVersionInfo(
     const base::ListValue* args) {
+  VersionHandler::HandleRequestVersionInfo(args);
+
   // Start the asynchronous load of the versions.
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
@@ -33,9 +35,6 @@ void VersionHandlerChromeOS::HandleRequestVersionInfo(
       base::Bind(&chromeos::version_loader::GetARCVersion),
       base::Bind(&VersionHandlerChromeOS::OnARCVersion,
                  weak_factory_.GetWeakPtr()));
-
-  // Parent class takes care of the rest.
-  VersionHandler::HandleRequestVersionInfo(args);
 }
 
 void VersionHandlerChromeOS::OnVersion(const std::string& version) {

chrome/browser/ui/webui/version_handler_win.cc

@@ -17,15 +17,14 @@ VersionHandlerWindows::~VersionHandlerWindows() {}
 
 void VersionHandlerWindows::HandleRequestVersionInfo(
     const base::ListValue* args) {
+  VersionHandler::HandleRequestVersionInfo(args);
+
   // Start the asynchronous load of the versions.
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
       base::BindOnce(&version_utils::win::GetFullWindowsVersion),
       base::BindOnce(&VersionHandlerWindows::OnVersion,
                      weak_factory_.GetWeakPtr()));
-
-  // Parent class takes care of the rest.
-  VersionHandler::HandleRequestVersionInfo(args);
 }
 
 void VersionHandlerWindows::OnVersion(const std::string& version) {

components/version_ui/resources/about_version.html

@@ -22,6 +22,7 @@ about:version template page
     <script src="chrome://resources/js/ios/web_ui.js"></script>
 </if>
 
+    <script src="chrome://resources/js/promise_resolver.js"></script>
     <script src="chrome://resources/js/cr.js"></script>
     <script src="chrome://resources/js/load_time_data.js"></script>
     <script src="chrome://resources/js/parse_html_subset.js"></script>

components/version_ui/resources/about_version.js

@@ -2,44 +2,40 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Note: The handle* functions below are called internally on promise
+// resolution, unlike the other return* functions, which are called
+// asynchronously by the host.
+
 /**
- * Callback from the backend with the list of variations to display.
- * This call will build the variations section of the version page, or hide that
- * section if there are none to display.
- * @param {!Array<string>} variationsList The list of variations.
+ * Promise resolution handler for variations list and command line equivalent.
+ * @param {{variationsList: !Array<string>, variationsCmd: string=}}
  */
-function returnVariationInfo(variationsList) {
+function handleVariationInfo({variationsList, variationsCmd}) {
   $('variations-section').hidden = !variationsList.length;
   $('variations-list').appendChild(
       parseHtmlSubset(variationsList.join('<br>'), ['BR']));
-}
 
-/**
- * Callback from the backend with the variations formatted as command line
- * input. This call will build the variations-cmd section of the version page
- * if needed.
- * @param {string} variationsCmd The variations info in command line format.
- */
-function returnVariationCmd(variationsCmd) {
-  $('variations-cmd-section').hidden = !variationsCmd;
-  $('variations-cmd').textContent = variationsCmd;
+  if (variationsCmd) {
+    $('variations-cmd-section').hidden = !variationsCmd;
+    $('variations-cmd').textContent = variationsCmd;
+  }
 }
 
 /**
- * Callback from the backend with the executable and profile paths to display.
+ * Promise resolution handler for the executable and profile paths to display.
  * @param {string} execPath The executable path to display.
  * @param {string} profilePath The profile path to display.
  */
-function returnFilePaths(execPath, profilePath) {
+function handlePathInfo({execPath, profilePath}) {
   $('executable_path').textContent = execPath;
   $('profile_path').textContent = profilePath;
 }
 
 /**
- * Callback from the backend with the Flash version to display.
+ * Promise resolution handler for the Flash version to display.
  * @param {string} flashVersion The Flash version to display.
  */
-function returnFlashVersion(flashVersion) {
+function handlePluginInfo(flashVersion) {
   $('flash_version').textContent = flashVersion;
 }
 
@@ -83,6 +79,12 @@ function returnCustomizationId(response) {
 /* All the work we do onload. */
 function onLoadWork() {
   chrome.send('requestVersionInfo');
+  const includeVariationsCmd = location.search.includes("show-variations-cmd");
+  cr.sendWithPromise('requestVariationInfo', includeVariationsCmd)
+      .then(handleVariationInfo);
+  cr.sendWithPromise('requestPluginInfo').then(handlePluginInfo);
+  cr.sendWithPromise('requestPathInfo').then(handlePathInfo);
+
   if (cr.isChromeOS) {
     $('arc_holder').hidden = true;
     chrome.chromeosInfoPrivate.get(['customizationId'], returnCustomizationId);

components/version_ui/version_ui_constants.cc

@@ -12,10 +12,15 @@ const char kVersionJS[] = "version.js";
 
 // Message handlers.
 const char kRequestVersionInfo[] = "requestVersionInfo";
-const char kReturnFilePaths[] = "returnFilePaths";
-const char kReturnFlashVersion[] = "returnFlashVersion";
-const char kReturnVariationInfo[] = "returnVariationInfo";
-const char kReturnVariationCmd[] = "returnVariationCmd";
+const char kRequestVariationInfo[] = "requestVariationInfo";
+const char kRequestPluginInfo[] = "requestPluginInfo";
+const char kRequestPathInfo[] = "requestPathInfo";
+
+// Named keys used in message handler responses.
+const char kKeyVariationsList[] = "variationsList";
+const char kKeyVariationsCmd[] = "variationsCmd";
+const char kKeyExecPath[] = "execPath";
+const char kKeyProfilePath[] = "profilePath";
 
 // Strings.
 const char kApplicationLabel[] = "application_label";
@@ -75,7 +80,6 @@ const char kUserAgent[] = "useragent";
 const char kUserAgentName[] = "user_agent_name";
 const char kVariationsCmdName[] = "variations_cmd_name";
 const char kVariationsName[] = "variations_name";
-const char kVariationsShowCmdQuery[] = "show-variations-cmd";
 const char kVersion[] = "version";
 const char kVersionBitSize[] = "version_bitsize";
 const char kVersionModifier[] = "version_modifier";

components/version_ui/version_ui_constants.h

@@ -17,10 +17,14 @@ extern const char kVersionJS[];
 // Message handlers.
 // Must match the constants used in the resource files.
 extern const char kRequestVersionInfo[];
-extern const char kReturnFilePaths[];
-extern const char kReturnFlashVersion[];
-extern const char kReturnVariationInfo[];
-extern const char kReturnVariationCmd[];
+extern const char kRequestVariationInfo[];
+extern const char kRequestPluginInfo[];
+extern const char kRequestPathInfo[];
+
+extern const char kKeyVariationsList[];
+extern const char kKeyVariationsCmd[];
+extern const char kKeyExecPath[];
+extern const char kKeyProfilePath[];
 
 // Strings.
 // Must match the constants used in the resource files.
@@ -81,7 +85,6 @@ extern const char kUserAgent[];
 extern const char kUserAgentName[];
 extern const char kVariationsCmdName[];
 extern const char kVariationsName[];
-extern const char kVariationsShowCmdQuery[];
 extern const char kVersion[];
 extern const char kVersionBitSize[];
 extern const char kVersionModifier[];

ios/chrome/browser/ui/webui/version_handler.cc

@@ -18,14 +18,19 @@ VersionHandler::~VersionHandler() {}
 
 void VersionHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
-      version_ui::kRequestVersionInfo,
-      base::BindRepeating(&VersionHandler::HandleRequestVersionInfo,
+      version_ui::kRequestVariationInfo,
+      base::BindRepeating(&VersionHandler::HandleRequestVariationInfo,
                           base::Unretained(this)));
 }
 
-void VersionHandler::HandleRequestVersionInfo(const base::ListValue* args) {
+void VersionHandler::HandleRequestVariationInfo(const base::ListValue* args) {
   // Respond with the variations info immediately.
-  base::Value variations_list = version_ui::GetVariationsList()->Clone();
-  std::vector<const base::Value*> params{&variations_list};
-  web_ui()->CallJavascriptFunction(version_ui::kReturnVariationInfo, params);
+  std::string callback_id;
+  CHECK_EQ(2U, args->GetSize());
+  CHECK(args->GetString(0, &callback_id));
+
+  base::Value response(base::Value::Type::DICTIONARY);
+  response.SetKey(version_ui::kKeyVariationsList,
+                  std::move(*version_ui::GetVariationsList()));
+  web_ui()->ResolveJavascriptCallback(base::Value(callback_id), response);
 }

ios/chrome/browser/ui/webui/version_handler.h

@@ -21,9 +21,9 @@ class VersionHandler : public web::WebUIIOSMessageHandler {
   // content::WebUIMessageHandler implementation.
   void RegisterMessages() override;
 
-  // Callback for the "requestVersionInfo" message. This asynchronously requests
-  // the list of variations.
-  void HandleRequestVersionInfo(const base::ListValue* args);
+  // Callback for the "requestVariationInfo" message. This responds immediately
+  // with the list of variations.
+  void HandleRequestVariationInfo(const base::ListValue* args);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(VersionHandler);