Fix nits in CrOSComponentManager.

Change-Id: I22a02aaf461e425d5a1742370036a08c60319d69
Reviewed-on: https://chromium-review.googlesource.com/935757
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539303}

chrome/browser/chromeos/dbus/chrome_component_updater_service_provider_delegate.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/dbus/chrome_component_updater_service_provider_delegate.h"
 
+#include "base/bind.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/component_updater/cros_component_installer.h"
 
 namespace chromeos {

chrome/browser/chromeos/printing/printer_configurer.cc

@@ -16,6 +16,7 @@
 #include "base/logging.h"
 #include "base/md5.h"
 #include "base/memory/ref_counted.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/printing/ppd_provider_factory.h"
 #include "chrome/browser/component_updater/cros_component_installer.h"
 #include "chrome/browser/local_discovery/endpoint_resolver.h"
@@ -195,8 +196,8 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
                          const std::vector<std::string>& ppd_filters) {
     if (base::FeatureList::IsEnabled(features::kCrOSComponent)) {
       std::set<std::string> components_requested;
-      for (auto& ppd_filter : ppd_filters) {
-        for (auto& component : GetComponentizedFilters()) {
+      for (const auto& ppd_filter : ppd_filters) {
+        for (const auto& component : GetComponentizedFilters()) {
           if (component.first == ppd_filter) {
             components_requested.insert(component.second);
           }
@@ -212,7 +213,8 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
                            weak_factory_.GetWeakPtr(), printer, ppd_contents,
                            std::move(cb)));
         return;
-      } else if (components_requested.size() > 1) {
+      }
+      if (components_requested.size() > 1) {
         LOG(ERROR) << "More than one filter component is requested.";
         std::move(cb).Run(PrinterSetupResult::kFatalError);
         return;

chrome/browser/component_updater/cros_component_installer.cc

@@ -4,18 +4,22 @@
 
 #include "chrome/browser/component_updater/cros_component_installer.h"
 
+#include <map>
 #include <utility>
 
 #include "base/files/file_util.h"
-#include "base/optional.h"
 #include "base/path_service.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/task_scheduler/post_task.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/component_updater/component_installer_errors.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/image_loader_client.h"
 #include "components/component_updater/component_updater_paths.h"
+#include "components/component_updater/component_updater_service.h"
 #include "components/crx_file/id_util.h"
 #include "content/public/browser/browser_thread.h"
+#include "crypto/sha2.h"
 
 // ConfigMap list-initialization expression for all downloadable
 // Chrome OS components.
@@ -45,11 +49,14 @@
      {"sha2hashstr",                                                         \
       "5714811c04f0a63aac96b39096faa759ace4c04e9b68291e7c9716128f5a2722"}}}};
 
-#define COMPONENTS_ROOT_PATH "cros-components"
-
 namespace component_updater {
 
+using ConfigMap = std::map<std::string, std::map<std::string, std::string>>;
+
 namespace {
+
+constexpr char kComponentsRootPath[] = "cros-components";
+
 // TODO(xiaochu): add metrics for component usage (crbug.com/793052).
 void LogCustomUninstall(base::Optional<bool> result) {}
 
@@ -70,28 +77,51 @@ void CleanUpOldInstalls(const std::string& name) {
   if (base::PathExists(path))
     base::DeleteFile(path, true);
 }
-}  // namespace
 
-using ConfigMap = std::map<std::string, std::map<std::string, std::string>>;
+// Returns all installed components.
+std::vector<ComponentConfig> GetInstalled() {
+  std::vector<ComponentConfig> configs;
+  base::FilePath root;
+  if (!PathService::Get(DIR_COMPONENT_USER, &root))
+    return configs;
+
+  root = root.Append(kComponentsRootPath);
+  const ConfigMap components = CONFIG_MAP_CONTENT;
+  for (auto it : components) {
+    const std::string& name = it.first;
+    const std::map<std::string, std::string>& props = it.second;
+    base::FilePath component_path = root.Append(name);
+    if (base::PathExists(component_path)) {
+      ComponentConfig config(name, props.find("env_version")->second,
+                             props.find("sha2hashstr")->second);
+      configs.push_back(config);
+    }
+  }
+  return configs;
+}
+
+}  // namespace
 
 ComponentConfig::ComponentConfig(const std::string& name,
                                  const std::string& env_version,
                                  const std::string& sha2hashstr)
     : name(name), env_version(env_version), sha2hashstr(sha2hashstr) {}
+
 ComponentConfig::~ComponentConfig() {}
 
 CrOSComponentInstallerPolicy::CrOSComponentInstallerPolicy(
     const ComponentConfig& config)
-    : name(config.name), env_version(config.env_version) {
-  if (config.sha2hashstr.length() != 64)
+    : name_(config.name), env_version_(config.env_version) {
+  if (config.sha2hashstr.length() != crypto::kSHA256Length * 2)
     return;
-  auto strstream = config.sha2hashstr;
-  for (auto& cell : kSha2Hash_) {
-    cell = stoul(strstream.substr(0, 2), nullptr, 16);
-    strstream.erase(0, 2);
-  }
+
+  bool converted = base::HexStringToBytes(config.sha2hashstr, &sha2_hash_);
+  DCHECK(converted);
+  DCHECK_EQ(crypto::kSHA256Length, sha2_hash_.size());
 }
 
+CrOSComponentInstallerPolicy::~CrOSComponentInstallerPolicy() = default;
+
 bool CrOSComponentInstallerPolicy::SupportsGroupPolicyEnabledComponentUpdates()
     const {
   return true;
@@ -105,8 +135,8 @@ update_client::CrxInstaller::Result
 CrOSComponentInstallerPolicy::OnCustomInstall(
     const base::DictionaryValue& manifest,
     const base::FilePath& install_dir) {
-  // TODO(xiaochu): remove this at M66 (crbug.com/792203).
-  CleanUpOldInstalls(name);
+  // TODO(xiaochu): remove after M66 ships to stable. https://crbug.com/792203
+  CleanUpOldInstalls(name_);
 
   return update_client::CrxInstaller::Result(update_client::InstallError::NONE);
 }
@@ -114,10 +144,10 @@ CrOSComponentInstallerPolicy::OnCustomInstall(
 void CrOSComponentInstallerPolicy::OnCustomUninstall() {
   g_browser_process->platform_part()
       ->cros_component_manager()
-      ->UnregisterCompatiblePath(name);
+      ->UnregisterCompatiblePath(name_);
 
   chromeos::DBusThreadManager::Get()->GetImageLoaderClient()->UnmountComponent(
-      name, base::BindOnce(&LogCustomUninstall));
+      name_, base::BindOnce(&LogCustomUninstall));
 }
 
 void CrOSComponentInstallerPolicy::ComponentReady(
@@ -125,13 +155,15 @@ void CrOSComponentInstallerPolicy::ComponentReady(
     const base::FilePath& path,
     std::unique_ptr<base::DictionaryValue> manifest) {
   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()
-          ->cros_component_manager()
-          ->RegisterCompatiblePath(GetName(), path);
-    }
-  }
+  if (!manifest || !manifest->GetString("min_env_version", &min_env_version))
+    return;
+
+  if (!IsCompatible(env_version_, min_env_version))
+    return;
+
+  g_browser_process->platform_part()
+      ->cros_component_manager()
+      ->RegisterCompatiblePath(GetName(), path);
 }
 
 bool CrOSComponentInstallerPolicy::VerifyInstallation(
@@ -141,22 +173,22 @@ bool CrOSComponentInstallerPolicy::VerifyInstallation(
 }
 
 base::FilePath CrOSComponentInstallerPolicy::GetRelativeInstallDir() const {
-  base::FilePath path = base::FilePath(COMPONENTS_ROOT_PATH);
-  return path.Append(name);
+  base::FilePath path = base::FilePath(kComponentsRootPath);
+  return path.Append(name_);
 }
 
 void CrOSComponentInstallerPolicy::GetHash(std::vector<uint8_t>* hash) const {
-  hash->assign(kSha2Hash_, kSha2Hash_ + arraysize(kSha2Hash_));
+  *hash = sha2_hash_;
 }
 
 std::string CrOSComponentInstallerPolicy::GetName() const {
-  return name;
+  return name_;
 }
 
 update_client::InstallerAttributes
 CrOSComponentInstallerPolicy::GetInstallerAttributes() const {
   update_client::InstallerAttributes attrs;
-  attrs["_env_version"] = env_version;
+  attrs["_env_version"] = env_version_;
   return attrs;
 }
 
@@ -203,19 +235,15 @@ bool CrOSComponentManager::Unload(const std::string& name) {
     // Component |name| does not exist.
     return false;
   }
-  component_updater::ComponentUpdateService* updater =
-      g_browser_process->component_updater();
+  ComponentUpdateService* updater = g_browser_process->component_updater();
   const std::string id = GenerateId(it->second.find("sha2hashstr")->second);
   return updater->UnregisterComponent(id);
 }
 
 void CrOSComponentManager::RegisterInstalled() {
   base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&component_updater::CrOSComponentManager::GetInstalled,
-                     base::Unretained(this)),
-      base::BindOnce(&component_updater::CrOSComponentManager::RegisterN,
-                     base::Unretained(this)));
+      FROM_HERE, {base::MayBlock()}, base::BindOnce(GetInstalled),
+      base::BindOnce(&CrOSComponentManager::RegisterN, base::Unretained(this)));
 }
 
 void CrOSComponentManager::RegisterCompatiblePath(const std::string& name,
@@ -322,32 +350,10 @@ void CrOSComponentManager::FinishLoad(LoadCallback load_callback,
   }
 }
 
