Check env-version upon component load

chrome/browser/component_updater/cros_component_installer.cc

Index: chrome/browser/component_updater/cros_component_installer.cc
diff --git a/chrome/browser/component_updater/cros_component_installer.cc b/chrome/browser/component_updater/cros_component_installer.cc
index f2ed8cf6ca8871d65a050bcc410508145f14cb9b..22be81ea05d110a9f5e3a856f3acd6f9bcc962fc 100644
--- a/chrome/browser/component_updater/cros_component_installer.cc
+++ b/chrome/browser/component_updater/cros_component_installer.cc
@@ -11,11 +11,16 @@
 #include "content/public/browser/browser_thread.h"
 
 #if defined(OS_CHROMEOS)
-#include "chromeos/dbus/dbus_method_call_status.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/image_loader_client.h"
 #endif  // defined(OS_CHROMEOS)
 
+#define CONFIG_MAP_CONTENT        \
+  {{"epson-inkjet-printer-escpr", \
+    {{"env_version", "2.1"},      \
+     {"sha2hashstr",              \
+      "1913a5e0a6cad30b6f03e176177e0d7ed62c5d6700a9c66da556d7c3f5d6a47e"}}}};
+
 using content::BrowserThread;
 
 namespace component_updater {
@@ -81,7 +86,6 @@ update_client::CrxInstaller::Result
 CrOSComponentInstallerTraits::OnCustomInstall(
     const base::DictionaryValue& manifest,
     const base::FilePath& install_dir) {
-  DVLOG(1) << "[CrOSComponentInstallerTraits::OnCustomInstall]";
   std::string version;
   if (!manifest.GetString("version", &version)) {
     return ToInstallerResult(update_client::InstallError::GENERIC_ERROR);
@@ -96,7 +100,12 @@ void CrOSComponentInstallerTraits::ComponentReady(
     const base::Version& version,
     const base::FilePath& path,
     std::unique_ptr<base::DictionaryValue> manifest) {
-  g_browser_process->platform_part()->AddCompatibleCrOSComponent(GetName());
+  std::string min_env_version;
+  if (manifest && manifest->GetString("min_env_version", &min_env_version)) {
+    if (IsCompatible(env_version, min_env_version)) {
+      g_browser_process->platform_part()->AddCompatibleCrOSComponent(GetName());
+    }
+  }
 }
 
 bool CrOSComponentInstallerTraits::VerifyInstallation(
@@ -129,89 +138,105 @@ std::vector<std::string> CrOSComponentInstallerTraits::GetMimeTypes() const {
   return mime_types;
 }
 
-void CrOSComponent::RegisterCrOSComponentInternal(
+bool CrOSComponentInstallerTraits::IsCompatible(
+    const std::string& env_version_str,
+    const std::string& min_env_version_str) {
+  base::Version env_version(env_version_str);
+  base::Version min_env_version(min_env_version_str);
+  return env_version.IsValid() && min_env_version.IsValid() &&
+         env_version >= min_env_version;
+}
+
+void CrOSComponent::LoadComponent(
+    const std::string& name,
+    const base::Callback<void(const std::string&)>& load_callback) {
+  if (!g_browser_process->platform_part()->IsCompatibleCrOSComponent(name)) {
+    // A compatible component is not installed, start installation process.
+    InstallComponent(name, load_callback);

[waffles, 2017/06/01 00:08:14]

Seems simpler to just call the three-argument version here and get rid of the
two-argument version.

[xiaochu, 2017/06/01 02:58:09]

Done.

+  } else {
+    // A compatible component is intalled, load it directly.
+    LoadComponentInternal(name, load_callback);
+  }
+}
+
+void CrOSComponent::InstallComponent(
+    const std::string& name,
+    const base::Callback<void(const std::string&)>& load_callback) {
+  auto* const cus = g_browser_process->component_updater();
+  InstallComponent(cus, name, load_callback);
+}
+
+void CrOSComponent::InstallComponent(
     ComponentUpdateService* cus,
-    const ComponentConfig& config,
-    const base::Closure& installcallback) {
+    const std::string& name,
+    const base::Callback<void(const std::string&)>& load_callback) {
+  const ConfigMap components = CONFIG_MAP_CONTENT;
+  const auto it = components.find(name);
+  if (name.empty() || it == components.end()) {
+    base::PostTask(FROM_HERE, base::Bind(load_callback, ""));
+    return;
+  }
+  ComponentConfig config(it->first, it->second.find("env_version")->second,
+                         it->second.find("sha2hashstr")->second);
+  RegisterComponent(cus, config,
+                    base::Bind(RegisterResult, cus,
+                               crx_file::id_util::GenerateIdFromHex(
+                                   it->second.find("sha2hashstr")->second)
+                                   .substr(0, 32),
+                               base::Bind(InstallResult, name, load_callback)));
+}
+
+void CrOSComponent::RegisterComponent(ComponentUpdateService* cus,
+                                      const ComponentConfig& config,
+                                      const base::Closure& register_callback) {
   std::unique_ptr<ComponentInstallerTraits> traits(
       new CrOSComponentInstallerTraits(config));
   // |cus| will take ownership of |installer| during
   // installer->Register(cus).
   DefaultComponentInstaller* installer =
       new DefaultComponentInstaller(std::move(traits));
-  installer->Register(cus, installcallback);
+  installer->Register(cus, register_callback);
 }
 
-void CrOSComponent::InstallChromeOSComponent(
+void CrOSComponent::RegisterResult(
     ComponentUpdateService* cus,
     const std::string& id,
     const update_client::Callback& install_callback) {
   cus->GetOnDemandUpdater().OnDemandUpdate(id, install_callback);
 }
 
-bool CrOSComponent::InstallCrOSComponent(
+void CrOSComponent::InstallResult(
     const std::string& name,
-    const update_client::Callback& install_callback) {
-  auto* const cus = g_browser_process->component_updater();
-  const ConfigMap components = {
-      {"epson-inkjet-printer-escpr",
-       {{"env_version", "0.0"},
-        {"sha2hashstr",
-         "1913a5e0a6cad30b6f03e176177e0d7ed62c5d6700a9c66da556d7c3f5d6a47e"}}}};
-  if (name.empty()) {
-    base::PostTask(
-        FROM_HERE,
-        base::Bind(install_callback, update_client::Error::INVALID_ARGUMENT));
-    return false;
-  }
-  const auto it = components.find(name);
-  if (it == components.end()) {
-    DVLOG(1) << "[RegisterCrOSComponents] component " << name
-             << " is not in configuration.";
-    base::PostTask(
-        FROM_HERE,
-        base::Bind(install_callback, update_client::Error::INVALID_ARGUMENT));
-    return false;
-  }
-  ComponentConfig config(it->first, it->second.find("env_version")->second,
-                         it->second.find("sha2hashstr")->second);
-  RegisterCrOSComponentInternal(
-      cus, config,
-      base::Bind(InstallChromeOSComponent, cus,
-                 crx_file::id_util::GenerateIdFromHex(
-                     it->second.find("sha2hashstr")->second)
-                     .substr(0, 32),
-                 install_callback));
-  return true;
+    const base::Callback<void(const std::string&)>& load_callback,
+    update_client::Error error) {
+  LoadComponentInternal(name, load_callback);
 }
 
-void MountResult(const base::Callback<void(const std::string&)>& mount_callback,
-                 chromeos::DBusMethodCallStatus call_status,
-                 const std::string& result) {
-  if (call_status != chromeos::DBUS_METHOD_CALL_SUCCESS) {
-    DVLOG(1) << "Call to imageloader service failed.";
-    base::PostTask(FROM_HERE, base::Bind(mount_callback, ""));
-    return;
-  }
-  if (result.empty()) {
-    DVLOG(1) << "Component load failed";
-    base::PostTask(FROM_HERE, base::Bind(mount_callback, ""));
-    return;
+void CrOSComponent::LoadComponentInternal(
+    const std::string& name,
+    const base::Callback<void(const std::string&)>& load_callback) {
+  if (g_browser_process->platform_part()->IsCompatibleCrOSComponent(name)) {

[waffles, 2017/06/01 00:08:14]

This check seems redundant with the check the caller performs. You could DCHECK
it if you want.

[xiaochu, 2017/06/01 02:58:09]

I am checking this since otherwise it might load an incompatible component? 

[xiaochu, 2017/06/01 16:26:50]

Done.
Sorry about last message. I get it now.

+    chromeos::ImageLoaderClient* loader =
+        chromeos::DBusThreadManager::Get()->GetImageLoaderClient();
+    if (loader) {
+      loader->LoadComponent(name, base::Bind(&LoadResult, load_callback));
+      return;
+    }
   }
-  base::PostTask(FROM_HERE, base::Bind(mount_callback, result));
+  base::PostTask(FROM_HERE, base::Bind(load_callback, ""));
 }
 
-void CrOSComponent::LoadCrOSComponent(
-    const std::string& name,
-    const base::Callback<void(const std::string&)>& mount_callback) {
-  chromeos::ImageLoaderClient* loader =
-      chromeos::DBusThreadManager::Get()->GetImageLoaderClient();
-  if (loader) {
-    loader->LoadComponent(name, base::Bind(&MountResult, mount_callback));
-  } else {
-    DVLOG(1) << "Failed to get ImageLoaderClient object.";
-  }
+void CrOSComponent::LoadResult(
+    const base::Callback<void(const std::string&)>& load_callback,
+    chromeos::DBusMethodCallStatus call_status,
+    const std::string& result) {
+  PostTask(
+      FROM_HERE,
+      base::Bind(
+          load_callback,
+          call_status != chromeos::DBUS_METHOD_CALL_SUCCESS ? "" : result));
 }
+
 #endif  // defined(OS_CHROMEOS
 
 }  // namespace component_updater