sync: Remove SyncManager::TestingMode in favour of InternalComponentsFactory.

sync: Remove SyncManager::TestingMode in favour of InternalComponentsFactory.

Turns SyncScheduler into an interface and adds a FakeSyncScheduler class for tests.

BUG=117836
TEST=

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

[tim (not reviewing), 2012-07-18 20:46:39]

Actually, I might as well rebase this before you review since there are a
bunch of changes on trunk.  Doing so now...


On Wed, Jul 18, 2012 at 1:34 PM, <tim@chromium.org> wrote:

> Reviewers: nzea,
>
> Description:
> sync: Remove SyncManager::TestingMode in favour of
> InternalComponentsFactory.
>
> Adds a FakeSyncScheduler class for tests.
>
> BUG=117836
> TEST=
>
>
> Please review this at
http://codereview.chromium.**org/10701046/<http://codereview.chromium.org/107...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/sync/glue/sync_**backend_host.h
>   M chrome/browser/sync/glue/sync_**backend_host.cc
>   M chrome/browser/sync/test_**profile_sync_service.cc
>   M sync/engine/sync_scheduler.h
>   M sync/engine/sync_scheduler.cc
>   M sync/engine/sync_scheduler_**unittest.cc
>   A sync/internal_api/internal_**components_factory_impl.cc
>   A sync/internal_api/public/**internal_components_factory.h
>   A sync/internal_api/public/**internal_components_factory_**impl.h
>   M sync/internal_api/public/sync_**manager.h
>   A sync/internal_api/public/test/**test_internal_components_**factory.h
>   M sync/internal_api/sync_**manager.cc
>   M sync/internal_api/syncapi_**unittest.cc
>   A sync/internal_api/test/test_**internal_components_factory.cc
>   M sync/sync.gyp
>   A sync/test/engine/fake_sync_**scheduler.h
>   A sync/test/engine/fake_sync_**scheduler.cc
>
>
>

[Nicolas Zea, 2012-07-18 21:10:12]

Some initial comments

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

http://codereview.chromium.org/10701046/diff/3018/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:412: new
InternalComponentsFactoryImpl(),
does it make more sense to have an internal components factory member of SBH,
and let the SBH retain ownership? You won't have to do the delete existing
factory and pass new factory dance in tests that way.

http://codereview.chromium.org/10701046/diff/3018/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:107: virtual void RequestStop(const base::Closure&
callback);
Do I dare wonder if this should be an interface?

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/in...
File sync/internal_api/public/internal_components_factory.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/in...
sync/internal_api/public/internal_components_factory.h:16: #include
"sync/internal_api/public/engine/model_safe_worker.h"
can you just forward declare modelsaferoutinginfo since you're passing by
reference?

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/sy...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:23: #include
"sync/internal_api/public/internal_components_factory.h"
forward declare?

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:25: #include
"sync/internal_api/public/util/unrecoverable_error_handler.h"
this can be forward declared too I think

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/te...
File sync/internal_api/public/test/test_internal_components_factory.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/te...
sync/internal_api/public/test/test_internal_components_factory.h:45: const
StorageOption storage_option_;
nit: newline after?

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/sync_mana...
sync/internal_api/sync_manager.cc:35: #include
"sync/internal_api/public/internal_components_factory_impl.h"
this doesn't need to be the impl does it?

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/sync_mana...
sync/internal_api/sync_manager.cc:186: scoped_ptr<InternalComponentsFactory>
internal_components_factory,
see comment in SBH about whether this should take ownership or not.

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/test/test...
File sync/internal_api/test/test_internal_components_factory.cc (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/test/test...
sync/internal_api/test/test_internal_components_factory.cc:15: StorageOption
option) : storage_option_(option) {
nit: move initializer list onto next line

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
File sync/test/engine/fake_sync_scheduler.cc (right):

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.cc:38: const ConfigurationParams& params) {
probably should invoke the ready callback.

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.cc:71: }  // namespace csync
csync -> syncer

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
File sync/test/engine/fake_sync_scheduler.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.h:48: const sessions::SyncSessionSnapshot&
snapshot) OVERRIDE;
nit: newline after

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.h:53: }  // namespace csync
csync -> syncer

[tim (not reviewing), 2012-07-18 21:57:47]

I'll work on making SyncScheduler an interface, addressed other comments.

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

http://codereview.chromium.org/10701046/diff/3018/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:412: new
InternalComponentsFactoryImpl(),
On 2012/07/18 21:10:12, nzea wrote:
> does it make more sense to have an internal components factory member of SBH,
> and let the SBH retain ownership? You won't have to do the delete existing
> factory and pass new factory dance in tests that way.

I don't think so, because this is only needed for the lifetime of this call /
Init. We could have a virtual override to create the factory. A
factory-factory-function...

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/in...
File sync/internal_api/public/internal_components_factory.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/in...
sync/internal_api/public/internal_components_factory.h:16: #include
"sync/internal_api/public/engine/model_safe_worker.h"
On 2012/07/18 21:10:12, nzea wrote:
> can you just forward declare modelsaferoutinginfo since you're passing by
> reference? 

nope, typedef.

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/sy...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:23: #include
"sync/internal_api/public/internal_components_factory.h"
On 2012/07/18 21:10:12, nzea wrote:
> forward declare?

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/sy...
sync/internal_api/public/sync_manager.h:25: #include
"sync/internal_api/public/util/unrecoverable_error_handler.h"
On 2012/07/18 21:10:12, nzea wrote:
> this can be forward declared too I think

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/te...
File sync/internal_api/public/test/test_internal_components_factory.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/public/te...
sync/internal_api/public/test/test_internal_components_factory.h:45: const
StorageOption storage_option_;
On 2012/07/18 21:10:12, nzea wrote:
> nit: newline after?

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/sync_mana...
sync/internal_api/sync_manager.cc:35: #include
"sync/internal_api/public/internal_components_factory_impl.h"
On 2012/07/18 21:10:12, nzea wrote:
> this doesn't need to be the impl does it?

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/test/test...
File sync/internal_api/test/test_internal_components_factory.cc (right):

http://codereview.chromium.org/10701046/diff/3018/sync/internal_api/test/test...
sync/internal_api/test/test_internal_components_factory.cc:15: StorageOption
option) : storage_option_(option) {
On 2012/07/18 21:10:12, nzea wrote:
> nit: move initializer list onto next line

This isn't really clear from the style guide, I always took it to mean if the
whole initializer didn't fit on one line, split.  I moved it, although I'm not
sure what the indent should be on the next line... 4 or even with
'StorageOption'.

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
File sync/test/engine/fake_sync_scheduler.cc (right):

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.cc:38: const ConfigurationParams& params) {
On 2012/07/18 21:10:12, nzea wrote:
> probably should invoke the ready callback.

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.cc:71: }  // namespace csync
On 2012/07/18 21:10:12, nzea wrote:
> csync -> syncer

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
File sync/test/engine/fake_sync_scheduler.h (right):

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.h:48: const sessions::SyncSessionSnapshot&
snapshot) OVERRIDE;
On 2012/07/18 21:10:12, nzea wrote:
> nit: newline after

Done.

http://codereview.chromium.org/10701046/diff/3018/sync/test/engine/fake_sync_...
sync/test/engine/fake_sync_scheduler.h:53: }  // namespace csync
On 2012/07/18 21:10:12, nzea wrote:
> csync -> syncer

Done.

[tim (not reviewing), 2012-07-19 03:17:14]

Added the interface, also removed SetSchedulerForTest from SyncManager, and
created a special InternalComponentsFactory subclass in syncapi_unittest.  PTAL.

[Nicolas Zea, 2012-07-19 18:01:51]

http://codereview.chromium.org/10701046/diff/6022/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10701046/diff/6022/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:14: #include "base/memory/linked_ptr.h"
nit: most of these can be removed

http://codereview.chromium.org/10701046/diff/6022/sync/internal_api/public/in...
File sync/internal_api/public/internal_components_factory.h (right):

http://codereview.chromium.org/10701046/diff/6022/sync/internal_api/public/in...
sync/internal_api/public/internal_components_factory.h:42: virtual
SyncScheduler* BuildScheduler(
These should probably all be returning scoped_ptr's

http://codereview.chromium.org/10701046/diff/6022/sync/internal_api/syncapi_u...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10701046/diff/6022/sync/internal_api/syncapi_u...
sync/internal_api/syncapi_unittest.cc:2522: SyncScheduler* scheduler_to_use =
scheduler_to_use_;
Once the factory returns scoped ptr's, you can keep scheduler_to_use in a
scoped_ptr and not worry about it leaking if not passed down.

[tim (not reviewing), 2012-07-19 19:44:05]

Addressed all comments!  PTAL.

[Nicolas Zea, 2012-07-19 19:51:41]

LGTM!

http://codereview.chromium.org/10701046/diff/9003/sync/internal_api/test/test...
File sync/internal_api/test/test_internal_components_factory.cc (right):

http://codereview.chromium.org/10701046/diff/9003/sync/internal_api/test/test...
sync/internal_api/test/test_internal_components_factory.cc:41: new
sessions::SyncSessionContext(connection_manager, directory,
nit: I think style guide prefers the args all on individual lines if they can't
fit on one, unless they're intimately related.

[tim (not reviewing), 2012-07-19 20:40:30]

Thanks for the review!

http://codereview.chromium.org/10701046/diff/9003/sync/internal_api/test/test...
File sync/internal_api/test/test_internal_components_factory.cc (right):

http://codereview.chromium.org/10701046/diff/9003/sync/internal_api/test/test...
sync/internal_api/test/test_internal_components_factory.cc:41: new
sessions::SyncSessionContext(connection_manager, directory,
On 2012/07/19 19:51:41, nzea wrote:
> nit: I think style guide prefers the args all on individual lines if they
can't
> fit on one, unless they're intimately related.

It says to consider one-line per arg if it makes the code more readable, I just
didn't feel it did much / was a bit redundant here given that the factory
function lays all the params out one-per line above.

[Nicolas Zea, 2012-07-19 20:42:37]

http://codereview.chromium.org/10701046/diff/9003/sync/internal_api/test/test...
File sync/internal_api/test/test_internal_components_factory.cc (right):

http://codereview.chromium.org/10701046/diff/9003/sync/internal_api/test/test...
sync/internal_api/test/test_internal_components_factory.cc:41: new
sessions::SyncSessionContext(connection_manager, directory,
On 2012/07/19 20:40:30, timsteele wrote:
> On 2012/07/19 19:51:41, nzea wrote:
> > nit: I think style guide prefers the args all on individual lines if they
> can't
> > fit on one, unless they're intimately related.
> 
> It says to consider one-line per arg if it makes the code more readable, I
just
> didn't feel it did much / was a bit redundant here given that the factory
> function lays all the params out one-per line above.

sg, although should probably remove the stray space at the beginning of the
third line of args

[commit-bot: I haz the power, 2012-07-19 20:44:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10701046/9003

[commit-bot: I haz the power, 2012-07-19 21:55:47]

Try job failure for 10701046-9003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-19 22:08:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10701046/9003