[Sync] Convert NonBlockingDataTypeController to be single-thread.

[Sync] Convert NonBlockingDataTypeController to be single-thread.

This change makes ModelTypeService support WeakPtr, which I believe
would have been necessary eventually anyways.

After this change, the only calls left on the model thread are the
DataTypeErrorHandler calls.

BUG=638771
Committed: https://crrev.com/102ca3cf8350354d28ae382fda25a355d1c4069a
Cr-Commit-Position: refs/heads/master@{#413777}

[maxbogue, 2016-08-21 17:41:59]

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

[commit-bot: I haz the power, 2016-08-21 17:42:06]

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-08-21 17:52:16]

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

[commit-bot: I haz the power, 2016-08-21 17:52:17]

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...)

[maxbogue, 2016-08-21 18:03:52]

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

[commit-bot: I haz the power, 2016-08-21 18:04:02]

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-08-21 20:09:02]

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

[commit-bot: I haz the power, 2016-08-21 20:09:03]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-08-21 22:47:11]

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

[maxbogue, 2016-08-21 22:47:12]

Hey Sky, PTAL!

[skym, 2016-08-22 17:34:17]

https://codereview.chromium.org/2254083002/diff/60001/components/sync/api/mod...
File components/sync/api/model_type_service.h (right):

https://codereview.chromium.org/2254083002/diff/60001/components/sync/api/mod...
components/sync/api/model_type_service.h:33: class ModelTypeService : public
base::SupportsWeakPtr<ModelTypeService> {
In addition to talking to zea@ as we discussed offline, can you clean up
DeviceInfoService to now use this method to vend weakptrs? It currently has a
weak_factory_ member field which is now redundant.

https://codereview.chromium.org/2254083002/diff/60001/components/sync/api/mod...
components/sync/api/model_type_service.h:116: void
OnSyncStarting(syncer::DataTypeErrorHandler* error_handler,
So is the next step then changing the first argument of OnSyncStarting to be a
base::WeakPtr<syncer::DataTypeErrorHandler> ?

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
File components/sync/driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
components/sync/driver/non_blocking_data_type_controller.cc:71:
base::Bind(&syncer_v2::ModelTypeService::OnSyncStarting,
Looks like you've removed the error checking (and TODO) if a service gets
destroyed as we start up. I'm leaning towards not caring, what was your
rationale?

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
File components/sync/driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
components/sync/driver/non_blocking_data_type_controller.h:22: class
ModelTypeService;
Did you mean to leave this in here?

[maxbogue, 2016-08-22 23:21:41]

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

[commit-bot: I haz the power, 2016-08-22 23:22:18]

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-08-23 02:14:48]

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

[commit-bot: I haz the power, 2016-08-23 02:14:49]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-08-23 16:58:52]

https://codereview.chromium.org/2254083002/diff/60001/components/sync/api/mod...
File components/sync/api/model_type_service.h (right):

https://codereview.chromium.org/2254083002/diff/60001/components/sync/api/mod...
components/sync/api/model_type_service.h:33: class ModelTypeService : public
base::SupportsWeakPtr<ModelTypeService> {
On 2016/08/22 17:34:17, skym wrote:
> In addition to talking to zea@ as we discussed offline, can you clean up
> DeviceInfoService to now use this method to vend weakptrs? It currently has a
> weak_factory_ member field which is now redundant.

Done. Talked to zea@ on chat and I think this approach is fine for now.

https://codereview.chromium.org/2254083002/diff/60001/components/sync/api/mod...
components/sync/api/model_type_service.h:116: void
OnSyncStarting(syncer::DataTypeErrorHandler* error_handler,
On 2016/08/22 17:34:16, skym wrote:
> So is the next step then changing the first argument of OnSyncStarting to be a
> base::WeakPtr<syncer::DataTypeErrorHandler> ?

Actually, since part of the error handling is done on the model thread (it dumps
the stack trace), I'm making a DataTypeErrorHandlerImpl object instead of having
the DTCs implement the interface.

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
File components/sync/driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
components/sync/driver/non_blocking_data_type_controller.cc:71:
base::Bind(&syncer_v2::ModelTypeService::OnSyncStarting,
On 2016/08/22 17:34:17, skym wrote:
> Looks like you've removed the error checking (and TODO) if a service gets
> destroyed as we start up. I'm leaning towards not caring, what was your
> rationale?

The null check is handled by the WeakPtr. I don't think the TODO matters, since
if the service is null then the entire browser is probably shutting down...

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
File components/sync/driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/2254083002/diff/60001/components/sync/driver/...
components/sync/driver/non_blocking_data_type_controller.h:22: class
ModelTypeService;
On 2016/08/22 17:34:17, skym wrote:
> Did you mean to leave this in here?

Newp. Can't even remember why I would have added it.

[skym, 2016-08-23 17:09:33]

lgtm

[maxbogue, 2016-08-23 17:26:54]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2016-08-23 17:27:32]

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-08-23 17:33:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-23 17:36:31]

Description was changed from

==========
[Sync] Convert NonBlockingDataTypeController to be single-thread.

This change makes ModelTypeService support WeakPtr, which I believe
would have been necessary eventually anyways.

After this change, the only calls left on the model thread are the
DataTypeErrorHandler calls.

BUG=638771
==========

to

==========
[Sync] Convert NonBlockingDataTypeController to be single-thread.

This change makes ModelTypeService support WeakPtr, which I believe
would have been necessary eventually anyways.

After this change, the only calls left on the model thread are the
DataTypeErrorHandler calls.

BUG=638771

Committed: https://crrev.com/102ca3cf8350354d28ae382fda25a355d1c4069a
Cr-Commit-Position: refs/heads/master@{#413777}
==========

[commit-bot: I haz the power, 2016-08-23 17:36:32]

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