[Sync] Refactor sync manager into interface.

[Sync] Refactor sync manager into interface.

sync_manager.h now defines a pure interface. The actual implementation is
in sync_manager_impl.h/cc. In order to support this, we also create a
SyncManagerFactory, which allows us to dependency inject a SyncManager
implementation into the SyncBackendHost. Follow up patches will
make use of this injection, as well as split the SyncManager tests
out of syncapi_unittest.

R=akalin@chromium.org
BUG=133061
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147076

[Nicolas Zea, 2012-07-14 01:49:54]

PTAL

[tim (not reviewing), 2012-07-16 21:25:30]

drive-by review

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:314: void
AddObserver(SyncManagerImpl::Observer* observer);
SyncManager::Observer is still correct (here and all over this file).

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:347: // See SyncManagerImpl::Shutdown*
for information.
The comments are actually @ SyncManager::Shutdown.

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:555:
syncer::WeakHandle<SyncManagerImpl::ChangeObserver> change_observer_;
SyncManager::ChangeObserver is still correct (here and all over this file).

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:1070:
FOR_EACH_OBSERVER(SyncManagerImpl::Observer, observers_,
SyncManager::Observer is still correct.

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:88: // Functions used for testing.
I was just thinking about going all the way and removing these ForTest functions
(not asking for this in the patch, just food for thought).  Would you be opposed
to a 'SyncManagerTestHarness' subclass (or a friend, I guess) that would expose
the test-only methods and interact with the SyncInternal object so we don't have
to keep test code in the binary?  With that + my patch to remove the TestingMode
enum + your CL here, we would be free of all test-only code in SyncManager!

[Nicolas Zea, 2012-07-16 21:54:22]

Addressed overzealous use of Search/Replace. PTAL

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:314: void
AddObserver(SyncManagerImpl::Observer* observer);
On 2012/07/16 21:25:30, timsteele wrote:
> SyncManager::Observer is still correct (here and all over this file).

Done.

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:347: // See SyncManagerImpl::Shutdown*
for information.
On 2012/07/16 21:25:30, timsteele wrote:
> The comments are actually @ SyncManager::Shutdown.

Done.

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:555:
syncer::WeakHandle<SyncManagerImpl::ChangeObserver> change_observer_;
On 2012/07/16 21:25:30, timsteele wrote:
> SyncManager::ChangeObserver is still correct (here and all over this file).

Done.

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.cc:1070:
FOR_EACH_OBSERVER(SyncManagerImpl::Observer, observers_,
On 2012/07/16 21:25:30, timsteele wrote:
> SyncManager::Observer is still correct.

Done.

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/10704214/diff/3001/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:88: // Functions used for testing.
On 2012/07/16 21:25:30, timsteele wrote:
> I was just thinking about going all the way and removing these ForTest
functions
> (not asking for this in the patch, just food for thought).  Would you be
opposed
> to a 'SyncManagerTestHarness' subclass (or a friend, I guess) that would
expose
> the test-only methods and interact with the SyncInternal object so we don't
have
> to keep test code in the binary?  With that + my patch to remove the
TestingMode
> enum + your CL here, we would be free of all test-only code in SyncManager!

Yeah, I definitely want to get rid of this test code. SGTM.

[Nicolas Zea, 2012-07-16 22:08:10]

Fixed sync client, NOW PTAL

[tim (not reviewing), 2012-07-16 23:07:47]

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1203: reason =
syncer::CONFIGURE_REASON_NEWLY_ENABLED_DATA_TYPE;
Hmm.  restart == true doesn't imply a newly enabled type.  Are you sure about
this logic?

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:797: // Factory the backend will use
to build the sync manager.
nit: build the SyncManager

[Nicolas Zea, 2012-07-17 00:56:17]

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1203: reason =
syncer::CONFIGURE_REASON_NEWLY_ENABLED_DATA_TYPE;
On 2012/07/16 23:07:47, timsteele wrote:
> Hmm.  restart == true doesn't imply a newly enabled type.  Are you sure about
> this logic?

Discussed offline. I adjusted the comments, and added a TODO to try and
distinguish configuration recovery from newly supported datatypes.

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10704214/diff/2003/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:797: // Factory the backend will use
to build the sync manager.
On 2012/07/16 23:07:47, timsteele wrote:
> nit: build the SyncManager

Done.

[tim (not reviewing), 2012-07-17 16:11:54]

LGTM

http://codereview.chromium.org/10704214/diff/2026/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10704214/diff/2026/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:973:
options.make_http_bridge_factory_fn.Run().Pass(),
Ah ha.. I was wondering how this worked since Pass is non-const but the Run()
makes it work.

[commit-bot: I haz the power, 2012-07-17 18:10:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10704214/2026

[commit-bot: I haz the power, 2012-07-17 20:12:17]

Change committed as 147076