-std::vector<ComponentConfig> CrOSComponentManager::GetInstalled() {
-  std::vector<ComponentConfig> configs;
-  base::FilePath root;
-  if (!PathService::Get(DIR_COMPONENT_USER, &root))
-    return configs;
-
-  root = root.Append(COMPONENTS_ROOT_PATH);
-  const ConfigMap components = CONFIG_MAP_CONTENT;
-  for (auto it : components) {
-    const std::string& name = it.first;
-    const std::map<std::string, std::string>& props = it.second;
-    base::FilePath component_path = root.Append(name);
-    if (base::PathExists(component_path)) {
-      ComponentConfig config(name, props.find("env_version")->second,
-                             props.find("sha2hashstr")->second);
-      configs.push_back(config);
-    }
-  }
-  return configs;
-}
-
 void CrOSComponentManager::RegisterN(
     const std::vector<ComponentConfig>& configs) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  component_updater::ComponentUpdateService* updater =
-      g_browser_process->component_updater();
+  ComponentUpdateService* updater = g_browser_process->component_updater();
   for (const auto& config : configs) {
     Register(updater, config, base::OnceClosure());
   }

chrome/browser/component_updater/cros_component_installer.h

@@ -5,41 +5,43 @@
 #ifndef CHROME_BROWSER_COMPONENT_UPDATER_CROS_COMPONENT_INSTALLER_H_
 #define CHROME_BROWSER_COMPONENT_UPDATER_CROS_COMPONENT_INSTALLER_H_
 
