[Sync] Fill in the required fields before logging the protocol event

Before posting a client to server message, the sync cycle is informed
about an upcoming ProtocolEvent.
ProtocolEvent's are only consumed in
SyncInternalsMessageHandler::OnProtocolEvent()
using the ToValue() method. It's basically used to log the data to the
sync-internal pages.

All other occurrences of ProtoclEvent in the code are just to forward
that protocol event across different layers of sync.

One issue is the request object used to create the ProtoclEvent is
incomplete and some fields that as store_birthday are actually set
when posting the message to the server.

This CL is splitting the functionality in PostClientToServerMessage()
by creating another method to fill in the required fields. Such that,
those fields get filled before reporting the ProtocolEvent.

Bug: 925839
Change-Id: If6e09dd65acea99619f5ebe82a18492ef3c322ee
Reviewed-on: https://chromium-review.googlesource.com/c/1456076
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629976}

components/sync/engine_impl/clear_server_data.cc

@@ -29,6 +29,7 @@ SyncerError ClearServerData::SendRequest(SyncCycle* cycle) {
   }
 
   DVLOG(1) << "Sending ClearServerData message.";
+  SyncerProtoUtil::AddRequiredFieldsToClientToServerMessage(cycle, &request_);
 
   const ClearServerDataRequestEvent request_event(base::Time::Now(), request_);
   cycle->SendProtocolEvent(request_event);
