Add cache_guid accessor to UserShare

Add cache_guid accessor to UserShare

This helps reduce the number of places in chrome/browser/sync that need
access to the syncable::Directory.

BUG=131130

[rlarocque, 2012-09-20 01:20:34]

This is another fragment of the DeviceInfo mega-patch
(http://codereview.chromium.org/10911073/).  Please review.

[Nicolas Zea, 2012-09-20 18:01:34]

+Tim for UserShare change.

Also, is it possible to remove the DEPS we mentioned before with this? I already
removed the need to sync/util/time.h

[rlarocque, 2012-09-24 22:16:37]

On 2012/09/20 18:01:34, nzea wrote:
> +Tim for UserShare change.
> 
> Also, is it possible to remove the DEPS we mentioned before with this? I
already
> removed the need to sync/util/time.h

ping.

[rlarocque, 2012-09-28 17:38:22]

On 2012/09/24 22:16:37, rlarocque wrote:
> On 2012/09/20 18:01:34, nzea wrote:
> > +Tim for UserShare change.
> > 
> > Also, is it possible to remove the DEPS we mentioned before with this? I
> already
> > removed the need to sync/util/time.h
> 
> ping.

re-ping.

[tim (not reviewing), 2012-09-28 19:00:54]

At one time I had hoped to remove the need for UserShare, since SBH should
always know which SyncManager it's talking to (there's only ever one!). But, I
guess this is better than depending on Directory.

http://codereview.chromium.org/10966002/diff/1/sync/internal_api/public/user_...
File sync/internal_api/public/user_share.h (right):

http://codereview.chromium.org/10966002/diff/1/sync/internal_api/public/user_...
sync/internal_api/public/user_share.h:33: std::string cache_guid();
structs shouldn't have methods :/  make it a class?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...


Notably, "The accessing/setting of fields is done by directly accessing the
fields rather than through method invocations."

also, const?

[rlarocque, 2012-09-28 19:06:42]

You're right, it probably should be a class.  I'll try to hide the members,
provide accessor functions, etc. and see how many compile errors I get.

[rlarocque, 2012-09-28 20:38:21]

That didn't turn out very well.

There's a circular dependency during initialization.  The UserShare requires a
Directory, the Directory requires a SyncEncryptionHandler, and the
SyncEncryptionHandler requires a UserShare.  To break the cycle, the UserShare
must have a default constructor and setter methods.

The resulting UserShare class is not very class-like.

Also, I was planning to submit a change immediately after this one that will
remove the 'name' member from UserShare, since it's not really necessary.  That
will leave UserShare with one member (the scoped_ptr to the Directory), a setter
and getter for that member, and the cache_guid fetching function.  

I'm not convinced that UserShare qualifies as a class.  However, the style guide
does say "If in doubt, make it a class."   Thoughts?

[rlarocque, 2012-10-02 01:05:57]

I tried to move the assignment of SyncEncryptionHandler's user_share_ pointer to
its Init() function in order to break the dependency cycle.  That sort of works,
though it breaks some tests that seem to want the user_share_ to be set before
Init().  It also touches a lot of code.

If it's alright with you, I'd prefer to just make UserShare a struct with an
accessor method.  I know it's unusual, but I think that's a better idea than
writing a 200+ line patch to change interfaces that I don't fully understand.  

Either that, or we could just drop this change.  I'm not convinced that this is
the best solution.  Maybe we need more time to figure out what would be the
correct way to access the cache_guid.

[rlarocque, 2012-11-13 21:57:56]

Ping.

Would there be any objection to committing this according to the original plan,
which was to simply add an accessor to the UserShare?  

If not, then I have to rewrite my DeviceInfo ChangeProcessor patch.  It's not a
big rewrite, but I need to know either way.

[rlarocque, 2012-11-13 22:33:20]

On 2012/11/13 21:57:56, rlarocque wrote:
> Ping.
> 
> Would there be any objection to committing this according to the original
plan,
> which was to simply add an accessor to the UserShare?  
> 
> If not, then I have to rewrite my DeviceInfo ChangeProcessor patch.  It's not
a
> big rewrite, but I need to know either way.

Patch updated.

[tim (not reviewing), 2012-11-14 00:04:55]

