Allow customization of HttpPostProviderFactory via ProfileSyncService

Allow customization of HttpPostProviderFactory via ProfileSyncService

This change is being made so that Sync integration tests can eventually
 use an in-process, C++ fake server.

BUG=323265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239636

[pval...(no longer on Chromium), 2013-11-19 05:20:49]

I think this is almost 100% ready. Some points to discuss:

1) I think we need a better solution for creating the real
HttpPostProviderFactory (see line 496 in profile_sync_service.cc). Richard had
suggested perhaps overriding a function (pass a function pointer in on iOS since
we can’t override ProfileSyncService?).

2) There are currently no tests here for ProfileSyncService’s handling of
overriding the factory. I’m not sure of the best way to do this since existing
tests override this value via TestProfileSyncService.

[rlarocque, 2013-11-19 18:41:43]

On 2013/11/19 05:20:49, pvalenzuela wrote:
> I think this is almost 100% ready. Some points to discuss:
> 
> 1) I think we need a better solution for creating the real
> HttpPostProviderFactory (see line 496 in profile_sync_service.cc). Richard had
> suggested perhaps overriding a function (pass a function pointer in on iOS
since
> we can’t override ProfileSyncService?).
> 
> 2) There are currently no tests here for ProfileSyncService’s handling of
> overriding the factory. I’m not sure of the best way to do this since existing
> tests override this value via TestProfileSyncService.

Re #2: I'm not sure you need to test test-only code.  If the override is broken,
then a whole bunch of other, unrelated tests will break with it, right?  I don't
think there's any risk that we'll break the override code and not notice.

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

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc (right):

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc:199:
scoped_ptr<syncer::HttpPostProviderFactory>(
nit: Are you sure you want to use a test factory here?

I think that using a non-test factory would reduce the overall amount of change
in this CL.  On the other hand, I doubt the HttpPostProviderFactory is actually
used, so you might be fine either way.

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:1022:
scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
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]

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:1022:
scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
On 2013/11/19 18:41:58, rlarocque wrote:
> 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?

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:49]

