[Sync] SyncEngine refactor part 1: interfaces.

[Sync] SyncEngine refactor part 1: interfaces.

- SyncBackendHost -> SyncEngine
- SyncFrontend -> SyncEngineHost
- BackendDataTypeConfigurer -> ModelTypeConfigurer
- SyncBackendHostMock -> FakeSyncEngine

Note that this CL intentionally does not update variable and method
names inside ProfileSyncService because there are so many that I'm
making it a separate CL.

BUG=669967
Committed: https://crrev.com/6dcda76d27564c8a9d854803762fa64bf62c670e
Cr-Commit-Position: refs/heads/master@{#436396}

[maxbogue, 2016-11-28 21:57:00]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 21:58:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 01:07:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 01:07:06]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-12-02 19:03:42]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 19:04:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-02 19:39:51]

Description was changed from

==========
[Sync] SyncEngine refactor part 1: interfaces.

BUG=
==========

to

==========
[Sync] SyncEngine refactor part 1: interfaces.

- SyncBackendHost -> SyncEngine
- SyncFrontend -> SyncEngineHost
- BackendDataTypeConfigurer -> ModelTypeConfigurer
- SyncBackendHostMock -> FakeSyncEngine

Note that this CL intentionally does not update variable and method
names inside ProfileSyncService because there are so many that I'm
making it a separate CL.

BUG=669967
==========

[maxbogue, 2016-12-02 19:40:12]

maxbogue@chromium.org changed reviewers:
+ skym@chromium.org

[maxbogue, 2016-12-02 19:40:13]

Sky, PTAL!

[commit-bot: I haz the power, 2016-12-02 19:58:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 19:58:54]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-12-02 20:37:51]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 20:38:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-12-02 20:54:55]

As we talked about offline, the deletion of sync_backend_host_mock.cc needs to
added.

Pretty much just comments about comments. Sorry for all the nits.

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:832: // into ShutdownImpl or
SyncEngine itself.
This is so ambiguous and confusing with this name. Are you advocating for moving
this logic into the SyncEngine facade? Or the interface? Or onto some logic
that's inside the "engine"?

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:842: bool backend_initialized_;
Should his be engine_initialized_? Also, does this mean we've round tripped to
the sync thread and the directory is up? Or we just have passed the init args to
the UI thread facade of the engine?

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_core.h:228: // Enables the
forwarding of directory type debug counters to the SyncEngine.
But, this class, SyncBackxendHostCore, is part of what constitutes sync's
engine. Everything on the sync thread is part of our engine, right? This
interface name doesn't make sense for anything that's actually in the engine to
refer to. Maybe it'd be better we had left hand and right hand side interfaces
the SyncBackendHostImpl.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:167: // This should only
be called by the host.
Do we really need this comment? This isn't verifying that we're actually being
called by the PSS, we're only doing a thread check. It should be interfaces and
accessibility that forces this.

