[Sync] Add ordinal_in_parent to SyncEntity/CommitResponse protobuf

[Sync] Add ordinal_in_parent to SyncEntity/CommitResponse protobuf

This will replace position_in_parent, which has only 64 bytes of
resolution.

Add detailed comments explaining how ordinal_in_parent should be set.

BUG=112201
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128159

[akalin, 2012-02-09 00:31:38]

+nick for review (since this will involve server changes)

[ncarter (slow), 2012-02-09 00:54:46]

https://chromiumcodereview.appspot.com/9369005/diff/1/chrome/browser/sync/pro...
File chrome/browser/sync/protocol/sync.proto (right):

https://chromiumcodereview.appspot.com/9369005/diff/1/chrome/browser/sync/pro...
chrome/browser/sync/protocol/sync.proto:191: // the bytes of
|position_in_parent| in big-endian order (MSB first).
Since these values are signed, I believe the following is the way we convert
from int64's to byte[]s. Java code from the server:

  private byte[] encodePosition(long position) {
    byte[] result = new byte[8];
    position ^= 0x8000000000000000L;
    for (int i = 7; i >= 0; --i) {
      result[i] = (byte) position;
      position >>= 8;
    }
    return result;
  }

That's a bijection between int64's and byte[8]. The implication being that if
you allow byte[7]'s and shorter to be mapped into byte strings, the result is no
longer bijective.

For this reason we may wish to enforce a minimum byte length of 8. Or not. But
now is the time to make up our mind.

[akalin, 2012-02-09 01:10:23]

PTAL

https://chromiumcodereview.appspot.com/9369005/diff/1/chrome/browser/sync/pro...
File chrome/browser/sync/protocol/sync.proto (right):

https://chromiumcodereview.appspot.com/9369005/diff/1/chrome/browser/sync/pro...
chrome/browser/sync/protocol/sync.proto:191: // the bytes of
|position_in_parent| in big-endian order (MSB first).
On 2012/02/09 00:54:47, ncarter wrote:
> Since these values are signed, I believe the following is the way we convert
> from int64's to byte[]s. Java code from the server:
> 
>   private byte[] encodePosition(long position) {
>     byte[] result = new byte[8];
>     position ^= 0x8000000000000000L;
>     for (int i = 7; i >= 0; --i) {
>       result[i] = (byte) position;
>       position >>= 8;
>     }
>     return result;
>   }
> 
> That's a bijection between int64's and byte[8]. The implication being that if
> you allow byte[7]'s and shorter to be mapped into byte strings, the result is
no
> longer bijective.

You're right.  Clarified comment.

> For this reason we may wish to enforce a minimum byte length of 8. Or not. But
> now is the time to make up our mind.

Went with mandating at least 8 bytes.

Also mandated that ordinals cannot be all 0s (so that we can always insert a
position before).

[ncarter (slow), 2012-02-09 01:30:57]

https://chromiumcodereview.appspot.com/9369005/diff/4003/chrome/browser/sync/...
File chrome/browser/sync/protocol/sync.proto (right):

https://chromiumcodereview.appspot.com/9369005/diff/4003/chrome/browser/sync/...
chrome/browser/sync/protocol/sync.proto:185: // instead.  In the interim,
clients should supply both
nit: The phrase "In the interim" made sense when the language was "This is
*being* deprecated", but it's a little temporally confusing as appears here.

https://chromiumcodereview.appspot.com/9369005/diff/4003/chrome/browser/sync/...
chrome/browser/sync/protocol/sync.proto:187: // |ordinal_in_parent| value
returned in the CommitResponse.
There is no ordinal_in_parent value returned in the CommitResponse (and we don't
need one).

https://chromiumcodereview.appspot.com/9369005/diff/4003/chrome/browser/sync/...
chrome/browser/sync/protocol/sync.proto:193: optional int64 position_in_parent =
15;
A draft of some possible language about the deprecation -- please revise/riff as
appropriate:

"Until such time as clients can be sure that all production server instances
fully support ordinal_in_parent, when committing, clients should set all three
position fields (position_in_parent, insert_after_item_id, and ordinal_in_parent
in commits), and continue to respect any position_in_parent values returned by
CommitResponses. Similarly, when ordinal_in_parent is absent in a GetUpdates,
and position_in_parent is present, clients should continue to honor the
position_in_parent value (possibly by transforming it as previously described)"

https://chromiumcodereview.appspot.com/9369005/diff/4003/chrome/browser/sync/...
chrome/browser/sync/protocol/sync.proto:205: // an absolute |position_in_parent|
value at commit time.  In the
This comment (which redirects to another deprecated feature) may create some
confusion. You might just want to redirect to the other comment.

[akalin, 2012-03-20 23:32:07]

I think I addressed all your comments, and also added a paragraph about adding
randomness on conflicts, per our discussion.

PTAL.

[ncarter (slow), 2012-03-21 23:53:27]

lgtm

[akalin, 2012-03-21 23:55:41]

On 2012/03/21 23:53:27, ncarter wrote:
> lgtm

Just to capture our discussion, you mentioned that clients may have
position_in_parent that maps to the '00000000' ordinal.  We should special-case
those to map to '000000001' instead.

[commit-bot: I haz the power, 2012-03-22 02:22:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/9369005/16001

[commit-bot: I haz the power, 2012-03-22 03:31:58]

Change committed as 128159