Remove syncproto.h

Remove syncproto.h

Replace sync/engine/syncproto.h with sync/syncable/syncable_proto_util.h and
.cc.  The tasks that used to be performed by member functions of the syncer::
proto wrapper classes are now handled by static member functions.

Unfortunately, serialization and de-serialization of syncable::Id to/from proto
fields has gotten a bit uglier.  On the other hand, it's now much less magical
and mysterious.

The test intended to prevent regressions of crbug.com/134715 has been replaced
with a DCHECK.  We'll have to rely on it to ensure that the protocol_version
field is always explicitly set.

BUG=136454
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146393

[rlarocque, 2012-07-11 00:27:22]

Please review.

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

http://codereview.chromium.org/10735041/diff/1001/sync/engine/build_commit_co...
File sync/engine/build_commit_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/build_commit_co...
sync/engine/build_commit_command.cc:103: DCHECK(meta_entry->GetModelType() ==
GetModelType(*sync_entry));
DCHECK_EQ?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/build_commit_co...
sync/engine/build_commit_command.cc:129:
static_cast<sync_pb::SyncEntity*>(commit_message->add_entries());
no need for this static cast anymore, right?  may want to grep for other
instances

http://codereview.chromium.org/10735041/diff/1001/sync/engine/commit.cc
File sync/engine/commit.cc (left):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/commit.cc#oldco...
sync/engine/commit.cc:73:
commit_message->set_protocol_version(commit_message->protocol_version());
im guessing this is what replaces the call in the old syncproto.h?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/download_update...
File sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/download_update...
sync/engine/download_updates_command.cc:32:
client_to_server_message.set_protocol_version(
hmm multiple calls?  Maybe put this call into PostClientToServerMessage?  I
guess that'd mean having to pass a pointer or copy the protobuf.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command.cc:209: *static_cast<const
sync_pb::CommitResponse_EntryResponse*>(
static cast not needed anymore?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
File sync/engine/process_commit_response_command_unittest.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command_unittest.cc:25: using
sync_pb::CommitResponse;
move into namespace?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command_unittest.cc:143:
sync_pb::SyncEntity* entity = static_cast<sync_pb::SyncEntity*>(
static cast?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_updates...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_updates...
sync/engine/process_updates_command.cc:99: *static_cast<const
sync_pb::SyncEntity*>(&proto_update);
static cast

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:427: CHECK(local_entry.Get(ID) ==
SyncableIdFromProto(server_entry.id_string())) <<
CHECK_EQ?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_util.cc#...
sync/engine/syncer_util.cc:372: DCHECK(target->Get(ID) ==
SyncableIdFromProto(update.id_string()))
DCHECK_EQ?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_util.cc#...
sync/engine/syncer_util.cc:390: DCHECK(GetModelType(update) != UNSPECIFIED)
DCHECK_NE?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/verify_updates_...
File sync/engine/verify_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/verify_updates_...
sync/engine/verify_updates_command.cc:112: *reinterpret_cast<const
sync_pb::SyncEntity *>(&(updates.entries(i)));
reinterpret_cast? (urgggh)

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/model_type.cc...
sync/syncable/model_type.cc:150: static_cast<const
sync_pb::SyncEntity&>(sync_pb_entity);
static cast

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
File sync/syncable/syncable_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
sync/syncable/syncable_proto_util.cc:20: return ((entity.has_folder() &&
entity.folder()) ||
is has_folder() necessary?

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
sync/syncable/syncable_proto_util.cc:21: (entity.has_bookmarkdata() &&
is has_bookmarkdata() necessary?

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

PTAL.

I created a function, SyncerProtoUtil::SetProtocolVersion(), and used it to
replace all instances of msg.set_protocol_version(msg.protocol_version());

I also removed all the static and reinterpret casts I could find.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/build_commit_co...
File sync/engine/build_commit_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/build_commit_co...
sync/engine/build_commit_command.cc:129:
static_cast<sync_pb::SyncEntity*>(commit_message->add_entries());
On 2012/07/11 01:42:22, akalin wrote:
> no need for this static cast anymore, right?  may want to grep for other
> instances

Good point.  Will fix.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/commit.cc
File sync/engine/commit.cc (left):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/commit.cc#oldco...
sync/engine/commit.cc:73:
commit_message->set_protocol_version(commit_message->protocol_version());
On 2012/07/11 01:42:22, akalin wrote:
> im guessing this is what replaces the call in the old syncproto.h?

Yes.  We want to set the protocol version to the current default protocol
version.  We do it that way because it makes it easier to share the constant
between the client and server.

Come to think of it, though, I could rewrite this a bit more clearly.  Will fix.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/download_update...
File sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/download_update...
sync/engine/download_updates_command.cc:32:
client_to_server_message.set_protocol_version(
On 2012/07/11 01:42:22, akalin wrote:
> hmm multiple calls?  Maybe put this call into PostClientToServerMessage?  I
> guess that'd mean having to pass a pointer or copy the protobuf.

Done.  This has been converted to a function call and moved next to the
AddRequestBirthday() call, which performs a similar function: setting a required
field.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command.cc:209: *static_cast<const
sync_pb::CommitResponse_EntryResponse*>(
On 2012/07/11 01:42:22, akalin wrote:
> static cast not needed anymore?

Done.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
File sync/engine/process_commit_response_command_unittest.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command_unittest.cc:25: using
sync_pb::CommitResponse;
On 2012/07/11 01:42:22, akalin wrote:
> move into namespace?

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?

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command_unittest.cc:143:
sync_pb::SyncEntity* entity = static_cast<sync_pb::SyncEntity*>(
On 2012/07/11 01:42:22, akalin wrote:
> static cast?

Done.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_updates...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_updates...
sync/engine/process_updates_command.cc:99: *static_cast<const
sync_pb::SyncEntity*>(&proto_update);
On 2012/07/11 01:42:22, akalin wrote:
> static cast

Done.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_proto_ut...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/syncer_proto_ut...
sync/engine/syncer_proto_util.cc:427: CHECK(local_entry.Get(ID) ==
SyncableIdFromProto(server_entry.id_string())) <<
On 2012/07/11 01:42:22, akalin wrote:
> CHECK_EQ?

Done.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/verify_updates_...
File sync/engine/verify_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/verify_updates_...
sync/engine/verify_updates_command.cc:112: *reinterpret_cast<const
sync_pb::SyncEntity *>(&(updates.entries(i)));
On 2012/07/11 01:42:22, akalin wrote:
> reinterpret_cast? (urgggh)

Done.

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/model_type.cc...
sync/syncable/model_type.cc:150: static_cast<const
sync_pb::SyncEntity&>(sync_pb_entity);
On 2012/07/11 01:42:22, akalin wrote:
> static cast

Done.

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
File sync/syncable/syncable_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
sync/syncable/syncable_proto_util.cc:20: return ((entity.has_folder() &&
entity.folder()) ||
On 2012/07/11 01:42:22, akalin wrote:
> is has_folder() necessary?

Probably not.  I think has_foo() should be considered harmful because it makes
the protobuf's default value mechanism ineffective.  

However, this is how the old functions were written, and I'd rather not risk
changing it.

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

LGTM!

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
File sync/engine/process_commit_response_command_unittest.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command_unittest.cc:25: using
sync_pb::CommitResponse;
On 2012/07/11 19:22:16, rlarocque wrote:
> On 2012/07/11 01:42:22, akalin wrote:
> > move into namespace?
> 
> 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?

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.).

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
File sync/syncable/syncable_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
sync/syncable/syncable_proto_util.cc:20: return ((entity.has_folder() &&
entity.folder()) ||
On 2012/07/11 19:22:16, rlarocque wrote:
> On 2012/07/11 01:42:22, akalin wrote:
> > is has_folder() necessary?
> 
> Probably not.  I think has_foo() should be considered harmful because it makes
> the protobuf's default value mechanism ineffective.  
> 
> However, this is how the old functions were written, and I'd rather not risk
> changing it.

Good point.  Add TODO?

http://codereview.chromium.org/10735041/diff/11004/sync/engine/build_commit_c...
File sync/engine/build_commit_command.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/build_commit_c...
sync/engine/build_commit_command.cc:164: SyncableIdToProto(new_parent_id));
doesn't fit on prev line?

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_commit...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:251: syncable::Id server_entry_id
= SyncableIdFromProto(server_entry.id_string());
const &

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:322: syncable::Id
entry_response_id =
const &

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_update...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_update...
sync/engine/process_updates_command.cc:97: syncable::Id server_id =
SyncableIdFromProto(update.id_string());
const &

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_proto_u...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_proto_u...
sync/engine/syncer_proto_util.cc:167: int current_version =
const

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_proto_u...
sync/engine/syncer_proto_util.cc:178: DCHECK(msg.has_protocol_version());
maybe this is important enough to use a CHECK?

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_util.cc...
sync/engine/syncer_util.cc:81: syncable::Id update_id =
SyncableIdFromProto(update.id_string());
const & like client_id?

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_util.cc...
sync/engine/syncer_util.cc:602: syncable::Id update_id =
SyncableIdFromProto(update.id_string());
const &

http://codereview.chromium.org/10735041/diff/11004/sync/syncable/syncable_pro...
File sync/syncable/syncable_proto_util.h (right):

http://codereview.chromium.org/10735041/diff/11004/sync/syncable/syncable_pro...
sync/syncable/syncable_proto_util.h:18: syncable::Id SyncableIdFromProto(const
std::string& proto_string);
#include <string>

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

Addressed most comments.

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
File sync/engine/process_commit_response_command_unittest.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/engine/process_commit_...
sync/engine/process_commit_response_command_unittest.cc:25: using
sync_pb::CommitResponse;
On 2012/07/11 23:10:40, akalin wrote:
> On 2012/07/11 19:22:16, rlarocque wrote:
> > On 2012/07/11 01:42:22, akalin wrote:
> > > move into namespace?
> > 
> > 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?
> 
> 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.).

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.

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
File sync/syncable/syncable_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/1001/sync/syncable/syncable_prot...
sync/syncable/syncable_proto_util.cc:20: return ((entity.has_folder() &&
entity.folder()) ||
On 2012/07/11 23:10:40, akalin wrote:
> On 2012/07/11 19:22:16, rlarocque wrote:
> > On 2012/07/11 01:42:22, akalin wrote:
> > > is has_folder() necessary?
> > 
> > Probably not.  I think has_foo() should be considered harmful because it
makes
> > the protobuf's default value mechanism ineffective.  
> > 
> > However, this is how the old functions were written, and I'd rather not risk
> > changing it.
> 
> Good point.  Add TODO?

Done.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/build_commit_c...
File sync/engine/build_commit_command.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/build_commit_c...
sync/engine/build_commit_command.cc:164: SyncableIdToProto(new_parent_id));
On 2012/07/11 23:10:40, akalin wrote:
> doesn't fit on prev line?

Fixed.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_commit...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:251: syncable::Id server_entry_id
= SyncableIdFromProto(server_entry.id_string());
On 2012/07/11 23:10:40, akalin wrote:
> const &

Done.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:322: syncable::Id
entry_response_id =
On 2012/07/11 23:10:40, akalin wrote:
> const &

Done.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_update...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/process_update...
sync/engine/process_updates_command.cc:97: syncable::Id server_id =
SyncableIdFromProto(update.id_string());
On 2012/07/11 23:10:40, akalin wrote:
> const &

Done.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_proto_u...
File sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_proto_u...
sync/engine/syncer_proto_util.cc:178: DCHECK(msg.has_protocol_version());
On 2012/07/11 23:10:40, akalin wrote:
> maybe this is important enough to use a CHECK?

I'm always in favor of harsher CHECKs, but I'm not sure that doing so would be
consistent with our choices elsewhere.  In general, given a choice between sync
behaving badly and crashing the browser, we prefer to not crash.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_util.cc...
sync/engine/syncer_util.cc:81: syncable::Id update_id =
SyncableIdFromProto(update.id_string());
On 2012/07/11 23:10:40, akalin wrote:
> const & like client_id?

Done.

http://codereview.chromium.org/10735041/diff/11004/sync/engine/syncer_util.cc...
sync/engine/syncer_util.cc:602: syncable::Id update_id =
SyncableIdFromProto(update.id_string());
On 2012/07/11 23:10:40, akalin wrote:
> const &

Done.

http://codereview.chromium.org/10735041/diff/11004/sync/syncable/syncable_pro...
File sync/syncable/syncable_proto_util.h (right):

http://codereview.chromium.org/10735041/diff/11004/sync/syncable/syncable_pro...
sync/syncable/syncable_proto_util.h:18: syncable::Id SyncableIdFromProto(const
std::string& proto_string);
On 2012/07/11 23:10:40, akalin wrote:
> #include <string>

Done.

[commit-bot: I haz the power, 2012-07-12 17:14:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10735041/1011

[commit-bot: I haz the power, 2012-07-12 18:15:52]

Change committed as 146393