On 2012/11/13 22:33:20, rlarocque wrote:
> On 2012/11/13 21:57:56, rlarocque wrote:
> > Ping.
> > 
> > Would there be any objection to committing this according to the original
> plan,
> > which was to simply add an accessor to the UserShare?  
> > 
> > If not, then I have to rewrite my DeviceInfo ChangeProcessor patch.  It's
not
> a
> > big rewrite, but I need to know either way.
> 
> Patch updated.

Could we break the cycle by having UserShare have an Init method?

[Nicolas Zea, 2012-11-14 00:09:45]

Remind me, is there a reason we can't just have the PSS provide the cache guid?
Or are we worried about PSS function overload?

[rlarocque, 2012-11-14 00:32:28]

On 2012/11/14 00:09:45, Nicolas Zea wrote:
> Remind me, is there a reason we can't just have the PSS provide the cache
guid?
> Or are we worried about PSS function overload?

Tim: See above comment.  I haven't gone to dig up more context, but it seems
that I tried that at one point and decided the complexity wasn't worth it.  I'd
rather not make a possibly dangerous 200 line refactoring change to fix an issue
that no user could possibly care about.  I'd prefer to keep or extend the DEPS
exception.

Nicolas: In addition to style arguments for or against, there's the issue that
the PSS hasn't received OnBackendInitialized by the time we're setting up the
SyncedDeviceTracker.  The accessor would not help us in that case.

[tim (not reviewing), 2012-11-14 00:58:16]

On 2012/11/14 00:32:28, rlarocque wrote:
> On 2012/11/14 00:09:45, Nicolas Zea wrote:
> > Remind me, is there a reason we can't just have the PSS provide the cache
> guid?
> > Or are we worried about PSS function overload?
> 
> Tim: See above comment.  I haven't gone to dig up more context, but it seems
> that I tried that at one point and decided the complexity wasn't worth it. 
I'd
> rather not make a possibly dangerous 200 line refactoring change to fix an
issue
> that no user could possibly care about.  I'd prefer to keep or extend the DEPS
> exception.

I thought that was what happened when you tried to add an Init to
SyncEncryptionHandler?
> 
> Nicolas: In addition to style arguments for or against, there's the issue that
> the PSS hasn't received OnBackendInitialized by the time we're setting up the
> SyncedDeviceTracker.  The accessor would not help us in that case.

We're really trying to use the user share before OnBackendInitialized?  I
thought we had a DCHECK to prevent that?

[Nicolas Zea, 2012-11-14 01:01:59]

On 2012/11/14 00:32:28, rlarocque wrote:
> On 2012/11/14 00:09:45, Nicolas Zea wrote:
> > Remind me, is there a reason we can't just have the PSS provide the cache
> guid?
> > Or are we worried about PSS function overload?
> 
> Tim: See above comment.  I haven't gone to dig up more context, but it seems
> that I tried that at one point and decided the complexity wasn't worth it. 
I'd
> rather not make a possibly dangerous 200 line refactoring change to fix an
issue
> that no user could possibly care about.  I'd prefer to keep or extend the DEPS
> exception.
> 
> Nicolas: In addition to style arguments for or against, there's the issue that
> the PSS hasn't received OnBackendInitialized by the time we're setting up the
> SyncedDeviceTracker.  The accessor would not help us in that case.

We're setting up the SyncedDeviceTracker from within the SBH anyways right? You
don't need to access the PSS then, the SyncManager can pass up the cache guid to
the SBH via its OnInitializationComplete call.

[rlarocque, 2012-11-14 01:10:14]

On 2012/11/14 00:58:16, timsteele wrote:
> On 2012/11/14 00:32:28, rlarocque wrote:
> > On 2012/11/14 00:09:45, Nicolas Zea wrote:
> > > Remind me, is there a reason we can't just have the PSS provide the cache
> > guid?
> > > Or are we worried about PSS function overload?
> > 
> > Tim: See above comment.  I haven't gone to dig up more context, but it seems
> > that I tried that at one point and decided the complexity wasn't worth it. 
> I'd
> > rather not make a possibly dangerous 200 line refactoring change to fix an
> issue
> > that no user could possibly care about.  I'd prefer to keep or extend the
DEPS
> > exception.
> 
> I thought that was what happened when you tried to add an Init to
> SyncEncryptionHandler?

