[Sync] Generalize StringOrdinal to handle ordinal_in_parent field

[Sync] Generalize StringOrdinal to handle ordinal_in_parent field

Rename StringOrdinal to Ordinal, move it to sync/, and templatize it.
Make StringOrdinal be an instantiation of Ordinal that matches its
previous behavior.

Create NodeOrdinal, which is another instantiation of Ordinal for
the ordinal_in_parent field in SyncEntity.

Rework Ordinal to handle arbitrary byte ranges and to simplify the
interpolation code a bit.

Generalize StringOrdinal unit tests for Ordinal.

Update users of StringOrdinal to prepend syncer::.

BUG=145412
TBR=estade@chromium.org,jianli@chromium.org,brettw@chromium.org, 

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

[akalin, 2012-08-30 20:28:25]

+tim for sync/ and c/b/sync stuff
+asargent for c/b/extensions

[akalin, 2012-08-31 21:31:35]

Ping!

[akalin, 2012-09-05 17:06:11]

-tim +rlarocque for review

[rlarocque, 2012-09-05 21:43:46]

Taking a break for now.  I might have more comments to add later.

http://codereview.chromium.org/10920017/diff/7043/sync/api/string_ordinal.h
File sync/api/string_ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/api/string_ordinal.h#n...
sync/api/string_ordinal.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
Why is this file under sync/api, while other seemingly related files are under
sync/internal_api/public/base?

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
I'm not convinced the functions in this file are endian-safe.  

Your tests look pretty good, though.  Have they been run on non-x86 platforms
yet?

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:13: y ^= 0x8000000000000000ULL;
What is the intent of this statement?

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:18:
ordinal_string.push_back('\x80');
This is not symmetric, right?

Because of this, 
NodeOrdinalToInt64(Int64ToNodeOrdinal(0)) != 0

Is it OK that the conversion is not 100% reversible?

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:21: ordinal_string[i] =
static_cast<uint8>(y);
I looked up the spec because I wasn't sure the result of this statement was well
defined.  It turns out that it is defined for unsigned types.

Looks good, I guess.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:32: const std::string& s =
ordinal.ToString();
Huh.  I did not expect ToString() to be used this way.  

I think there's room for both a 'ToPrintableString()' and
'GetInternalRepresentation()' function on the NodeOrdinal class.  I just hadn't
expected that ToString() == GetInternalRepresentation().

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:33: const size_t l =
std::min(NodeOrdinal::kMinLength, s.length());
Won't this always equal kMinLength?  I think that 's' would fail
IsValidOrdinalString() if  s.length() < kMinLength.  That's not supposed to
happen, is it?

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:39: return static_cast<int64>(y);
Now this I think actually is undefined if y > INT64_MAX.  It's probably going to
work as expected anyway, though.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.h:19: static const uint8 kZeroDigit =
0;
This mean we could end up with zeroes embedded throughout the string, right? 
Will NULL characters break std::string?

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.h:24: typedef
Ordinal<NodeOrdinalTraits> NodeOrdinal;
Ordinal<T>'s ToString() methods* probably won't work very well for this new set
of traits.  Even if the NULL character issue is handled, there probably should
be some escaping or hex-encoding of the output.

It would be nice to override them, but, as you've pointed out before, it's
probably not a good idea to inherit from an instantiated template.

Maybe the right thing to do would be to wrap Ordinal<NodeOrdinalTraits> in a new
class whose methods are almost all thin wrappers around its
Ordinal<NodeOrdinalTraits> member's methods.  That would have the side-benefit
of keeping the inline template instantiation code confined to a few .cpp files. 
And it would give you a class to which you could add the {To,From}Int64 methods.
 It would be tedious, though.  

Thoughts?