@@ -37,7 +38,7 @@ SyncerError ClearServerData::SendRequest(SyncCycle* cycle) {
 
   TRACE_EVENT_BEGIN0("sync", "PostClearServerData");
   const SyncerError post_result = SyncerProtoUtil::PostClientToServerMessage(
-      &request_, &response, cycle, nullptr);
+      request_, &response, cycle, nullptr);
   TRACE_EVENT_END0("sync", "PostClearServerData");
 
   const ClearServerDataResponseEvent response_event(base::Time::Now(),

components/sync/engine_impl/commit.cc

@@ -122,6 +122,7 @@ SyncerError Commit::PostAndProcessResponse(
   }
 
   DVLOG(1) << "Sending commit message.";
+  SyncerProtoUtil::AddRequiredFieldsToClientToServerMessage(cycle, &message_);
 
   CommitRequestEvent request_event(base::Time::Now(),
                                    message_.commit().entries_size(),
@@ -131,7 +132,7 @@ SyncerError Commit::PostAndProcessResponse(
   TRACE_EVENT_BEGIN0("sync", "PostCommit");
   sync_pb::ClientToServerResponse response;
   const SyncerError post_result = SyncerProtoUtil::PostClientToServerMessage(
-      &message_, &response, cycle, nullptr);
+      message_, &response, cycle, nullptr);
   TRACE_EVENT_END0("sync", "PostCommit");
 
   // TODO(rlarocque): Use result that includes errors captured later?

components/sync/engine_impl/get_updates_processor.cc

@@ -215,13 +215,15 @@ SyncerError GetUpdatesProcessor::ExecuteDownloadUpdates(
     CopyClientDebugInfo(cycle->context()->debug_info_getter(), debug_info);
   }
 
+  SyncerProtoUtil::AddRequiredFieldsToClientToServerMessage(cycle, msg);
+
   cycle->SendProtocolEvent(
       *(delegate_.GetNetworkRequestEvent(base::Time::Now(), *msg)));
 
   ModelTypeSet partial_failure_data_types;
 
   SyncerError result = SyncerProtoUtil::PostClientToServerMessage(
-      msg, &update_response, cycle, &partial_failure_data_types);
+      *msg, &update_response, cycle, &partial_failure_data_types);
 
   DVLOG(2) << SyncerProtoUtil::ClientToServerResponseDebugString(
       update_response);

components/sync/engine_impl/syncer_proto_util.cc

@@ -430,15 +430,10 @@ base::TimeDelta SyncerProtoUtil::GetThrottleDelay(
 }
 
 // static
-SyncerError SyncerProtoUtil::PostClientToServerMessage(
-    ClientToServerMessage* msg,
-    ClientToServerResponse* response,
-    SyncCycle* cycle,
-    ModelTypeSet* partial_failure_data_types) {
-  DCHECK(response);
-  DCHECK(!msg->get_updates().has_from_timestamp());   // Deprecated.
-
-  // Add must-have fields.
+void SyncerProtoUtil::AddRequiredFieldsToClientToServerMessage(
+    const SyncCycle* cycle,
+    sync_pb::ClientToServerMessage* msg) {
+  DCHECK(msg);
   SetProtocolVersion(msg);
   AddRequestBirthday(cycle->context()->directory(), msg);
   DCHECK(msg->has_store_birthday() || !IsBirthdayRequired(*msg));
@@ -446,12 +441,26 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
   msg->set_api_key(google_apis::GetAPIKey());
   msg->mutable_client_status()->CopyFrom(cycle->context()->client_status());
   msg->set_invalidator_client_id(cycle->context()->invalidator_client_id());
+}
 
-  syncable::Directory* dir = cycle->context()->directory();
-
-  LogClientToServerMessage(*msg);
-  if (!PostAndProcessHeaders(cycle->context()->connection_manager(), cycle,
-                             *msg, response)) {
+// static
+SyncerError SyncerProtoUtil::PostClientToServerMessage(
+    const ClientToServerMessage& msg,
+    ClientToServerResponse* response,
+    SyncCycle* cycle,
+    ModelTypeSet* partial_failure_data_types) {
+  DCHECK(response);
+  DCHECK(msg.has_protocol_version());
+  DCHECK(msg.has_store_birthday() || !IsBirthdayRequired(msg));
+  DCHECK(msg.has_bag_of_chips());
+  DCHECK(msg.has_api_key());
+  DCHECK(msg.has_client_status());
+  DCHECK(msg.has_invalidator_client_id());
+  DCHECK(!msg.get_updates().has_from_timestamp());  // Deprecated.
+
+  LogClientToServerMessage(msg);
+  if (!PostAndProcessHeaders(cycle->context()->connection_manager(), cycle, msg,
+                             response)) {
     // There was an error establishing communication with the server.
     // We can not proceed beyond this point.
     const HttpResponse::ServerConnectionCode server_status =
@@ -466,6 +475,7 @@ SyncerError SyncerProtoUtil::PostClientToServerMessage(
   }
   LogClientToServerResponse(*response);
 
+  syncable::Directory* dir = cycle->context()->directory();
   // Persist a bag of chips if it has been sent by the server.
   PersistBagOfChips(dir, *response);
 

components/sync/engine_impl/syncer_proto_util.h

@@ -41,13 +41,19 @@ SyncProtocolError ConvertErrorPBToSyncProtocolError(
 
 class SyncerProtoUtil {
  public:
+  // Adds all fields that must be sent on every request, which includes store
+  // birthday, protocol version, client chips, api keys, etc. |msg| must be not
+  // null. Must be called before calling PostClientToServerMessage().
+  static void AddRequiredFieldsToClientToServerMessage(
+      const SyncCycle* cycle,
+      sync_pb::ClientToServerMessage* msg);
+
   // Posts the given message and fills the buffer with the returned value.
   // Returns true on success.  Also handles store birthday verification: will
-  // produce a SyncError if the birthday is incorrect.
-  // NOTE: This will add all fields that must be sent on every request, which
-  // includes store birthday, protocol version, client chips, api keys, etc.
+  // produce a SyncError if the birthday is incorrect. Before calling this
+  // method, AddRequiredFieldsToClientToServerMessage() must be called.
   static SyncerError PostClientToServerMessage(
-      sync_pb::ClientToServerMessage* msg,
+      const sync_pb::ClientToServerMessage& msg,
       sync_pb::ClientToServerResponse* response,
       SyncCycle* cycle,
       ModelTypeSet* partial_failure_data_types);