Are any of these methods allowed to be called from the wrong thread anymore? I
thought you fixed that part.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:168:
DCHECK(thread_checker_.CalledOnValidThread());
Is it just me, or is it a bit odd that these CalledOnValidThread checks are not
the first thing happening in these methods? Is it okay to call from the wrong
thread when nigori is disabled?

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:192: // This should only
be called by the host.
Same

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:193:
DCHECK(thread_checker_.CalledOnValidThread());
Same

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:51: // The only real
implementation of the SyncEngine.  See that interface's
You don't normally leave in double space sentence starts when you edit lines.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:57: // Create a SyncEngine
with a reference to the |host| that it serves and
Where's the reference to the host in this ctor? Is this comment obsolete?

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:70: void Initialize(
I'm still a bit confused why exactly the second step of initialization needs to
be part of the interface. But I guess if it fits, okay.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:312: // The host which we
serve (and are owned by).
Might be a good place to comment about the lifecycle of this field.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/sync_api_component_factory.h:81: // Instantiates a new
DataTypeManager with a SyncEngine, a list of data
This is kind of stating the obvious. Yes, we know it takes a SyncEngine and a
DataTypeManagerObserser, those are literally the arguments below. Doesn't
actually take a list of DTCs, but actually a map. Might be valuable to just
remove the first sentence here.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service.h:203: // Returns true if the SyncEngine has
told us it's ready to accept changes.
SyncEngine actually works great here. Could probably get away with "sync engine"
instead as well if you wanted. Mentioning how SyncService is implemented isn't
precisely the job of this interface, I think.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine.h (left):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:81: // Called on |frontend_loop_|.
Yeah! Like this!

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:34: // An API to "host" the top level
SyncAPI element.
Still using the word host. Can you rewrite all of SyncEngine's class comments?
It should be clear from the comments why SyncEngine is a good name.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:40: typedef SyncStatus Status;
This typedef is weird.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:49: // Called on the host's thread to kick
off asynchronous initialization.
All methods on this class should now be on the host's thread. Why does this
class need 2 step init again?

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:72: // Called on the host's thread to
trigger a refresh.
I'd just remove all of these comments. They no longer provide value, right?
Everything should be on the same thread, this should be a single threaded object
now.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:75: // Called on the host's thread to
update SyncCredentials.
Same.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine_host.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:30: // entity
Line breaks here should get cleaned up.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:32: // NOTE: All methods will be
invoked by a SyncEngine on the same thread used to
I worry that this comment is pretty confusing if you don't already know what's
going on.

The way I see it, SyncEngineHost is essentially the list of the things that the
engine should notify the driver component of when they occur. It is the engine's
responsibility to make sure these get called at the appropriate times.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:55: // Informs the frontned of some
network event.  These notifications are
This misspelling evaded you!

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:127: // Inform the host that new
datatypes are available for registration.
I like the 'Called when...' phrasing more than this.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/syncabl...
File components/sync/syncable/test_user_share.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/syncabl...
components/sync/syncable/test_user_share.h:71: // methods normally handled via
the SyncEngine
Should this have a period at the end?

[commit-bot: I haz the power, 2016-12-02 21:33:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 21:33:40]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-12-02 23:59:10]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[maxbogue, 2016-12-02 23:59:28]

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:832: // into ShutdownImpl or
SyncEngine itself.
On 2016/12/02 20:54:54, skym wrote:
> This is so ambiguous and confusing with this name. Are you advocating for
moving
> this logic into the SyncEngine facade? Or the interface? Or onto some logic
> that's inside the "engine"?

Clarified it to "sync engine" (aka the last of the options you mentioned).

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:842: bool backend_initialized_;
On 2016/12/02 20:54:54, skym wrote:
> Should his be engine_initialized_? Also, does this mean we've round tripped to
> the sync thread and the directory is up? Or we just have passed the init args
to
> the UI thread facade of the engine?

As I said in the CL description, PSS naming updates will be coming in a followup
because there are so many. It means a round-trip has been done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_core.h:228: // Enables the
forwarding of directory type debug counters to the SyncEngine.
On 2016/12/02 20:54:54, skym wrote:
> But, this class, SyncBackxendHostCore, is part of what constitutes sync's
> engine. Everything on the sync thread is part of our engine, right? This
> interface name doesn't make sense for anything that's actually in the engine
to
> refer to. Maybe it'd be better we had left hand and right hand side interfaces
> the SyncBackendHostImpl.

Fixed to SyncEngineHost (the thing listening to the sync engine).

And your last sentence is what this entire refactoring project is allll about :)

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:167: // This should only
be called by the host.
On 2016/12/02 20:54:54, skym wrote:
> Do we really need this comment? This isn't verifying that we're actually being
> called by the PSS, we're only doing a thread check. It should be interfaces
and
> accessibility that forces this.
> 
> Are any of these methods allowed to be called from the wrong thread anymore? I
> thought you fixed that part.

