Enable Null Syncable ID which is different than Root ID.

Enable Null Syncable ID which is different than Root ID.

This patch prepares sync codebase for supporting datatypes with
implicit permanent forlders by introducing an empty/unset
syncable ID.
There was a notion of Null ID before but it was equivalent
to the root ID which caused some confusion in the code - a Null
ID would sometimes be used where a "root" would be appropriate
and vice versa.
See the following code fragment for an example:
// TODO(sync): We could use null here, but to ease conversion we use "r".
// fix this, this is madness :)
inline bool IsNull() const {
  return IsRoot();
}

This change will be followed by one or two separate changes
that will actually remove dependencies on PARENT_ID and
SERVER_PARENT_ID and use a different way to organize nodes
in the directory based on node's datatype rather than parent ID.

This change impacts the following:
1) The default ID value in EntryKernel ID fields and elsewhere is
Null ID and not Root ID as it used to be.
2) This means that some tests that depended on Root ID being
set implicitly
in PARENT_ID and SERVER_PARENT_ID now need to set these IDs explicitly.
3) Node's GetPredecessorId, GetSuccessorId, and GetFirstChildId
methods now return a null ID rather than a root ID when there
is no predecessor, successor, or children.
4) Small changed throughout the code to make sure that
null IDs and root IDs are used accordingly (now that they
are no longer equivalent).
5) Changed some validation methods to accept Null IDs
to avoid large changes in the test code.

