Remove DataTypeDebugInfoEmitter

This is class is unused following crrev.com/c/2470877. This CL removes,
together with the Commit*::CleanUp() methods which are now unnecessary.

Fixed: 1137896
Change-Id: I75a7e4b64da970cc8f84966c57d1225693737103
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470528Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#817576}

components/sync/BUILD.gn

@@ -101,8 +101,6 @@ static_library("rest_of_sync") {
     "engine_impl/commit_processor.h",
     "engine_impl/commit_util.cc",
     "engine_impl/commit_util.h",
-    "engine_impl/cycle/data_type_debug_info_emitter.cc",
-    "engine_impl/cycle/data_type_debug_info_emitter.h",
     "engine_impl/cycle/data_type_tracker.cc",
     "engine_impl/cycle/data_type_tracker.h",
     "engine_impl/cycle/debug_info_getter.h",

components/sync/engine_impl/commit.cc

@@ -80,12 +80,9 @@ Commit::Commit(ContributionMap contributions,
                ExtensionsActivity::Records extensions_activity_buffer)
     : contributions_(std::move(contributions)),
       message_(message),
-      extensions_activity_buffer_(extensions_activity_buffer),
-      cleaned_up_(false) {}
+      extensions_activity_buffer_(extensions_activity_buffer) {}
 
-Commit::~Commit() {
-  DCHECK(cleaned_up_);
-}
+Commit::~Commit() = default;
 
 // static
 std::unique_ptr<Commit> Commit::Init(ModelTypeSet enabled_types,
@@ -243,14 +240,6 @@ ModelTypeSet Commit::GetContributingDataTypes() const {
   return contributed_data_types;
 }
 
-void Commit::CleanUp() {
-  for (ContributionMap::const_iterator it = contributions_.begin();
-       it != contributions_.end(); ++it) {
-    it->second->CleanUp();
-  }
-  cleaned_up_ = true;
-}
-
 void Commit::ReportFullCommitFailure(SyncerError syncer_error) {
   const SyncCommitError commit_error = GetSyncCommitError(syncer_error);
   for (auto& model_type_and_contribution : contributions_) {

components/sync/engine_impl/commit.h

@@ -42,7 +42,6 @@ class Commit {
          const sync_pb::ClientToServerMessage& message,
          ExtensionsActivity::Records extensions_activity_buffer);
 
-  // This destructor will DCHECK if CleanUp() has not been called.
   ~Commit();
 
   // |extensions_activity| may be null.
@@ -63,10 +62,6 @@ class Commit {
 
   ModelTypeSet GetContributingDataTypes() const;
 
-  // Cleans up state associated with this commit.  Must be called before the
-  // destructor.
-  void CleanUp();
-
  private:
   // Report commit failure to each contribution.
   void ReportFullCommitFailure(SyncerError syncer_error);
@@ -76,9 +71,6 @@ class Commit {
   sync_pb::ClientToServerMessage message_;
   ExtensionsActivity::Records extensions_activity_buffer_;
 
-  // Debug only flag used to indicate if it's safe to destruct the object.
-  bool cleaned_up_;
-
   DISALLOW_COPY_AND_ASSIGN(Commit);
 };
 

components/sync/engine_impl/commit_contribution.h

@@ -47,10 +47,6 @@ class CommitContribution {
   // was never called.
   virtual void ProcessCommitFailure(SyncCommitError commit_error) = 0;
 
-  // Cleans up any temporary state associated with the commit. Must be called
-  // before destruction.
-  virtual void CleanUp() = 0;
-
   // Returns the number of entries included in this contribution.
   virtual size_t GetNumEntries() const = 0;
 };

components/sync/engine_impl/commit_contribution_impl.cc

@@ -37,12 +37,9 @@ CommitContributionImpl::CommitContributionImpl(
       passphrase_type_(passphrase_type),
       context_(context),
       commit_requests_(std::move(commit_requests)),
-      cleaned_up_(false),
       only_commit_specifics_(only_commit_specifics) {}
 
-CommitContributionImpl::~CommitContributionImpl() {
-  DCHECK(cleaned_up_);
-}
+CommitContributionImpl::~CommitContributionImpl() = default;
 
 void CommitContributionImpl::AddToCommitMessage(
     sync_pb::ClientToServerMessage* msg) {
@@ -186,11 +183,6 @@ void CommitContributionImpl::ProcessCommitFailure(
   on_commit_response_callback_.Reset();
 }
 
-void CommitContributionImpl::CleanUp() {
-  // TODO(crbug.com/1137896): Remove this method, is has no use anymore.
-  cleaned_up_ = true;
-}
-
 size_t CommitContributionImpl::GetNumEntries() const {
   return commit_requests_.size();
 }

components/sync/engine_impl/commit_contribution_impl.h

@@ -51,7 +51,6 @@ class CommitContributionImpl : public CommitContribution {
       const sync_pb::ClientToServerResponse& response,
       StatusController* status) override;
   void ProcessCommitFailure(SyncCommitError commit_error) override;
-  void CleanUp() override;
   size_t GetNumEntries() const override;
 
   // Public for testing.
@@ -93,10 +92,6 @@ class CommitContributionImpl : public CommitContribution {
   // added.  Used to correlate per-item requests with per-item responses.
   size_t entries_start_index_;
 
-  // A flag used to ensure this object's contract is respected.  Helps to check
-  // that CleanUp() is called before the object is destructed.
-  bool cleaned_up_;
-
   // Don't send any metadata to server, only specifics. This is needed for
   // commit only types to save bandwidth.
   bool only_commit_specifics_;

components/sync/engine_impl/commit_contribution_impl_unittest.cc

@@ -179,7 +179,6 @@ TEST(CommitContributionImplTest,
 
   sync_pb::ClientToServerMessage msg;
   contribution.AddToCommitMessage(&msg);
-  contribution.CleanUp();
 
   ASSERT_EQ(1, msg.commit().entries().size());
   SyncEntity entity = msg.commit().entries(0);
@@ -242,7 +241,6 @@ TEST(CommitContributionImplTest,
 
   sync_pb::ClientToServerMessage msg;
   contribution.AddToCommitMessage(&msg);
-  contribution.CleanUp();
 
   ASSERT_EQ(1, msg.commit().entries().size());
   SyncEntity entity = msg.commit().entries(0);
@@ -311,7 +309,6 @@ TEST(CommitContributionImplTest, ShouldPropagateFailedItemsOnCommitResponse) {
 
   StatusController status;
   contribution.ProcessCommitResponse(response, &status);
-  contribution.CleanUp();
 
   ASSERT_EQ(1u, actual_error_response_list.size());
   FailedCommitResponseData failed_item = actual_error_response_list[0];
@@ -335,7 +332,6 @@ TEST(CommitContributionImplTest, ShouldPropagateFullCommitFailure) {
       /*only_commit_specifics=*/false);
 
   contribution.ProcessCommitFailure(SyncCommitError::kNetworkError);
-  contribution.CleanUp();
 }
 
 }  // namespace

components/sync/engine_impl/commit_processor_unittest.cc

@@ -42,7 +42,6 @@ class FakeCommitContribution : public CommitContribution {
     return SyncerError();
   }
   void ProcessCommitFailure(SyncCommitError commit_error) override {}
-  void CleanUp() override {}
   size_t GetNumEntries() const override { return num_entries_; }
 
  private:

components/sync/engine_impl/cycle/data_type_debug_info_emitter.cc

-// Copyright 2016 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 "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
-
-#include <string>
-
-#include "base/metrics/histogram.h"
-
-namespace syncer {
-
-DataTypeDebugInfoEmitter::DataTypeDebugInfoEmitter(ModelType type) {}
-
-DataTypeDebugInfoEmitter::~DataTypeDebugInfoEmitter() {}
-
-const CommitCounters& DataTypeDebugInfoEmitter::GetCommitCounters() const {
-  return commit_counters_;
-}
-
-CommitCounters* DataTypeDebugInfoEmitter::GetMutableCommitCounters() {
-  return &commit_counters_;
-}
-
-void DataTypeDebugInfoEmitter::EmitCommitCountersUpdate() {
-  // Mark the current state of the counters as uploaded to UMA.
-  emitted_commit_counters_ = commit_counters_;
-}
-
-const UpdateCounters& DataTypeDebugInfoEmitter::GetUpdateCounters() const {
-  return update_counters_;
-}
-
-UpdateCounters* DataTypeDebugInfoEmitter::GetMutableUpdateCounters() {
-  return &update_counters_;
-}
-
-void DataTypeDebugInfoEmitter::EmitUpdateCountersUpdate() {
-  // Mark the current state of the counters as uploaded to UMA.
-  emitted_update_counters_ = update_counters_;
-}
-
-}  // namespace syncer

components/sync/engine_impl/cycle/data_type_debug_info_emitter.h

-// Copyright 2016 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.
-
-#ifndef COMPONENTS_SYNC_ENGINE_IMPL_CYCLE_DATA_TYPE_DEBUG_INFO_EMITTER_H_
-#define COMPONENTS_SYNC_ENGINE_IMPL_CYCLE_DATA_TYPE_DEBUG_INFO_EMITTER_H_
-
-#include <memory>
-
-#include "base/macros.h"
-#include "components/sync/base/model_type.h"
-
-namespace syncer {
-
-// A class to maintain counts related to sync commit requests and responses.
-struct CommitCounters {
-  CommitCounters() = default;
-  ~CommitCounters() = default;
-
-  // Counters updated before sending a commit message to the server.
-  int num_creation_commits_attempted = 0;
-  int num_deletion_commits_attempted = 0;
-  int num_update_commits_attempted = 0;
-};
-
-// A class to maintain counts related to the update requests and responses for
-// a particular sync type.
-struct UpdateCounters {
-  UpdateCounters() = default;
-  ~UpdateCounters() = default;
-
-  int num_initial_updates_received = 0;
-  int num_non_initial_updates_received = 0;
-  int num_non_initial_tombstone_updates_received = 0;
-};
-
-// Supports various kinds of debugging requests for a certain directory type.
-//
-// TODO(crbug.com/1102849): Rename Emit*() methods to mention UMA, and update
-// the documentation to not mention any observers.
-// The Emit*() functions send updates to registered TypeDebugInfoObservers.
-// The DataTypeDebugInfoEmitter does not directly own that list; it is
-// managed by the ModelTypeRegistry.
-//
-// For Update and Commit counters, the job of keeping the counters up to date
-// is delegated to the UpdateHandler and CommitContributors. For the Stats
-// counters, the emitter will let sub class to fetch all the required
-// information on demand.
-// TODO(crbug.com/1137896): This class is unused, remove it.
-class DataTypeDebugInfoEmitter {
- public:
-  explicit DataTypeDebugInfoEmitter(ModelType type);
-
-  virtual ~DataTypeDebugInfoEmitter();
-
-  // Returns a reference to the current commit counters.
-  const CommitCounters& GetCommitCounters() const;
-
-  // Allows others to mutate the commit counters.
-  CommitCounters* GetMutableCommitCounters();
-
-  // Triggers a commit counters update to registered observers.
-  void EmitCommitCountersUpdate();
-
-  // Returns a reference to the current update counters.
-  const UpdateCounters& GetUpdateCounters() const;
-
-  // Allows others to mutate the update counters.
-  UpdateCounters* GetMutableUpdateCounters();
-
-  // Triggers an update counters update to registered observers.
-  void EmitUpdateCountersUpdate();
-
- private:
-  // The actual up-to-date counters.
-  CommitCounters commit_counters_;
-  UpdateCounters update_counters_;
-
-  // The last state of the counters emitted to UMA. In the next round of
-  // emitting to UMA, we only need to upload the diff between the actual
-  // counters and the counts here.
-  CommitCounters emitted_commit_counters_;
-  UpdateCounters emitted_update_counters_;
-
-  DISALLOW_COPY_AND_ASSIGN(DataTypeDebugInfoEmitter);
-};
-
-}  // namespace syncer
-
-#endif  // COMPONENTS_SYNC_ENGINE_IMPL_CYCLE_DATA_TYPE_DEBUG_INFO_EMITTER_H_

components/sync/engine_impl/model_type_registry.cc

@@ -15,7 +15,6 @@
 #include "components/sync/engine/commit_queue.h"
 #include "components/sync/engine/data_type_activation_response.h"
 #include "components/sync/engine/model_type_processor.h"
-#include "components/sync/engine_impl/cycle/data_type_debug_info_emitter.h"
 #include "components/sync/engine_impl/model_type_worker.h"
 #include "components/sync/nigori/cryptographer.h"
 #include "components/sync/nigori/keystore_keys_handler.h"
@@ -237,15 +236,6 @@ void ModelTypeRegistry::OnEncryptionStateChanged() {
   }
 }
 
-DataTypeDebugInfoEmitter* ModelTypeRegistry::GetEmitter(ModelType type) {
-  DataTypeDebugInfoEmitter* raw_emitter = nullptr;
-  auto it = data_type_debug_info_emitter_map_.find(type);
-  if (it != data_type_debug_info_emitter_map_.end()) {
-    raw_emitter = it->second.get();
-  }
-  return raw_emitter;
-}
-
 ModelTypeSet ModelTypeRegistry::GetEnabledDataTypes() const {
   ModelTypeSet enabled_types;
   for (const auto& worker : model_type_workers_) {

components/sync/engine_impl/model_type_registry.h

@@ -23,7 +23,6 @@ namespace syncer {
 
 class CancelationSignal;
 class CommitContributor;
-class DataTypeDebugInfoEmitter;
 class KeystoreKeysHandler;
 class ModelTypeWorker;
 class UpdateHandler;
@@ -85,14 +84,8 @@ class ModelTypeRegistry : public ModelTypeConnector,
   base::WeakPtr<ModelTypeConnector> AsWeakPtr();
 
  private:
-  using DataTypeDebugInfoEmitterMap =
-      std::map<ModelType, std::unique_ptr<DataTypeDebugInfoEmitter>>;
-
   void OnEncryptionStateChanged();
 
-  // DebugInfoEmitters are never deleted. Returns an existing one if we have it.
-  DataTypeDebugInfoEmitter* GetEmitter(ModelType type);
-
   ModelTypeSet GetEnabledDataTypes() const;
 
   // Enabled proxy types, which don't have a worker.
@@ -105,9 +98,6 @@ class ModelTypeRegistry : public ModelTypeConnector,
   UpdateHandlerMap update_handler_map_;
   CommitContributorMap commit_contributor_map_;
 
-  // Map of DebugInfoEmitters for sync data types. Does not own its contents.
-  DataTypeDebugInfoEmitterMap data_type_debug_info_emitter_map_;
-
   // A copy of the most recent cryptographer.
   std::unique_ptr<Cryptographer> cryptographer_;
 

components/sync/engine_impl/model_type_worker.cc

@@ -168,17 +168,6 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
     const sync_pb::DataTypeContext& mutated_context,
     const SyncEntityList& applicable_updates,
     StatusController* status) {
-  return ProcessGetUpdatesResponse(progress_marker, mutated_context,
-                                   applicable_updates,
-                                   /*from_uss_migrator=*/false, status);
-}
-
-SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
-    const sync_pb::DataTypeProgressMarker& progress_marker,
-    const sync_pb::DataTypeContext& mutated_context,
-    const SyncEntityList& applicable_updates,
-    bool from_uss_migrator,
-    StatusController* status) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   const bool is_initial_sync = !model_type_state_.initial_sync_done();
@@ -188,14 +177,10 @@ SyncerError ModelTypeWorker::ProcessGetUpdatesResponse(
   *model_type_state_.mutable_progress_marker() = progress_marker;
 
   for (const sync_pb::SyncEntity* update_entity : applicable_updates) {
-    // TODO(crbug.com/1137896): The USS migrator doesn't exist anymore, remove
-    // this parameter.
-    if (!from_uss_migrator) {
-      RecordEntityChangeMetrics(
-          type_, is_initial_sync
-                     ? ModelTypeEntityChange::kRemoteInitialUpdate
-                     : ModelTypeEntityChange::kRemoteNonInitialUpdate);
-    }
+    RecordEntityChangeMetrics(
+        type_, is_initial_sync
+                   ? ModelTypeEntityChange::kRemoteInitialUpdate
+                   : ModelTypeEntityChange::kRemoteNonInitialUpdate);
 
     if (update_entity->deleted()) {
       status->increment_num_tombstone_updates_downloaded_by(1);

components/sync/engine_impl/model_type_worker.h

@@ -102,16 +102,6 @@ class ModelTypeWorker : public UpdateHandler,
   std::unique_ptr<CommitContribution> GetContribution(
       size_t max_entries) override;
 
-  // Extended overload of ProcessGetUpdatesResponse() that allows specifying
-  // whether the updates are coming from the USS migrator, which influences how
-  // UMA metrics are logged.
-  SyncerError ProcessGetUpdatesResponse(
-      const sync_pb::DataTypeProgressMarker& progress_marker,
-      const sync_pb::DataTypeContext& mutated_context,
-      const SyncEntityList& applicable_updates,
-      bool from_uss_migrator,
-      StatusController* status);
-
   bool HasLocalChangesForTest() const;
 
   // An alternative way to drive sending data to the processor, that should be

components/sync/engine_impl/model_type_worker_unittest.cc

@@ -391,17 +391,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
   void PumpModelThread() { processor()->RunQueuedTasks(); }
 
   // Returns true if the |worker_| is ready to commit something.
-  bool WillCommit() {
-    std::unique_ptr<CommitContribution> contribution(
-        worker()->GetContribution(INT_MAX));
-
-    if (contribution) {
-      contribution->CleanUp();  // Gracefully abort the commit.
-      return true;
-    } else {
-      return false;
-    }
-  }
+  bool WillCommit() { return worker()->GetContribution(INT_MAX) != nullptr; }
 
   // Pretend to successfully commit all outstanding unsynced items.
   // It is safe to call this only if WillCommit() returns true.
@@ -421,7 +411,6 @@ class ModelTypeWorkerTest : public ::testing::Test {
         server()->DoSuccessfulCommit(message);
 
     contribution->ProcessCommitResponse(response, &status_controller_);
-    contribution->CleanUp();
   }
 
   void DoCommitFailure() {
@@ -430,7 +419,6 @@ class ModelTypeWorkerTest : public ::testing::Test {
     DCHECK(contribution);
 
     contribution->ProcessCommitFailure(SyncCommitError::kNetworkError);
-    contribution->CleanUp();
   }
 
   // Callback when processor got disconnected with sync.

components/sync/engine_impl/syncer.cc

@@ -165,7 +165,6 @@ SyncerError Syncer::BuildAndPostCommits(const ModelTypeSet& request_types,
       base::UmaHistogramEnumeration(kPrefix + ModelTypeToHistogramSuffix(type),
                                     error.value());
     }
-    commit->CleanUp();
     if (error.value() != SyncerError::SYNCER_OK) {
       return error;
     }