a related question: Why is HttpBridge not called HttpPostProvider (or
HttpPostProviderImpl)? It seems this code would be a little easier to comprehend
if that were the case.

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc (right):

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc:199:
scoped_ptr<syncer::HttpPostProviderFactory>(
On 2013/11/19 18:41:58, rlarocque wrote:
> nit: Are you sure you want to use a test factory here?
> 
> I think that using a non-test factory would reduce the overall amount of
change
> in this CL.  On the other hand, I doubt the HttpPostProviderFactory is
actually
> used, so you might be fine either way.

How would using a non-test factory reduce the amount of code here?

You're right that it's currently not used by FakeSyncManager::Init (where the
HttpPostProviderFactory gets passed). IMO it makes sense to use the
TestHttpBridgeFactory here since this is (supposedly) a unit test and
TestHttpBridgeFactory is essentially a null object
(http://en.wikipedia.org/wiki/Null_Object_pattern).

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:1022:
scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
On 2013/11/19 18:41:58, rlarocque wrote:
> 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?

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

https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:1022:
scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
On 2013/11/19 20:29:26, timsteele wrote:
> On 2013/11/19 18:41:58, rlarocque wrote:
> > 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?
> 
> 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?).  
>  

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

[tim (not reviewing), 2013-11-21 18:15:10]

On 2013/11/21 01:32:49, pvalenzuela wrote:
> a related question: Why is HttpBridge not called HttpPostProvider (or
> HttpPostProviderImpl)? It seems this code would be a little easier to
comprehend
> if that were the case.
> 
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
> File chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc:199:
> scoped_ptr<syncer::HttpPostProviderFactory>(
> On 2013/11/19 18:41:58, rlarocque wrote:
> > nit: Are you sure you want to use a test factory here?
> > 
> > I think that using a non-test factory would reduce the overall amount of
> change
> > in this CL.  On the other hand, I doubt the HttpPostProviderFactory is
> actually
> > used, so you might be fine either way.
> 
> How would using a non-test factory reduce the amount of code here?
> 
> You're right that it's currently not used by FakeSyncManager::Init (where the
> HttpPostProviderFactory gets passed). IMO it makes sense to use the
> TestHttpBridgeFactory here since this is (supposedly) a unit test and
> TestHttpBridgeFactory is essentially a null object
> (http://en.wikipedia.org/wiki/Null_Object_pattern).
> 
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
> chrome/browser/sync/profile_sync_service.h:1022:
> scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
> On 2013/11/19 18:41:58, rlarocque wrote:
> > 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?
> 
> Correct about iOS. The PSS is already created by the time we have access to
it.
> 
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
> chrome/browser/sync/profile_sync_service.h:1022:
> scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
> On 2013/11/19 20:29:26, timsteele wrote:
> > On 2013/11/19 18:41:58, rlarocque wrote:
> > > 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?
> > 
> > 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?).  
> >  
> 
> I've uploaded a new patchset with a factory-factory that I believe
accomplishes
> what we want here. PTAL.

Good call on naming. Calling it URLFetcherHttpPostProvider would be an
improvement.
I'd like to avoid just 'HttpPostProvider' though, and have it be something that
ties
it to the chromium net-stack based implementation.

Some history is that originally, HttpBridge lived in chrome/browser/sync/glue,
as the
Chrome "glue" that bridged http from sync to Chrome's network stack using the
interface
that sync happened to expose (from Chrome's perspective), called
HttpPostProvider. 

We moved the bridge into sync when net/URLFetcher was pulled out to net/
entirely.  However,
technically speaking it's the only "hard" dependency sync/ takes on net/, and
for example,
I could imagine a world where we want a different HttpPostProvider
implementation if Chrome's
net stack was not available or there is a better option for a different
environment (this is
a real question still for Android).  Hence why I'd like to qualify the name with
something
tying it to the current Chrome Net Stack URLFetcher-based implementation of
HttpPostProvider.

[rlarocque, 2013-11-21 18:48:51]

This looks better.  I admit the FactoryFactory name looks a bit ugly, but at
least the structure of the code is correct.

We can fix the naming issue with a few find and replace operations once the
HttpPostProviderFactory has been renamed or refactored into something reasonable
(eg. "ChromeSyncNetworkProvider").

This solution will let you make progress towards your goal without getting in
the way of future cleanups.

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
sync/internal_api/public/http_bridge.h:267: class SYNC_EXPORT
HttpBridgeFactoryFactory
This should be in separate .h and .cc files.

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
File sync/internal_api/public/http_post_provider_factory.h (right):

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
sync/internal_api/public/http_post_provider_factory.h:28: typedef
base::Callback<void(const base::Time&,
Re-declaring this here is bad.  If someone wants to change the definition of
NetworkTimeTracker::UpdateCallback, they should not need to change it both here
and in network_time_tracker.h.

You should find a way to share the definition.

The most flexible solution would be to make the UpdateCallback into a class and
make it available in a separate file included by both network_time_tracker.h and
this file.  I'm not sure you want to go that route, but it's an option to
consider.

[pval...(no longer on Chromium), 2013-11-22 02:11:38]

On 2013/11/21 18:15:10, timsteele wrote:
> On 2013/11/21 01:32:49, pvalenzuela wrote:
> > a related question: Why is HttpBridge not called HttpPostProvider (or
> > HttpPostProviderImpl)? It seems this code would be a little easier to
> comprehend
> > if that were the case.
> > 
> >
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
> > File chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/glue...
> > chrome/browser/sync/glue/sync_backend_host_impl_unittest.cc:199:
> > scoped_ptr<syncer::HttpPostProviderFactory>(
> > On 2013/11/19 18:41:58, rlarocque wrote:
> > > nit: Are you sure you want to use a test factory here?
> > > 
> > > I think that using a non-test factory would reduce the overall amount of
> > change
> > > in this CL.  On the other hand, I doubt the HttpPostProviderFactory is
> > actually
> > > used, so you might be fine either way.
> > 
> > How would using a non-test factory reduce the amount of code here?
> > 
> > You're right that it's currently not used by FakeSyncManager::Init (where
the
> > HttpPostProviderFactory gets passed). IMO it makes sense to use the
> > TestHttpBridgeFactory here since this is (supposedly) a unit test and
> > TestHttpBridgeFactory is essentially a null object
> > (http://en.wikipedia.org/wiki/Null_Object_pattern).
> > 
> >
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
> > File chrome/browser/sync/profile_sync_service.h (right):
> > 
> >
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
> > chrome/browser/sync/profile_sync_service.h:1022:
> > scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
> > On 2013/11/19 18:41:58, rlarocque wrote:
> > > 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?
> > 
> > Correct about iOS. The PSS is already created by the time we have access to
> it.
> > 
> >
>
https://codereview.chromium.org/73723006/diff/200001/chrome/browser/sync/prof...
> > chrome/browser/sync/profile_sync_service.h:1022:
> > scoped_ptr<syncer::HttpPostProviderFactory> http_post_provider_factory_;
> > On 2013/11/19 20:29:26, timsteele wrote:
> > > On 2013/11/19 18:41:58, rlarocque wrote:
> > > > 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?
> > > 
> > > 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?).  
> > >  
> > 
> > I've uploaded a new patchset with a factory-factory that I believe
> accomplishes
> > what we want here. PTAL.
> 
> Good call on naming. Calling it URLFetcherHttpPostProvider would be an
> improvement.
> I'd like to avoid just 'HttpPostProvider' though, and have it be something
that
> ties
> it to the chromium net-stack based implementation.
> 
> Some history is that originally, HttpBridge lived in chrome/browser/sync/glue,
> as the
> Chrome "glue" that bridged http from sync to Chrome's network stack using the
> interface
> that sync happened to expose (from Chrome's perspective), called
> HttpPostProvider. 
> 
> We moved the bridge into sync when net/URLFetcher was pulled out to net/
> entirely.  However,
> technically speaking it's the only "hard" dependency sync/ takes on net/, and
> for example,
> I could imagine a world where we want a different HttpPostProvider
> implementation if Chrome's
> net stack was not available or there is a better option for a different
> environment (this is
> a real question still for Android).  Hence why I'd like to qualify the name
with
> something
> tying it to the current Chrome Net Stack URLFetcher-based implementation of
> HttpPostProvider.

Thanks for the info. I think the right thing to do here is consider the rename
after this CL (as to not distract further from the goal). Sound ok?

[pval...(no longer on Chromium), 2013-11-22 02:13:17]

Sorry about the mid-review rebase (I did some incorrect git commands that broke
my client). I also forgot to compile/run sync_unit_tests, so there are some
additional modified files in the latest patchset.

Please take another look and keep in mind that I'm a C++ noob, so all feedback
is welcome. :-)

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
sync/internal_api/public/http_bridge.h:267: class SYNC_EXPORT
HttpBridgeFactoryFactory
On 2013/11/21 18:48:51, rlarocque wrote:
> This should be in separate .h and .cc files.

Done. I also put the interface (HttpPostProviderFactoryFactory) in its own h/cc

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
File sync/internal_api/public/http_post_provider_factory.h (right):

https://codereview.chromium.org/73723006/diff/450002/sync/internal_api/public...
sync/internal_api/public/http_post_provider_factory.h:28: typedef
base::Callback<void(const base::Time&,
On 2013/11/21 18:48:51, rlarocque wrote:
> Re-declaring this here is bad.  If someone wants to change the definition of
> NetworkTimeTracker::UpdateCallback, they should not need to change it both
here
> and in network_time_tracker.h.
> 
> You should find a way to share the definition.
> 
> The most flexible solution would be to make the UpdateCallback into a class
and
> make it available in a separate file included by both network_time_tracker.h
and
> this file.  I'm not sure you want to go that route, but it's an option to
> consider.

Richard and I talked about this offline. This (bad) pattern already exists in
the codebase (I just moved the re-definition of the typedef from http_bridge.h).

In an effort to clean this up a bit, I've moved the typedef to its own header
file: network_time_update_callback.h

[rlarocque, 2013-11-22 18:30:56]

Could you add a BUG= line to the CL description?

https://codereview.chromium.org/73723006/diff/680001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/73723006/diff/680001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:112:
http_post_provider_factory->RegisterCancelationSignal(
Since you are using a factory now, you could probably avoid this.

You could pass the FactoryFactory into the Initialize() call, but allow the
ProfileSyncService to retain ownership of it.  Then define a function 
scoped_ptr<HttpPostProviderFactory>
CreateHttpPostProviderFactory(CancelationSignal*) on the FactoryFactory.  Have
the SyncBackendHostImpl invoke it here.

Then you could keep the HttpPostProviderFactory's old constructor and wouldn't
need this RegisterCancelationSignal() call.

[pval...(no longer on Chromium), 2013-11-23 02:28:39]

bug line added (there's no bug for this. should we make one?)

https://codereview.chromium.org/73723006/diff/680001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/73723006/diff/680001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:112:
http_post_provider_factory->RegisterCancelationSignal(
On 2013/11/22 18:30:56, rlarocque wrote:
> Since you are using a factory now, you could probably avoid this.
> 
> You could pass the FactoryFactory into the Initialize() call, but allow the
> ProfileSyncService to retain ownership of it.  Then define a function 
> scoped_ptr<HttpPostProviderFactory>
> CreateHttpPostProviderFactory(CancelationSignal*) on the FactoryFactory.  Have
> the SyncBackendHostImpl invoke it here.
> 
> Then you could keep the HttpPostProviderFactory's old constructor and wouldn't
> need this RegisterCancelationSignal() call.

Done. I think this is close.

[rlarocque, 2013-11-23 02:43:06]

On 2013/11/23 02:28:39, pvalenzuela wrote:
> bug line added (there's no bug for this. should we make one?)

Yes.

> 
>
https://codereview.chromium.org/73723006/diff/680001/chrome/browser/sync/glue...
> File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):
> 
>
https://codereview.chromium.org/73723006/diff/680001/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host_impl.cc:112:
> http_post_provider_factory->RegisterCancelationSignal(
> On 2013/11/22 18:30:56, rlarocque wrote:
> > Since you are using a factory now, you could probably avoid this.
> > 
> > You could pass the FactoryFactory into the Initialize() call, but allow the
> > ProfileSyncService to retain ownership of it.  Then define a function 
> > scoped_ptr<HttpPostProviderFactory>
> > CreateHttpPostProviderFactory(CancelationSignal*) on the FactoryFactory. 
Have
> > the SyncBackendHostImpl invoke it here.
> > 
> > Then you could keep the HttpPostProviderFactory's old constructor and
wouldn't
> > need this RegisterCancelationSignal() call.
> 
> Done. I think this is close.

LGTM (once bug is filed).

[rlarocque, 2013-11-23 02:44:00]

One last comment.  I forgot to publish this one.

https://codereview.chromium.org/73723006/diff/750001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/73723006/diff/750001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:706:
syncer::HttpPostProviderFactoryFactory*
nit: We often use scoped_ptr<> parameters to indicate ownership transfer.

[pval...(no longer on Chromium), 2013-11-25 21:30:09]

https://codereview.chromium.org/73723006/diff/750001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/73723006/diff/750001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:706:
syncer::HttpPostProviderFactoryFactory*
On 2013/11/23 02:44:01, rlarocque wrote:
> nit: We often use scoped_ptr<> parameters to indicate ownership transfer.

Done.

https://codereview.chromium.org/73723006/diff/1020001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/73723006/diff/1020001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:2272:
http_post_provider_factory_factory.Pass();
please double check that this is the right way to do this (seems to be the right
way after looking at other similar pieces of code like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...
)

[rlarocque, 2013-11-25 21:40:32]

LGTM.

[pval...(no longer on Chromium), 2013-11-26 00:54:52]

There were some offline discussions today about the correct name for
HttpPostProviderFactoryFactory and its associated classes.

1) HttpPostProviderFactoryFactory is not the best long-term name for this class,
but I don't think we quite agree what it should be just yet.

2) Adding top-level network factory to handle all network-related object
creation (see the Abstract Factory design pattern) is a good goal and
HttpPostProviderFactoryFactory will probably morph into that at some point.
Potential names for this include NetworkFactory or NetworkComponentsFactory.

3) HttpBridge should eventually be renamed to to reference its interface
(currently named HttpPostProviderInterface). If we kept this interface name as
is, then a rename such as URLFetcherHttpPostProvider would make sense.

4) HttpPostProvider{Interface,Factory} are misleading names. An HttpPostProvider
does not create ("provide") an object that performs HTTP posts; it *is* the
object that performs HTTP posts. A name that reflected this would make
everything better. I don't have a good suggestion right now, but the first thing
that pops into my mind is HttpPoster.

I believe #1-3 are consensus opinions while #4 is potentially up for debate.

That said, I'd like to move forward here and I think the best way to do that is
to submit this CL as-is (yes, with the HttpPostProviderFactoryFactory name).
This isn't ideal, but the name makes sense in this current iteration of the
code.

Another option is putting this on hold until next week (I'm unavailable
Wednesday) where Richard, Tim, and myself can talk about this in person and
resolve the naming issues.

Tim, I'll let you decide which way to go with this (I am fine with either
approach).

[tim (not reviewing), 2013-11-26 15:00:46]

On 2013/11/26 00:54:52, pvalenzuela wrote:
> There were some offline discussions today about the correct name for
> HttpPostProviderFactoryFactory and its associated classes.
> 
> 1) HttpPostProviderFactoryFactory is not the best long-term name for this
class,
> but I don't think we quite agree what it should be just yet.
> 
> 2) Adding top-level network factory to handle all network-related object
> creation (see the Abstract Factory design pattern) is a good goal and
> HttpPostProviderFactoryFactory will probably morph into that at some point.
> Potential names for this include NetworkFactory or NetworkComponentsFactory.
> 
> 3) HttpBridge should eventually be renamed to to reference its interface
> (currently named HttpPostProviderInterface). If we kept this interface name as
> is, then a rename such as URLFetcherHttpPostProvider would make sense.
> 
> 4) HttpPostProvider{Interface,Factory} are misleading names. An
HttpPostProvider
> does not create ("provide") an object that performs HTTP posts; it *is* the
> object that performs HTTP posts. A name that reflected this would make
> everything better. I don't have a good suggestion right now, but the first
thing
> that pops into my mind is HttpPoster.
> 
> I believe #1-3 are consensus opinions while #4 is potentially up for debate.
> 
> That said, I'd like to move forward here and I think the best way to do that
is
> to submit this CL as-is (yes, with the HttpPostProviderFactoryFactory name).
> This isn't ideal, but the name makes sense in this current iteration of the
> code.
> 
> Another option is putting this on hold until next week (I'm unavailable
> Wednesday) where Richard, Tim, and myself can talk about this in person and
> resolve the naming issues.
> 
> Tim, I'll let you decide which way to go with this (I am fine with either
> approach).