Removed it. They aren't allowed to be called from the wrong thread.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:168:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/12/02 20:54:54, skym wrote:
> Is it just me, or is it a bit odd that these CalledOnValidThread checks are
not
> the first thing happening in these methods? Is it okay to call from the wrong
> thread when nigori is disabled?

No you're right. Moved to the top.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:192: // This should only
be called by the host.
On 2016/12/02 20:54:54, skym wrote:
> Same

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:193:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/12/02 20:54:54, skym wrote:
> Same

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:51: // The only real
implementation of the SyncEngine.  See that interface's
On 2016/12/02 20:54:54, skym wrote:
> You don't normally leave in double space sentence starts when you edit lines.

You know me too well.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:57: // Create a SyncEngine
with a reference to the |host| that it serves and
On 2016/12/02 20:54:54, skym wrote:
> Where's the reference to the host in this ctor? Is this comment obsolete?

Removing the whole comment.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:70: void Initialize(
On 2016/12/02 20:54:54, skym wrote:
> I'm still a bit confused why exactly the second step of initialization needs
to
> be part of the interface. But I guess if it fits, okay.

I dunno about before but now it's important because the second step will
effectively be going straight to the sync thread.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:312: // The host which we
serve (and are owned by).
On 2016/12/02 20:54:54, skym wrote:
> Might be a good place to comment about the lifecycle of this field.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/sync_api_component_factory.h:81: // Instantiates a new
DataTypeManager with a SyncEngine, a list of data
On 2016/12/02 20:54:54, skym wrote:
> This is kind of stating the obvious. Yes, we know it takes a SyncEngine and a
> DataTypeManagerObserser, those are literally the arguments below. Doesn't
> actually take a list of DTCs, but actually a map. Might be valuable to just
> remove the first sentence here.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service.h:203: // Returns true if the SyncEngine has
told us it's ready to accept changes.
On 2016/12/02 20:54:55, skym wrote:
> SyncEngine actually works great here. Could probably get away with "sync
engine"
> instead as well if you wanted. Mentioning how SyncService is implemented isn't
> precisely the job of this interface, I think.

Removed the second sentence.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine.h (left):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:81: // Called on |frontend_loop_|.
On 2016/12/02 20:54:55, skym wrote:
> Yeah! Like this!

Acknowledged.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:34: // An API to "host" the top level
SyncAPI element.
On 2016/12/02 20:54:55, skym wrote:
> Still using the word host. Can you rewrite all of SyncEngine's class comments?
> It should be clear from the comments why SyncEngine is a good name.

Done, PTAL.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:40: typedef SyncStatus Status;
On 2016/12/02 20:54:55, skym wrote:
> This typedef is weird.

Super weird but I ain't touching it in this CL.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:49: // Called on the host's thread to kick
off asynchronous initialization.
On 2016/12/02 20:54:55, skym wrote:
> All methods on this class should now be on the host's thread. Why does this
> class need 2 step init again?

I don't know why before; going forward it's how the core will be initialized on
the sync thread so I'm keeping it. Updated the comment though.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:72: // Called on the host's thread to
trigger a refresh.
On 2016/12/02 20:54:55, skym wrote:
> I'd just remove all of these comments. They no longer provide value, right?
> Everything should be on the same thread, this should be a single threaded
object
> now.

I re-wrote them to be more descriptive instead of just removing them.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:75: // Called on the host's thread to
update SyncCredentials.
On 2016/12/02 20:54:55, skym wrote:
> Same.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine_host.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:30: // entity
On 2016/12/02 20:54:55, skym wrote:
> Line breaks here should get cleaned up.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:32: // NOTE: All methods will be
invoked by a SyncEngine on the same thread used to
On 2016/12/02 20:54:55, skym wrote:
> I worry that this comment is pretty confusing if you don't already know what's
> going on.
> 
> The way I see it, SyncEngineHost is essentially the list of the things that
the
> engine should notify the driver component of when they occur. It is the
engine's
> responsibility to make sure these get called at the appropriate times.

Changed it, PTAL.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:55: // Informs the frontned of some
network event.  These notifications are
On 2016/12/02 20:54:55, skym wrote:
> This misspelling evaded you!

I'm personally offended by its audacity.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:127: // Inform the host that new
datatypes are available for registration.
On 2016/12/02 20:54:55, skym wrote:
> I like the 'Called when...' phrasing more than this.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/syncabl...
File components/sync/syncable/test_user_share.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/syncabl...
components/sync/syncable/test_user_share.h:71: // methods normally handled via
the SyncEngine
On 2016/12/02 20:54:55, skym wrote:
> Should this have a period at the end?

PFFFT punctuation who does that

[maxbogue, 2016-12-02 23:59:29]

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:832: // into ShutdownImpl or
SyncEngine itself.
On 2016/12/02 20:54:54, skym wrote:
> This is so ambiguous and confusing with this name. Are you advocating for
moving
> this logic into the SyncEngine facade? Or the interface? Or onto some logic
> that's inside the "engine"?

Clarified it to "sync engine" (aka the last of the options you mentioned).

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:842: bool backend_initialized_;
On 2016/12/02 20:54:54, skym wrote:
> Should his be engine_initialized_? Also, does this mean we've round tripped to
> the sync thread and the directory is up? Or we just have passed the init args
to
> the UI thread facade of the engine?

As I said in the CL description, PSS naming updates will be coming in a followup
because there are so many. It means a round-trip has been done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_core.h:228: // Enables the
forwarding of directory type debug counters to the SyncEngine.
On 2016/12/02 20:54:54, skym wrote:
> But, this class, SyncBackxendHostCore, is part of what constitutes sync's
> engine. Everything on the sync thread is part of our engine, right? This
> interface name doesn't make sense for anything that's actually in the engine
to
> refer to. Maybe it'd be better we had left hand and right hand side interfaces
> the SyncBackendHostImpl.

Fixed to SyncEngineHost (the thing listening to the sync engine).

And your last sentence is what this entire refactoring project is allll about :)

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:167: // This should only
be called by the host.
On 2016/12/02 20:54:54, skym wrote:
> Do we really need this comment? This isn't verifying that we're actually being
> called by the PSS, we're only doing a thread check. It should be interfaces
and
> accessibility that forces this.
> 
> Are any of these methods allowed to be called from the wrong thread anymore? I
> thought you fixed that part.

