Remove syncproto.h

sync/engine/process_commit_response_command_unittest.cc

Index: sync/engine/process_commit_response_command_unittest.cc
diff --git a/sync/engine/process_commit_response_command_unittest.cc b/sync/engine/process_commit_response_command_unittest.cc
index be9fa22216b231f3e5ef2a58ac2d78dbae8074e1..fccb1ea561d61ebf03aa490cd1257ae9293f8ef4 100644
--- a/sync/engine/process_commit_response_command_unittest.cc
+++ b/sync/engine/process_commit_response_command_unittest.cc
@@ -15,16 +15,20 @@
 #include "sync/syncable/mutable_entry.h"
 #include "sync/syncable/read_transaction.h"
 #include "sync/syncable/syncable_id.h"
+#include "sync/syncable/syncable_proto_util.h"
 #include "sync/syncable/write_transaction.h"
 #include "sync/test/engine/fake_model_worker.h"
 #include "sync/test/engine/syncer_command_test.h"
 #include "sync/test/engine/test_id_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using sync_pb::CommitResponse;

[akalin, 2012/07/11 01:42:22]

move into namespace?

[rlarocque, 2012/07/11 19:22:16]

Wouldn't it make more sense to move the other one out of the namespace?  sync_pb
is not nested within syncable.  Although, I suppose std::string isn't under
syncer, either.

What's the reasoning for putting all the using declarations within the
namespace?

[akalin, 2012/07/11 23:10:40]

I could be wrong, but 'using' declarations are scoped, right?  So if the only
things that are relying on the using declarations are in the syncer namespace,
then the using declarations should be in that namespace, too.

(Although I guess it doesn't matter too much since it's a .cc file.)

And you're right that sync_pb/std aren't nested within syncable.  You could
either explicitly qualify it (::std::string / ::sync_pb) or rely on the lookup
mechanism (which looks for syncer::std first, then ::std, etc.).

[rlarocque, 2012/07/11 23:45:36]

If that's the strongest argument in favor of it, then I would prefer to leave
them outside of the scope.  

If it were inside the scope, I think we might risk implicitly 'using
syncer::sync_pb::CommitReponse' by when the code reads (and we intend) 'using
sync_pb::CommitResponse'.

I realize that scenario will never happen.  My argument is that there are fewer
possible interpretations of 'using sync_pb::CommitResponse' when it's located
outside of a namespace rather than within, and that less ambiguity is a good
thing. 

+
 namespace syncer {
 
 using sessions::SyncSession;
 using std::string;
+using sync_pb::ClientToServerMessage;
 using syncable::BASE_VERSION;
 using syncable::Entry;
 using syncable::IS_DIR;
@@ -118,8 +122,8 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
       const string& name,
       syncer::ModelType model_type,
       sessions::OrderedCommitSet *commit_set,
-      syncer::ClientToServerMessage *commit,
-      syncer::ClientToServerResponse *response) {
+      sync_pb::ClientToServerMessage *commit,
+      sync_pb::ClientToServerResponse *response) {
     bool is_folder = true;
     int64 metahandle = 0;
     CreateUnsyncedItem(item_id, parent_id, name, is_folder, model_type,
@@ -136,14 +140,14 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
 
     // Add to the commit message.
     commit->set_message_contents(ClientToServerMessage::COMMIT);
-    SyncEntity* entity = static_cast<SyncEntity*>(
+    sync_pb::SyncEntity* entity = static_cast<sync_pb::SyncEntity*>(

[akalin, 2012/07/11 01:42:22]

static cast?

[rlarocque, 2012/07/11 19:22:16]

Done.

         commit->mutable_commit()->add_entries());
     entity->set_non_unique_name(name);
     entity->set_folder(is_folder);
-    entity->set_parent_id(parent_id);
+    entity->set_parent_id_string(SyncableIdToProto(parent_id));
     entity->set_version(entry.Get(syncable::BASE_VERSION));
     entity->mutable_specifics()->CopyFrom(entry.Get(syncable::SPECIFICS));
-    entity->set_id(item_id);
+    entity->set_id_string(SyncableIdToProto(item_id));
 
     // Add to the response message.
     response->set_error_code(sync_pb::SyncEnums::SUCCESS);
@@ -172,7 +176,7 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
     }
   }
 
-  void SetLastErrorCode(CommitResponse::ResponseType error_code,
+  void SetLastErrorCode(sync_pb::CommitResponse::ResponseType error_code,
                         sync_pb::ClientToServerResponse* response) {
     sync_pb::CommitResponse_EntryResponse* entry_response =
         response->mutable_commit()->mutable_entryresponse(
@@ -190,8 +194,8 @@ class ProcessCommitResponseCommandTest : public SyncerCommandTest {
 
 TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
   sessions::OrderedCommitSet commit_set(session()->routing_info());
-  syncer::ClientToServerMessage request;
-  syncer::ClientToServerResponse response;
+  sync_pb::ClientToServerMessage request;
+  sync_pb::ClientToServerResponse response;
 
   Id bookmark_folder_id = id_factory_.NewLocalId();
   Id bookmark_id1 = id_factory_.NewLocalId();
@@ -274,8 +278,8 @@ TEST_F(ProcessCommitResponseCommandTest, MultipleCommitIdProjections) {
 // of the children.
 TEST_F(ProcessCommitResponseCommandTest, NewFolderCommitKeepsChildOrder) {
   sessions::OrderedCommitSet commit_set(session()->routing_info());
-  syncer::ClientToServerMessage request;
-  syncer::ClientToServerResponse response;
+  sync_pb::ClientToServerMessage request;
+  sync_pb::ClientToServerResponse response;
 
   // Create the parent folder, a new item whose ID will change on commit.
   Id folder_id = id_factory_.NewLocalId();
@@ -402,8 +406,8 @@ INSTANTIATE_TEST_CASE_P(ProcessCommitResponse,
 // depending on the test parameter.
 TEST_P(MixedResult, ExtensionActivity) {
   sessions::OrderedCommitSet commit_set(session()->routing_info());
-  syncer::ClientToServerMessage request;
-  syncer::ClientToServerResponse response;
+  sync_pb::ClientToServerMessage request;
+  sync_pb::ClientToServerResponse response;
 
   EXPECT_NE(routing_info().find(syncer::BOOKMARKS)->second,
             routing_info().find(syncer::AUTOFILL)->second)