BUG=438313
Committed: https://crrev.com/60c25aebaf6c3bbc90c9c82f83a752ca58c7ddce
Cr-Commit-Position: refs/heads/master@{#309495}

[stanisc, 2014-12-18 22:54:40]

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

[stanisc, 2014-12-18 22:56:40]

Nicolas, PTAL.

[Nicolas Zea, 2014-12-20 00:59:25]

I didn't manage to get to this today. Might be better to have Nick or Pavel take
a look since I'll be out for a while.

[stanisc, 2014-12-20 01:10:46]

stanisc@chromium.org changed reviewers:
+ pavely@chromium.org

[stanisc, 2014-12-20 01:10:46]

stanisc@chromium.org changed required reviewers:
+ pavely@chromium.org

[stanisc, 2014-12-20 01:10:47]

Pavely@chromium.org, please take a look.

[pavely, 2014-12-22 21:38:09]

https://codereview.chromium.org/805633004/diff/20001/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/805633004/diff/20001/sync/engine/commit_util....
sync/engine/commit_util.cc:154: if (new_parent_id != server_parent_id &&
!server_parent_id.IsNull() &&
Could you give examples when server_parent_id will be empty? It seems like there
is change of behavior that might affect server.

https://codereview.chromium.org/805633004/diff/20001/sync/engine/directory_up...
File sync/engine/directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/805633004/diff/20001/sync/engine/directory_up...
sync/engine/directory_update_handler_unittest.cc:32: using syncable::Id;
nit: There are plenty of places in this file with syncable::Id. With your change
there are both: Id and syncable::Id. I think you should choose one and make sure
it is used consistently in the file.

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_id.h
File sync/syncable/syncable_id.h (right):

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_i...
sync/syncable/syncable_id.h:46: inline Id() : s_("") {}
Did you write s_("") to make it more explicit and readable? s_() will have the
same behavior.

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_i...
sync/syncable/syncable_id.h:64: inline bool IsNull() const { return s_.size() ==
0; }
nit: 
replace "s_.size() == 0" with "s_.empty()"
On next line replace s_ = "" with s_.clear()

[stanisc, 2014-12-22 21:56:01]

https://codereview.chromium.org/805633004/diff/20001/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/805633004/diff/20001/sync/engine/commit_util....
sync/engine/commit_util.cc:154: if (new_parent_id != server_parent_id &&
!server_parent_id.IsNull() &&
On 2014/12/22 21:38:09, pavely wrote:
> Could you give examples when server_parent_id will be empty? It seems like
there
> is change of behavior that might affect server.

Currently parent IDs are never nulls (empty) so there shouldn't be any change in
the behavior, at least not until the next change that will really introduce
empty parent IDs.
I made this change in anticipation of the future change and to avoid changing a
lot of tests that don't bother setting parent IDs explicitly. With the previous
implementation unset parent IDs would all be root IDs (with "r" as their
content). Now that uninitialized IDs end up being empty strings
SyncableIdToProto fails to process an empty string based ID. 
I had two choices. Either adding an extra check here or modify SyncableIdToProto
to handle a null ID the same way it handled a root ID.

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_id.h
File sync/syncable/syncable_id.h (right):

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_i...
sync/syncable/syncable_id.h:46: inline Id() : s_("") {}
On 2014/12/22 21:38:09, pavely wrote:
> Did you write s_("") to make it more explicit and readable? s_() will have the
> same behavior.

You are right. s_ would auto-initialize to an empty string. I think I ended up
having it this way because I simply edited the old initialization with "r",
replacing "r" with an empty string. I'll fix this.

[pavely, 2014-12-22 22:00:48]

lgtm after addressing small comments.

https://codereview.chromium.org/805633004/diff/20001/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/805633004/diff/20001/sync/engine/commit_util....
sync/engine/commit_util.cc:154: if (new_parent_id != server_parent_id &&
!server_parent_id.IsNull() &&
On 2014/12/22 21:56:01, stanisc wrote:
> On 2014/12/22 21:38:09, pavely wrote:
> > Could you give examples when server_parent_id will be empty? It seems like
> there
> > is change of behavior that might affect server.
> 
> Currently parent IDs are never nulls (empty) so there shouldn't be any change
in
> the behavior, at least not until the next change that will really introduce
> empty parent IDs.
> I made this change in anticipation of the future change and to avoid changing
a
> lot of tests that don't bother setting parent IDs explicitly. With the
previous
> implementation unset parent IDs would all be root IDs (with "r" as their
> content). Now that uninitialized IDs end up being empty strings
> SyncableIdToProto fails to process an empty string based ID. 
> I had two choices. Either adding an extra check here or modify
SyncableIdToProto
> to handle a null ID the same way it handled a root ID.

sgtm

[stanisc, 2014-12-22 22:42:36]

Patchset #3 (id:40001) has been deleted

[stanisc, 2014-12-22 22:44:20]

Addressed CR feedback.

https://codereview.chromium.org/805633004/diff/20001/sync/engine/directory_up...
File sync/engine/directory_update_handler_unittest.cc (right):

https://codereview.chromium.org/805633004/diff/20001/sync/engine/directory_up...
sync/engine/directory_update_handler_unittest.cc:32: using syncable::Id;
On 2014/12/22 21:38:09, pavely wrote:
> nit: There are plenty of places in this file with syncable::Id. With your
change
> there are both: Id and syncable::Id. I think you should choose one and make
sure
> it is used consistently in the file.

Done.

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_id.h
File sync/syncable/syncable_id.h (right):

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_i...
sync/syncable/syncable_id.h:46: inline Id() : s_("") {}
On 2014/12/22 21:56:01, stanisc wrote:
> On 2014/12/22 21:38:09, pavely wrote:
> > Did you write s_("") to make it more explicit and readable? s_() will have
the
> > same behavior.
> 
> You are right. s_ would auto-initialize to an empty string. I think I ended up
> having it this way because I simply edited the old initialization with "r",
> replacing "r" with an empty string. I'll fix this.

Done.

https://codereview.chromium.org/805633004/diff/20001/sync/syncable/syncable_i...
sync/syncable/syncable_id.h:64: inline bool IsNull() const { return s_.size() ==
0; }
On 2014/12/22 21:38:09, pavely wrote:
> nit: 
> replace "s_.size() == 0" with "s_.empty()"
> On next line replace s_ = "" with s_.clear()

Done.

[stanisc, 2014-12-22 23:50:56]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-12-22 23:52:11]

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

[commit-bot: I haz the power, 2014-12-23 00:03:09]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-12-23 00:04:01]

Patchset 3 (id:??) landed as
https://crrev.com/60c25aebaf6c3bbc90c9c82f83a752ca58c7ddce
Cr-Commit-Position: refs/heads/master@{#309495}