-#include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
-#include "chrome/browser/browser_process.h"
-#include "chromeos/dbus/dbus_method_call_status.h"
+#include "base/containers/flat_map.h"
+#include "base/gtest_prod_util.h"
+#include "base/optional.h"
 #include "components/component_updater/component_installer.h"
-#include "components/component_updater/component_updater_service.h"
 #include "components/update_client/update_client.h"
-#include "crypto/sha2.h"
 
 namespace component_updater {
 
+class ComponentUpdateService;
+
 struct ComponentConfig {
-  std::string name;
-  std::string env_version;
-  std::string sha2hashstr;
   ComponentConfig(const std::string& name,
                   const std::string& env_version,
                   const std::string& sha2hashstr);
   ~ComponentConfig();
+
+  std::string name;
+  std::string env_version;
+  std::string sha2hashstr;
 };
 
 class CrOSComponentInstallerPolicy : public ComponentInstallerPolicy {
  public:
   explicit CrOSComponentInstallerPolicy(const ComponentConfig& config);
-  ~CrOSComponentInstallerPolicy() override {}
+  ~CrOSComponentInstallerPolicy() override;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest, IsCompatibleOrNot);
   FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest, CompatibilityOK);
   FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest,
                            CompatibilityMissingManifest);
-  // The following methods override ComponentInstallerPolicy.
+
+  // ComponentInstallerPolicy:
   bool SupportsGroupPolicyEnabledComponentUpdates() const override;
   bool RequiresNetworkEncryption() const override;
   update_client::CrxInstaller::Result OnCustomInstall(
@@ -57,11 +59,13 @@ class CrOSComponentInstallerPolicy : public ComponentInstallerPolicy {
   update_client::InstallerAttributes GetInstallerAttributes() const override;
   std::vector<std::string> GetMimeTypes() const override;
 
+  // This is virtual so unit tests can override it.
   virtual bool IsCompatible(const std::string& env_version_str,
                             const std::string& min_env_version_str);
-  std::string name;
-  std::string env_version;
-  uint8_t kSha2Hash_[crypto::kSHA256Length] = {};
+
+  const std::string name_;
+  const std::string env_version_;
+  std::vector<uint8_t> sha2_hash_;
 
   DISALLOW_COPY_AND_ASSIGN(CrOSComponentInstallerPolicy);
 };
@@ -85,6 +89,7 @@ class CrOSComponentManager {
 
   CrOSComponentManager();
   ~CrOSComponentManager();
+
   // Installs a component and keeps it up-to-date. |load_callback| returns the
   // mount point path.
   void Load(const std::string& name,
@@ -148,9 +153,6 @@ class CrOSComponentManager {
   void FinishLoad(LoadCallback load_callback,
                   base::Optional<base::FilePath> result);
 
-  // Returns all installed components.
-  std::vector<ComponentConfig> GetInstalled();
-
   // Registers component |configs| to be updated.
   void RegisterN(const std::vector<ComponentConfig>& configs);
 

chrome/browser/extensions/api/media_perception_private/media_perception_api_delegate_chromeos.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/api/media_perception_private/media_perception_api_delegate_chromeos.h"
 
+#include "base/bind.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/component_updater/cros_component_installer.h"
 
 namespace media_perception = extensions::api::media_perception_private;