Check env-version upon component load

chrome/browser/component_updater/cros_component_installer.h

Index: chrome/browser/component_updater/cros_component_installer.h
diff --git a/chrome/browser/component_updater/cros_component_installer.h b/chrome/browser/component_updater/cros_component_installer.h
index 7673d744f2d468add48964847aa5d17111e3694d..d91322729a7c3bac6099b8146b084e9e4b17352f 100644
--- a/chrome/browser/component_updater/cros_component_installer.h
+++ b/chrome/browser/component_updater/cros_component_installer.h
@@ -13,6 +13,24 @@
 #include "components/update_client/update_client.h"
 #include "crypto/sha2.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/dbus/dbus_method_call_status.h"
+#endif  // defined(OS_CHROMEOS)
+
+//  Developer API usage:
+//  ...
+//  void LoadCallback(const std::string& mount_point){
+//    if (mount_point.empty) {

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

empty()?

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

Done.

+//      // component is not loaded.
+//      return;
+//    }
+//    ...
+//  }
+//  ...
+//  component_updater::CrOSComponent::LoadComponent(
+//            name,
+//            base::Bind(&LoadCallback));
+//
 namespace component_updater {
 
 #if defined(OS_CHROMEOS)
@@ -34,6 +52,11 @@ class CrOSComponentInstallerTraits : public ComponentInstallerTraits {
   ~CrOSComponentInstallerTraits() override {}
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest, IsCompatibleOrNot);
+  FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest,
+                           ComponentReadyCorrectManifest);
+  FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest,
+                           ComponentReadyWrongManifest);
   // The following methods override ComponentInstallerTraits.
   bool SupportsGroupPolicyEnabledComponentUpdates() const override;
   bool RequiresNetworkEncryption() const override;
@@ -50,6 +73,9 @@ class CrOSComponentInstallerTraits : public ComponentInstallerTraits {
   std::string GetName() const override;
   update_client::InstallerAttributes GetInstallerAttributes() const override;
   std::vector<std::string> GetMimeTypes() const override;
+
+  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] = {};
@@ -60,62 +86,47 @@ class CrOSComponentInstallerTraits : public ComponentInstallerTraits {
 // This class contains functions used to register and install a component.
 class CrOSComponent {

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

This class has only static members, can we avoid using a class? Move all private
methods into an anonymous namespace in the .cc file, and move the public method
into a named namespace.

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

We are adding this class in order to make it friend for OnDemandUpdater
(https://cs.chromium.org/chromium/src/components/component_updater/component_u...).


Please advise if this needs to change.

[waffles, 2017/06/01 22:42:41]

Ah! I forgot about that. Can we just friend the public method (or create a
public adapter method in another .h), then? Let me know if this will complicate
your testing too much - it's just unfortunate to expose all of these
implementation details in the header. At minimum, some of the private methods
could be moved out of the class.

[xiaochu, 2017/06/02 04:42:15]

Done.

There are 3 functions left in header:
LoadComponent - public API
RegisterResult - which calls onDemandUpdate
InstallComponent - it accepts a CUS object (mocked) so my unittest could work.

Unfotunately, public API is not what calls OnDemandUpdate so caller has to be
exposed (if not CrOSComponent class name). I can remove that particular unittest
and hide InstallComponent as well... Please advise.

  public:
-  //  Register and start installing a CrOS component.
-  //  |install_callback| is triggered after install finishes and returns error
-  //  code.
-  //
-  //  example:
-  //  ...
-  //  void load_callback(const std::string& result){
-  //    if (result.empty) {
-  //      // component is not mounted.
-  //      return;
-  //    }
-  //    // [component mount point: result]
-  //  }
-  //  void install_callback(update_client::Error error){
-  //    // switch(error){
-  //    //   case update_client::Error::NONE:
-  //    //     // component is installed
-  //    //     break;
-  //    //   case update_client::Error::INVALID_ARGUMENT:
-  //    //     // your install failed due to your wrong parameters.
-  //    //     break;
-  //    //   default:
-  //    //     // your install failed due to system failure.
-  //    //     break;
-  //    // }
-  //    // Even when error code other than NONE returned (your install failed),
-  //    // component might has already being installed previously.
-  //    // Plus, if you want to know current version of component.
-  //    component_updater:CrOSComponent::LoadCrOSComponent(name, load_callback);
-  //  }
-  //  ...
-  //    component_updater::CrOSComponent::InstallCrOSComponent(
-  //              name,
-  //              base::Bind(&install_callback));
-  //
-  static bool InstallCrOSComponent(
-      const std::string& name,
-      const update_client::Callback& install_callback);
-
-  static void LoadCrOSComponent(
+  static void LoadComponent(
       const std::string& name,
-      const base::Callback<void(const std::string&)>& mount_callback);
+      const base::Callback<void(const std::string&)>& load_callback);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest,
+                           RegisterComponentSuccess);
+  FRIEND_TEST_ALL_PREFIXES(CrOSComponentInstallerTest, RegisterComponentFail);
   CrOSComponent() {}
-  // Register a component.
-  static void RegisterCrOSComponentInternal(ComponentUpdateService* cus,
-                                            const ComponentConfig& config,
-                                            const base::Closure& callback);
-  // A helper function to pass into RegisterCrOSComonentInternal as a callback.
+  // Install a component.
+  static void InstallComponent(
+      const std::string& name,
+      const base::Callback<void(const std::string&)>& load_callback);
+  // Install a component with a dedicated ComponentUpdateServie instance.
+  static void InstallComponent(
+      ComponentUpdateService* cus,
+      const std::string& name,
+      const base::Callback<void(const std::string&)>& load_callback);
+  // Register a component with a dedicated ComponentUpdateService instance.
+  static void RegisterComponent(ComponentUpdateService* cus,
+                                const ComponentConfig& config,
+                                const base::Closure& callback);
   // It calls OnDemandUpdate to install the component right after being
   // registered.
-  static void InstallChromeOSComponent(
-      ComponentUpdateService* cus,
-      const std::string& id,
-      const update_client::Callback& install_callback);
+  static void RegisterResult(ComponentUpdateService* cus,
+                             const std::string& id,
+                             const update_client::Callback& install_callback);
+  // It call LoadComponentInternal to load the installed component.
+  static void InstallResult(
+      const std::string& name,
+      const base::Callback<void(const std::string&)>& load_callback,
+      update_client::Error error);
+  // Internal function to load a component.
+  static void LoadComponentInternal(
+      const std::string& name,
+      const base::Callback<void(const std::string&)>& load_callback);
+  // It returns load result.
+  static void LoadResult(
+      const base::Callback<void(const std::string&)>& load_callback,
+      chromeos::DBusMethodCallStatus call_status,
+      const std::string& result);
 };
 #endif  // defined(OS_CHROMEOS)