Removed it. They aren't allowed to be called from the wrong thread.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:168:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/12/02 20:54:54, skym wrote:
> Is it just me, or is it a bit odd that these CalledOnValidThread checks are
not
> the first thing happening in these methods? Is it okay to call from the wrong
> thread when nigori is disabled?

No you're right. Moved to the top.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:192: // This should only
be called by the host.
On 2016/12/02 20:54:54, skym wrote:
> Same

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.cc:193:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/12/02 20:54:54, skym wrote:
> Same

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/glue/sync_backend_host_impl.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:51: // The only real
implementation of the SyncEngine.  See that interface's
On 2016/12/02 20:54:54, skym wrote:
> You don't normally leave in double space sentence starts when you edit lines.

You know me too well.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:57: // Create a SyncEngine
with a reference to the |host| that it serves and
On 2016/12/02 20:54:54, skym wrote:
> Where's the reference to the host in this ctor? Is this comment obsolete?

Removing the whole comment.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:70: void Initialize(
On 2016/12/02 20:54:54, skym wrote:
> I'm still a bit confused why exactly the second step of initialization needs
to
> be part of the interface. But I guess if it fits, okay.

I dunno about before but now it's important because the second step will
effectively be going straight to the sync thread.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/glue/sync_backend_host_impl.h:312: // The host which we
serve (and are owned by).
On 2016/12/02 20:54:54, skym wrote:
> Might be a good place to comment about the lifecycle of this field.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/sync_api_component_factory.h:81: // Instantiates a new
DataTypeManager with a SyncEngine, a list of data
On 2016/12/02 20:54:54, skym wrote:
> This is kind of stating the obvious. Yes, we know it takes a SyncEngine and a
> DataTypeManagerObserser, those are literally the arguments below. Doesn't
> actually take a list of DTCs, but actually a map. Might be valuable to just
> remove the first sentence here.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service.h:203: // Returns true if the SyncEngine has
told us it's ready to accept changes.
On 2016/12/02 20:54:55, skym wrote:
> SyncEngine actually works great here. Could probably get away with "sync
engine"
> instead as well if you wanted. Mentioning how SyncService is implemented isn't
> precisely the job of this interface, I think.

Removed the second sentence.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine.h (left):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:81: // Called on |frontend_loop_|.
On 2016/12/02 20:54:55, skym wrote:
> Yeah! Like this!

Acknowledged.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:34: // An API to "host" the top level
SyncAPI element.
On 2016/12/02 20:54:55, skym wrote:
> Still using the word host. Can you rewrite all of SyncEngine's class comments?
> It should be clear from the comments why SyncEngine is a good name.

Done, PTAL.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:40: typedef SyncStatus Status;
On 2016/12/02 20:54:55, skym wrote:
> This typedef is weird.

Super weird but I ain't touching it in this CL.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:49: // Called on the host's thread to kick
off asynchronous initialization.
On 2016/12/02 20:54:55, skym wrote:
> All methods on this class should now be on the host's thread. Why does this
> class need 2 step init again?

I don't know why before; going forward it's how the core will be initialized on
the sync thread so I'm keeping it. Updated the comment though.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:72: // Called on the host's thread to
trigger a refresh.
On 2016/12/02 20:54:55, skym wrote:
> I'd just remove all of these comments. They no longer provide value, right?
> Everything should be on the same thread, this should be a single threaded
object
> now.

I re-wrote them to be more descriptive instead of just removing them.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine.h:75: // Called on the host's thread to
update SyncCredentials.
On 2016/12/02 20:54:55, skym wrote:
> Same.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
File components/sync/engine/sync_engine_host.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:30: // entity
On 2016/12/02 20:54:55, skym wrote:
> Line breaks here should get cleaned up.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:32: // NOTE: All methods will be
invoked by a SyncEngine on the same thread used to
On 2016/12/02 20:54:55, skym wrote:
> I worry that this comment is pretty confusing if you don't already know what's
> going on.
> 
> The way I see it, SyncEngineHost is essentially the list of the things that
the
> engine should notify the driver component of when they occur. It is the
engine's
> responsibility to make sure these get called at the appropriate times.

Changed it, PTAL.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:55: // Informs the frontned of some
network event.  These notifications are
On 2016/12/02 20:54:55, skym wrote:
> This misspelling evaded you!

I'm personally offended by its audacity.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/engine/...
components/sync/engine/sync_engine_host.h:127: // Inform the host that new
datatypes are available for registration.
On 2016/12/02 20:54:55, skym wrote:
> I like the 'Called when...' phrasing more than this.

Done.

https://codereview.chromium.org/2533083002/diff/20001/components/sync/syncabl...
File components/sync/syncable/test_user_share.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/sync/syncabl...
components/sync/syncable/test_user_share.h:71: // methods normally handled via
the SyncEngine
On 2016/12/02 20:54:55, skym wrote:
> Should this have a period at the end?

PFFFT punctuation who does that

[commit-bot: I haz the power, 2016-12-02 23:59:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-12-03 00:03:47]

lgtm

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2533083002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:842: bool backend_initialized_;
On 2016/12/02 23:59:27, maxbogue wrote:
> On 2016/12/02 20:54:54, skym wrote:
> > Should his be engine_initialized_? Also, does this mean we've round tripped
to
> > the sync thread and the directory is up? Or we just have passed the init
args
> to
> > the UI thread facade of the engine?
> 
> As I said in the CL description, PSS naming updates will be coming in a
followup
> because there are so many. It means a round-trip has been done.

Ah, that's fair. And when I asked what this means, I was trying to imply that
the comment should make that clear, sorry my wording was bad. But I can wait for
the next CL to bug you about this again :)

