sync: add InternalComponentsFactory::Switches to simplify passing switches to internal components.

sync/internal_api/public/internal_components_factory.h

Index: sync/internal_api/public/internal_components_factory.h
diff --git a/sync/internal_api/public/internal_components_factory.h b/sync/internal_api/public/internal_components_factory.h
index 145c2258ba4ce9020f773830d365c5a1e7ce2d6a..4830ce17b19099d3bfc107089a09196331b28875 100644
--- a/sync/internal_api/public/internal_components_factory.h
+++ b/sync/internal_api/public/internal_components_factory.h
@@ -37,6 +37,30 @@ class DirectoryBackingStore;
 
 class InternalComponentsFactory {
  public:
+  enum EncryptionMethod {
+    ENCRYPTION_LEGACY,
+    // Option to enable support for keystore key based encryption.
+    ENCRYPTION_KEYSTORE
+  };
+
+  enum BackoffOverride {
+    BACKOFF_NORMAL,
+    // Use this value for integration testing to avoid long delays /
+    // timing out tests. Uses kInitialBackoffShortRetrySeconds (see
+    // polling_constants.h) for all initial retries.
+    BACKOFF_SHORT_INITIAL_RETRY_OVERRIDE
+  };
+
+  // Configuration options for internal components. This struct is expected
+  // to grow and shrink over time with transient features / experiments,
+  // roughly following command line flags in chrome. Implementations of
+  // InternalComponentsFactory can use this information to build components
+  // with appropriate bells and whistles.
+  struct Switches {

[rlarocque, 2012/08/14 19:54:07]

I'd prefer to see this struct's members embedded directly in the factory.  It
would be simpler to pass these flags directly to the factory's constructor,
rather than constructing this object then passing it to the factory's
constructor. 

[tim (not reviewing), 2012/08/14 20:20:51]

That's exactly what I wanted to avoid.  In the past we've had to change
constructors in interfaces, in real impls, in fake impls, etc for config or
transient flags, and this has proven to be a pain and considerably more prone to
branch merge conflict (which is one reason why, for example, I hacked a smaller
patch together for the backoff retry last week).  In cases where we've done
something like this struct instead, it has made it very easy to only change code
in the places that actually care about the value (similar to command line
flags).  I'm speaking based on not only my own feelings but feedback I received
from other folks when they had to e.g. add a new counter to SessionState (now
ModelNeutralState).

An alternative is the java-y builder pattern, and while it might make for pretty
c++ I think it's overkill / not as elegant here as java (particularly when
non-const& return values are not permitted by style guide).

[rlarocque, 2012/08/14 21:13:04]

I could write a long rant about why this is wrong.  I dislike sacrificing code
cleanliness in favor of less typing.  

However, you're probably right that this will be more maintainable.  If you're
going to evaluate the code according to that criteria, then this is the right
decision.  

+    EncryptionMethod encryption_method;
+    BackoffOverride backoff_override;
+  };
+
   virtual ~InternalComponentsFactory() {}
 
   virtual scoped_ptr<SyncScheduler> BuildScheduler(
@@ -51,13 +75,16 @@ class InternalComponentsFactory {
       ThrottledDataTypeTracker* throttled_data_type_tracker,
       const std::vector<SyncEngineEventListener*>& listeners,
       sessions::DebugInfoGetter* debug_info_getter,
-      TrafficRecorder* traffic_recorder,
-      bool keystore_encryption_enabled) = 0;
+      TrafficRecorder* traffic_recorder) = 0;
 
   virtual scoped_ptr<syncable::DirectoryBackingStore>
   BuildDirectoryBackingStore(
       const std::string& dir_name,
       const FilePath& backing_filepath) = 0;
+
+  // Returns the Switches struct that this object is using as configuration, if
+  // the implementation is making use of one.
+  virtual Switches GetSwitches() const = 0;
 };
 
 }  // namespace syncer