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

sync/internal_api/public/internal_components_factory.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // InternalComponentsFactory exists so that tests can override creation of
 // components used by the SyncManager that are not exposed across the sync
 // API boundary.
 
 #ifndef SYNC_INTERNAL_API_PUBLIC_INTERNAL_COMPONENTS_FACTORY_H_
 #define SYNC_INTERNAL_API_PUBLIC_INTERNAL_COMPONENTS_FACTORY_H_
@@ skipping 19 matching lines @@
 class SyncSessionContext;
 }
 
 namespace syncable {
 class Directory;
 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(
       const std::string& name,
       sessions::SyncSessionContext* context) = 0;
 
   virtual scoped_ptr<sessions::SyncSessionContext> BuildContext(
       ServerConnectionManager* connection_manager,
       syncable::Directory* directory,
       const std::vector<ModelSafeWorker*> workers,
       ExtensionsActivityMonitor* monitor,
       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
 
 #endif  // SYNC_INTERNAL_API_PUBLIC_INTERNAL_COMPONENTS_FACTORY_H_