[Sync] Remove UIDataTypeController

[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505
TBR=sdefresne@chromium.org

Committed: https://crrev.com/fb4af853e48387330a8f89bc55562e1cc4c42f77
Cr-Commit-Position: refs/heads/master@{#434759}

[pavely, 2016-11-18 01:05:35]

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

[commit-bot: I haz the power, 2016-11-18 01:07:19]

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-18 02:06:24]

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

[commit-bot: I haz the power, 2016-11-18 02:06:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pavely, 2016-11-18 17:33:19]

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

[commit-bot: I haz the power, 2016-11-18 17:34:19]

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-18 17:39:23]

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

[commit-bot: I haz the power, 2016-11-18 17:39:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[maxbogue, 2016-11-18 19:31:04]

Major comments are on NonUIDataTypeController.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h:30:
// NonUIDataTypeController implementations.
super nit: don't we usually say implementation, singular?

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:562:
BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE)));
I think that it runs on the FILE thread is an inherent part of
ExtensionSettingDataTypeController, no? It could just pass it into the
superclass constructor without taking it as a parameter.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/extension_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
chrome/browser/sync/glue/extension_data_type_controller.h:32: //
NonUIDataTypeController implementations.
-s

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:36: const
scoped_refptr<base::SequencedTaskRunner>& file_thread);
I recently learned that it is now preferable to pass ref-counted things by value
(not const-ref), and then use std::move(). See
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/theme_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
chrome/browser/sync/glue/theme_data_type_controller.h:24: //
NonUIDataTypeController implementations.
-s

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h:32: //
NonUIDataTypeController implementations.
-s

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_data_type_controller.cc
(right):

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_data_type_controller.cc:17:
const scoped_refptr<base::SingleThreadTaskRunner>& db_thread,
by value

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h:29:
const scoped_refptr<base::SingleThreadTaskRunner>& db_thread,
by value

https://codereview.chromium.org/2511703004/diff/20001/components/history/core...
File components/history/core/browser/typed_url_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/components/history/core...
components/history/core/browser/typed_url_data_type_controller.h:38: //
NonUIDataTypeController interface.
implementation

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

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:24: class
NonUIDataTypeController : public DirectoryDataTypeController {
This is now a Very Bad Name for this class, as UI types now use it. What would
you think of AsyncDirectoryTypeController instead? (I also think
DirectoryDataTypeController should just be DirectoryTypeController but that
could be a separate CL.)

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:32: const
scoped_refptr<base::SequencedTaskRunner>& backend_thread);
by value

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:71: virtual bool
PostTaskOnBackendThread(
Can we change this to PostTaskOnModelThread?

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:122:
std::unique_ptr<GenericChangeProcessorFactory> processor_factory_;
We have so many processor classes that I actually think "gcp_factory_" would be
clearer here.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:133:
scoped_refptr<base::SequencedTaskRunner> backend_thread_;
I'd prefer model_thread_ for consistency.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
File components/sync/driver/ui_data_type_controller.cc (left):

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/ui_data_type_controller.cc:117: // TODO(robliao): Remove
ScopedTracker below once https://crbug.com/471403 is
worth noting that https://crbug.com/471403 is not fixed; do we care about
removing these?

https://codereview.chromium.org/2511703004/diff/20001/components/sync_session...
File components/sync_sessions/session_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/components/sync_session...
components/sync_sessions/session_data_type_controller.h:29: //
NonUIDataTypeController implementations.
-s

[pavely, 2016-11-18 23:18:54]

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

[commit-bot: I haz the power, 2016-11-18 23:19:44]

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-18 23:24:47]

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

[commit-bot: I haz the power, 2016-11-18 23:24:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pavely, 2016-11-19 00:04:51]

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

[commit-bot: I haz the power, 2016-11-19 00:06:08]

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-19 00:51:30]

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

[commit-bot: I haz the power, 2016-11-19 00:51:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pavely, 2016-11-19 01:09:18]

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

[commit-bot: I haz the power, 2016-11-19 01:09:54]

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

[pavely, 2016-11-19 01:11:43]

I have brought back UIDataTypeController and switched SessionDataTypeController
to it. The reason is that SessionSyncManager is sensitive to StopSyncing() being
called synchronously during ModelAssociationManager::Stop(). I'll address that
in a separate CL.

