[Sync] Merge NBDTC and subclasses into ModelTypeController.

[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better TestBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement TestModelTypeProcessor elegantly.

BUG=652398
Committed: https://crrev.com/f16e665b08f0c11aa735afc6ecc76e2905ae95da
Cr-Commit-Position: refs/heads/master@{#423019}

[maxbogue, 2016-10-03 20:21:07]

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

[commit-bot: I haz the power, 2016-10-03 20:21:52]

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

[maxbogue, 2016-10-03 22:10:01]

Description was changed from

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test files as well:

- They no longer use the real SharedModelTypeProcessor implementation.
- They don't even pretend to have a sync thread anymore.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

BUG=652398
==========

to

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- They no longer use the real SharedModelTypeProcessor implementation.
- They don't even pretend to have a sync thread anymore.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

BUG=652398
==========

[maxbogue, 2016-10-03 22:10:18]

Description was changed from

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- They no longer use the real SharedModelTypeProcessor implementation.
- They don't even pretend to have a sync thread anymore.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

BUG=652398
==========

to

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- They no longer use the real SharedModelTypeProcessor implementation.
- They don't even pretend to have a sync thread anymore.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

BUG=652398
==========

[maxbogue, 2016-10-03 22:11:35]

Description was changed from

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- They no longer use the real SharedModelTypeProcessor implementation.
- They don't even pretend to have a sync thread anymore.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

BUG=652398
==========

to

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement MockModelTypeProcessor elegantly.

BUG=652398
==========

[maxbogue, 2016-10-03 22:11:53]

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

[maxbogue, 2016-10-03 22:11:53]

maxbogue@chromium.org changed required reviewers:
+ pavely@chromium.org

[maxbogue, 2016-10-03 22:11:54]

Pavely, Sky, PTAL!

[commit-bot: I haz the power, 2016-10-03 22:21:52]

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

[commit-bot: I haz the power, 2016-10-03 22:21:53]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[skym, 2016-10-04 16:28:49]

lgtm

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_type_controller.h (right):

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:25: // more complicated
RunOnModelThread implementation they can create a derived
See below, I don't like this "feature".

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:30: // |model_thread| can be
nullptr if a derived class provides its own
See below, I don't like this being nullable.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:82: // The thread the model type
lives on, or nullptr if a derived class is
Err what? I don't like this. If we have a subclass that doesn't want to set this
model_thread_ field, perhaps we should fix this by breaking this current
ModelTypeController into two separate classes, and then derive this new subclass
from the one that doesn't have model_thread_ and RunOnModelThread().

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_type_controller_unittest.cc (right):

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:39: // Mock change
processor that connects across threads and tracks calls.
Comment about connecting across threads is a little bit odd. I can't seem to
find any thread jumping code in this class, all it does it run the callbacks its
supposed to, the jumping is abstracted away from it.

Also seems like this thing isn't model_type_controller_unittest specific, maybe
it should also be moved out to a separate file?

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:40: class
MockModelTypeProcessor : public FakeModelTypeChangeProcessor,
Mock? Why not Fake?

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:75: class
MockBackendDataTypeConfigurer : public BackendDataTypeConfigurer {
Mock? Why not Fake?

Also, if you took the type in your constructor, you could move this into a
different file as well. Just a thought.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:162: // These threads
can ping-pong for a bit so we run the model thread twice.
This seems a bit hacky. Though I have no idea how to do this more elegantly.

[pavely, 2016-10-04 18:41:12]

lgtm

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_type_controller.h (right):

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:58: virtual bool
RunOnModelThread(const tracked_objects::Location& from_here,
I don't think we currently have/anticipate types that would want to override
this. Let's remove this method. I think the reason it was introduced is because
UIDTC runs callback directly while NonUIDTC posts. After this change everybody
will post.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_type_controller_unittest.cc (right):

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:174:
base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop)));
Can you use run_loop.QuitClosure() instead of this Bind?

[maxbogue, 2016-10-04 20:37:34]

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

[maxbogue, 2016-10-04 20:37:54]

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_type_controller.h (right):

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:25: // more complicated
RunOnModelThread implementation they can create a derived
On 2016/10/04 16:28:48, skym wrote:
> See below, I don't like this "feature".

Done.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:30: // |model_thread| can be
nullptr if a derived class provides its own
On 2016/10/04 16:28:48, skym wrote:
> See below, I don't like this being nullable.

Done.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:58: virtual bool
RunOnModelThread(const tracked_objects::Location& from_here,
On 2016/10/04 18:41:12, pavely wrote:
> I don't think we currently have/anticipate types that would want to override
> this. Let's remove this method. I think the reason it was introduced is
because
> UIDTC runs callback directly while NonUIDTC posts. After this change everybody
> will post.

Done.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller.h:82: // The thread the model type
lives on, or nullptr if a derived class is
On 2016/10/04 16:28:48, skym wrote:
> Err what? I don't like this. If we have a subclass that doesn't want to set
this
> model_thread_ field, perhaps we should fix this by breaking this current
> ModelTypeController into two separate classes, and then derive this new
subclass
> from the one that doesn't have model_thread_ and RunOnModelThread().

Done.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_type_controller_unittest.cc (right):

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:39: // Mock change
processor that connects across threads and tracks calls.
On 2016/10/04 16:28:48, skym wrote:
> Comment about connecting across threads is a little bit odd. I can't seem to
> find any thread jumping code in this class, all it does it run the callbacks
its
> supposed to, the jumping is abstracted away from it.
> 
> Also seems like this thing isn't model_type_controller_unittest specific,
maybe
> it should also be moved out to a separate file?

Nobody else will need this afaict (wanting to actually test connect logic is
rare) so I'm leaving it here. I updated the comment though.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:40: class
MockModelTypeProcessor : public FakeModelTypeChangeProcessor,
On 2016/10/04 16:28:48, skym wrote:
> Mock? Why not Fake?

Because it implements FakeModelTypeProcessor :P Renamed to
TestModelTypeProcessor.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:75: class
MockBackendDataTypeConfigurer : public BackendDataTypeConfigurer {
On 2016/10/04 16:28:48, skym wrote:
> Mock? Why not Fake?
> 
> Also, if you took the type in your constructor, you could move this into a
> different file as well. Just a thought.

Renamed to TestBackendDataTypeConfigurer. Again, I don't think anyone else needs
this, so I'm not gonna make a new file for it.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:162: // These threads
can ping-pong for a bit so we run the model thread twice.
On 2016/10/04 16:28:48, skym wrote:
> This seems a bit hacky. Though I have no idea how to do this more elegantly.

Acknowledged.

https://codereview.chromium.org/2384153004/diff/1/components/sync/driver/mode...
components/sync/driver/model_type_controller_unittest.cc:174:
base::Bind(&base::RunLoop::Quit, base::Unretained(&run_loop)));
On 2016/10/04 18:41:12, pavely wrote:
> Can you use run_loop.QuitClosure() instead of this Bind?

Done.

[commit-bot: I haz the power, 2016-10-04 20:38:42]

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

[maxbogue, 2016-10-04 20:43:10]

The CQ bit was unchecked by maxbogue@chromium.org

[maxbogue, 2016-10-04 20:43:14]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-10-04 20:43:15]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2384153004/#ps20001
(title: "Address comments + rebase.")

[commit-bot: I haz the power, 2016-10-04 20:43:37]

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

[maxbogue, 2016-10-04 20:44:01]

Description was changed from

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better MockBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement MockModelTypeProcessor elegantly.

BUG=652398
==========

to

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better TestBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement TestModelTypeProcessor elegantly.

BUG=652398
==========

[commit-bot: I haz the power, 2016-10-04 22:21:02]

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

[commit-bot: I haz the power, 2016-10-04 22:21:04]

Failed to apply patch for components/sync/core/shared_model_type_processor.cc:
While running git apply --index -3 -p1;
  error: patch failed: components/sync/core/shared_model_type_processor.cc:20
  Falling back to three-way merge...
  Applied patch to 'components/sync/core/shared_model_type_processor.cc' with
conflicts.
  U components/sync/core/shared_model_type_processor.cc

Patch:       components/sync/core/shared_model_type_processor.cc
Index: components/sync/core/shared_model_type_processor.cc
diff --git a/components/sync/core/shared_model_type_processor.cc
b/components/sync/core/shared_model_type_processor.cc
index
3d86598a12ba5e95d52fa3ff502842c1fc03dcee..fcb6cb81610cf24fbebaacb88ed89c6b2cd20919
100644
--- a/components/sync/core/shared_model_type_processor.cc
+++ b/components/sync/core/shared_model_type_processor.cc
@@ -13,6 +13,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/sync/core/activation_context.h"
+#include "components/sync/core/model_type_processor_proxy.h"
 #include "components/sync/core/processor_entity_tracker.h"
 #include "components/sync/engine/commit_queue.h"
 #include "components/sync/protocol/proto_value_conversions.h"
@@ -20,63 +21,6 @@
 
 namespace syncer {
 
-namespace {
-
-class ModelTypeProcessorProxy : public ModelTypeProcessor {
- public:
-  ModelTypeProcessorProxy(
-      const base::WeakPtr<ModelTypeProcessor>& processor,
-      const scoped_refptr<base::SequencedTaskRunner>& processor_task_runner);
-  ~ModelTypeProcessorProxy() override;
-
-  void ConnectSync(std::unique_ptr<CommitQueue> worker) override;
-  void DisconnectSync() override;
-  void OnCommitCompleted(const sync_pb::DataTypeState& type_state,
-                         const CommitResponseDataList& response_list) override;
-  void OnUpdateReceived(const sync_pb::DataTypeState& type_state,
-                        const UpdateResponseDataList& updates) override;
-
- private:
-  base::WeakPtr<ModelTypeProcessor> processor_;
-  scoped_refptr<base::SequencedTaskRunner> processor_task_runner_;
-};
-
-ModelTypeProcessorProxy::ModelTypeProcessorProxy(
-    const base::WeakPtr<ModelTypeProcessor>& processor,
-    const scoped_refptr<base::SequencedTaskRunner>& processor_task_runner)
-    : processor_(processor), processor_task_runner_(processor_task_runner) {}
-
-ModelTypeProcessorProxy::~ModelTypeProcessorProxy() {}
-
-void ModelTypeProcessorProxy::ConnectSync(std::unique_ptr<CommitQueue> worker)
{
-  processor_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&ModelTypeProcessor::ConnectSync, processor_,
-                            base::Passed(std::move(worker))));
-}
-
-void ModelTypeProcessorProxy::DisconnectSync() {
-  processor_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&ModelTypeProcessor::DisconnectSync, processor_));
-}
-
-void ModelTypeProcessorProxy::OnCommitCompleted(
-    const sync_pb::DataTypeState& type_state,
-    const CommitResponseDataList& response_list) {
-  processor_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&ModelTypeProcessor::OnCommitCompleted, processor_,
-                            type_state, response_list));
-}
-
-void ModelTypeProcessorProxy::OnUpdateReceived(
-    const sync_pb::DataTypeState& type_state,
-    const UpdateResponseDataList& updates) {
-  processor_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&ModelTypeProcessor::OnUpdateReceived, processor_,
-                            type_state, updates));
-}
-
-}  // namespace
-
 SharedModelTypeProcessor::SharedModelTypeProcessor(ModelType type,
                                                    ModelTypeService* service)
     : type_(type),

