[Sync] Refactored ProfileSyncService and remove its backend() function

[Sync] Refactored ProfileSyncService and remove its backend() function

Moved bodies of test functions into .cc files.

Remove sync_service() accessors from various model associators.

Renamed GetUserShareHandle to GetUserShare() and removed typedefs for its return types.

Added plenty of TODOs for future improvements.

BUG=None
TEST=unit_tests

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

[akalin, 2011-01-21 08:35:10]

+tim for review

[tim (not reviewing), 2011-01-21 18:16:59]

I think this is a good change, but in general I cry every time we have to add
something to the ProfileSyncService api.  So much stuff in there (e.g. all of
the passphrase / signin state and logic) could be a class of its own and owned
by the PSS.  I'm not sure if that could be applied to any of the stuff being
added here.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:154:
sync_api::WriteTransaction trans(sync_service_->GetUserShareHandle());
Maybe what I was thinking before, since the handle never changes, was to pass it
in the ctor or via a set_ method? Anything we can trim off of the PSS interface
is a win IMO.

What we really need is an abstract model associator since there's so much
duplicated code (and the share handle could be stored in there).

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_model_associator.h (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.h:92: ProfileSyncService*
sync_service_;
I think you should instead move the sync_service() accessor to the protected
section.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1070:
backend_->GetModelSafeRoutingInfo(out);
return; ?

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:93: typedef
browser_sync::SyncBackendHost::UserShareHandle UserShareHandle;
ring around the rosie... :(

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:340: bool HasUnsyncedItems() const;
The comments should probably have moved too

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:517:
browser_sync::SyncBackendHost::UserShareHandle user_share =
something about getting a pointer from the service that is typedef'd from SBH
seems weird. Should we just use sync_api::UserShare* and be done with it?  Or
maybe just move the typedef into browser_sync vs inside SBH.  That would be a
little less circuitous I think.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:571:
service_->GetTestBackend()->GetWorkers(&workers);
why GetTestBackend()? I would have just expected GetBackendForTest() since you
added that. GetWorkers exists there. Do we srsly need both?

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_unittest.cc:63: virtual bool
GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id) {
Im not sure I get what happened in here. Are you sure it isn't the case that
every other test could and *should* use TestModelAssociatorHelper but decided to
do things themselves instead (probably 'cause they didn't know this existed)? 
If so we just took a step backwards :)

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/test_pr...
File chrome/browser/sync/test_profile_sync_service.h (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/test_pr...
chrome/browser/sync/test_profile_sync_service.h:72: //     OnBackendInitialized
callback fires.
nit - indent

[akalin, 2011-01-21 21:21:09]

Addressed all comments, please take another look!

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:154:
sync_api::WriteTransaction trans(sync_service_->GetUserShareHandle());
On 2011/01/21 18:17:00, timsteele wrote:
> Maybe what I was thinking before, since the handle never changes, was to pass
it
> in the ctor or via a set_ method? Anything we can trim off of the PSS
interface
> is a win IMO.
> 
> What we really need is an abstract model associator since there's so much
> duplicated code (and the share handle could be stored in there).

Yeah, I agree.  I actually tried going down that path and it got pretty
monstrous.  I settled for removing the backend() accessor and temporarily adding
these functions.  I added TODOs for what needs to be done for all the functions
I added to PSS.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_model_associator.h (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_model_associator.h:92: ProfileSyncService*
sync_service_;
On 2011/01/21 18:17:00, timsteele wrote:
> I think you should instead move the sync_service() accessor to the protected
> section.

Done.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1070:
backend_->GetModelSafeRoutingInfo(out);
On 2011/01/21 18:17:00, timsteele wrote:
> return; ?

Done.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:93: typedef
browser_sync::SyncBackendHost::UserShareHandle UserShareHandle;
On 2011/01/21 18:17:00, timsteele wrote:
> ring around the rosie... :(

Removed!

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:340: bool HasUnsyncedItems() const;
On 2011/01/21 18:17:00, timsteele wrote:
> The comments should probably have moved too

Done.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:571:
service_->GetTestBackend()->GetWorkers(&workers);
On 2011/01/21 18:17:00, timsteele wrote:
> why GetTestBackend()? I would have just expected GetBackendForTest() since you
> added that. GetWorkers exists there. Do we srsly need both?

The alternative was to override GetBackendForTest() and make it public.  I guess
that's less smelly, though.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_unittest.cc (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_unittest.cc:63: virtual bool
GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id) {
On 2011/01/21 18:17:00, timsteele wrote:
> Im not sure I get what happened in here. Are you sure it isn't the case that
> every other test could and *should* use TestModelAssociatorHelper but decided
to
> do things themselves instead (probably 'cause they didn't know this existed)? 
> If so we just took a step backwards :)

So this logic basically lazily creates any tagged node that is requested.  I
think all the other datatype unittests instead manually create sync nodes
themselves by converting from the chrome type (e.g., the autofill test has
AddAutofillSyncNode() functions), which I think is the better way to go.  If
anything, the same thing probably should be done for bookmark tests (which
should be moved out of this class) and this logic removed.

Added TODOs.

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/test_pr...
File chrome/browser/sync/test_profile_sync_service.h (right):

http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/test_pr...
chrome/browser/sync/test_profile_sync_service.h:72: //     OnBackendInitialized
callback fires.
On 2011/01/21 18:17:00, timsteele wrote:
> nit - indent

Done.

[akalin, 2011-01-24 22:51:33]

Ping!

On 2011/01/21 21:21:09, akalin wrote:
> Addressed all comments, please take another look!
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/au...
> File chrome/browser/sync/glue/autofill_model_associator.cc (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/au...
> chrome/browser/sync/glue/autofill_model_associator.cc:154:
> sync_api::WriteTransaction trans(sync_service_->GetUserShareHandle());
> On 2011/01/21 18:17:00, timsteele wrote:
> > Maybe what I was thinking before, since the handle never changes, was to
pass
> it
> > in the ctor or via a set_ method? Anything we can trim off of the PSS
> interface
> > is a win IMO.
> > 
> > What we really need is an abstract model associator since there's so much
> > duplicated code (and the share handle could be stored in there).
> 
> Yeah, I agree.  I actually tried going down that path and it got pretty
> monstrous.  I settled for removing the backend() accessor and temporarily
adding
> these functions.  I added TODOs for what needs to be done for all the
functions
> I added to PSS.
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/bo...
> File chrome/browser/sync/glue/bookmark_model_associator.h (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/glue/bo...
> chrome/browser/sync/glue/bookmark_model_associator.h:92: ProfileSyncService*
> sync_service_;
> On 2011/01/21 18:17:00, timsteele wrote:
> > I think you should instead move the sync_service() accessor to the protected
> > section.
> 
> Done.
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.cc:1070:
> backend_->GetModelSafeRoutingInfo(out);
> On 2011/01/21 18:17:00, timsteele wrote:
> > return; ?
> 
> Done.
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.h:93: typedef
> browser_sync::SyncBackendHost::UserShareHandle UserShareHandle;
> On 2011/01/21 18:17:00, timsteele wrote:
> > ring around the rosie... :(
> 
> Removed!
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.h:340: bool HasUnsyncedItems() const;
> On 2011/01/21 18:17:00, timsteele wrote:
> > The comments should probably have moved too
> 
> Done.
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_autofill_unittest.cc:571:
> service_->GetTestBackend()->GetWorkers(&workers);
> On 2011/01/21 18:17:00, timsteele wrote:
> > why GetTestBackend()? I would have just expected GetBackendForTest() since
you
> > added that. GetWorkers exists there. Do we srsly need both?
> 
> The alternative was to override GetBackendForTest() and make it public.  I
guess
> that's less smelly, though.
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_unittest.cc (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_unittest.cc:63: virtual bool
> GetSyncIdForTaggedNode(const std::string& tag, int64* sync_id) {
> On 2011/01/21 18:17:00, timsteele wrote:
> > Im not sure I get what happened in here. Are you sure it isn't the case that
> > every other test could and *should* use TestModelAssociatorHelper but
decided
> to
> > do things themselves instead (probably 'cause they didn't know this
existed)? 
> > If so we just took a step backwards :)
> 
> So this logic basically lazily creates any tagged node that is requested.  I
> think all the other datatype unittests instead manually create sync nodes
> themselves by converting from the chrome type (e.g., the autofill test has
> AddAutofillSyncNode() functions), which I think is the better way to go.  If
> anything, the same thing probably should be done for bookmark tests (which
> should be moved out of this class) and this logic removed.
> 
> Added TODOs.
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/test_pr...
> File chrome/browser/sync/test_profile_sync_service.h (right):
> 
>
http://codereview.chromium.org/6375007/diff/27002/chrome/browser/sync/test_pr...
> chrome/browser/sync/test_profile_sync_service.h:72: //    
OnBackendInitialized
> callback fires.
> On 2011/01/21 18:17:00, timsteele wrote:
> > nit - indent
> 
> Done.

[tim (not reviewing), 2011-01-24 23:00:13]

LGTM