Add LazySyncableAdapter

blimp/helium/lazy_syncable_adapter.h

Index: blimp/helium/lazy_syncable_adapter.h
diff --git a/blimp/helium/lazy_syncable_adapter.h b/blimp/helium/lazy_syncable_adapter.h
new file mode 100644
index 0000000000000000000000000000000000000000..5e460621c4f1e2de4071edea2efd762b6653ccb8
--- /dev/null
+++ b/blimp/helium/lazy_syncable_adapter.h
@@ -0,0 +1,96 @@
+// 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.
+
+#ifndef BLIMP_HELIUM_LAZY_SYNCABLE_ADAPTER_H_
+#define BLIMP_HELIUM_LAZY_SYNCABLE_ADAPTER_H_
+
+#include <string>
+
+#include "base/memory/ptr_util.h"
+#include "blimp/helium/blimp_helium_export.h"
+#include "blimp/helium/syncable.h"
+#include "third_party/protobuf/src/google/protobuf/io/coded_stream.h"
+#include "third_party/protobuf/src/google/protobuf/io/zero_copy_stream_impl_lite.h"
+
+namespace blimp {
+namespace helium {
+
+namespace {
+template <class ChangesetType>
+std::unique_ptr<ChangesetType> ParseChangesetFromString(

[CJ, 2016/11/23 22:39:27]

Just curious, why is this function in an anon namespace?

[steimel, 2016/11/28 17:19:26]

I was on the fence as to whether it should be a private function of the class or
just a outside helper function, but since it doesn't need to know any members or
anything, I decided to make it a helper function. Since it's a helper function
that's only used within this file and I didn't want to pollute the blimp::helium
namespace, I put it in an anon namespace

[CJ, 2016/11/28 19:41:15]

Acknowledged.

+    const std::string& changeset) {
+  google::protobuf::io::ArrayInputStream raw_input_stream(changeset.data(),
+                                                          changeset.size());
+  google::protobuf::io::CodedInputStream input_stream(&raw_input_stream);
+  std::unique_ptr<ChangesetType> parsed_changeset =
+      base::MakeUnique<ChangesetType>();
+  parsed_changeset->Parse(&input_stream);
+  return parsed_changeset;
+}
+}  // namespace
+
+// The LazySyncableAdapter is used to wrap LazySyncables of any changeset type
+// into a LazySyncable with a string changeset by serializing the changesets
+// into a string and parsing them from strings. That way, ObjectSyncState can
+// just work with LazySyncables of type string.
+template <class ChangesetType>
+class BLIMP_HELIUM_EXPORT LazySyncableAdapter
+    : public LazySyncable<std::string> {
+ public:
+  explicit LazySyncableAdapter(LazySyncable<ChangesetType>* object)

[scf, 2016/11/22 23:35:23]

nit: rename to inner_syncable_ ?

[steimel, 2016/11/23 15:56:43]

Done.

+      : object_(object) {}
+  ~LazySyncableAdapter() = default;
+
+  // LazySyncable implementation.
+  std::unique_ptr<std::string> CreateChangeset(Revision from) const override;
+  void ApplyChangeset(const std::string& changeset) override;
+  bool ValidateChangeset(const std::string& changeset) const override;
+  void SetLocalUpdateCallback(
+      const base::Closure& local_update_callback) override {

[scf, 2016/11/22 23:35:22]

for consistency put implementation outside?

[steimel, 2016/11/23 15:56:43]

Done.

+    object_->SetLocalUpdateCallback(local_update_callback);
+  }
+  void ReleaseBefore(Revision checkpoint) override {

[scf, 2016/11/22 23:35:23]

according to @wez i think this was renamed to |before|

[steimel, 2016/11/23 15:56:43]

Okay, changed. Strangely enough, it's currently |checkpoint| in syncable.h, but
the comment above it uses |before|. Something should be changed there

+    object_->ReleaseBefore(checkpoint);
+  }
+  Revision GetRevision() const override { return object_->GetRevision(); }
+  void PrepareToCreateChangeset(Revision from, base::Closure done) override {
+    object_->PrepareToCreateChangeset(from, done);
+  }
+
+ private:
+  LazySyncable<ChangesetType>* object_;
+
+  DISALLOW_COPY_AND_ASSIGN(LazySyncableAdapter);
+};
+
+template <class ChangesetType>
+std::unique_ptr<std::string>
+LazySyncableAdapter<ChangesetType>::CreateChangeset(Revision from) const {
+  std::unique_ptr<ChangesetType> changeset = object_->CreateChangeset(from);

[scf, 2016/11/22 23:35:22]

It feels to me that there is a bit of inefficiency here. I assume 99% of the
objects being synchronized will be CompoundSyncables which have an interface
CreateChangeset(Revision from,     google::protobuf::io::CodedOutputStream*
output_stream) which would be perfect use for the ObjectSyncState. Many
CodedOutputStreams and strings are being created with the current approach.

Chatted offline with @wez, he suggested one thing we could do is have something
similar to 

```
CreateChangeset(
    Revision from,
    google::protobuf::io::CodedOutputStream* output_stream)
```

At a new class which is the base of Syncable. This class is not templatized so
that ObjectSyncState can use directly. ObjectSyncState could use this and this
could simplify the implementation of CompoundSyncable too. 

Something like the following:
* BaseLazeSyncable: Not templatized class that contains all methods of
LazySyncables with correspondent Create and Apply changeset that would receive a
CodedOutputStream.
* BaseChangesetLazySyncable: Provides default implementation for serializing
changesets in coded output streams. Basically calls the Serializable methods
(Serialize/Parse)
* CompoundSyncable: Gets updated to use this interface.


WDYT?

[steimel, 2016/11/23 15:56:43]

That's pretty much exactly what Kevin had in his original HybridSyncable design
(Syncable was top-level non-templated with Coded* and HybridSyncable was
templated and used Serialize/Parse). I actually liked that setup and would be
fine moving back to it. Thoughts? 

+  std::unique_ptr<std::string> output = base::MakeUnique<std::string>();
+  google::protobuf::io::StringOutputStream raw_output_stream(output.get());
+  google::protobuf::io::CodedOutputStream output_stream(&raw_output_stream);
+  changeset->Serialize(&output_stream);
+  return output;
+}
+
+template <class ChangesetType>
+void LazySyncableAdapter<ChangesetType>::ApplyChangeset(
+    const std::string& changeset) {
+  std::unique_ptr<ChangesetType> parsed_changeset =
+      ParseChangesetFromString<ChangesetType>(changeset);
+  object_->ApplyChangeset(*parsed_changeset);
+}
+
+template <class ChangesetType>
+bool LazySyncableAdapter<ChangesetType>::ValidateChangeset(
+    const std::string& changeset) const {
+  std::unique_ptr<ChangesetType> parsed_changeset =
+      ParseChangesetFromString<ChangesetType>(changeset);
+  return object_->ValidateChangeset(*parsed_changeset);
+}
+
+}  // namespace helium
+}  // namespace blimp
+#endif  // BLIMP_HELIUM_LAZY_SYNCABLE_ADAPTER_H_