[Sync] Eliminate friends from Directory by exposing kernel via accessor

[Sync] Eliminate friends from Directory by exposing kernel via accessor

This is a minor refactoring of Directory that eliminates non-test
friends and provides access to Directory's kernel using public accessor
methods.

Make Directory::Close private as it's automatically called from the
dtor.

Make Directory::Kernel public instead of having Directory be-friend
several classes.

Make Directory::Kernel::mutex mutable to eliminate the need for a
const_cast in ScopedKernelLock.

Make OnDiskDirectoryBackingStore::TryLoad private.

BUG=475557
Committed: https://crrev.com/089b9366f4cb360bda3245d8c831e3362bfc839c
Cr-Commit-Position: refs/heads/master@{#323850}

[maniscalco, 2015-04-01 21:07:08]

Nicolas, would you please review this change?

It's a relatively minor refactoring change that looks bigger than it is.  There
should be no behavior change.  The best way to review would be to start with
directory.h.  Everything else is pretty much mechanical.

My thinking here is that it's better to make something public (in this case
Directory's kernel_) than to leave it as private, but be-friend a large number
of non-test classes.  I'd like to get your thoughts on this.  Longer term (i.e.
in future CLs) we'd whittle away at the uses of Directory::kernel() and replace
them with public methods that manipulate kernel_ rather than publicly expose
kernel_ via the new kernel() accessor.  LMKWYT about this approach.

[Nicolas Zea, 2015-04-03 00:14:50]

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

[Nicolas Zea, 2015-04-03 00:16:20]

Just saw this (looks like I wasn't actually added as a reviewer). Adding myself
and will take a look.

On 2015/04/01 21:07:08, maniscalco wrote:
> Nicolas, would you please review this change?
> 
> It's a relatively minor refactoring change that looks bigger than it is. 
There
> should be no behavior change.  The best way to review would be to start with
> directory.h.  Everything else is pretty much mechanical.
> 
> My thinking here is that it's better to make something public (in this case
> Directory's kernel_) than to leave it as private, but be-friend a large number
> of non-test classes.  I'd like to get your thoughts on this.  Longer term
(i.e.
> in future CLs) we'd whittle away at the uses of Directory::kernel() and
replace
> them with public methods that manipulate kernel_ rather than publicly expose
> kernel_ via the new kernel() accessor.  LMKWYT about this approach.

[maniscalco, 2015-04-03 15:31:52]

Oops, thanks, that was my intention.  Rietveld is good, but I guess it
doesn't read minds (yet).

On Thu, Apr 2, 2015 at 5:16 PM, <zea@chromium.org> wrote:

> Just saw this (looks like I wasn't actually added as a reviewer). Adding
> myself
> and will take a look.
>
> On 2015/04/01 21:07:08, maniscalco wrote:
>
>> Nicolas, would you please review this change?
>>
>
>  It's a relatively minor refactoring change that looks bigger than it is.
>>
> There
>
>> should be no behavior change.  The best way to review would be to start
>> with
>> directory.h.  Everything else is pretty much mechanical.
>>
>
>  My thinking here is that it's better to make something public (in this
>> case
>> Directory's kernel_) than to leave it as private, but be-friend a large
>> number
>> of non-test classes.  I'd like to get your thoughts on this.  Longer term
>>
> (i.e.
>
>> in future CLs) we'd whittle away at the uses of Directory::kernel() and
>>
> replace
>
>> them with public methods that manipulate kernel_ rather than publicly
>> expose
>> kernel_ via the new kernel() accessor.  LMKWYT about this approach.
>>
>
>
>
> https://codereview.chromium.org/1057663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nicolas Zea, 2015-04-03 17:54:50]

This is much cleaner. LGTM!

https://codereview.chromium.org/1057663002/diff/40001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/1057663002/diff/40001/sync/syncable/directory...
sync/syncable/directory.h:516: // For new entry creation only
nit: end with period

[maniscalco, 2015-04-03 22:57:28]

Thanks for the speedy review.

https://codereview.chromium.org/1057663002/diff/40001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/1057663002/diff/40001/sync/syncable/directory...
sync/syncable/directory.h:516: // For new entry creation only
On 2015/04/03 17:54:50, Nicolas Zea wrote:
> nit: end with period

Done.

[maniscalco, 2015-04-03 22:57:43]

The CQ bit was checked by maniscalco@chromium.org

[maniscalco, 2015-04-03 22:57:44]

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/1057663002/#ps60001
(title: "Changes from review.")

[commit-bot: I haz the power, 2015-04-03 22:58:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1057663002/60001

[commit-bot: I haz the power, 2015-04-03 23:45:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-03 23:46:37]

Patchset 4 (id:??) landed as
https://crrev.com/089b9366f4cb360bda3245d8c831e3362bfc839c
Cr-Commit-Position: refs/heads/master@{#323850}