[sync] Add store version to model type store backend

components/sync/model_impl/model_type_store_backend.h

Index: components/sync/model_impl/model_type_store_backend.h
diff --git a/components/sync/model_impl/model_type_store_backend.h b/components/sync/model_impl/model_type_store_backend.h
index e7d375d2ba5b8d688032a5fe21d9e1ac1c047319..75559a130d7e8233624d9adb28bc9754500d4594 100644
--- a/components/sync/model_impl/model_type_store_backend.h
+++ b/components/sync/model_impl/model_type_store_backend.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef COMPONENTS_SYNC_MODEL_IMPL_MODEL_TYPE_STORE_BACKEND_H_
-#define COMPONENTS_SYNC_MODEL_IMPL_MODEL_TYPE_STORE_BACKEND_H_
+#ifndef COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_

[maxbogue, 2016/10/18 00:17:51]

Bad merge?

[Patrick Noland, 2016/10/20 19:15:59]

Done.

+#define COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_
 
 #include <memory>
 #include <string>
@@ -20,8 +20,17 @@ class Env;
 class WriteBatch;
 }  // namespace leveldb
 
+namespace sync_pb {
+class ModelTypeStoreSchemaDescriptor;
+}  // namespace sync_pb
+
+using sync_pb::ModelTypeStoreSchemaDescriptor;

[maxbogue, 2016/10/18 00:17:51]

Never use "using" statements in header files; my understanding is that this puts
ModelTypeStoreSchemaDescriptor into the global namespace for anyone that
includes this file.

[Patrick Noland, 2016/10/20 19:15:59]

Done.

+
 namespace syncer {
 
+extern const int64_t kLatestSchemaVersion;
+extern const char kDBSchemaDescriptorRecordId[];

[pavely, 2016/10/18 18:58:26]

These two constants should not be needed outside of store backend
implementation. Could you move them to .cc file?

[Patrick Noland, 2016/10/20 19:15:59]

They're used in the unit test file as well. Is there a good way to make a
constant visible to the test class but not anywhere else?

[pavely, 2016/10/20 21:33:52]

directory_backing_store_unittest circumvents this problem by placing this
"extern ..." declaration in test file.
Alternatively you can make these constants static const members of
ModelTypeStoreBackend. Either make them private and write accessor in test class
or make them public.

The issue with current approach is that these names become available to anyone
including this header file and names themselves don't indicate that they are
related to store.

+
 // ModelTypeStoreBackend handles operations with leveldb. It is oblivious of the
 // fact that it is called from separate thread (with the exception of ctor),
 // meaning it shouldn't deal with callbacks and task_runners.
@@ -97,6 +106,20 @@ class ModelTypeStoreBackend
   ModelTypeStore::Result Init(const std::string& path,
                               std::unique_ptr<leveldb::Env> env);
 
+  // Attempts to read the database's schema descriptor into |schema_descriptor|,
+  // returning true on success. If there is not a schema descriptor present, it
+  // reports success and sets schema_descriptor's version_number to 0.
+  bool GetStoreDescriptor(ModelTypeStoreSchemaDescriptor* schema_descriptor);

[pavely, 2016/10/18 18:58:25]

We discussed this in person, it is better not to expose schema descriptor
without a good reason. Caller of Migrate for example cannot/shouldn't know what
desired_descriptor should look like as it might get some field values from
intermediate migrations. I propose to pass version number through function
parameters and only use schema descriptor for serialization.

[Patrick Noland, 2016/10/20 19:15:59]

Done.

+
+  // Migrate the db schema from |current_descriptor| to |desired_descriptor|,
+  // returning true on success.
+  bool Migrate(const ModelTypeStoreSchemaDescriptor& current_descriptor,
+               const ModelTypeStoreSchemaDescriptor& desired_descriptor);
+
+  // Migrates from no version record at all (version 0) to version 1 of
+  // the schema, returning true on success.
+  bool Migrate0To1();
+
   // Macro wrapped mutex to guard against concurrent calls in debug builds.
   DFAKE_MUTEX(push_pop_);
 
@@ -105,4 +128,4 @@ class ModelTypeStoreBackend
 
 }  // namespace syncer
 
-#endif  // COMPONENTS_SYNC_MODEL_IMPL_MODEL_TYPE_STORE_BACKEND_H_
+#endif  // COMPONENTS_SYNC_CORE_MODEL_TYPE_STORE_BACKEND_H_

[maxbogue, 2016/10/18 00:17:51]

Also here.

[Patrick Noland, 2016/10/20 19:15:59]

Done.