Mojo-ifying chrome://version.

chrome/browser/ui/webui/version_handler.cc

Index: chrome/browser/ui/webui/version_handler.cc
diff --git a/chrome/browser/ui/webui/version_handler.cc b/chrome/browser/ui/webui/version_handler.cc
index adef9f837ac8e3658003c595886dd5e7e06d84f0..4ed8141d6a1933de993bbeac81e67d124841510e 100644
--- a/chrome/browser/ui/webui/version_handler.cc
+++ b/chrome/browser/ui/webui/version_handler.cc
@@ -14,19 +14,23 @@
 #include "chrome/grit/generated_resources.h"
 #include "components/variations/active_field_trials.h"
 #include "components/version_ui/version_handler_helper.h"
-#include "components/version_ui/version_ui_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/common/content_constants.h"
 #include "grit/components_strings.h"
+#include "mojo/common/common_type_converters.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/system/version_loader.h"
+#endif
+
 namespace {
 
 // Retrieves the executable and profile paths on the FILE thread.
-void GetFilePaths(const base::FilePath& profile_path,
+void RetrieveFilePaths(const base::FilePath& profile_path,
                   base::string16* exec_path_out,
                   base::string16* profile_path_out) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
@@ -47,60 +51,81 @@ void GetFilePaths(const base::FilePath& profile_path,
 
 }  // namespace
 
-VersionHandler::VersionHandler()
-    : weak_ptr_factory_(this) {
+VersionHandler::VersionHandler(
+    content::WebUI* web_ui,
+    mojo::InterfaceRequest<VersionHandlerMojo> request)
+    : web_ui_(web_ui), binding_(this, request.Pass()) {
 }
 
 VersionHandler::~VersionHandler() {
 }
 
-void VersionHandler::RegisterMessages() {
-  web_ui()->RegisterMessageCallback(
-      version_ui::kRequestVersionInfo,
-      base::Bind(&VersionHandler::HandleRequestVersionInfo,
-                 base::Unretained(this)));
-}
-
-void VersionHandler::HandleRequestVersionInfo(const base::ListValue* args) {
-#if defined(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()));
-#endif
-
+void VersionHandler::GetFilePaths(const GetFilePathsCallback& callback) {
   // Grab the executable path on the FILE thread. It is returned in
   // OnGotFilePaths.
   base::string16* exec_path_buffer = new base::string16;
   base::string16* profile_path_buffer = new base::string16;
   content::BrowserThread::PostTaskAndReply(
       content::BrowserThread::FILE, FROM_HERE,
-          base::Bind(&GetFilePaths, Profile::FromWebUI(web_ui())->GetPath(),
+          base::Bind(&RetrieveFilePaths, Profile::FromWebUI(web_ui_)->GetPath(),
                      base::Unretained(exec_path_buffer),
                      base::Unretained(profile_path_buffer)),
           base::Bind(&VersionHandler::OnGotFilePaths,
-                     weak_ptr_factory_.GetWeakPtr(),
+                     AsWeakPtr(),

[Evan Stade, 2015/12/04 22:30:20]

why this change? AFAIK AsWeakPtr is used when external classes need a weak
reference, and a private factory is used when only |this| should have access (as
is the case here, I believe).

[dpapad, 2015/12/04 23:28:13]

My understanding is that inheriting from base::SupportsWeakPtr and using
AsWeakPtr() is preferred because it requires less boilerplate (than having a
private weak_ptr_factory_), and that other than that those two patterns are
interchangeable (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).

I am fine with undoing this change, but it would be nice to know if my
understanding is invalid for future reference.

[Dan Beam, 2015/12/04 23:32:55]

WeakPtrFactory is safer
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2p...

[dpapad, 2015/12/04 23:42:26]

Reverted. Thanks for the link.

                      base::Owned(exec_path_buffer),
-                     base::Owned(profile_path_buffer)));
-
-  // Respond with the variations info immediately.
-  web_ui()->CallJavascriptFunction(version_ui::kReturnVariationInfo,
-                                   *version_ui::GetVariationsList());
+                     base::Owned(profile_path_buffer),
+                     callback));
 }
 
 void VersionHandler::OnGotFilePaths(base::string16* executable_path_data,
-                                    base::string16* profile_path_data) {
+                                    base::string16* profile_path_data,
+                                    const GetFilePathsCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  callback.Run(mojo::String::From(*executable_path_data),
+               mojo::String::From(*profile_path_data));
+}
+
+void VersionHandler::GetFlashVersion(const GetFlashVersionCallback& callback) {
+#if defined(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,
+                 AsWeakPtr(),
+                 callback));
+#endif
+}
+
+void VersionHandler::GetVariations(const GetVariationsCallback& callback) {
+  std::vector<std::string> variations = version_ui::GetVariations();
+  mojo::Array<mojo::String> variations_array =
+      mojo::Array<mojo::String>::From(variations);
+  callback.Run(variations_array.Pass());
+}
+
+void VersionHandler::GetOsVersion(const GetOsVersionCallback& callback) {
+#if defined(OS_CHROMEOS)
+  base::PostTaskAndReplyWithResult(
+      content::BrowserThread::GetBlockingPool(),
+      FROM_HERE,
+      base::Bind(&chromeos::version_loader::GetVersion,
+                 chromeos::version_loader::VERSION_FULL),
+      base::Bind(&VersionHandler::OnGotOsVersion,
+                 AsWeakPtr(),
+                 callback));
+#endif  // defined(OS_CHROMEOS)
+}
 
-  base::StringValue exec_path(*executable_path_data);
-  base::StringValue profile_path(*profile_path_data);
-  web_ui()->CallJavascriptFunction(version_ui::kReturnFilePaths, exec_path,
-                                   profile_path);
+#if defined(OS_CHROMEOS)
+void VersionHandler::OnGotOsVersion(
+    const GetOsVersionCallback& callback, const std::string& version) {
+  callback.Run(mojo::String::From(version));
 }
+#endif  // defined(OS_CHROMEOS)
 
 #if defined(ENABLE_PLUGINS)
 void VersionHandler::OnGotPlugins(
+    const GetFlashVersionCallback& callback,
     const std::vector<content::WebPluginInfo>& plugins) {
   // Obtain the version of the first enabled Flash plugin.
   std::vector<content::WebPluginInfo> info_array;
@@ -109,7 +134,7 @@ void VersionHandler::OnGotPlugins(
   base::string16 flash_version =
       l10n_util::GetStringUTF16(IDS_PLUGINS_DISABLED_PLUGIN);
   PluginPrefs* plugin_prefs =
-      PluginPrefs::GetForProfile(Profile::FromWebUI(web_ui())).get();
+      PluginPrefs::GetForProfile(Profile::FromWebUI(web_ui_)).get();
   if (plugin_prefs) {
     for (size_t i = 0; i < info_array.size(); ++i) {
       if (plugin_prefs->IsPluginEnabled(info_array[i])) {
@@ -119,7 +144,6 @@ void VersionHandler::OnGotPlugins(
     }
   }
 
-  base::StringValue arg(flash_version);
-  web_ui()->CallJavascriptFunction(version_ui::kReturnFlashVersion, arg);
+  callback.Run(mojo::String::From(flash_version));
 }
 #endif  // defined(ENABLE_PLUGINS)