[commit-bot: I haz the power, 2016-12-03 00:04:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 00:04:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[maxbogue, 2016-12-03 00:04:13]

maxbogue@chromium.org changed reviewers:
+ jwd@chromium.org, raymes@chromium.org, scottbyer@chromium.org

[maxbogue, 2016-12-03 00:04:14]

Adding reviewers for non-sync files:

raymes:
  chrome/browser/permissions/permission_uma_util_unittest.cc
jwd:
  tools/metrics/histograms/histograms.xml
scottbyer:
  chrome/service/cloud_print/cloud_print_proxy_backend.h

[maxbogue, 2016-12-03 00:46:37]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 00:47:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 01:54:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 01:54:29]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-12-05 01:51:16]

lgtm

[Scott Byer, 2016-12-05 18:51:29]

lgtm

[jwd, 2016-12-05 18:57:02]

histograms lgtm

[maxbogue, 2016-12-05 18:57:23]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-12-05 18:57:24]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2533083002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-12-05 18:57:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 20:46:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480964243842060,
"parent_rev": "d2e5b00a67944c841a56c2216e37f74fb07f7817", "commit_rev":
"0ea30e41f2bc0756d8adc3761190aa67bb582738"}

[commit-bot: I haz the power, 2016-12-05 20:46:29]