[tim (not reviewing), 2013-11-26 17:27:08]

On 2013/11/26 15:00:46, timsteele wrote:
> On 2013/11/26 00:54:52, pvalenzuela wrote:
> > There were some offline discussions today about the correct name for
> > HttpPostProviderFactoryFactory and its associated classes.
> > 
> > 1) HttpPostProviderFactoryFactory is not the best long-term name for this
> class,
> > but I don't think we quite agree what it should be just yet.
> > 
> > 2) Adding top-level network factory to handle all network-related object
> > creation (see the Abstract Factory design pattern) is a good goal and
> > HttpPostProviderFactoryFactory will probably morph into that at some point.
> > Potential names for this include NetworkFactory or NetworkComponentsFactory.
> > 
> > 3) HttpBridge should eventually be renamed to to reference its interface
> > (currently named HttpPostProviderInterface). If we kept this interface name
as
> > is, then a rename such as URLFetcherHttpPostProvider would make sense.
> > 
> > 4) HttpPostProvider{Interface,Factory} are misleading names. An
> HttpPostProvider
> > does not create ("provide") an object that performs HTTP posts; it *is* the
> > object that performs HTTP posts. A name that reflected this would make
> > everything better. I don't have a good suggestion right now, but the first
> thing
> > that pops into my mind is HttpPoster.
> > 
> > I believe #1-3 are consensus opinions while #4 is potentially up for debate.
> > 
> > That said, I'd like to move forward here and I think the best way to do that
> is
> > to submit this CL as-is (yes, with the HttpPostProviderFactoryFactory name).
> > This isn't ideal, but the name makes sense in this current iteration of the
> > code.
> > 
> > Another option is putting this on hold until next week (I'm unavailable
> > Wednesday) where Richard, Tim, and myself can talk about this in person and
> > resolve the naming issues.
> > 
> > Tim, I'll let you decide which way to go with this (I am fine with either
> > approach).

