Remove syncproto.h

sync/engine/process_commit_response_command_unittest.cc

 // Copyright (c) 2012 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 "sync/engine/process_commit_response_command.h"
 
 #include <vector>
 
 #include "base/location.h"
 #include "base/stringprintf.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/sessions/sync_session.h"
 #include "sync/syncable/entry.h"
 #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;
 using syncable::IS_UNSYNCED;
 using syncable::Id;
 using syncable::MutableEntry;
 using syncable::NON_UNIQUE_NAME;
 using syncable::UNITTEST;
 using syncable::WriteTransaction;
 
@@ skipping 73 matching lines @@
   // Create a new unsynced item in the database, and synthesize a commit
   // record and a commit response for it in the syncer session.  If item_id
   // is a local ID, the item will be a create operation.  Otherwise, it
   // will be an edit.
   void CreateUnprocessedCommitResult(
       const Id& item_id,
       const Id& parent_id,
       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,
                        &metahandle);
 
     // ProcessCommitResponseCommand consumes commit_ids from the session
     // state, so we need to update that.  O(n^2) because it's a test.
     commit_set->AddCommitItem(metahandle, item_id, model_type);
 
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry(&trans, syncable::GET_BY_ID, item_id);
     ASSERT_TRUE(entry.good());
     entry.Put(syncable::SYNCING, true);
 
     // 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);
     sync_pb::CommitResponse_EntryResponse* entry_response =
         response->mutable_commit()->add_entryresponse();
     entry_response->set_response_type(CommitResponse::SUCCESS);
     entry_response->set_name("Garbage.");
     entry_response->set_non_unique_name(entity->name());
     if (item_id.ServerKnows())
       entry_response->set_id_string(entity->id_string());
     else
       entry_response->set_id_string(id_factory_.NewServerId().GetServerId());
     entry_response->set_version(next_new_revision_++);
     entry_response->set_position_in_parent(next_server_position_++);
 
     // If the ID of our parent item committed earlier in the batch was
     // rewritten, rewrite it in the entry response.  This matches
     // the server behavior.
     entry_response->set_parent_id_string(entity->parent_id_string());
     for (int i = 0; i < commit->commit().entries_size(); ++i) {
       if (commit->commit().entries(i).id_string() ==
           entity->parent_id_string()) {
         entry_response->set_parent_id_string(
             response->commit().entryresponse(i).id_string());
       }
     }
   }
 
-  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(
             response->mutable_commit()->entryresponse_size() - 1);
     entry_response->set_response_type(error_code);
   }
 
   TestIdFactory id_factory_;
  private:
   int64 next_old_revision_;
   int64 next_new_revision_;
   int64 next_server_position_;
   DISALLOW_COPY_AND_ASSIGN(ProcessCommitResponseCommandTest);
 };
 
 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();
   Id bookmark_id2 = id_factory_.NewLocalId();
   Id pref_id1 = id_factory_.NewLocalId(), pref_id2 = id_factory_.NewLocalId();
   Id autofill_id1 = id_factory_.NewLocalId();
   Id autofill_id2 = id_factory_.NewLocalId();
   CreateUnprocessedCommitResult(bookmark_folder_id, id_factory_.root(),
                                 "A bookmark folder", syncer::BOOKMARKS,
                                 &commit_set, &request, &response);
@@ skipping 62 matching lines @@
 // In particular, the folder has 50 children, which alternate between being
 // new items and preexisting items.  This mixture of new and old is meant to
 // be a torture test of the code in ProcessCommitResponseCommand that changes
 // an item's ID from a local ID to a server-generated ID on the first commit.
 // We commit only the first 25 children in the sibling order, leaving the
 // second 25 children as unsynced items.  http://crbug.com/33081 describes
 // how this scenario used to fail, reversing the order for the second half
 // 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();
   CreateUnprocessedCommitResult(folder_id, id_factory_.root(), "A",
                                 syncer::BOOKMARKS,
                                 &commit_set, &request, &response);
 
   // Verify that the item is reachable.
   {
     syncable::ReadTransaction trans(FROM_HERE, directory());
@@ skipping 106 matching lines @@
 };
 INSTANTIATE_TEST_CASE_P(ProcessCommitResponse,
                         MixedResult,
                         testing::Range(0, 1 << TEST_PARAM_BIT_COUNT));
 
 // This test commits 2 items (one bookmark, one autofill) and validates what
 // happens to the extensions activity records.  Commits could fail or succeed,
 // 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)
       << "To not be lame, this test requires more than one active group.";
 
   // Bookmark item setup.
   CreateUnprocessedCommitResult(id_factory_.NewServerId(),
       id_factory_.root(), "Some bookmark", syncer::BOOKMARKS,
       &commit_set, &request, &response);
   if (ShouldFailBookmarkCommit())
@@ skipping 28 matching lines @@
     EXPECT_EQ("xyz", final_monitor_records["xyz"].extension_id);
     EXPECT_EQ(2049U, final_monitor_records["ABC"].bookmark_write_count);
     EXPECT_EQ(4U,    final_monitor_records["xyz"].bookmark_write_count);
   } else {
     EXPECT_TRUE(final_monitor_records.empty())
         << "Should not restore records after successful bookmark commit.";
   }
 }
 
 }  // namespace syncer