Client-side changes to support retry GU.

sync/engine/download.cc

Index: sync/engine/download.cc
diff --git a/sync/engine/download.cc b/sync/engine/download.cc
index 2bc7f7a0a9d35be4e85e68ce60420826057f5458..4712857c8956bd2ee475b777f5877f69bd735f85 100644
--- a/sync/engine/download.cc
+++ b/sync/engine/download.cc
@@ -234,6 +234,7 @@ void BuildNormalDownloadUpdatesImpl(
 
   // Set the new and improved version of source, too.
   get_updates->set_get_updates_origin(sync_pb::SyncEnums::GU_TRIGGER);
+  get_updates->set_is_retry(nudge_tracker.IsRetryRequired());

[rlarocque, 2014/01/06 23:00:33]

This is good, but I think this flag should also be set in the other case (where
origin == RETRY).

[haitaol1, 2014/01/07 19:03:37]

origin == RETRY already indicates it's a retry GU, so seems redundant to set the
flag again.

On 2014/01/06 23:00:33, rlarocque wrote:

[rlarocque, 2014/01/07 19:43:41]

This would make the logic to detect retry GUs harder on the server.  With the
current code, if the server wanted to know if a particular GU was a retry GU it
would have to check both the flag and the origin.  

I'd prefer to have the flag always set so that when this trickles down into the
logs, we could run queries that of the form (WHERE gu_msg.is_retry), rather than
(WHERE gu_msg.is_rety || gu_msg.origin == RETRY).  It seems unreasonable to
expect people querying the logs to know they have to check one or the other. 
It's likely I'll have forgotten about this detail in a month or two.

(Yes, it would be possible to process these requests on the server side before
we write the logs to set the flag as appropriate, but I'd rather avoid it.  The
logging code is more likely to be correct if all it has to do is copy fields
directly from the request without doing any post-processing on them.)

[haitaol1, 2014/01/08 19:06:38]

Done.

 
   // Fill in the notification hints.
   for (int i = 0; i < get_updates->from_progress_marker_size(); ++i) {
@@ -294,26 +295,32 @@ void BuildDownloadUpdatesForConfigureImpl(
   get_updates->set_get_updates_origin(origin);
 }
 
-void BuildDownloadUpdatesForPoll(
+void BuildSupplementDownloadUpdates(
     SyncSession* session,
     bool create_mobile_bookmarks_folder,
     ModelTypeSet request_types,
+    sync_pb::SyncEnums::GetUpdatesOrigin gu_origin,
     sync_pb::ClientToServerMessage* client_to_server_message) {
-  DVLOG(1) << "Polling for types "
+  DVLOG(1) << "Polling/Retrying for types "
            << ModelTypeSetToString(request_types);
 
+  DCHECK(gu_origin == sync_pb::SyncEnums::PERIODIC ||
+         gu_origin == sync_pb::SyncEnums::RETRY);
+
   InitDownloadUpdatesContext(
       session,
       create_mobile_bookmarks_folder,
       client_to_server_message);
-  BuildDownloadUpdatesForPollImpl(
+  BuildSupplementDownloadUpdatesImpl(
       Intersection(request_types, ProtocolTypes()),
+      gu_origin,
       session->context()->update_handler_map(),
       client_to_server_message->mutable_get_updates());
 }
 
-void BuildDownloadUpdatesForPollImpl(
+void BuildSupplementDownloadUpdatesImpl(

[rlarocque, 2014/01/06 23:00:33]

I realize this is a bit subjective, but I think this function should be split
into two.  The function only does two things, and the second of those two
depends on the value of a flag that you have to pass in explicitly.  If 50% of
the method depends on the value of a flag, I think it makes more sense to have
two different methods.

I think a similar comment could be made about the function on line 298, too. 
You could write that method better if it was split in two.  eg. DCHECK the
specific gu_origin rather than checking them both; logging the exact origin
rather than logging that it's one or the other.

That's a nit-level complaint, though.  You can leave it as is if you feel
strongly about it.

[haitaol1, 2014/01/07 19:03:37]

Done.

     ModelTypeSet proto_request_types,
+    sync_pb::SyncEnums::GetUpdatesOrigin gu_origin,
     UpdateHandlerMap* update_handler_map,
     sync_pb::GetUpdatesMessage* get_updates) {
   DCHECK(!proto_request_types.Empty());
@@ -323,12 +330,16 @@ void BuildDownloadUpdatesForPollImpl(
       update_handler_map,
       get_updates);
 
-  // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
-  get_updates->mutable_caller_info()->set_source(
-      sync_pb::GetUpdatesCallerInfo::PERIODIC);
-
-  // Set the new and improved version of source, too.
-  get_updates->set_get_updates_origin(sync_pb::SyncEnums::PERIODIC);
+  // Set the new and improved version of source as well as the legacy
+  // GetUpdatesMessage.GetUpdatesCallerInfo information.
+  get_updates->set_get_updates_origin(gu_origin);
+  if (gu_origin == sync_pb::SyncEnums::PERIODIC) {
+    get_updates->mutable_caller_info()->set_source(
+        sync_pb::GetUpdatesCallerInfo::PERIODIC);
+  } else {
+    get_updates->mutable_caller_info()->set_source(
+        sync_pb::GetUpdatesCallerInfo::RETRY);
+  }
 }
 
 SyncerError ExecuteDownloadUpdates(