PTAL.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h:30:
// NonUIDataTypeController implementations.
On 2016/11/18 19:31:03, maxbogue wrote:
> super nit: don't we usually say implementation, singular?

Done.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:562:
BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE)));
On 2016/11/18 19:31:03, maxbogue wrote:
> I think that it runs on the FILE thread is an inherent part of
> ExtensionSettingDataTypeController, no? It could just pass it into the
> superclass constructor without taking it as a parameter.

Done.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/extension_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
chrome/browser/sync/glue/extension_data_type_controller.h:32: //
NonUIDataTypeController implementations.
On 2016/11/18 19:31:03, maxbogue wrote:
> -s

Done.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:36: const
scoped_refptr<base::SequencedTaskRunner>& file_thread);
On 2016/11/18 19:31:03, maxbogue wrote:
> I recently learned that it is now preferable to pass ref-counted things by
value
> (not const-ref), and then use std::move(). See
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/theme_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/sync/glu...
chrome/browser/sync/glue/theme_data_type_controller.h:24: //
NonUIDataTypeController implementations.
On 2016/11/18 19:31:03, maxbogue wrote:
> -s

Done.

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h:32: //
NonUIDataTypeController implementations.
On 2016/11/18 19:31:03, maxbogue wrote:
> -s

Done.

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_data_type_controller.cc
(right):

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_data_type_controller.cc:17:
const scoped_refptr<base::SingleThreadTaskRunner>& db_thread,
On 2016/11/18 19:31:03, maxbogue wrote:
> by value

Done.

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/20001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h:29:
const scoped_refptr<base::SingleThreadTaskRunner>& db_thread,
On 2016/11/18 19:31:03, maxbogue wrote:
> by value

Done.

https://codereview.chromium.org/2511703004/diff/20001/components/history/core...
File components/history/core/browser/typed_url_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/components/history/core...
components/history/core/browser/typed_url_data_type_controller.h:38: //
NonUIDataTypeController interface.
On 2016/11/18 19:31:03, maxbogue wrote:
> implementation

Done.

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

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:24: class
NonUIDataTypeController : public DirectoryDataTypeController {
On 2016/11/18 19:31:03, maxbogue wrote:
> This is now a Very Bad Name for this class, as UI types now use it. What would
> you think of AsyncDirectoryTypeController instead? (I also think
> DirectoryDataTypeController should just be DirectoryTypeController but that
> could be a separate CL.)

Async in AsyncDirectoryTypeController doesn't mean anything (async as opposed to
what?). I would rather prefer "Controller for Directory backed SyncableService
based type" and its counterpart "Controller for Directory based Old API based
type" (FrontendDataTypeController).

That being said... Because of the issues with SessionsSyncManager
UIDataTypeController is back. Renaming will have to wait till better times.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:32: const
scoped_refptr<base::SequencedTaskRunner>& backend_thread);
On 2016/11/18 19:31:03, maxbogue wrote:
> by value

Done.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:71: virtual bool
PostTaskOnBackendThread(
On 2016/11/18 19:31:03, maxbogue wrote:
> Can we change this to PostTaskOnModelThread?

The names don't feel that different to me, but ok.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:122:
std::unique_ptr<GenericChangeProcessorFactory> processor_factory_;
On 2016/11/18 19:31:03, maxbogue wrote:
> We have so many processor classes that I actually think "gcp_factory_" would
be
> clearer here.

The name came from ui_data_type_controller.h and I actually like it better. I
more often refer to a processor by the word "processor". If I see gcp_factory_
in the code it will be harder to recall what gcp is, where is processor will
refer to different class in different controller, but it is usually clear from
context.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/non_ui_data_type_controller.h:133:
scoped_refptr<base::SequencedTaskRunner> backend_thread_;
On 2016/11/18 19:31:03, maxbogue wrote:
> I'd prefer model_thread_ for consistency.

backend_thread_ goes more consistent with PostTaskOnBackendThread() function
name.

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
File components/sync/driver/ui_data_type_controller.cc (left):

https://codereview.chromium.org/2511703004/diff/20001/components/sync/driver/...
components/sync/driver/ui_data_type_controller.cc:117: // TODO(robliao): Remove
ScopedTracker below once https://crbug.com/471403 is
On 2016/11/18 19:31:03, maxbogue wrote:
> worth noting that https://crbug.com/471403 is not fixed; do we care about
> removing these?

The bug tracks the effort to investigate jankiness of model association. There
was no activity on the bug for more than an year. I'd say if that investigation
resumes we'll instrument next target.

https://codereview.chromium.org/2511703004/diff/20001/components/sync_session...
File components/sync_sessions/session_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/20001/components/sync_session...
components/sync_sessions/session_data_type_controller.h:29: //
NonUIDataTypeController implementations.
On 2016/11/18 19:31:04, maxbogue wrote:
> -s

Done.

[commit-bot: I haz the power, 2016-11-19 01:48:07]

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

[commit-bot: I haz the power, 2016-11-19 01:48:08]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pavely, 2016-11-21 19:35:44]

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