If we all agree HttpPostProviderFactoryFactory isn't a good name and we're
adding in this CL then we should add it with the right name. It makes little
sense to add something we already unanimously disagree with :)

re #4 - an HttpPostProvider provides http post functionality. The interface
defines how to provide http post functionality for the syncer. There are other
examples of this usage of the word 'Provider' in the codebase (e.g
AutocompleteProviders). I think this name is actually pretty good, and doesn't
need to be changed.

As for what to do: the sync/ library needs a bunch of things to be provided by
the embedder and one of them is a network abstraction.  We should (now or in a
later patch, doesn't matter to me) combine the server url + port + use_ssl flag
into the same single object. cacheinvalidation uses 'SystemResources' as the
abstraction name for a similar purpose.  I think a name like
syncer::NetworkResources would do well for us here.

[pval...(no longer on Chromium), 2013-11-26 21:32:48]

On 2013/11/26 17:27:08, timsteele wrote:
> On 2013/11/26 15:00:46, timsteele wrote:
> > On 2013/11/26 00:54:52, pvalenzuela wrote:
> > > There were some offline discussions today about the correct name for
> > > HttpPostProviderFactoryFactory and its associated classes.
> > > 
> > > 1) HttpPostProviderFactoryFactory is not the best long-term name for this
> > class,
> > > but I don't think we quite agree what it should be just yet.
> > > 
> > > 2) Adding top-level network factory to handle all network-related object
> > > creation (see the Abstract Factory design pattern) is a good goal and
> > > HttpPostProviderFactoryFactory will probably morph into that at some
point.
> > > Potential names for this include NetworkFactory or
NetworkComponentsFactory.
> > > 
> > > 3) HttpBridge should eventually be renamed to to reference its interface
> > > (currently named HttpPostProviderInterface). If we kept this interface
name
> as
> > > is, then a rename such as URLFetcherHttpPostProvider would make sense.
> > > 
> > > 4) HttpPostProvider{Interface,Factory} are misleading names. An
> > HttpPostProvider
> > > does not create ("provide") an object that performs HTTP posts; it *is*
the
> > > object that performs HTTP posts. A name that reflected this would make
> > > everything better. I don't have a good suggestion right now, but the first
> > thing
> > > that pops into my mind is HttpPoster.
> > > 
> > > I believe #1-3 are consensus opinions while #4 is potentially up for
debate.
> > > 
> > > That said, I'd like to move forward here and I think the best way to do
that
> > is
> > > to submit this CL as-is (yes, with the HttpPostProviderFactoryFactory
name).
> > > This isn't ideal, but the name makes sense in this current iteration of
the
> > > code.
> > > 
> > > Another option is putting this on hold until next week (I'm unavailable
> > > Wednesday) where Richard, Tim, and myself can talk about this in person
and
> > > resolve the naming issues.
> > > 
> > > Tim, I'll let you decide which way to go with this (I am fine with either
> > > approach).
> 
> If we all agree HttpPostProviderFactoryFactory isn't a good name and we're
> adding in this CL then we should add it with the right name. It makes little
> sense to add something we already unanimously disagree with :)
> 
> re #4 - an HttpPostProvider provides http post functionality. The interface
> defines how to provide http post functionality for the syncer. There are other
> examples of this usage of the word 'Provider' in the codebase (e.g
> AutocompleteProviders). I think this name is actually pretty good, and doesn't
> need to be changed.
> 
> As for what to do: the sync/ library needs a bunch of things to be provided by
> the embedder and one of them is a network abstraction.  We should (now or in a
> later patch, doesn't matter to me) combine the server url + port + use_ssl
flag
> into the same single object. cacheinvalidation uses 'SystemResources' as the
> abstraction name for a similar purpose.  I think a name like
> syncer::NetworkResources would do well for us here.

Re: Provider naming. I'm ok with it given the precedence. 

I have some concerns about the NetworkResources name (although I like the
general idea of the network abstraction). One is that this class is screaming to
be named as a factory (IMO, that's what it is), so I'm leaning towards that type
of naming.

I'm probably not going to get this submitted before being away on vacation, so
let's reconvene next week and figure out the correct names and plan future CLs.
This doesn't block my other work, so I'm fine with waiting.

[pval...(no longer on Chromium), 2013-12-06 23:48:20]

My exploration of a shortcut method wasn't successful, so I've decided to
proceed with this CL. I've renamed HttpPostProviderFactoryFactory to
NetworkResources. We can eventually add other stuff to it (like server URL).

PTAL.

[rlarocque, 2013-12-07 01:02:11]

On 2013/12/06 23:48:20, pvalenzuela wrote:
> My exploration of a shortcut method wasn't successful, so I've decided to
> proceed with this CL. I've renamed HttpPostProviderFactoryFactory to
> NetworkResources. We can eventually add other stuff to it (like server URL).
> 
> PTAL.

lgtm

[tim (not reviewing), 2013-12-09 19:06:41]

lgtm

[commit-bot: I haz the power, 2013-12-09 20:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/73723006/1070001

[commit-bot: I haz the power, 2013-12-09 22:37:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/73723006/1090001

[commit-bot: I haz the power, 2013-12-10 01:47:48]

Change committed as 239636