Helium: Initial proto and Syncable interface definition

blimp/net/helium/vector_clock.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "blimp/net/helium/vector_clock.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 
 namespace blimp {
 
 VectorClock::VectorClock() {}
 
 VectorClock::VectorClock(Revision local_revision, Revision remote_revision)
     : local_revision_(local_revision), remote_revision_(remote_revision) {}
 
 VectorClock::Comparison VectorClock::CompareTo(const VectorClock& other) const {

[Sriram, 2016/09/29 18:52:21]

Nit: Might be worth adding Unit tests to for the methods.

   DCHECK(local_revision_ >= other.local_revision());

[Sriram, 2016/09/29 18:52:21]

Can you add comment as to why Comparison() expects the other.local_revision() to
be greater than or equal to this objects?

 
   if (local_revision_ == other.local_revision()) {
     if (remote_revision_ == other.remote_revision()) {
       return VectorClock::Comparison::EqualTo;
     } else if (remote_revision_ < other.remote_revision()) {
       return VectorClock::Comparison::LessThan;
     } else {
       return VectorClock::Comparison::GreaterThan;
     }
   } else {
@@ skipping 17 matching lines @@
 VectorClock VectorClock::MergeWith(const VectorClock& other) const {
   VectorClock result(std::max(local_revision_, other.local_revision()),
                      std::max(remote_revision_, other.remote_revision()));
   return result;
 }
 
 void VectorClock::IncrementLocal() {
   local_revision_++;
 }
 
+helium::VectorClock VectorClock::CreateProto() {
+  helium::VectorClock result;

[CJ, 2016/09/29 20:36:55]

Is helium::VectorClock the message? Hard to tell. I'd suggest if so, add
"Message" at the end of the VectorClock proto name to make this explicit. 

[scf, 2016/09/29 23:13:39]

I was following the convention of other proto files. They neither specify a
namespace of a suffix in the class name. Thoughts?

[CJ, 2016/09/29 23:44:00]

The file names leave off the "message" part, but the actual messages defined in
the proto file have "Message".
Example:
https://cs.chromium.org/chromium/src/blimp/common/proto/ime.proto?rcl=1475162...

[scf, 2016/09/30 17:26:23]

HeliumMessage sounds good?

[CJ, 2016/09/30 19:47:12]

Well, this is for a VectorClock message right? So it should be
VectorClockMessage.

[scf, 2016/09/30 22:13:23]

talked offline. Suffixing everything to contain Message.

+  result.set_local_revision(local_revision_);
+  result.set_remote_revision(remote_revision_);
+  return result;
+}
+
+VectorClock VectorClock::Invert() {
+  VectorClock result;
+  result.set_local_revision(remote_revision_);
+  result.set_remote_revision(local_revision_);
+  return result;
+}
+
 }  // namespace blimp