sync: Refactor MutablEntry internals

sync: Refactor MutablEntry internals

This is a follow-up to r223111.  That change modified the public
interface of MutableEntry to expose one setter function for each mutable
field in the EntryKernel.

This change modifies the internals of MutableEntry to match the external
API.  Many of the shared setter functions used to diverge significantly
based on which field was being set.  Now that each field has its own
setter, it's easier to provide different setter implementations for each
field.

Some of these changes might make it look like we're unnecessarily
introducing copy+paste code.  However, we have more CLs on the way that
introduce more differences among the setters.  In particular, many of
the SERVER_* field setters will be modified to no longer call
SaveOriginal().

BUG=284672
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223537

[rlarocque, 2013-09-16 18:31:28]

Please review.

I'm not too happy with some of this repeated code, but I can't see any
alternatives that are significantly less bad.  I guess we could use templates,
but I'd rather not.  We could try to share code between setters for the same
field types (which is close to what we have now), but most fields are special in
some way that would break that abstraction.

Let me know if you have any better ideas.

[Nicolas Zea, 2013-09-16 22:55:07]

You mention sharing code between setters is an option, and I gotta admit that I
think that's preferable to this duplication of code. I'm worried that the
semantics are changing.

What specifically is preventing that from being useful? (you mention most fields
are special; in what way? isn't that already encapsulated in the existing
logic?) Can't we categorize the setters in some way, such as those model
changing ones, those requiring kernel locks, etc.? Or is there not enough shared
logic?

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:136: ScopedKernelLock lock(dir());
The scoped kernel lock is only necessary when modifying a field that changes the
indexing right? Why is it introduced in these methods?

[rlarocque, 2013-09-16 23:49:55]

The problem with categorization is that they type system limits us to sharing
code only in cases where the field type is the same.  The EntryKernel defines
twelve different field types, so in the best case we'd need at least twelve
different setter functions.

In practice, many fields have specific indices or unique behavior.  These
require the addition of special case code to the generic setters.  At least some
of the time, those special cases are better expressed as separate functions. 
See, for example, PutIsDel() or PutUniqueClientTag(), both of which existed
before this refactor.

Finally, I'd like to not call SaveOriginals() in the non-SERVER fields.  We have
two options for implementing this:
1. Add a ShouldSaveOriginals bool argument to each generic setter function.
2. Use separate setter functions.
Of the two, I prefer the latter.

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
File sync/syncable/mutable_entry.cc (left):

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:469: if (IS_UNSYNCED == field) {
Here's an example of branching code that doesn't actually save us much trouble
in the end.  This function does different things based on which IndexedBitField
was passed in.  I think it makes more sense to define separate functions for
each bit field, especially since we now define PutIsUnsynced(), PutIsUnapplied()
as part of the interface anyway.

If you look around, you'll find that almost all of the setters have some
field-specific code.  After refactoring out each instance of field-specific
behavior, I ended up with patch set #3.  After staring at it for a bit, I
decided to be more consistent and remove the common setters for TimeFields and
Int64 fields, though the argument for removing them is not as strong.

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:136: ScopedKernelLock lock(dir());
On 2013/09/16 22:55:07, Nicolas Zea wrote:
> The scoped kernel lock is only necessary when modifying a field that changes
the
> indexing right? Why is it introduced in these methods?

It's not new.  The old definition of Put(Int64Field, ...) used a
ScopedKernelLock(), so I copied that behavior in this definition.

Put(TimeField, ...) did not use that lock, so the PutMtime() and related
functions below don't use it.  

I think the lock was intended to protect the dirty_metahandles index, but that
would be redundant because no one touches that field without owning the
transaction lock.

I decided not to change it because I want this new code to be exactly the same
as the old.

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:152: void MutableEntry::PutMtime(base::Time
value) {
I think these time setters are the strongest argument against this CL.  More so
than other fields, these four setters are almost identical.  For that reason, I
had an earlier version of the patch that kept the shared setter code for
TimeField values.  

I went back on that decision and decided to split these up when I realized that
doing so would make this class more consistent, and that we'd need to either add
a flag to the common setter or define two different setters for the SERVER and
non-SERVER cases.  We should only call SaveOriginal() in the non-SERVER cases.

[Nicolas Zea, 2013-09-17 00:54:46]

LGTM with nits

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:136: ScopedKernelLock lock(dir());
On 2013/09/16 23:49:56, rlarocque wrote:
> On 2013/09/16 22:55:07, Nicolas Zea wrote:
> > The scoped kernel lock is only necessary when modifying a field that changes
> the
> > indexing right? Why is it introduced in these methods?
> 
> It's not new.  The old definition of Put(Int64Field, ...) used a
> ScopedKernelLock(), so I copied that behavior in this definition.
> 
> Put(TimeField, ...) did not use that lock, so the PutMtime() and related
> functions below don't use it.  
> 
> I think the lock was intended to protect the dirty_metahandles index, but that
> would be redundant because no one touches that field without owning the
> transaction lock.
> 
> I decided not to change it because I want this new code to be exactly the same
> as the old.

Ah, I had assumed ServerVersion and BaseVersion had the same implementation
previously, but it appears not. Not making any change makes sense.

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:430: && tag != kernel_->ref(UNIQUE_BOOKMARK_TAG))
{
nit: move && to end of previous line

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:451: != value.SerializeAsString()) {
nit: move != to end of previous line

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:464: != value.SerializeAsString()) {
here too

[rlarocque, 2013-09-17 01:13:47]

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:430: && tag != kernel_->ref(UNIQUE_BOOKMARK_TAG))
{
On 2013/09/17 00:54:46, Nicolas Zea wrote:
> nit: move && to end of previous line

Done.

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:451: != value.SerializeAsString()) {
On 2013/09/17 00:54:46, Nicolas Zea wrote:
> nit: move != to end of previous line

Done.

https://codereview.chromium.org/23435008/diff/17001/sync/syncable/mutable_ent...
sync/syncable/mutable_entry.cc:464: != value.SerializeAsString()) {
On 2013/09/17 00:54:46, Nicolas Zea wrote:
> here too

Done.

[commit-bot: I haz the power, 2013-09-17 01:17:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/23435008/25001

[commit-bot: I haz the power, 2013-09-17 04:21:46]

Change committed as 223537