[Sync] A few random cleanups

- Tightening access to ServerConnectionManager members
- Removing redindant functions in url_translator
- raw pointer => unique_ptr in commit.h
- Iterators => range-based loop in ProcessDownloadedUpdates

BUG=
R=pnoland@chromium.org

Change-Id: I11775719ee08d901bc2568e8298af396f417b751
Reviewed-on: https://chromium-review.googlesource.com/565650Reviewed-by: Patrick Noland <pnoland@chromium.org>
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485402}

components/sync/engine_impl/commit.cc

@@ -50,15 +50,16 @@ Commit::~Commit() {
   DCHECK(cleaned_up_);
 }
 
-Commit* Commit::Init(ModelTypeSet requested_types,
-                     ModelTypeSet enabled_types,
-                     size_t max_entries,
-                     const std::string& account_name,
-                     const std::string& cache_guid,
-                     bool cookie_jar_mismatch,
-                     bool cookie_jar_empty,
-                     CommitProcessor* commit_processor,
-                     ExtensionsActivity* extensions_activity) {
+// static
+std::unique_ptr<Commit> Commit::Init(ModelTypeSet requested_types,
+                                     ModelTypeSet enabled_types,
+                                     size_t max_entries,
+                                     const std::string& account_name,
+                                     const std::string& cache_guid,
+                                     bool cookie_jar_mismatch,
+                                     bool cookie_jar_empty,
+                                     CommitProcessor* commit_processor,
+                                     ExtensionsActivity* extensions_activity) {
   // Gather per-type contributions.
   ContributionMap contributions;
   commit_processor->GatherCommitContributions(requested_types, max_entries,
@@ -109,8 +110,8 @@ Commit* Commit::Init(ModelTypeSet requested_types,
   }
 
   // If we made it this far, then we've successfully prepared a commit message.
-  return new Commit(std::move(contributions), message,
-                    extensions_activity_buffer);
+  return base::MakeUnique<Commit>(std::move(contributions), message,
+                                  extensions_activity_buffer);
 }
 
 SyncerError Commit::PostAndProcessResponse(

components/sync/engine_impl/commit.h

@@ -47,15 +47,15 @@ class Commit {
   ~Commit();
 
   // |extensions_activity| may be null.
-  static Commit* Init(ModelTypeSet requested_types,
-                      ModelTypeSet enabled_types,
-                      size_t max_entries,
-                      const std::string& account_name,
-                      const std::string& cache_guid,
-                      bool cookie_jar_mismatch,
-                      bool cookie_jar_empty,
-                      CommitProcessor* commit_processor,
-                      ExtensionsActivity* extensions_activity);
+  static std::unique_ptr<Commit> Init(ModelTypeSet requested_types,
+                                      ModelTypeSet enabled_types,
+                                      size_t max_entries,
+                                      const std::string& account_name,
+                                      const std::string& cache_guid,
+                                      bool cookie_jar_mismatch,
+                                      bool cookie_jar_empty,
+                                      CommitProcessor* commit_processor,
+                                      ExtensionsActivity* extensions_activity);
 
   // |extensions_activity| may be null.
   SyncerError PostAndProcessResponse(NudgeTracker* nudge_tracker,

components/sync/engine_impl/net/server_connection_manager.cc

@@ -222,7 +222,7 @@ void ServerConnectionManager::InvalidateAndClearAuthToken() {
   // Copy over the token to previous invalid token.
   if (!auth_token_.empty()) {
     previously_invalidated_token.assign(auth_token_);
-    auth_token_ = std::string();
+    auth_token_.clear();
   }
 }
 

components/sync/engine_impl/net/server_connection_manager.h

@@ -228,6 +228,27 @@ class ServerConnectionManager : public CancelationObserver {
   // ServerConnectionManager to cleanup active connections.
   void OnConnectionDestroyed(Connection* connection);
 
+ private:
+  // A class to help deal with cleaning up active Connection objects when (for
+  // ex) multiple early-exits are present in some scope. ScopedConnectionHelper
+  // informs the ServerConnectionManager before the Connection object it takes
+  // ownership of is destroyed.
+  class ScopedConnectionHelper {
+   public:
+    // |manager| must outlive this. Takes ownership of |connection|.
+    ScopedConnectionHelper(ServerConnectionManager* manager,
+                           Connection* connection);
+    ~ScopedConnectionHelper();
+    Connection* get();
+
+   private:
+    ServerConnectionManager* manager_;
+    std::unique_ptr<Connection> connection_;
+    DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper);
+  };
+
+  void NotifyStatusChanged();
+
   // The sync_server_ is the server that requests will be made to.
   std::string sync_server_;
 
@@ -266,27 +287,6 @@ class ServerConnectionManager : public CancelationObserver {
   // it if necessary.
   Connection* active_connection_;
 
- private:
-  // A class to help deal with cleaning up active Connection objects when (for
-  // ex) multiple early-exits are present in some scope. ScopedConnectionHelper
-  // informs the ServerConnectionManager before the Connection object it takes
-  // ownership of is destroyed.
-  class ScopedConnectionHelper {
-   public:
-    // |manager| must outlive this. Takes ownership of |connection|.
-    ScopedConnectionHelper(ServerConnectionManager* manager,
-                           Connection* connection);
-    ~ScopedConnectionHelper();
-    Connection* get();
-
-   private:
-    ServerConnectionManager* manager_;
-    std::unique_ptr<Connection> connection_;
-    DISALLOW_COPY_AND_ASSIGN(ScopedConnectionHelper);
-  };
-
-  void NotifyStatusChanged();
-
   CancelationSignal* const cancelation_signal_;
   bool signal_handler_registered_;
 

components/sync/engine_impl/net/url_translator.cc

@@ -22,15 +22,6 @@ const char kClientName[] = "Chromium";
 #endif  // defined(GOOGLE_CHROME_BUILD)
 }  // namespace
 
-// Convenience wrappers around CgiEscapePath().
-string CgiEscapeString(const char* src) {
-  return CgiEscapeString(string(src));
-}
-
-string CgiEscapeString(const string& src) {
-  return net::EscapeUrlEncodedData(src, true);
-}
-
 // This method appends the query string to the sync server path.
 string MakeSyncServerPath(const string& path, const string& query_string) {
   string result = path;
@@ -42,10 +33,10 @@ string MakeSyncServerPath(const string& path, const string& query_string) {
 string MakeSyncQueryString(const string& client_id) {
   string query;
   query += kParameterClient;
-  query += "=" + CgiEscapeString(kClientName);
+  query += "=" + net::EscapeUrlEncodedData(kClientName, true);
   query += "&";
   query += kParameterClientID;
-  query += "=" + CgiEscapeString(client_id);
+  query += "=" + net::EscapeUrlEncodedData(client_id, true);
   return query;
 }
 

components/sync/engine_impl/net/url_translator.h

@@ -12,10 +12,6 @@ namespace syncer {
 // Contains the declaration of a few helper functions used for generating sync
 // URLs.
 
-// Convenience wrappers around CgiEscapePath(), used by gaia_auth.
-std::string CgiEscapeString(const char* src);
-std::string CgiEscapeString(const std::string& src);
-
 // This method appends the query string to the sync server path.
 std::string MakeSyncServerPath(const std::string& path,
                                const std::string& query_string);

components/sync/engine_impl/process_updates_util.cc

@@ -281,25 +281,23 @@ void ProcessDownloadedUpdates(syncable::Directory* dir,
                               const SyncEntityList& applicable_updates,
                               StatusController* status,
                               UpdateCounters* counters) {
-  for (SyncEntityList::const_iterator update_it = applicable_updates.begin();
-       update_it != applicable_updates.end(); ++update_it) {
-    DCHECK_EQ(type, GetModelType(**update_it));
-    if (!UpdateContainsNewVersion(trans, **update_it)) {
+  for (const auto* update : applicable_updates) {
+    DCHECK_EQ(type, GetModelType(*update));
+    if (!UpdateContainsNewVersion(trans, *update)) {
       status->increment_num_reflected_updates_downloaded_by(1);
       counters->num_reflected_updates_received++;
     }
-    if ((*update_it)->deleted()) {
+    if (update->deleted()) {
       status->increment_num_tombstone_updates_downloaded_by(1);
       counters->num_tombstone_updates_received++;
     }
-    VerifyResult verify_result = VerifyUpdate(trans, **update_it, type);
+    VerifyResult verify_result = VerifyUpdate(trans, *update, type);
     if (verify_result != VERIFY_SUCCESS && verify_result != VERIFY_UNDELETE)
       continue;
-    ProcessUpdate(**update_it, dir->GetCryptographer(trans), trans);
-    if ((*update_it)->ByteSize() > 0) {
+    ProcessUpdate(*update, dir->GetCryptographer(trans), trans);
+    if (update->ByteSize() > 0) {
       SyncRecordDatatypeBin("DataUse.Sync.Download.Bytes",
-                            ModelTypeToHistogramInt(type),
-                            (*update_it)->ByteSize());
+                            ModelTypeToHistogramInt(type), update->ByteSize());
     }
     UMA_HISTOGRAM_SPARSE_SLOWLY("DataUse.Sync.Download.Count",
                                 ModelTypeToHistogramInt(type));

components/sync/syncable/base_node.h

@@ -39,7 +39,6 @@ namespace syncer {
 class BaseTransaction;
 
 namespace syncable {
-class BaseTransaction;
 class Entry;
 class Id;
 }

components/sync/test/engine/mock_connection_manager.cc

@@ -763,15 +763,10 @@ void MockConnectionManager::SetServerNotReachable() {
 
 void MockConnectionManager::UpdateConnectionStatus() {
   if (!server_reachable_) {
-    server_status_ = HttpResponse::CONNECTION_UNAVAILABLE;
+    SetServerStatus(HttpResponse::CONNECTION_UNAVAILABLE);
   } else {
-    server_status_ = HttpResponse::SERVER_CONNECTION_OK;
+    SetServerStatus(HttpResponse::SERVER_CONNECTION_OK);
   }
 }
 
-void MockConnectionManager::SetServerStatus(
-    HttpResponse::ServerConnectionCode server_status) {
-  server_status_ = server_status;
-}
-
 }  // namespace syncer

components/sync/test/engine/mock_connection_manager.h

@@ -255,7 +255,7 @@ class MockConnectionManager : public ServerConnectionManager {
   // requests.
   void UpdateConnectionStatus();
 
-  void SetServerStatus(HttpResponse::ServerConnectionCode server_status);
+  using ServerConnectionManager::SetServerStatus;
 
   // Return by copy to be thread-safe.
   const std::string store_birthday() {
@@ -272,7 +272,7 @@ class MockConnectionManager : public ServerConnectionManager {
   // Adds a new progress marker to the last update.
   sync_pb::DataTypeProgressMarker* AddUpdateProgressMarker();
 
-  void ResetAuthToken() { auth_token_.clear(); }
+  void ResetAuthToken() { InvalidateAndClearAuthToken(); }
 
  private:
   sync_pb::SyncEntity* AddUpdateFull(syncable::Id id,