Sync: Refactoring DataTypeController to make the design shareable with USS datatypes.

Sync: Refactoring DataTypeController to make the design
shareable with USS datatypes.

1) Moved the code that activates and deactivates datatypes from
DataTypeManager to DataTypeController. This should allow controllers
to implement the activation differently from directory based and USS
datatypes.

2) Removed any directory specific code from DataTypeManager and
DataTypeController and made theme generic enough to work for
USS datatypes. More specifically:
- GetChangeProcessor() and model_safe_group() are moved to
  DirectoryDataTypeController - see below.
- OnUserShareChanged(), user_share() are not needed anymore.

3) Introduced new base class for directory based DTC - DirectoryDataTypeController.
This class provides directory specific implementation for the datatype
activation/deactivation and declares some directory specific virtual
functions that formerly were in DataTypeController.

4) Changed the mechanism how some specific DTC get the user share.
Instead of being pushed to each DTC, user share can now be accessed
via SyncClient which is already passed to each controller. I had to fix
some mock classes and test code to make it work for all tests.

BUG=515962
Committed: https://crrev.com/de3d02a6ecdc7dcf7cfcf1c9c596777172568e78
Cr-Commit-Position: refs/heads/master@{#347111}

[stanisc, 2015-09-02 17:23:30]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org

[stanisc, 2015-09-02 17:23:30]

stanisc@chromium.org changed required reviewers:
+ zea@chromium.org

[stanisc, 2015-09-02 17:27:05]

stanisc@chromium.org changed required reviewers:
- zea@chromium.org

[stanisc, 2015-09-02 17:27:05]

zea@, made you the primary reviewers since this leverages your recent
refactoring changes.
gangwu@, maxbogue@, pavely@ - included you to help you understand design changes
relevant to USS. You should at least look at data_type_controller.* and
directory_data_type_controller.*

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:341: // TODO: this should merge with
RegisterDataTypeController
Will address this in the next patch. Let me know if I should remove this from
this patch.

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:151: 
Will remove this empty line before landing the patch.

[Nicolas Zea, 2015-09-02 22:52:46]

LGTM!

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:341: // TODO: this should merge with
RegisterDataTypeController
On 2015/09/02 17:27:05, stanisc wrote:
> Will address this in the next patch. Let me know if I should remove this from
> this patch.

You can leave this, but it would be good to mark yourself as the owner and
mention a bug. TODO(stanisc) crbug.com/xxx ...

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:151: 
On 2015/09/02 17:27:05, stanisc wrote:
> Will remove this empty line before landing the patch.

Acknowledged.

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
File components/sync_driver/directory_data_type_controller.h (right):

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
components/sync_driver/directory_data_type_controller.h:18: // Activates this
controller's data type.
nit: would be good to explain a bit more about what these two methods involve,
and why they're something directory specific. Also worth calling out which
thread they're called on, since as I recall one of them is the UI thread, the
other is the datatype thread.

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
components/sync_driver/directory_data_type_controller.h:35:
scoped_refptr<base::SingleThreadTaskRunner> ui_thread,
nit: pass scoped_refptrs by const ref ("const scoped_refptr<..>&")

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
File components/sync_driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
components/sync_driver/proxy_data_type_controller.cc:67:
BackendDataTypeConfigurer* configurer) {}
Neat! Glad that this abstractions gives us the ability to better control
datatypes that can't be activated/deactivated.

[stanisc, 2015-09-03 01:17:43]

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:341: // TODO: this should merge with
RegisterDataTypeController
On 2015/09/02 22:52:45, Nicolas Zea wrote:
> On 2015/09/02 17:27:05, stanisc wrote:
> > Will address this in the next patch. Let me know if I should remove this
from
> > this patch.
> 
> You can leave this, but it would be good to mark yourself as the owner and
> mention a bug. TODO(stanisc) crbug.com/xxx ...

Done.

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/1314903004/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:151: 
On 2015/09/02 22:52:45, Nicolas Zea wrote:
> On 2015/09/02 17:27:05, stanisc wrote:
> > Will remove this empty line before landing the patch.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
File components/sync_driver/directory_data_type_controller.h (right):

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
components/sync_driver/directory_data_type_controller.h:18: // Activates this
controller's data type.
On 2015/09/02 22:52:45, Nicolas Zea wrote:
> nit: would be good to explain a bit more about what these two methods involve,
> and why they're something directory specific. Also worth calling out which
> thread they're called on, since as I recall one of them is the UI thread, the
> other is the datatype thread.

Provided a bit more detailed comments here and in the base class.
I'll revise these comments once again when I'll have an alternative mechanism
for 
USS datatypes (which I think will be overloaded pair of methods on the
configurer).

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
components/sync_driver/directory_data_type_controller.h:35:
scoped_refptr<base::SingleThreadTaskRunner> ui_thread,
On 2015/09/02 22:52:45, Nicolas Zea wrote:
> nit: pass scoped_refptrs by const ref ("const scoped_refptr<..>&")

OK, changed here and in derived classes.

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
File components/sync_driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/1314903004/diff/100001/components/sync_driver...
components/sync_driver/proxy_data_type_controller.cc:67:
BackendDataTypeConfigurer* configurer) {}
On 2015/09/02 22:52:45, Nicolas Zea wrote:
> Neat! Glad that this abstractions gives us the ability to better control
> datatypes that can't be activated/deactivated.

Acknowledged.

[stanisc, 2015-09-03 04:10:27]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2015-09-03 04:10:28]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1314903004/#ps120001
(title: "Addressed CR comments")

[commit-bot: I haz the power, 2015-09-03 04:10:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314903004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314903004/120001

[commit-bot: I haz the power, 2015-09-03 04:15:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-03 04:16:14]

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