[maxbogue, 2016-10-04 23:11:27]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-10-04 23:11:28]

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

[commit-bot: I haz the power, 2016-10-04 23:11:49]

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

[maxbogue, 2016-10-04 23:15:34]

The CQ bit was unchecked by maxbogue@chromium.org

[maxbogue, 2016-10-04 23:25:47]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-10-04 23:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2384153004/#ps60001
(title: "Fix bad rebase.")

[commit-bot: I haz the power, 2016-10-04 23:26:24]

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-10-05 00:29:43]

Description was changed from

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better TestBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement TestModelTypeProcessor elegantly.

BUG=652398
==========

to

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better TestBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement TestModelTypeProcessor elegantly.

BUG=652398
==========

[commit-bot: I haz the power, 2016-10-05 00:29:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-05 00:33:45]

Description was changed from

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better TestBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement TestModelTypeProcessor elegantly.

BUG=652398
==========

to

==========
[Sync] Merge NBDTC and subclasses into ModelTypeController.

ModelTypeController simply takes a model thread into its constructor and
treats the UI thread the same as any other.

This CL contains significant cleanup of the test file as well, which contains
all the test cases from NonUiModelTypeControllerTest and
NonBlockingDataTypeControllerTest:

- It no longer uses the real SharedModelTypeProcessor implementation.
- It no longer pretends to have a sync thread that's really just the UI thread.
- There's a private section to make it clear which pieces are internal.
- A better TestBackendDataTypeConfigurer with no MockSyncBackend.

Note that ModelTypeProcessorProxy was promoted to its own file in order
to implement TestModelTypeProcessor elegantly.

BUG=652398

Committed: https://crrev.com/f16e665b08f0c11aa735afc6ecc76e2905ae95da
Cr-Commit-Position: refs/heads/master@{#423019}
==========

[commit-bot: I haz the power, 2016-10-05 00:33:46]

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