Client-side changes to support retry GU.

sync/engine/download.cc

 // Copyright 2013 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/download.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "sync/engine/process_updates_util.h"
@@ skipping 216 matching lines @@
       proto_request_types,
       update_handler_map,
       get_updates);
 
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(
       nudge_tracker.updates_source());
 
   // 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) {
     sync_pb::DataTypeProgressMarker* progress_marker =
         get_updates->mutable_from_progress_marker(i);
     ModelType type = GetModelTypeFromSpecificsFieldNumber(
         progress_marker->data_type_id());
 
     DCHECK(!nudge_tracker.IsTypeThrottled(type))
         << "Throttled types should have been removed from the request_types.";
@@ skipping 40 matching lines @@
 
   // Set legacy GetUpdatesMessage.GetUpdatesCallerInfo information.
   get_updates->mutable_caller_info()->set_source(source);
 
   // Set the new and improved version of source, too.
   sync_pb::SyncEnums::GetUpdatesOrigin origin =
       ConvertConfigureSourceToOrigin(source);
   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());
 
   InitDownloadUpdatesProgress(
       proto_request_types,
       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(
     ModelTypeSet request_types,
     SyncSession* session,
     sync_pb::ClientToServerMessage* msg) {
   sync_pb::ClientToServerResponse update_response;
   StatusController* status = session->mutable_status_controller();
   bool need_encryption_key = ShouldRequestEncryptionKey(session->context());
 
@@ skipping 75 matching lines @@
 void CopyClientDebugInfo(
     sessions::DebugInfoGetter* debug_info_getter,
     sync_pb::DebugInfo* debug_info) {
   DVLOG(1) << "Copying client debug info to send.";
   debug_info_getter->GetDebugInfo(debug_info);
 }
 
 }  // namespace download
 
 }  // namespace syncer