[Sync] Make generic non-frontend thread datatype controller.

[Sync] Make generic non-frontend thread datatype controller.

Refactor Autofill/Password/Typed URL datatypes to use it. Add generic unit
tests to cover the the various thready issues.

BUG=77964
TEST=NonFrontendDatatypeController* unit tests.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81454

[Nicolas Zea, 2011-04-07 17:03:33]

Looking into integration test issues still, but figure I'd put it out for review
since it's a reasonably large patch.

[tim (not reviewing), 2011-04-07 22:04:25]

mostly just some naming / style things thus far

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_data_type_controller.h (right):

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_data_type_controller.h:13: #include
"chrome/browser/sync/glue/non_frontend_data_type_controller.h"
i know it's a bit late now but.. maybe in a follow up, could we rename
"frontend" to be just "ui" ?

It's consistent with GROUP_UI as well as UIModelWorker.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller.h (right):

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:29: // funtionality
is implemented by default. Derived classes must implement:
I like the comment.  Just a suggestion -- abbreviate the signatures e.g.
"... must implement: 

type()
CreateSyncComponents()
RecordUnrecoverableError(...)
RecordAssociationTime(...)
RecordStartFailure(...)
"

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:43:
ProfileSyncService* sync_service);
do we really need both the profile and service if one can be derived?

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:62: // Kick of any
dependent services that need to be running before we can
a comment like "kick off" on a method called Start when there is also a method
called "KickOff..." is confusing :)

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:71: // Kick off the
the association on the thread the datatype lives on.
"the the" > "the"

I think we should be as consistent as possible with naming.  I'd prefer
StartAssociateAsync() Associate() AssociateFailed AssociateDone 

vs Start/Kickoff/Association/StartFailed/StartDone?

Lastly, I'm fine with Kickoff but I think convention in chrome would be to use
the verb 'Post'

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:104: virtual void
CleanUpState();
I think just Cleanup or Teardown is a more familiar name for this sort of thing.
 Consider suffixing "OnFrontendLoop" or something.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:110: virtual bool
KickOffDestroy() = 0;
same as above

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:133:
ProfileSyncFactory* const profile_sync_factory_;
strictly speaking, style-guide-says members should be private with accessors...

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/pas...
File chrome/browser/sync/glue/password_data_type_controller.h (right):

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/pas...
chrome/browser/sync/glue/password_data_type_controller.h:32: //
NonFrontendDataTypeController interface.
although I myself have once or twice enjoyed pulling this visibility hack on
inherited methods, we shouldn't do it.
It's discussed in effective c++, but basically, you can always cast this to
NonFrontendDataTypeController and get at these anyway, so making them private is
really pointless.

[Nicolas Zea, 2011-04-08 16:53:42]

Feedback incorporated, please take another look.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_data_type_controller.h (right):

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_data_type_controller.h:13: #include
"chrome/browser/sync/glue/non_frontend_data_type_controller.h"
On 2011/04/07 22:04:25, timsteele wrote:
> i know it's a bit late now but.. maybe in a follow up, could we rename
> "frontend" to be just "ui" ?
> 
> It's consistent with GROUP_UI as well as UIModelWorker.

My logic with this was to follow the whole "frontend vs backend" model; that
profile_sync_service implements the frontend (as opposed to the sync backend
host), which is where the frontend datatypes are doing work. Plus
UI_data_type_controller sounded weirder to me. I can add a todo though.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller.h (right):

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:29: // funtionality
is implemented by default. Derived classes must implement:
On 2011/04/07 22:04:25, timsteele wrote:
> I like the comment.  Just a suggestion -- abbreviate the signatures e.g.
> "... must implement: 
> 
> type()
> CreateSyncComponents()
> RecordUnrecoverableError(...)
> RecordAssociationTime(...)
> RecordStartFailure(...)
> "

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:43:
ProfileSyncService* sync_service);
On 2011/04/07 22:04:25, timsteele wrote:
> do we really need both the profile and service if one can be derived?

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:62: // Kick of any
dependent services that need to be running before we can
On 2011/04/07 22:04:25, timsteele wrote:
> a comment like "kick off" on a method called Start when there is also a method
> called "KickOff..." is confusing :)

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:71: // Kick off the
the association on the thread the datatype lives on.
On 2011/04/07 22:04:25, timsteele wrote:
> "the the" > "the"
> 
> I think we should be as consistent as possible with naming.  I'd prefer
> StartAssociateAsync() Associate() AssociateFailed AssociateDone 
> 
> vs Start/Kickoff/Association/StartFailed/StartDone?
> 
> Lastly, I'm fine with Kickoff but I think convention in chrome would be to use
> the verb 'Post'

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:104: virtual void
CleanUpState();
On 2011/04/07 22:04:25, timsteele wrote:
> I think just Cleanup or Teardown is a more familiar name for this sort of
thing.
>  Consider suffixing "OnFrontendLoop" or something.

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:110: virtual bool
KickOffDestroy() = 0;
On 2011/04/07 22:04:25, timsteele wrote:
> same as above

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.h:133:
ProfileSyncFactory* const profile_sync_factory_;
On 2011/04/07 22:04:25, timsteele wrote:
> strictly speaking, style-guide-says members should be private with
accessors... 

Done.

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/pas...
File chrome/browser/sync/glue/password_data_type_controller.h (right):

http://codereview.chromium.org/6811003/diff/3001/chrome/browser/sync/glue/pas...
chrome/browser/sync/glue/password_data_type_controller.h:32: //
NonFrontendDataTypeController interface.
On 2011/04/07 22:04:25, timsteele wrote:
> although I myself have once or twice enjoyed pulling this visibility hack on
> inherited methods, we shouldn't do it.
> It's discussed in effective c++, but basically, you can always cast this to
> NonFrontendDataTypeController and get at these anyway, so making them private
is
> really pointless.

They're protected on the NonFrontendDataTypeController, so they wouldn't be
accessible anyway. I supposed I could change this to be protected here too
though. Done.

[tim (not reviewing), 2011-04-08 17:56:08]

I just think UI* is more familiar to folks coming to write a new implementation
because they'll know whether their model lives on the UI thread or not. 
Frontend requires an extra lookup or two :\ So I'd prefer to change the names...

besides that, this LGTM!

[Nicolas Zea, 2011-04-08 18:59:47]

Added todo's re name change, and made some changes to profile_impl and
profile_sync_factory to decouple creating the profile sync service from
registering/creating the datatype controllers.

[Nicolas Zea, 2011-04-11 21:13:54]

ping? I just want to make sure the profile_impl/profile_sync_factory changes are
okay before I commit.

[tim (not reviewing), 2011-04-13 18:40:46]

LGTM