USS SyncContextProxy / data type activation refactoring

USS SyncContextProxy / data type activation refactoring

This refactoring prepares the code to introduction of
NonBlockingDataTypeController for USS datatypes. The goal
was to split the very large NonBlockingDataTypeController
change into a couple of smaller to make it easier to review
and verify.

The following changes are included here:
1) Introduced ActivationContext which is a structure that
   contains all arguments needed to activate a USS datatype.
   For now ActivationContext is passed via SyncContext /
   SyncContextProxy, but the goal is to pass it directly
   via BackendDataTypeConfigurer as an argument for
   ActivateNonBlockingDataType.
   ActivationContext is needed as a separate class because
   NonBlockingDataTypeController will have to receive it
   from the type processor's callback and temporarily hold
   on to it.
2) BackendDataTypeConfigurer - two activation methods are
   renamed to be directory specific and two more activation
   methods for non-blocking data types are added.
3) DataTypeController cleanup - OnModelLoaded() virtual
   method shouldn't be on the base class because it applies
   only to some of the subclasses and is never invoked via the
   base class.

BUG=515962
Committed: https://crrev.com/a4e4a10e92844d0274895c1d078d9119fa76f041
Cr-Commit-Position: refs/heads/master@{#351110}

[stanisc, 2015-09-25 05:45:26]

Patchset #1 (id:1) has been deleted

[stanisc, 2015-09-25 05:45:46]

Patchset #1 (id:20001) has been deleted

[stanisc, 2015-09-25 16:21:53]

Patchset #1 (id:40001) has been deleted

[stanisc, 2015-09-25 16:22:02]

Patchset #1 (id:60001) has been deleted

[stanisc, 2015-09-25 16:26:08]

stanisc@chromium.org changed reviewers:
+ maxbogue@chromium.org, pavely@chromium.org

[stanisc, 2015-09-25 16:26:08]

maxbodgue@, pavely@, could either of you review this?
This is mostly code removal / cleanup and doesn't contain the actual DataType
controller changes yet.

[pavely, 2015-09-25 18:22:39]

Mostly nits.

https://codereview.chromium.org/1368683003/diff/80001/components/sync_driver/...
File components/sync_driver/non_blocking_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/1368683003/diff/80001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:186:
base::ThreadTaskRunnerHandle model_thread_handle_;
Are you using model_thread_handle_? I didn't find any references.

https://codereview.chromium.org/1368683003/diff/80001/sync/engine/model_type_...
File sync/engine/model_type_processor_impl.cc (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/engine/model_type_...
sync/engine/model_type_processor_impl.cc:56: ActivationContext*
activation_context = new ActivationContext;
Please change this to scoped_ptr<ActivationContext> activation_context(new
ActivationContext());

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/activ...
File sync/internal_api/activation_context.cc (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/activ...
sync/internal_api/activation_context.cc:13: }  // namespace syncer
nit: Should be syncer_v2

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
File sync/internal_api/public/activation_context.h (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
sync/internal_api/public/activation_context.h:8: #include "base/basictypes.h"
I think you can remove basictypes.h. This file is not using it.
Also for scoped_refptr you need to include base/memory/ref_counted.h

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
sync/internal_api/public/activation_context.h:17: // The state passed from
ModelType Processor to Sync thread during DataType
nit: ModelType Processor => ModelTypeProcessor

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
File sync/internal_api/public/sync_context.h (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
sync/internal_api/public/sync_context.h:31: scoped_ptr<ActivationContext>
activation_context) = 0;
nit: When using scoped_ptr it is enough to forward declare enclosed class, you
don't need to include header with class declaration.

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/sync_...
File sync/internal_api/sync_context_proxy_impl.h (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/sync_...
sync/internal_api/sync_context_proxy_impl.h:18: struct DataTypeState;
ModelTypeProcessor and DataTypeState are no longer needed.

https://codereview.chromium.org/1368683003/diff/80001/sync/sessions/model_typ...
File sync/sessions/model_type_registry_unittest.cc (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/sessions/model_typ...
sync/sessions/model_type_registry_unittest.cc:40: syncer_v2::ActivationContext*
context = new syncer_v2::ActivationContext;
Same comment as in the other file. Use scoped_ptr, when possible don't work with
raw pointers.

[stanisc, 2015-09-26 00:16:01]

https://codereview.chromium.org/1368683003/diff/80001/components/sync_driver/...
File components/sync_driver/non_blocking_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/1368683003/diff/80001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:186:
base::ThreadTaskRunnerHandle model_thread_handle_;
On 2015/09/25 18:22:38, pavely wrote:
> Are you using model_thread_handle_? I didn't find any references.

This was needed to set the task runner on the TLS so that the
base::ThreadTaskRunnerHandle::Get() call in ModelTypeProcessorImpl doesn't crash
in the test.
In the previous implementation this code was in SyncContextProxy. Tests used a
fake proxy which bypassed that code and never actually accessed the current
thread's task runner. Since SyncContextProxy is going away I needed to move
base::ThreadTaskRunnerHandle::Get() to ModelTypeProcessorImpl so that now it
gets executed in the tests.

https://codereview.chromium.org/1368683003/diff/80001/sync/engine/model_type_...
File sync/engine/model_type_processor_impl.cc (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/engine/model_type_...
sync/engine/model_type_processor_impl.cc:56: ActivationContext*
activation_context = new ActivationContext;
On 2015/09/25 18:22:38, pavely wrote:
> Please change this to scoped_ptr<ActivationContext> activation_context(new
> ActivationContext());

Done.

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/activ...
File sync/internal_api/activation_context.cc (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/activ...
sync/internal_api/activation_context.cc:13: }  // namespace syncer
On 2015/09/25 18:22:38, pavely wrote:
> nit: Should be syncer_v2

Done.

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
File sync/internal_api/public/activation_context.h (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
sync/internal_api/public/activation_context.h:8: #include "base/basictypes.h"
On 2015/09/25 18:22:38, pavely wrote:
> I think you can remove basictypes.h. This file is not using it.
> Also for scoped_refptr you need to include base/memory/ref_counted.h

Done.

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
sync/internal_api/public/activation_context.h:17: // The state passed from
ModelType Processor to Sync thread during DataType
On 2015/09/25 18:22:39, pavely wrote:
> nit: ModelType Processor => ModelTypeProcessor

Done.

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
File sync/internal_api/public/sync_context.h (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/publi...
sync/internal_api/public/sync_context.h:31: scoped_ptr<ActivationContext>
activation_context) = 0;
On 2015/09/25 18:22:39, pavely wrote:
> nit: When using scoped_ptr it is enough to forward declare enclosed class, you
> don't need to include header with class declaration.

I wanted to avoid having to include activation_context.h everywhere
sync_context.h and sync_context_proxy.h are included.
But OK, I've changed this.

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/sync_...
File sync/internal_api/sync_context_proxy_impl.h (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/internal_api/sync_...
sync/internal_api/sync_context_proxy_impl.h:18: struct DataTypeState;
On 2015/09/25 18:22:39, pavely wrote:
> ModelTypeProcessor and DataTypeState are no longer needed.

Done.

https://codereview.chromium.org/1368683003/diff/80001/sync/sessions/model_typ...
File sync/sessions/model_type_registry_unittest.cc (right):

https://codereview.chromium.org/1368683003/diff/80001/sync/sessions/model_typ...
sync/sessions/model_type_registry_unittest.cc:40: syncer_v2::ActivationContext*
context = new syncer_v2::ActivationContext;
On 2015/09/25 18:22:39, pavely wrote:
> Same comment as in the other file. Use scoped_ptr, when possible don't work
with
> raw pointers.

Done.

[pavely, 2015-09-28 17:04:42]

lgtm

https://codereview.chromium.org/1368683003/diff/80001/components/sync_driver/...
File components/sync_driver/non_blocking_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/1368683003/diff/80001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller_unittest.cc:186:
base::ThreadTaskRunnerHandle model_thread_handle_;
On 2015/09/26 00:16:01, stanisc wrote:
> On 2015/09/25 18:22:38, pavely wrote:
> > Are you using model_thread_handle_? I didn't find any references.
> 
> This was needed to set the task runner on the TLS so that the
> base::ThreadTaskRunnerHandle::Get() call in ModelTypeProcessorImpl doesn't
crash
> in the test.
> In the previous implementation this code was in SyncContextProxy. Tests used a
> fake proxy which bypassed that code and never actually accessed the current
> thread's task runner. Since SyncContextProxy is going away I needed to move
> base::ThreadTaskRunnerHandle::Get() to ModelTypeProcessorImpl so that now it
> gets executed in the tests.

Acknowledged.

[stanisc, 2015-09-28 17:49:46]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2015-09-28 17:50:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1368683003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1368683003/100001

[commit-bot: I haz the power, 2015-09-28 19:28:56]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2015-09-28 19:31:04]

Patchset 2 (id:??) landed as
https://crrev.com/a4e4a10e92844d0274895c1d078d9119fa76f041
Cr-Commit-Position: refs/heads/master@{#351110}