sync: Mass rename of non-blocking sync classes

sync: Mass rename of non-blocking sync classes

Renames many of the classes involved in non-blocking sync:
- Renames SyncCore, SyncCoreProxy to SyncContext and SyncContextProxy.
- Renames NonBlockingTypeProcessor and NonBlockingTypeProcessorInterface
  to ModelTypeSyncProxyImpl and ModelTypeSyncProxy, respectively.
- Renames NonBlockingTypeProcessorCore and
  NonBlockingTypeProcessorCoreInterface to ModelTypeSyncWorkerImpl and
  ModelTypeSyncWorker, respectively.
- Renames ModelThreadSyncEntity to ModelTypeEntity.
- Renames SyncThreadSyncEntity to EntityTracker.

Renames any Mock, Test, Wrapper, or Impl classes associated with the
above, too.

This is only the first part of the planned refactoring.  The second
part, which involves some changes to the inheritance hierarchy, will be
implemented in a future CL.

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

[rlarocque, 2014-06-23 22:57:58]

Here are the renames we discussed last week.

Most of this refactoring was done using automated tools, like
move_source_file.py, git cl format, and a global search and replace.  I had to
amend the comments and variable names by hand, so those parts may contain bugs.

I decided to limit the scope of this patch to renames only.  I will implement
the planned changes to the inheritance hierarchy in another CL.

Please review.

[stanisc, 2014-06-24 21:59:01]

LGTM. 
I have a few minor comments - feel free addressing them in the next change.

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

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:653: sync_context_proxy_ =
sync_context_proxy->Clone();
Preexisting: should sync_context_proxy be cloned in non-frontend case? Perhaps
this code should be moved to after the frontend check below.

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:402:
non_blocking_data_type_manager_.InitializeTypeSyncProxy(
Should non_blocking_data_type_manager_ be renamed too?

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:64: void
InitializeTypeSyncProxy(
Function names don't have to repeat Proxy or SyncProxy everywhere - that seems
redundant. How about names like InitializeType, InitializeSyncContext,
ClearSyncContext?

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:132:
scoped_ptr<syncer::SyncContextProxy> sync_context_proxy_;
I wonder if sync_context_proxy_ and type_sync_proxy_ should have "proxy" in
their field names. That seems redundant.

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_manager.h (right):

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_manager.h:47: void
InitializeTypeSyncProxy(
Consider a shorter function name, for example InitializeType.

https://codereview.chromium.org/351523003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_worker.h (right):

https://codereview.chromium.org/351523003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker.h:12: // An interface representing a
ModelTypeSyncWorker and its thread.
ModelTypeSyncWorker - an interface representing a ModelTypeSyncWorker...
Is this correct?

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/sync_context_proxy.h (right):

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/public...
sync/internal_api/public/sync_context_proxy.h:24: // Attempts to connect a
non-blocking type to the sync core.
Does this comment still apply?

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/sync_c...
File sync/internal_api/sync_context.h (right):

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/sync_c...
sync/internal_api/sync_context.h:33: // Initializes the connection between the
sync context and its delegate on
This comment needs to be more specific. The connection is type specific and the
comment doesn't mention that.

[Nicolas Zea, 2014-06-24 22:42:48]

LGTM, with the caveat that in general scoped_refptrs and WeakPtrs should be
passed by const ref (I commented some, but stopped after a bit).

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

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:348:
base::WeakPtr<syncer::ModelTypeSyncProxyImpl> proxy);
nit: both of these should be passed by const ref

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:66:
base::WeakPtr<syncer::ModelTypeSyncProxyImpl> type_sync_proxy);
nit: const ref here too

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
File sync/internal_api/sync_context.h (right):

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
sync/internal_api/sync_context.h:40: base::WeakPtr<ModelTypeSyncProxyImpl>
type_sync_proxy);
const ref for both

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
File sync/internal_api/sync_context_proxy_impl.h (right):

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
sync/internal_api/sync_context_proxy_impl.h:30: base::WeakPtr<SyncContext>
sync_context);
const ref for both

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
sync/internal_api/sync_context_proxy_impl.h:43:
base::WeakPtr<ModelTypeSyncProxyImpl> sync_proxy_impl) OVERRIDE;
const ref

https://codereview.chromium.org/351523003/diff/40001/sync/sessions/model_type...
File sync/sessions/model_type_registry.h (right):

https://codereview.chromium.org/351523003/diff/40001/sync/sessions/model_type...
sync/sessions/model_type_registry.h:62: base::WeakPtr<ModelTypeSyncProxyImpl>
proxy);
const ref for both

[rlarocque, 2014-06-24 23:08:33]

Two new patches uploaded.

In an attempt to commit quickly and avoid rebase issues, I'm going to try to
commit this as is.  Let me know if you see anything in the latest uploads that
suggests I should cancel the commit.

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

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:653: sync_context_proxy_ =
sync_context_proxy->Clone();
On 2014/06/24 21:59:00, stanisc wrote:
> Preexisting: should sync_context_proxy be cloned in non-frontend case? Perhaps
> this code should be moved to after the frontend check below.

