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..5bfa9164d708f7c2e51318990595c7c2b3267651 100644
--- a/chrome/browser/component_updater/cros_component_installer.cc
+++ b/chrome/browser/component_updater/cros_component_installer.cc
@@ -81,7 +81,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 +95,18 @@ void CrOSComponentInstallerTraits::ComponentReady(
     const base::Version& version,
     const base::FilePath& path,
     std::unique_ptr<base::DictionaryValue> manifest) {
-  g_browser_process->platform_part()->AddCompatibleCrOSComponent(GetName());
+  const ConfigMap components = CONFIG_MAP_CONTENT;
+  const auto it = components.find(GetName());
+  if (it != components.end()) {
+    std::string env_version = it->second.find("env_version")->second;
+    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,6 +139,78 @@ std::vector<std::string> CrOSComponentInstallerTraits::GetMimeTypes() const {
   return mime_types;
 }
 
+bool IsValidEnvVersion(const std::string& env_version,
+                       std::string& major,
+                       std::string& minor) {
+  // 1) contains dot.
+  if (env_version.find('.') == std::string::npos)
+    return false;
+  // 2) contains only 1 dot.
+  if (env_version.find_first_of('.') != env_version.find_last_of('.'))
+    return false;
+  // 3) major and minor are all digits.
+  std::string major_ = env_version.substr(0, env_version.find('.'));
+  std::string minor_ = env_version.substr(env_version.find('.') + 1);
+  if (major_.find_first_not_of("1234567890") != std::string::npos)
+    return false;
+  if (minor_.find_first_not_of("1234567890") != std::string::npos)
+    return false;
+  major = major_;
+  minor = minor_;
+  return true;
+}

[waffles, 2017/05/26 17:28:26]

empty line between function definitions

[xiaochu, 2017/05/26 19:44:02]

Done.

+bool CrOSComponentInstallerTraits::IsCompatible(
+    const std::string& env_version,
+    const std::string& min_env_version) {
+  // env_version format: [0-9]+.[0-9]+

[waffles, 2017/05/26 17:28:26]

Use base::Version, then you can get rid of IsValidEnvVersion.

[xiaochu, 2017/05/26 19:44:02]

Done.

+  std::string env_version_major;
+  std::string env_version_minor;
+  std::string min_env_version_major;
+  std::string min_env_version_minor;
+  if (IsValidEnvVersion(env_version, env_version_major, env_version_minor) &&
+      IsValidEnvVersion(min_env_version, min_env_version_major,
+                        min_env_version_minor)) {
+    if (env_version_major == min_env_version_major)

[waffles, 2017/05/26 17:28:26]

You also need to check that min_env_version_minor <= env_version_minor.

[xiaochu, 2017/05/26 19:44:02]

Done.

+      return true;
+  }
+  return false;
+}
+
+void CrOSComponent::InstallCrOSComponent(
+    const std::string& name,
+    const update_client::Callback& install_callback) {
+  auto* const cus = g_browser_process->component_updater();
+  InstallCrOSComponent(cus, name, install_callback);
+}

[waffles, 2017/05/26 17:28:26]

Recommend you place this definition near the other InstallCrOSComponent.

[xiaochu, 2017/05/26 19:44:02]

Done.

I reordered all functions so caller and callee are close together.

+
+void LoadResult(const base::Callback<void(const std::string&)>& load_callback,
+                chromeos::DBusMethodCallStatus call_status,
+                const std::string& result) {
+  if (call_status != chromeos::DBUS_METHOD_CALL_SUCCESS) {
+    base::PostTask(FROM_HERE, base::Bind(load_callback, ""));
+    return;
+  }
+  if (result.empty()) {

[waffles, 2017/05/26 17:28:26]

Why is this block necessary?

[xiaochu, 2017/05/26 19:44:02]

Done.

+    base::PostTask(FROM_HERE, base::Bind(load_callback, ""));
+    return;
+  }
+  base::PostTask(FROM_HERE, base::Bind(load_callback, result));

[waffles, 2017/05/26 17:28:26]

Maybe just PostTask(FROM_HERE, base::Bind(load_callback, call_status !=
chromeos::DBUS_METHOD_CALL_SUCCESS ? "" : result))?

[xiaochu, 2017/05/26 19:44:02]

Done.

+}
+
+void CrOSComponent::LoadCrOSComponent(
+    const std::string& name,
+    const base::Callback<void(const std::string&)>& load_callback) {
+  if (!g_browser_process->platform_part()->IsCompatibleCrOSComponent(name))
+    return;
+  chromeos::ImageLoaderClient* loader =
+      chromeos::DBusThreadManager::Get()->GetImageLoaderClient();
+  if (loader) {
+    loader->LoadComponent(name, base::Bind(&LoadResult, load_callback));
+    return;
+  }
+  base::PostTask(FROM_HERE, base::Bind(load_callback, ""));
+}
+
 void CrOSComponent::RegisterCrOSComponentInternal(
     ComponentUpdateService* cus,
     const ComponentConfig& config,
@@ -142,36 +224,17 @@ void CrOSComponent::RegisterCrOSComponentInternal(
   installer->Register(cus, installcallback);
 }
 
-void CrOSComponent::InstallChromeOSComponent(
+void CrOSComponent::InstallCrOSComponent(
     ComponentUpdateService* cus,
-    const std::string& id,
-    const update_client::Callback& install_callback) {
-  cus->GetOnDemandUpdater().OnDemandUpdate(id, install_callback);
-}
-
-bool CrOSComponent::InstallCrOSComponent(
     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 ConfigMap components = CONFIG_MAP_CONTENT;
   const auto it = components.find(name);
-  if (it == components.end()) {
-    DVLOG(1) << "[RegisterCrOSComponents] component " << name
-             << " is not in configuration.";
+  if (name.empty() || it == components.end()) {
     base::PostTask(
         FROM_HERE,
         base::Bind(install_callback, update_client::Error::INVALID_ARGUMENT));
-    return false;
+    return;
   }
   ComponentConfig config(it->first, it->second.find("env_version")->second,
                          it->second.find("sha2hashstr")->second);
@@ -182,36 +245,15 @@ bool CrOSComponent::InstallCrOSComponent(
                      it->second.find("sha2hashstr")->second)
                      .substr(0, 32),
                  install_callback));
-  return true;
 }
 
-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;
-  }
-  base::PostTask(FROM_HERE, base::Bind(mount_callback, result));
+void CrOSComponent::InstallChromeOSComponent(
+    ComponentUpdateService* cus,
+    const std::string& id,
+    const update_client::Callback& install_callback) {
+  cus->GetOnDemandUpdater().OnDemandUpdate(id, install_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.";
-  }
-}
 #endif  // defined(OS_CHROMEOS
 
 }  // namespace component_updater