Description was changed from

==========
[Sync] SyncEngine refactor part 1: interfaces.

- SyncBackendHost -> SyncEngine
- SyncFrontend -> SyncEngineHost
- BackendDataTypeConfigurer -> ModelTypeConfigurer
- SyncBackendHostMock -> FakeSyncEngine

Note that this CL intentionally does not update variable and method
names inside ProfileSyncService because there are so many that I'm
making it a separate CL.

BUG=669967
==========

to

==========
[Sync] SyncEngine refactor part 1: interfaces.

- SyncBackendHost -> SyncEngine
- SyncFrontend -> SyncEngineHost
- BackendDataTypeConfigurer -> ModelTypeConfigurer
- SyncBackendHostMock -> FakeSyncEngine

Note that this CL intentionally does not update variable and method
names inside ProfileSyncService because there are so many that I'm
making it a separate CL.

BUG=669967
==========

[commit-bot: I haz the power, 2016-12-05 20:46:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-05 20:49:55]

Description was changed from

==========
[Sync] SyncEngine refactor part 1: interfaces.

- SyncBackendHost -> SyncEngine
- SyncFrontend -> SyncEngineHost
- BackendDataTypeConfigurer -> ModelTypeConfigurer
- SyncBackendHostMock -> FakeSyncEngine

Note that this CL intentionally does not update variable and method
names inside ProfileSyncService because there are so many that I'm
making it a separate CL.

BUG=669967
==========

to

==========
[Sync] SyncEngine refactor part 1: interfaces.

- SyncBackendHost -> SyncEngine
- SyncFrontend -> SyncEngineHost
- BackendDataTypeConfigurer -> ModelTypeConfigurer
- SyncBackendHostMock -> FakeSyncEngine

Note that this CL intentionally does not update variable and method
names inside ProfileSyncService because there are so many that I'm
making it a separate CL.

BUG=669967

Committed: https://crrev.com/6dcda76d27564c8a9d854803762fa64bf62c670e
Cr-Commit-Position: refs/heads/master@{#436396}
==========

[commit-bot: I haz the power, 2016-12-05 20:49:56]

Patchset 5 (id:??) landed as
https://crrev.com/6dcda76d27564c8a9d854803762fa64bf62c670e
Cr-Commit-Position: refs/heads/master@{#436396}