Allow customization of HttpPostProviderFactory via ProfileSyncService

chrome/browser/sync/profile_sync_service.h

Index: chrome/browser/sync/profile_sync_service.h
diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h
index 88e960150b51a832890bff4c335e9bf898f17b16..8f9076ae33e2986c9894c5e8fc5c0de56105193c 100644
--- a/chrome/browser/sync/profile_sync_service.h
+++ b/chrome/browser/sync/profile_sync_service.h
@@ -68,6 +68,7 @@ class SyncSessionSnapshot;
 
 namespace syncer {
 class BaseTransaction;
+class HttpPostProviderFactory;
 struct SyncCredentials;
 struct UserShare;
 }  // namespace syncer
@@ -675,6 +676,10 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   // Return sync token status.
   SyncTokenStatus GetSyncTokenStatus() const;
 
+  // Overrides the HttpPostProviderFactory used for Sync connections.
+  void OverrideHttpPostProviderFactoryForTest(
+      syncer::HttpPostProviderFactory* http_post_provider_factory);
+
  protected:
   // Used by test classes that derive from ProfileSyncService.
   virtual browser_sync::SyncBackendHost* GetBackendForTest();
@@ -1014,6 +1019,8 @@ class ProfileSyncService : public ProfileSyncServiceBase,
   GoogleServiceAuthError last_get_token_error_;
   base::Time next_token_request_time_;
 
+  scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;

[rlarocque, 2013/11/19 18:41:58]

I'm pretty sure a boolean flag or a factory object would be preferable to this.

With your current solution, we'd have to re-override the HttpPostProviderFactory
with each Initialize/Shutdown cycle.  I also think that keeping this
possibly-NULL, possibly subtle behavior affecting pointer around in the PSS is
bad hygiene.

Injecting a factory might allow for some other refactorings down the road.  For
example, it might be nice to inject an object that controls not only whether or
not the PSS talks to the in-process mock sync server, but also whether it talks
to the alpha, dev, or regular sync server.  You might even be able to hide a
reference to the Profile's RequestContextGetter in there.  It would be nice to
pull some of that configuration code out into a separate object.

Unless you're willing to go down that path, though, I think the flag is good
enough.

Also, I'm guessing that a constructor parameter is out of the question due to
iOS testing limitations, right?

[tim (not reviewing), 2013/11/19 20:29:26]

An object responsible for server selection and configuration could be nice.  It
would also be nice to avoid a FactoryFactory or FactoryBuilder or whatever for
the confusing nature of the name. But if there's a purpose for a higher level
net configuration object then that makes sense.  I'd also like to avoid having
PSS explicitly know about the test factory (which may or may not be what you
meant by having a bool flag, where PSS would instantiate a TestFactory if
true?).  
 

[pval...(no longer on Chromium), 2013/11/21 01:32:50]

Correct about iOS. The PSS is already created by the time we have access to it.

[pval...(no longer on Chromium), 2013/11/21 01:32:50]

I've uploaded a new patchset with a factory-factory that I believe accomplishes
what we want here. PTAL.

+
   DISALLOW_COPY_AND_ASSIGN(ProfileSyncService);
 };