[commit-bot: I haz the power, 2016-11-21 19:36:17]

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-21 20:24:56]

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

[commit-bot: I haz the power, 2016-11-21 20:24:58]

Dry run: This issue passed the CQ dry run.

[pavely, 2016-11-21 21:15:22]

Max, PTAL.
I've fixed SyncErrorTests issue. I'll convert sessions in a separate change.

[maxbogue, 2016-11-21 21:44:23]

lgtm w/ nits

https://codereview.chromium.org/2511703004/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h:51:
syncer::SyncClient* sync_client_;
DirectoryDataTypeController already has a protected field for this so I think
you can remove it.

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_wallet_data_type_controller.cc
(right):

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.cc:21:
const scoped_refptr<base::SingleThreadTaskRunner>& db_thread,
Remove const&.

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.cc:29:
db_thread),
std::move(db_thread)

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_wallet_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.h:45:
syncer::SyncClient* const sync_client_;
Remove.

https://codereview.chromium.org/2511703004/diff/100001/components/browser_syn...
File components/browser_sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/2511703004/diff/100001/components/browser_syn...
components/browser_sync/profile_sync_components_factory_impl.cc:60: using
syncer::NonUIDataTypeController;
Move below ModelTypeController.

[pavely, 2016-11-22 00:37:23]

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

[commit-bot: I haz the power, 2016-11-22 00:37:42]

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

[pavely, 2016-11-22 00:53:03]

Description was changed from

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505
==========

to

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505
==========

[pavely, 2016-11-22 00:53:03]

pavely@chromium.org changed reviewers:
+ pkasting@chromium.org, sdefresne@chromium.org, stevenjb@chromium.org,
treib@chromium.org, vabr@chromium.org

[pavely, 2016-11-22 00:53:37]

pavely@chromium.org changed reviewers:
- pkasting@chromium.org, sdefresne@chromium.org, stevenjb@chromium.org,
treib@chromium.org, vabr@chromium.org

[pavely, 2016-11-22 00:53:51]

https://codereview.chromium.org/2511703004/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h:51:
syncer::SyncClient* sync_client_;
On 2016/11/21 21:44:23, maxbogue wrote:
> DirectoryDataTypeController already has a protected field for this so I think
> you can remove it.

Done.

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_wallet_data_type_controller.cc
(right):

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.cc:21:
const scoped_refptr<base::SingleThreadTaskRunner>& db_thread,
On 2016/11/21 21:44:23, maxbogue wrote:
> Remove const&.

Done.

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.cc:29:
db_thread),
On 2016/11/21 21:44:23, maxbogue wrote:
> std::move(db_thread)

Done.

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
File components/autofill/core/browser/autofill_wallet_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/100001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.h:45:
syncer::SyncClient* const sync_client_;
On 2016/11/21 21:44:23, maxbogue wrote:
> Remove.

Done.

https://codereview.chromium.org/2511703004/diff/100001/components/browser_syn...
File components/browser_sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/2511703004/diff/100001/components/browser_syn...
components/browser_sync/profile_sync_components_factory_impl.cc:60: using
syncer::NonUIDataTypeController;
On 2016/11/21 21:44:23, maxbogue wrote:
> Move below ModelTypeController.

Done.

[pavely, 2016-11-22 00:53:59]

pavely@chromium.org changed reviewers:
+ pkasting@chromium.org, sdefresne@chromium.org, stevenjb@chromium.org,
treib@chromium.org, vabr@chromium.org

[pavely, 2016-11-22 00:57:41]

Adding OWNERS reviews. Could you take a look:

treib@: chrome/browser/supervised_user/*

stevenjb@: chrome/browser/ui/app_list/arc/*

sdefresne@: components/history/core/browser/*

pkasting@: components/search_engines/*

vabr@: components/password_manager/sync/browser/*
       components/autofill/core/browser/*

[commit-bot: I haz the power, 2016-11-22 01:33:25]

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

[commit-bot: I haz the power, 2016-11-22 01:33:26]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-11-22 09:05:59]

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc
(left):

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc:554:
DCHECK(!sync_processor_);
Why this change?

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h:21: :
public syncer::NonUIDataTypeController {
Should NonUIDataTypeController be renamed now? IMO the name is misleading if
it's now used for both UI and non-UI thread controllers.

https://codereview.chromium.org/2511703004/diff/120001/components/sync_sessio...
File components/sync_sessions/session_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/120001/components/sync_sessio...
components/sync_sessions/session_data_type_controller.h:20: class
SessionDataTypeController : public syncer::UIDataTypeController {
I'm confused - this still uses UIDataTypeController?!

[vabr (Chromium), 2016-11-22 11:07:31]

components/password_manager/sync/browser/* and
components/autofill/core/browser/* LGTM.

Cheers,
Vaclav

[stevenjb, 2016-11-22 17:07:15]

https://codereview.chromium.org/2511703004/diff/120001/components/sync/driver...
File components/sync/driver/non_ui_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/120001/components/sync/driver...
components/sync/driver/non_ui_data_type_controller.h:23: 
If the goal is to remove UIDataTypeController, can we have a comment here
similar to the one in UIDataTypeController? It would be helpful to describe what
a "Non UI data type controller" is regardless.

[stevenjb, 2016-11-22 17:07:41]

chrome/browser/ui/app_list/arc/* lgtm

[pavely, 2016-11-22 19:17:53]

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc
(left):

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc:554:
DCHECK(!sync_processor_);
On 2016/11/22 09:05:59, Marc Treib wrote:
> Why this change?

GoogleSignedOut is called synchronously to both ProfileSyncService and
SupervisedUserSyncService. ProfileSyncService is lucky to be notified first, it
stops all datatypes and since UI datatypes are notified synchronously as well,
StopSyncing is always called before GoogleSignedOut. This guarantees that
sync_processor_ is cleared before GoogleSignedOut.

After my change StopSyncing is posted on UI thread instead of being directly
called which switches order of notifications. The same would happen if these two
were ordered differently in SigninManagerBase::observer_list. It is still safe
to keep valid sync_processor_. All sync processors are disconnected from backend
during sync shutdown, data passed to sync_processor_ after that will not be
synced or persisted.

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h
(right):

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h:21: :
public syncer::NonUIDataTypeController {
On 2016/11/22 09:05:59, Marc Treib wrote:
> Should NonUIDataTypeController be renamed now? IMO the name is misleading if
> it's now used for both UI and non-UI thread controllers.

I failed to remove all references to UIDataTypeController, switching Sessions to
NonUI caused integration test failures which I'm going to address in separate
CL. I'll rename it once I'm done.

https://codereview.chromium.org/2511703004/diff/120001/components/sync/driver...
File components/sync/driver/non_ui_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/120001/components/sync/driver...
components/sync/driver/non_ui_data_type_controller.h:23: 
On 2016/11/22 17:07:14, stevenjb wrote:
> If the goal is to remove UIDataTypeController, can we have a comment here
> similar to the one in UIDataTypeController? It would be helpful to describe
what
> a "Non UI data type controller" is regardless.

I've added comment explaining NonUIDataTypeController. I'll rename class and
adjust comment after fixing sessions.

https://codereview.chromium.org/2511703004/diff/120001/components/sync_sessio...
File components/sync_sessions/session_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/120001/components/sync_sessio...
components/sync_sessions/session_data_type_controller.h:20: class
SessionDataTypeController : public syncer::UIDataTypeController {
On 2016/11/22 09:05:59, Marc Treib wrote:
> I'm confused - this still uses UIDataTypeController?!

I ran into integration test issues related to SessionsSyncManager
(https://codereview.chromium.org/2511703004/#ps1). 
I'll address them in separate CL.

[Peter Kasting, 2016-11-22 20:16:02]

LGTM

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
File components/search_engines/search_engine_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
components/search_engines/search_engine_data_type_controller.h:33: //
NonUIDataTypeController implementation.
Nit: Just change the name here without modifying the rest of the comment
formatting (various places seem to use different patterns as to how they
construct these comments, and I'd rather not tweak this)

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
File components/search_engines/search_engine_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
components/search_engines/search_engine_data_type_controller_unittest.cc:160:
base::RunLoop().RunUntilIdle();
Does this deserve a comment about why we need to run to idle?

[Marc Treib, 2016-11-23 09:16:14]

lgtm

Thanks!

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
File chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc
(left):

https://codereview.chromium.org/2511703004/diff/120001/chrome/browser/supervi...
chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc:554:
DCHECK(!sync_processor_);
On 2016/11/22 19:17:52, pavely wrote:
> On 2016/11/22 09:05:59, Marc Treib wrote:
> > Why this change?
> 
> GoogleSignedOut is called synchronously to both ProfileSyncService and
> SupervisedUserSyncService. ProfileSyncService is lucky to be notified first,
it
> stops all datatypes and since UI datatypes are notified synchronously as well,
> StopSyncing is always called before GoogleSignedOut. This guarantees that
> sync_processor_ is cleared before GoogleSignedOut.
> 
> After my change StopSyncing is posted on UI thread instead of being directly
> called which switches order of notifications. The same would happen if these
two
> were ordered differently in SigninManagerBase::observer_list. It is still safe
> to keep valid sync_processor_. All sync processors are disconnected from
backend
> during sync shutdown, data passed to sync_processor_ after that will not be
> synced or persisted.

Thanks for the explanation!

[pavely, 2016-11-28 18:56:15]

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

[pavely, 2016-11-28 18:56:23]

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
File components/search_engines/search_engine_data_type_controller.h (right):

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
components/search_engines/search_engine_data_type_controller.h:33: //
NonUIDataTypeController implementation.
On 2016/11/22 20:16:02, Peter Kasting wrote:
> Nit: Just change the name here without modifying the rest of the comment
> formatting (various places seem to use different patterns as to how they
> construct these comments, and I'd rather not tweak this)

Done.

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
File components/search_engines/search_engine_data_type_controller_unittest.cc
(right):

https://codereview.chromium.org/2511703004/diff/140001/components/search_engi...
components/search_engines/search_engine_data_type_controller_unittest.cc:160:
base::RunLoop().RunUntilIdle();
On 2016/11/22 20:16:02, Peter Kasting wrote:
> Does this deserve a comment about why we need to run to idle?

Done.

[commit-bot: I haz the power, 2016-11-28 18:56:44]

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

[pavely, 2016-11-28 19:01:03]

Description was changed from

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505
==========

to

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505

TBR=sdefresne@chromium.org
==========

[pavely, 2016-11-28 19:01:16]

The CQ bit was checked by pavely@chromium.org

[pavely, 2016-11-28 19:01:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org, stevenjb@chromium.org, vabr@chromium.org,
pkasting@chromium.org, treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2511703004/#ps180001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-28 19:02:26]

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-28 19:40:37]

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

[commit-bot: I haz the power, 2016-11-28 19:40:39]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pavely, 2016-11-28 19:46:01]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2016-11-28 19:47:25]

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-28 20:29:49]

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

[commit-bot: I haz the power, 2016-11-28 20:29:51]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[pavely, 2016-11-28 21:26:41]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2016-11-28 21:27:33]

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-28 22:56:27]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480368401076270,
"parent_rev": "9b9e43f131836cba62ecc959c3b1575c72097fcd", "commit_rev":
"6d5fd258013f7693aad541cf2658b65ceab58a46"}

[commit-bot: I haz the power, 2016-11-28 22:56:58]

Description was changed from

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505

TBR=sdefresne@chromium.org
==========

to

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505

TBR=sdefresne@chromium.org
==========

[commit-bot: I haz the power, 2016-11-28 22:56:59]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-28 23:00:51]

Description was changed from

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505

TBR=sdefresne@chromium.org
==========

to

==========
[Sync] Remove UIDataTypeController

The goal is to reduce number of datatype controllers to simplify future
refactoring.

In this chagne:
- Removed UIDataTypeController
- All datatypes are switched to NonUIDataTypeController
- Removed model_safe_group method, in favor of DirectoryDataTypeController field
- Added backend_thread_ so that most of datatypes don't need to override
  PostTaskOnBackendThread

R=maxbogue@chromium.org
BUG=647505

TBR=sdefresne@chromium.org

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

[commit-bot: I haz the power, 2016-11-28 23:00:53]

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