Sorry, I misread my own comment.  Your interpretation is correct.

An Init() method on UserShare would look a lot like patch set #2.  The main
difference would be that instead of calling share_.set_name() and
share_.set_directory() in SyncManagerImpl::Init(name, directory).  If/when we
remove the name parameter, it becomes almost identical to patch set #2.

There's not really anything wrong with #2.  My only argument against it is that
I think it's pointless to have a class that includes a default contsutrctor and
getters and setters for all of its methods.  We're not really improving on the
existing struct in any way.  That, and the fact that the change is non-trivial,
is why I'd prefer to not to go that route.

> > 
> > Nicolas: In addition to style arguments for or against, there's the issue
that
> > the PSS hasn't received OnBackendInitialized by the time we're setting up
the
> > SyncedDeviceTracker.  The accessor would not help us in that case.
> 
> We're really trying to use the user share before OnBackendInitialized?  I
> thought we had a DCHECK to prevent that?

Well, the nigori change processor equivalents have early access to the
syncable::Directory.  DeviceInfo's ChangeProcessor is in a similar situation,
but it's not allowed to access syncable::Directory because it's a
ChangeProcessor.

[rlarocque, 2012-11-14 01:22:42]

On 2012/11/14 01:01:59, Nicolas Zea wrote:
> On 2012/11/14 00:32:28, rlarocque wrote:
> > On 2012/11/14 00:09:45, Nicolas Zea wrote:
> > > Remind me, is there a reason we can't just have the PSS provide the cache
> > guid?
> > > Or are we worried about PSS function overload?
> > 
> > Tim: See above comment.  I haven't gone to dig up more context, but it seems
> > that I tried that at one point and decided the complexity wasn't worth it. 
> I'd
> > rather not make a possibly dangerous 200 line refactoring change to fix an
> issue
> > that no user could possibly care about.  I'd prefer to keep or extend the
DEPS
> > exception.
> > 
> > Nicolas: In addition to style arguments for or against, there's the issue
that
> > the PSS hasn't received OnBackendInitialized by the time we're setting up
the
> > SyncedDeviceTracker.  The accessor would not help us in that case.
> 
> We're setting up the SyncedDeviceTracker from within the SBH anyways right?
You
> don't need to access the PSS then, the SyncManager can pass up the cache guid
to
> the SBH via its OnInitializationComplete call.

Yes, that would be possible.  I think we could pass in the cache_guid as a
paramater when we initialize it.

However, I don't think that's really an improvement in terms of design.  I don't
see why OnInitializationComplete ought to have a cache_guid parameter.  

Is it our intention to hide the fact that the UserShare contains (indirectly) a
cache_guid?

[Nicolas Zea, 2012-11-14 01:30:28]

On 2012/11/14 01:22:42, rlarocque wrote:
> On 2012/11/14 01:01:59, Nicolas Zea wrote:
> > On 2012/11/14 00:32:28, rlarocque wrote:
> > > On 2012/11/14 00:09:45, Nicolas Zea wrote:
> > > > Remind me, is there a reason we can't just have the PSS provide the
cache
> > > guid?
> > > > Or are we worried about PSS function overload?
> > > 
> > > Tim: See above comment.  I haven't gone to dig up more context, but it
seems
> > > that I tried that at one point and decided the complexity wasn't worth it.

> > I'd
> > > rather not make a possibly dangerous 200 line refactoring change to fix an
> > issue
> > > that no user could possibly care about.  I'd prefer to keep or extend the
> DEPS
> > > exception.
> > > 
> > > Nicolas: In addition to style arguments for or against, there's the issue
> that
> > > the PSS hasn't received OnBackendInitialized by the time we're setting up
> the
> > > SyncedDeviceTracker.  The accessor would not help us in that case.
> > 
> > We're setting up the SyncedDeviceTracker from within the SBH anyways right?
> You
> > don't need to access the PSS then, the SyncManager can pass up the cache
guid
> to
> > the SBH via its OnInitializationComplete call.
> 
> Yes, that would be possible.  I think we could pass in the cache_guid as a
> paramater when we initialize it.
> 
> However, I don't think that's really an improvement in terms of design.  I
don't
> see why OnInitializationComplete ought to have a cache_guid parameter.  
> 
> Is it our intention to hide the fact that the UserShare contains (indirectly)
a
> cache_guid?