* NOTE: For the context of this comment, I assumed that ToString() was intended
as "give me a string which I can print", not "give me the internal
representation of this object".  I can see in other parts of the review you rely
on it having the latter definition.  I think there's a need for functions which
implement either definition, and, once we settle on their names, the above
comment still applies.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal_unittest.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal_unittest.cc:95: TEST(NodeOrdinalTest,
ConsistentOrdering) {
Good test.  

I have one suggestion, though.  As I understand it, both of these arrays start
off sorted.  Could you mix up the order a bit so the sort function has some work
to do?  Otherwise, the LessThan function could be hard-coded to 'return false'
and this test would not catch it, for some implementations of sort().

One option would be to add a 'reversed_ordinal_ordering' and sort it, too, then
check that all three orderings end up being equal.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/ordinal.h:236: return is_valid_;
I'd feel safer if there was a DCHECK(IsInvalidOrdinalString(ordinal_string_))
here.

http://codereview.chromium.org/10920017/diff/7043/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

http://codereview.chromium.org/10920017/diff/7043/sync/protocol/sync.proto#ne...
sync/protocol/sync.proto:317: // same ordinal-in-parent, a client should
randomly pick one, and
"randomly pick one" will limit our ability to test this code.  I'd prefer if the
ordering were based on some known value.  The ID, for example, which is more or
less guaranteed to be unique, would be a good choice.

[akalin, 2012-09-06 00:04:54]

forgot to actually +asargent

[asargent_no_longer_on_chrome, 2012-09-06 16:31:37]

I'm swamped at the moment and need to defer some reviews for a few days -
if this is time sensitive please send to another reviewer.

On Wed, Sep 5, 2012 at 5:04 PM, <akalin@chromium.org> wrote:

> forgot to actually +asargent
>
>
http://codereview.chromium.**org/10920017/<http://codereview.chromium.org/109...
>

[akalin, 2012-09-06 18:59:21]

moving from asargent to yoz

[akalin, 2012-09-06 19:25:14]

Addressed all comments, PTAL

http://codereview.chromium.org/10920017/diff/7043/sync/api/string_ordinal.h
File sync/api/string_ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/api/string_ordinal.h#n...
sync/api/string_ordinal.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2012/09/05 21:43:46, rlarocque wrote:
> Why is this file under sync/api, while other seemingly related files are under
> sync/internal_api/public/base?

Because this is the only file that users of the "new" sync API should use. 
NodeOrdinal is definitely used only internally, and Ordinal in some sense is
internal-only.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/09/05 21:43:46, rlarocque wrote:
> I'm not convinced the functions in this file are endian-safe.  
> 
> Your tests look pretty good, though.  Have they been run on non-x86 platforms
> yet?

I asked, and I don't think we have access to big-endian machines.  FWIW,
sync.proto defines the conversion to be in network byte order, which is the
opposite from x86, so I'd assume any endian problems would show up in x86.

But shifts and masks are endian-neutral, so I think it's okay.  Also somewhat
related, see: http://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:13: y ^= 0x8000000000000000ULL;
On 2012/09/05 21:43:46, rlarocque wrote:
> What is the intent of this statement?

The ordering of an int differs depending on its signedness (basically, all
'negative' numbers compare greater than positive numbers when treated as
unsigned).  Flipping the high-order bit corrects for that.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:18:
ordinal_string.push_back('\x80');
On 2012/09/05 21:43:46, rlarocque wrote:
> This is not symmetric, right?
> 
> Because of this, 
> NodeOrdinalToInt64(Int64ToNodeOrdinal(0)) != 0
> 
> Is it OK that the conversion is not 100% reversible?

It's not symmetric, but it's still reversible, since NodeOrdinalToInt64 only
looks at the first 8 bytes, so your expression does evaluate to 0.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:21: ordinal_string[i] =
static_cast<uint8>(y);
On 2012/09/05 21:43:46, rlarocque wrote:
> I looked up the spec because I wasn't sure the result of this statement was
well
> defined.  It turns out that it is defined for unsigned types.
> 
> Looks good, I guess.

Yeap, most of the pitfalls lie with signed ints.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:32: const std::string& s =
ordinal.ToString();
On 2012/09/05 21:43:46, rlarocque wrote:
> Huh.  I did not expect ToString() to be used this way.  
> 
> I think there's room for both a 'ToPrintableString()' and
> 'GetInternalRepresentation()' function on the NodeOrdinal class.  I just
hadn't
> expected that ToString() == GetInternalRepresentation().

As discussed in the other comment, fixed.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:33: const size_t l =
std::min(NodeOrdinal::kMinLength, s.length());
On 2012/09/05 21:43:46, rlarocque wrote:
> Won't this always equal kMinLength?  I think that 's' would fail
> IsValidOrdinalString() if  s.length() < kMinLength.  That's not supposed to
> happen, is it?

You're right.  I still want to avoid out-of-bound memory reads, so I rewrote to
have a NOTREACHED.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:39: return static_cast<int64>(y);
On 2012/09/05 21:43:46, rlarocque wrote:
> Now this I think actually is undefined if y > INT64_MAX.  It's probably going
to
> work as expected anyway, though.

Implementation-defined, actually, so I think it's okay.  Added comment.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.h:19: static const uint8 kZeroDigit =
0;
On 2012/09/05 21:43:46, rlarocque wrote:
> This mean we could end up with zeroes embedded throughout the string, right? 
> Will NULL characters break std::string?

Yes, but std::string can have embedded NULLs.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.h:24: typedef
Ordinal<NodeOrdinalTraits> NodeOrdinal;
On 2012/09/05 21:43:46, rlarocque wrote:
> Ordinal<T>'s ToString() methods* probably won't work very well for this new
set
> of traits.  Even if the NULL character issue is handled, there probably should
> be some escaping or hex-encoding of the output.
> 
> It would be nice to override them, but, as you've pointed out before, it's
> probably not a good idea to inherit from an instantiated template.
> 
> Maybe the right thing to do would be to wrap Ordinal<NodeOrdinalTraits> in a
new
> class whose methods are almost all thin wrappers around its
> Ordinal<NodeOrdinalTraits> member's methods.  That would have the side-benefit
> of keeping the inline template instantiation code confined to a few .cpp
files. 
> And it would give you a class to which you could add the {To,From}Int64
methods.
>  It would be tedious, though.  
> 
> Thoughts?
> 
> * NOTE: For the context of this comment, I assumed that ToString() was
intended
> as "give me a string which I can print", not "give me the internal
> representation of this object".  I can see in other parts of the review you
rely
> on it having the latter definition.  I think there's a need for functions
which
> implement either definition, and, once we settle on their names, the above
> comment still applies.

You're right.  Split them up into ToInternalValue() and ToDebugString().  I
thought about the thin wrapper, but it seems like too much trouble.

Also changed the names used in ordinal.h to emphasize the binary nature of the
internal representation (basically use the name 'bytes' everywhere)

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal_unittest.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal_unittest.cc:95: TEST(NodeOrdinalTest,
ConsistentOrdering) {
On 2012/09/05 21:43:46, rlarocque wrote:
> Good test.  
> 
> I have one suggestion, though.  As I understand it, both of these arrays start
> off sorted.  Could you mix up the order a bit so the sort function has some
work
> to do?  Otherwise, the LessThan function could be hard-coded to 'return false'
> and this test would not catch it, for some implementations of sort().
> 
> One option would be to add a 'reversed_ordinal_ordering' and sort it, too,
then
> check that all three orderings end up being equal.

The sorting does do work.  Remember that these arrays represent indices into the
ordinals array, so they start off sorted, and then get jumbled up to represent
the sorted order of their respective arrays.

Added check to make sure that the original ordering != the sorted ordering,
though.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/ordinal.h:236: return is_valid_;
On 2012/09/05 21:43:46, rlarocque wrote:
> I'd feel safer if there was a DCHECK(IsInvalidOrdinalString(ordinal_string_))
> here.

assuming you meant DCHECK_EQ(is_valid_, ...), done

http://codereview.chromium.org/10920017/diff/7043/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

http://codereview.chromium.org/10920017/diff/7043/sync/protocol/sync.proto#ne...
sync/protocol/sync.proto:317: // same ordinal-in-parent, a client should
randomly pick one, and
On 2012/09/05 21:43:46, rlarocque wrote:
> "randomly pick one" will limit our ability to test this code.  I'd prefer if
the
> ordering were based on some known value.  The ID, for example, which is more
or
> less guaranteed to be unique, would be a good choice.  

A secondary goal was to minimize the number of conflicting nodes whenever two
end up with the same ordinal.  The most common case I'm guessing will be when
two clients both create the initial node at the same time, say, N1 and N2, with
N1's ID < N2's ID.  If both clients make a consistent choice regarding which
ordinal to leave alone, say, N1, then they will both end up perturbing N2's
ordinal, likely resulting in a conflict, which is theoretically harmless, but
you know how a lot of our bugs come from conflicts.

With a random choice of which ordinal to perturb, there's a 50% chance that each
client will perturb a different ordinal, resulting in no conflicts.

Of course, this breaks down for more than 2 nodes, but those should be rare
anyway.

[Yoyo Zhou, 2012-09-06 21:18:39]

From an extensions perspective, it is weird that StringOrdinal now is in sync/
since the uses of it in extensions (for ordering apps on the NTP) have nothing
to do with sync. I could see this contributing to DEPS violations/exceptions in
the future.

[Yoyo Zhou, 2012-09-06 21:20:42]

On 2012/09/06 21:18:39, Yoyo Zhou wrote:
> From an extensions perspective, it is weird that StringOrdinal now is in sync/
> since the uses of it in extensions (for ordering apps on the NTP) have nothing
> to do with sync. I could see this contributing to DEPS violations/exceptions
in
> the future.

...so, my question is, does it belong in base/?

[rlarocque, 2012-09-06 21:25:55]

Mostly there.  I think the only thing holding this back now is testing for the
ToDebugString() methods.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/09/06 19:25:14, akalin wrote:
> On 2012/09/05 21:43:46, rlarocque wrote:
> > I'm not convinced the functions in this file are endian-safe.  
> > 
> > Your tests look pretty good, though.  Have they been run on non-x86
platforms
> > yet?
> 
> I asked, and I don't think we have access to big-endian machines.  FWIW,
> sync.proto defines the conversion to be in network byte order, which is the
> opposite from x86, so I'd assume any endian problems would show up in x86.
> 
> But shifts and masks are endian-neutral, so I think it's okay.  Also somewhat
> related, see:
http://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

Sounds good.  I was mainly concerned about the bit-masks and cast to uint8_t. 
I'm now reasonably convinced that it's safe.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:13: y ^= 0x8000000000000000ULL;
On 2012/09/06 19:25:14, akalin wrote:
> On 2012/09/05 21:43:46, rlarocque wrote:
> > What is the intent of this statement?
> 
> The ordering of an int differs depending on its signedness (basically, all
> 'negative' numbers compare greater than positive numbers when treated as
> unsigned).  Flipping the high-order bit corrects for that.

IIRC, uint64 addition is defined to be correct modulo UINT64_MAX+1.

I think that means the same operation could be expressed as

y += 0x8000000000000000ULL

where 0x8000000000000000ULL == (UINT64_MAX+1)/2, if UINT64_MAX+1 did not
overflow.  

Another way to think about it would be that it subtracts the value of INT64_MIN.

y -= INT64_MIN

Hmm... maybe your way is better.  I was hoping there was some neat way to
express this so that it's more clear that you're trying to add a constant value
to all these numbers so they land in a positive range.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:18:
ordinal_string.push_back('\x80');
On 2012/09/06 19:25:14, akalin wrote:
> On 2012/09/05 21:43:46, rlarocque wrote:
> > This is not symmetric, right?
> > 
> > Because of this, 
> > NodeOrdinalToInt64(Int64ToNodeOrdinal(0)) != 0
> > 
> > Is it OK that the conversion is not 100% reversible?
> 
> It's not symmetric, but it's still reversible, since NodeOrdinalToInt64 only
> looks at the first 8 bytes, so your expression does evaluate to 0.

Now I understand.  I overlooked the fact that the first 8 bytes were already
initialized to zero by this point.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal_unittest.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal_unittest.cc:95: TEST(NodeOrdinalTest,
ConsistentOrdering) {
On 2012/09/06 19:25:14, akalin wrote:
> On 2012/09/05 21:43:46, rlarocque wrote:
> > Good test.  
> > 
> > I have one suggestion, though.  As I understand it, both of these arrays
start
> > off sorted.  Could you mix up the order a bit so the sort function has some
> work
> > to do?  Otherwise, the LessThan function could be hard-coded to 'return
false'
> > and this test would not catch it, for some implementations of sort().
> > 
> > One option would be to add a 'reversed_ordinal_ordering' and sort it, too,
> then
> > check that all three orderings end up being equal.
> 
> The sorting does do work.  Remember that these arrays represent indices into
the
> ordinals array, so they start off sorted, and then get jumbled up to represent
> the sorted order of their respective arrays.
> 
> Added check to make sure that the original ordering != the sorted ordering,
> though.

Sounds good.  I misread the kTestValues.

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/ordinal.h (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/ordinal.h:236: return is_valid_;
On 2012/09/06 19:25:14, akalin wrote:
> On 2012/09/05 21:43:46, rlarocque wrote:
> > I'd feel safer if there was a
DCHECK(IsInvalidOrdinalString(ordinal_string_))
> > here.
> 
> assuming you meant DCHECK_EQ(is_valid_, ...), done

Correct.  Thanks.

http://codereview.chromium.org/10920017/diff/7043/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

http://codereview.chromium.org/10920017/diff/7043/sync/protocol/sync.proto#ne...
sync/protocol/sync.proto:317: // same ordinal-in-parent, a client should
randomly pick one, and
On 2012/09/06 19:25:14, akalin wrote:
> On 2012/09/05 21:43:46, rlarocque wrote:
> > "randomly pick one" will limit our ability to test this code.  I'd prefer if
> the
> > ordering were based on some known value.  The ID, for example, which is more
> or
> > less guaranteed to be unique, would be a good choice.  
> 
> A secondary goal was to minimize the number of conflicting nodes whenever two
> end up with the same ordinal.  The most common case I'm guessing will be when
> two clients both create the initial node at the same time, say, N1 and N2,
with
> N1's ID < N2's ID.  If both clients make a consistent choice regarding which
> ordinal to leave alone, say, N1, then they will both end up perturbing N2's
> ordinal, likely resulting in a conflict, which is theoretically harmless, but
> you know how a lot of our bugs come from conflicts.
> 
> With a random choice of which ordinal to perturb, there's a 50% chance that
each
> client will perturb a different ordinal, resulting in no conflicts.
> 
> Of course, this breaks down for more than 2 nodes, but those should be rare
> anyway.

I'm not convinced, but this is the wrong place for this discussion.  I shouldn't
have brought it up here.

Let's defer this discussion until later.

http://codereview.chromium.org/10920017/diff/13010/chrome/browser/extensions/...
File chrome/browser/extensions/app_sync_data.cc (right):

http://codereview.chromium.org/10920017/diff/13010/chrome/browser/extensions/...
chrome/browser/extensions/app_sync_data.cc:68:
specifics->set_app_launch_ordinal(app_launch_ordinal_.ToInternalValue());
Much better!

Seeing this made me want to look up whether or not app_launch_ordinal was
declared as a string, and whether or not protobuf strings can contain binary
data.

It turns out that protobufs strings expect either 7-bit ASCII or UTF-8, so
binary data is not supported.  app_launch_ordinal is declared as a string.  

Then I remembered that none of this matters because this is a StringOrdinal. 
False alarm.  

I still think it's a good thing that the name prompted me to think about these
issues, though.

http://codereview.chromium.org/10920017/diff/13010/sync/internal_api/public/b...
File sync/internal_api/public/base/ordinal.h (right):

http://codereview.chromium.org/10920017/diff/13010/sync/internal_api/public/b...
sync/internal_api/public/base/ordinal.h:262: std::string
Ordinal<Traits>::ToDebugString() const {
Looks good.

Do you have tests that execute this code?  It would be nice to ensure that the
kTestValues from NodeOrdinalTest can be serialized without a DCHECK.

[akalin, 2012-09-06 22:33:21]

On 2012/09/06 21:18:39, Yoyo Zhou wrote:
> From an extensions perspective, it is weird that StringOrdinal now is in sync/
> since the uses of it in extensions (for ordering apps on the NTP) have nothing
> to do with sync. I could see this contributing to DEPS violations/exceptions
in
> the future.

Well, the use of Ordinal to order apps on the NTP is driven by sync, right?  I
mean, if it weren't for sync, we'd just use an int and be done with it.

I don't know about trying to move this to base/ just yet.  I think all uses are
sync-related...

[akalin, 2012-09-06 22:55:25]

PTAL!

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
File sync/internal_api/public/base/node_ordinal.cc (right):

http://codereview.chromium.org/10920017/diff/7043/sync/internal_api/public/ba...
sync/internal_api/public/base/node_ordinal.cc:13: y ^= 0x8000000000000000ULL;
On 2012/09/06 21:25:55, rlarocque wrote:
> On 2012/09/06 19:25:14, akalin wrote:
> > On 2012/09/05 21:43:46, rlarocque wrote:
> > > What is the intent of this statement?
> > 
> > The ordering of an int differs depending on its signedness (basically, all
> > 'negative' numbers compare greater than positive numbers when treated as
> > unsigned).  Flipping the high-order bit corrects for that.
> 
> IIRC, uint64 addition is defined to be correct modulo UINT64_MAX+1.
> 
> I think that means the same operation could be expressed as
> 
> y += 0x8000000000000000ULL
> 
> where 0x8000000000000000ULL == (UINT64_MAX+1)/2, if UINT64_MAX+1 did not
> overflow.  
> 
> Another way to think about it would be that it subtracts the value of
INT64_MIN.
> 
> y -= INT64_MIN
> 
> Hmm... maybe your way is better.  I was hoping there was some neat way to
> express this so that it's more clear that you're trying to add a constant
value
> to all these numbers so they land in a positive range.

Yeah, I think it's somewhat clearer to just twiddle the bits.  I wish there were
a concise way to say "flip the high bit" but this'll have to do.

http://codereview.chromium.org/10920017/diff/13010/chrome/browser/extensions/...
File chrome/browser/extensions/app_sync_data.cc (right):

http://codereview.chromium.org/10920017/diff/13010/chrome/browser/extensions/...
chrome/browser/extensions/app_sync_data.cc:68:
specifics->set_app_launch_ordinal(app_launch_ordinal_.ToInternalValue());
On 2012/09/06 21:25:56, rlarocque wrote:
> Much better!
> 
> Seeing this made me want to look up whether or not app_launch_ordinal was
> declared as a string, and whether or not protobuf strings can contain binary
> data.
> 
> It turns out that protobufs strings expect either 7-bit ASCII or UTF-8, so
> binary data is not supported.  app_launch_ordinal is declared as a string.  
> 
> Then I remembered that none of this matters because this is a StringOrdinal. 
> False alarm.  
> 
> I still think it's a good thing that the name prompted me to think about these
> issues, though.

Yeah.  I also added a comment to StringOrdinal explicitly saying it's safe to
store in a protobuf.

http://codereview.chromium.org/10920017/diff/13010/sync/internal_api/public/b...
File sync/internal_api/public/base/ordinal.h (right):

http://codereview.chromium.org/10920017/diff/13010/sync/internal_api/public/b...
sync/internal_api/public/base/ordinal.h:262: std::string
Ordinal<Traits>::ToDebugString() const {
On 2012/09/06 21:25:56, rlarocque wrote:
> Looks good.
> 
> Do you have tests that execute this code?  It would be nice to ensure that the
> kTestValues from NodeOrdinalTest can be serialized without a DCHECK.

Done.

[Yoyo Zhou, 2012-09-06 23:16:34]

On 2012/09/06 22:33:21, akalin wrote:
> On 2012/09/06 21:18:39, Yoyo Zhou wrote:
> > From an extensions perspective, it is weird that StringOrdinal now is in
sync/
> > since the uses of it in extensions (for ordering apps on the NTP) have
nothing
> > to do with sync. I could see this contributing to DEPS violations/exceptions
> in
> > the future.
> 
> Well, the use of Ordinal to order apps on the NTP is driven by sync, right?  I
> mean, if it weren't for sync, we'd just use an int and be done with it.

Huh, that appears to be true from my reading of the history. LGTM then.

[rlarocque, 2012-09-06 23:19:20]

LGTM.

http://codereview.chromium.org/10920017/diff/14008/sync/internal_api/public/b...
File sync/internal_api/public/base/ordinal_unittest.cc (right):

http://codereview.chromium.org/10920017/diff/14008/sync/internal_api/public/b...
sync/internal_api/public/base/ordinal_unittest.cc:214: EXPECT_EQ("INVALID[]",
TestOrdinal().ToDebugString());
IMO, this is a bit too strict.  There are other ways that one could choose to
represent an Ordinal as a string.  The only real requirement is that
ToDebugString() doesn't crash and that it produces a printable string as output.

From a more practical perspective, though, I doubt this will cause trouble for
anyone.  It's not like we'll be changing serialization formats very often.  It's
fine with me if you decide to leave it as is.

[akalin, 2012-09-06 23:44:32]

Committing via CQ

http://codereview.chromium.org/10920017/diff/14008/sync/internal_api/public/b...
File sync/internal_api/public/base/ordinal_unittest.cc (right):

http://codereview.chromium.org/10920017/diff/14008/sync/internal_api/public/b...
sync/internal_api/public/base/ordinal_unittest.cc:214: EXPECT_EQ("INVALID[]",
TestOrdinal().ToDebugString());
On 2012/09/06 23:19:21, rlarocque wrote:
> IMO, this is a bit too strict.  There are other ways that one could choose to
> represent an Ordinal as a string.  The only real requirement is that
> ToDebugString() doesn't crash and that it produces a printable string as
output.
> 
> From a more practical perspective, though, I doubt this will cause trouble for
> anyone.  It's not like we'll be changing serialization formats very often. 
It's
> fine with me if you decide to leave it as is.

Done.

[commit-bot: I haz the power, 2012-09-06 23:46:10]

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

[commit-bot: I haz the power, 2012-09-06 23:46:25]

Presubmit check for 10920017-8134 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui/webui/ntp
    chrome/browser/ui/panels
    chrome/browser/media_gallery

Presubmit checks took 3.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[akalin, 2012-09-06 23:56:48]

TBRing various folks

[commit-bot: I haz the power, 2012-09-06 23:57:54]

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

[commit-bot: I haz the power, 2012-09-07 12:53:03]

Change committed as 155368