True, these lines should be moved below the frontend_ check.

I this affects performance and readability, but not correctness.  The
sync_context_proxy_ is a scoped_ptr so at least we won't leak memory.

Will fix later.

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:402:
non_blocking_data_type_manager_.InitializeTypeSyncProxy(
On 2014/06/24 21:59:01, stanisc wrote:
> Should non_blocking_data_type_manager_ be renamed too?

We decided against renaming NonBlockingDataTypeController, so I figured
NonBlockingDataTypeManager should remain unchanged, too.

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

https://codereview.chromium.org/351523003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:348:
base::WeakPtr<syncer::ModelTypeSyncProxyImpl> proxy);
On 2014/06/24 22:42:48, Nicolas Zea wrote:
> nit: both of these should be passed by const ref

Done.

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:64: void
InitializeTypeSyncProxy(
On 2014/06/24 21:59:01, stanisc wrote:
> Function names don't have to repeat Proxy or SyncProxy everywhere - that seems
> redundant. How about names like InitializeType, InitializeSyncContext,
> ClearSyncContext?

Good point.  I've implemented the renames you mentioned here and in a few other
places where this naming convention was in use.

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:66:
base::WeakPtr<syncer::ModelTypeSyncProxyImpl> type_sync_proxy);
On 2014/06/24 22:42:48, Nicolas Zea wrote:
> nit: const ref here too

Done.

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:132:
scoped_ptr<syncer::SyncContextProxy> sync_context_proxy_;
On 2014/06/24 21:59:01, stanisc wrote:
> I wonder if sync_context_proxy_ and type_sync_proxy_ should have "proxy" in
> their field names. That seems redundant.

I see your point.  I agree with you in the case of sync_context_proxy_.

I'm on the fence about type_sync_proxy_.  Unlike SyncContext, there's no such
thing as a "TypeSync".  I'm not sure what to call that member if not
type_sync_proxy_.

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_manager.h (right):

https://codereview.chromium.org/351523003/diff/20001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_manager.h:47: void
InitializeTypeSyncProxy(
On 2014/06/24 21:59:01, stanisc wrote:
> Consider a shorter function name, for example InitializeType.

Done.

https://codereview.chromium.org/351523003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_worker.h (right):

https://codereview.chromium.org/351523003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker.h:12: // An interface representing a
ModelTypeSyncWorker and its thread.
On 2014/06/24 21:59:01, stanisc wrote:
> ModelTypeSyncWorker - an interface representing a ModelTypeSyncWorker...
> Is this correct?

Good catch.  I've updated the comment to something more useful.

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/public...
File sync/internal_api/public/sync_context_proxy.h (right):

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/public...
sync/internal_api/public/sync_context_proxy.h:24: // Attempts to connect a
non-blocking type to the sync core.
On 2014/06/24 21:59:01, stanisc wrote:
> Does this comment still apply?

Renamed core to context.

I'm not sure how to feel about the term "non-blocking type".  I think we intend
to keep it, right?

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/sync_c...
File sync/internal_api/sync_context.h (right):

https://codereview.chromium.org/351523003/diff/20001/sync/internal_api/sync_c...
sync/internal_api/sync_context.h:33: // Initializes the connection between the
sync context and its delegate on
On 2014/06/24 21:59:01, stanisc wrote:
> This comment needs to be more specific. The connection is type specific and
the
> comment doesn't mention that.

Comment updated.

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
File sync/internal_api/sync_context.h (right):

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
sync/internal_api/sync_context.h:40: base::WeakPtr<ModelTypeSyncProxyImpl>
type_sync_proxy);
On 2014/06/24 22:42:48, Nicolas Zea wrote:
> const ref for both

Done.

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
File sync/internal_api/sync_context_proxy_impl.h (right):

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
sync/internal_api/sync_context_proxy_impl.h:30: base::WeakPtr<SyncContext>
sync_context);
On 2014/06/24 22:42:48, Nicolas Zea wrote:
> const ref for both

Done.

https://codereview.chromium.org/351523003/diff/40001/sync/internal_api/sync_c...
sync/internal_api/sync_context_proxy_impl.h:43:
base::WeakPtr<ModelTypeSyncProxyImpl> sync_proxy_impl) OVERRIDE;
On 2014/06/24 22:42:48, Nicolas Zea wrote:
> const ref

Done.

https://codereview.chromium.org/351523003/diff/40001/sync/sessions/model_type...
File sync/sessions/model_type_registry.h (right):

https://codereview.chromium.org/351523003/diff/40001/sync/sessions/model_type...
sync/sessions/model_type_registry.h:62: base::WeakPtr<ModelTypeSyncProxyImpl>
proxy);
On 2014/06/24 22:42:48, Nicolas Zea wrote:
> const ref for both

Done.

[rlarocque, 2014-06-24 23:09:20]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-24 23:14:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/351523003/80001

[commit-bot: I haz the power, 2014-06-25 05:52:36]

Change committed as 279618