sync: Add ModelNeutralMutableEntry

sync: Add ModelNeutralMutableEntry

Add a ModelNeutralMutableEntry that exists between Entry and
MutableEntry in the hierarchy of syncable::Entry classes.

This new class inherits all of Entry's getter methods, so it can has the
same ability to read from entries as the current Entry and MutableEntry
classes.

All the setter methods from MutableEntry that are not model-changing
have been moved into ModelNeutralMutableEntry.  The non-model-changing
setters are those whose mutations do not need to be communicated to the
model-specific change processors.

Because we know can guarantee all changes made through the
ModelNeutralMutabeEntry functions will not need to be relayed to the
change processors, it would be safe to skip calling SaveOriginal() in
its setter functions.  For now, though, we leave these setter functions
unmodified.

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

[rlarocque, 2013-09-18 22:28:37]

Please review.

[Nicolas Zea, 2013-09-19 00:12:29]

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
File sync/syncable/model_neutral_mutable_entry.h (left):

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:37: MutableEntry(WriteTransaction*
trans, CreateNewUpdateItem, const Id& id);
do you not want CreateNewUpdateItem?

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
File sync/syncable/model_neutral_mutable_entry.h (right):

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:46: // Some of them trigger the
re-indexing of the entry.  Return true on success,
grammer nit: "Some of them trigger the re-indexing of the entry, and will return
true..."

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:49: void PutBaseVersion(int64
value);
isn't this only set by local changes?

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:68: // Do a simple property-only
update if the PARENT_ID field.  Use with caution.
nit: if -> of

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:86:
ModelNeutralMutableEntry(WriteTransaction* trans);
what is this for?

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:103: ModelNeutralMutableEntry();
nit: move to the above protected section, and leave the DISALLOW_COPY_AND_ASSIGN
in private

[rlarocque, 2013-09-19 00:40:42]

Patch updated.  PTAL.

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
File sync/syncable/model_neutral_mutable_entry.h (left):

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:37: MutableEntry(WriteTransaction*
trans, CreateNewUpdateItem, const Id& id);
On 2013/09/19 00:12:29, Nicolas Zea wrote:
> do you not want CreateNewUpdateItem?

I think that constructor will eventually need to live here, yes.

However, I'm not ready to make use of it yet, and the ProcessUpdates code will
continue to use MutableEntry's CreateNewUpdateItem for a while.  I'd prefer to
implement it later, when it's actually going to be used.

I intend to add support for this constructor when I try to convert
ProcessUpdates to using ModelNeutralMutableEntry.

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
File sync/syncable/model_neutral_mutable_entry.h (right):

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:46: // Some of them trigger the
re-indexing of the entry.  Return true on success,
On 2013/09/19 00:12:29, Nicolas Zea wrote:
> grammer nit: "Some of them trigger the re-indexing of the entry, and will
return
> true..."

Rephrased.

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:49: void PutBaseVersion(int64
value);
On 2013/09/19 00:12:29, Nicolas Zea wrote:
> isn't this only set by local changes?

Nope.  It gets updated in ProcessCommitResponseCommand and ProcessUpdates. 
Probably in other places, too.  

Fortunately, it's non-model-changing, so it's safe to define it here.

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:68: // Do a simple property-only
update if the PARENT_ID field.  Use with caution.
On 2013/09/19 00:12:29, Nicolas Zea wrote:
> nit: if -> of

Done.

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:86:
ModelNeutralMutableEntry(WriteTransaction* trans);
On 2013/09/19 00:12:29, Nicolas Zea wrote:
> what is this for?

This is for initializing the object when it is used as a base for MutableEntry.

https://codereview.chromium.org/23549038/diff/5001/sync/syncable/model_neutra...
sync/syncable/model_neutral_mutable_entry.h:103: ModelNeutralMutableEntry();
On 2013/09/19 00:12:29, Nicolas Zea wrote:
> nit: move to the above protected section, and leave the
DISALLOW_COPY_AND_ASSIGN
> in private

Apparently this constructor isn't even defined.  I was just carrying it over
from the old class for no good reason.

[rlarocque, 2013-09-23 17:22:42]

ping.

[Nicolas Zea, 2013-09-23 20:00:30]

LGTM

https://codereview.chromium.org/23549038/diff/10001/sync/syncable/model_neutr...
File sync/syncable/model_neutral_mutable_entry.h (right):

https://codereview.chromium.org/23549038/diff/10001/sync/syncable/model_neutr...
sync/syncable/model_neutral_mutable_entry.h:42: // to the node.  These fields
are important for book-keeping in the sync
nit: book-keeping -> bookkeeping

https://codereview.chromium.org/23549038/diff/10001/sync/syncable/model_neutr...
sync/syncable/model_neutral_mutable_entry.h:87:
ModelNeutralMutableEntry(WriteTransaction* trans);
nit: explicit

[rlarocque, 2013-09-23 21:28:58]

https://codereview.chromium.org/23549038/diff/10001/sync/syncable/model_neutr...
File sync/syncable/model_neutral_mutable_entry.h (right):

https://codereview.chromium.org/23549038/diff/10001/sync/syncable/model_neutr...
sync/syncable/model_neutral_mutable_entry.h:42: // to the node.  These fields
are important for book-keeping in the sync
On 2013/09/23 20:00:31, Nicolas Zea wrote:
> nit: book-keeping -> bookkeeping

Done.

https://codereview.chromium.org/23549038/diff/10001/sync/syncable/model_neutr...
sync/syncable/model_neutral_mutable_entry.h:87:
ModelNeutralMutableEntry(WriteTransaction* trans);
On 2013/09/23 20:00:31, Nicolas Zea wrote:
> nit: explicit

Done.

[commit-bot: I haz the power, 2013-09-23 21:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23549038/16001

[commit-bot: I haz the power, 2013-09-24 02:32:10]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-24 17:16:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/23549038/16001

[commit-bot: I haz the power, 2013-09-24 19:44:16]

Change committed as 225052