Ideally I think we want to hide the UserShare from datatypes completely.
Granted, old style datatypes require it to open transactions, but in the new API
they shouldn't need it for anything. If a datatype needs access to the cache
guid (like session does), it shouldn't need to use any sync/internal_api/
functionality.

[tim (not reviewing), 2012-11-14 01:37:30]

On 2012/11/14 01:30:28, Nicolas Zea wrote:
> On 2012/11/14 01:22:42, rlarocque wrote:
> > On 2012/11/14 01:01:59, Nicolas Zea wrote:
> > > On 2012/11/14 00:32:28, rlarocque wrote:
> > > > On 2012/11/14 00:09:45, Nicolas Zea wrote:
> > > > > Remind me, is there a reason we can't just have the PSS provide the
> cache
> > > > guid?
> > > > > Or are we worried about PSS function overload?
> > > > 
> > > > Tim: See above comment.  I haven't gone to dig up more context, but it
> seems
> > > > that I tried that at one point and decided the complexity wasn't worth
it.
> 
> > > I'd
> > > > rather not make a possibly dangerous 200 line refactoring change to fix
an
> > > issue
> > > > that no user could possibly care about.  I'd prefer to keep or extend
the
> > DEPS
> > > > exception.
> > > > 
> > > > Nicolas: In addition to style arguments for or against, there's the
issue
> > that
> > > > the PSS hasn't received OnBackendInitialized by the time we're setting
up
> > the
> > > > SyncedDeviceTracker.  The accessor would not help us in that case.
> > > 
> > > We're setting up the SyncedDeviceTracker from within the SBH anyways
right?
> > You
> > > don't need to access the PSS then, the SyncManager can pass up the cache
> guid
> > to
> > > the SBH via its OnInitializationComplete call.
> > 
> > Yes, that would be possible.  I think we could pass in the cache_guid as a
> > paramater when we initialize it.
> > 
> > However, I don't think that's really an improvement in terms of design.  I
> don't
> > see why OnInitializationComplete ought to have a cache_guid parameter.  
> > 
> > Is it our intention to hide the fact that the UserShare contains
(indirectly)
> a
> > cache_guid?
> 
> Ideally I think we want to hide the UserShare from datatypes completely.
> Granted, old style datatypes require it to open transactions, but in the new
API
> they shouldn't need it for anything. If a datatype needs access to the cache
> guid (like session does), it shouldn't need to use any sync/internal_api/
> functionality.

I tend to agree with that perspective.  Even with the old style, we never really
wanted to expose it :/

[rlarocque, 2012-11-14 19:31:19]

> > > Is it our intention to hide the fact that the UserShare contains
> (indirectly)
> > a
> > > cache_guid?
> > 
> > Ideally I think we want to hide the UserShare from datatypes completely.
> > Granted, old style datatypes require it to open transactions, but in the new
> API
> > they shouldn't need it for anything. If a datatype needs access to the cache
> > guid (like session does), it shouldn't need to use any sync/internal_api/
> > functionality.
> 
> I tend to agree with that perspective.  Even with the old style, we never
really
> wanted to expose it :/

OK, so how about this plan:
- Expose a cache_guid accessor on SyncManager.
- SyncBackendHost will pass it in to the SyncedDeviceTracker when we initialize
it.
- Do nothing to fix the DEPS exception currently required for the model
associator.

If we're OK with that, then I'll drop this review and move the above work into
the SyncedDeviceTracker patch.

[tim (not reviewing), 2012-11-14 21:04:15]

That sounds good.  Maybe we can store the cache_guid somewhere for the model
associator to grab it?  Could we set kSyncSessionsGuid and a PrefObserver?  That
might cause weirdness on android due to android_id, although we're going to
start using the guid there as well, iirc.  That could be handled separately,
though.

[rlarocque, 2012-11-15 01:10:18]

> OK, so how about this plan:
> - Expose a cache_guid accessor on SyncManager.
> - SyncBackendHost will pass it in to the SyncedDeviceTracker when we
initialize
> it.
> - Do nothing to fix the DEPS exception currently required for the model
> associator.
> 

This is done.  The changes have been squashed into
http://codereview.chromium.org/11360259/.