Take 2 at browser_sync::ExtensionsActivityMonitor.

Original: http://codereview.chromium.org/325001/show

TEST=ExtensionsActivityMonitorTest

Review URL: http://codereview.chromium.org/333041

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30313 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 76 insertions, 4 deletions

diff --git a/chrome/browser/sync/engine/build_commit_command.cc b/chrome/browser/sync/engine/build_commit_command.cc
index 46200c6..5058bba 100644
--- a/chrome/browser/sync/engine/build_commit_command.cc
+++ b/chrome/browser/sync/engine/build_commit_command.cc
@@ -30,6 +30,20 @@ namespace browser_sync {
 BuildCommitCommand::BuildCommitCommand() {}
 BuildCommitCommand::~BuildCommitCommand() {}
 
+void BuildCommitCommand::AddExtensionsActivityToMessage(
+    SyncerSession* session, CommitMessage* message) {
+  const ExtensionsActivityMonitor::Records& records =
+      session->extensions_activity();
+  for (ExtensionsActivityMonitor::Records::const_iterator it = records.begin();
+       it != records.end(); ++it) {
+    sync_pb::CommitMessage_ChromiumExtensionsActivity* activity_message =
+        message->add_extensions_activity();
+    activity_message->set_extension_id(it->second.extension_id);
+    activity_message->set_bookmark_writes_since_last_commit(
+        it->second.bookmark_write_count);
+  }
+}
+
 void BuildCommitCommand::ExecuteImpl(SyncerSession* session) {
   ClientToServerMessage message;
   message.set_share(ToUTF8(session->account_name()).get_string());
@@ -38,6 +52,7 @@ void BuildCommitCommand::ExecuteImpl(SyncerSession* session) {
   CommitMessage* commit_message = message.mutable_commit();
   commit_message->set_cache_guid(
       session->write_transaction()->directory()->cache_guid());
+  AddExtensionsActivityToMessage(session, commit_message);
 
   const vector<Id>& commit_ids = session->commit_ids();
   for (size_t i = 0; i < commit_ids.size(); i++) {
diff --git a/chrome/browser/sync/engine/build_commit_command.h b/chrome/browser/sync/engine/build_commit_command.h
index 445024f..1c29590 100644
--- a/chrome/browser/sync/engine/build_commit_command.h
+++ b/chrome/browser/sync/engine/build_commit_command.h
@@ -19,6 +19,8 @@ class BuildCommitCommand : public SyncerCommand {
   virtual void ExecuteImpl(SyncerSession *session);
 
  private:
+  void AddExtensionsActivityToMessage(SyncerSession* session,
+                                      CommitMessage* message);
   DISALLOW_COPY_AND_ASSIGN(BuildCommitCommand);
 };
 
diff --git a/chrome/browser/sync/engine/process_commit_response_command.cc b/chrome/browser/sync/engine/process_commit_response_command.cc
index 3deec6d..72fd0e6 100644
--- a/chrome/browser/sync/engine/process_commit_response_command.cc
+++ b/chrome/browser/sync/engine/process_commit_response_command.cc
@@ -52,11 +52,19 @@ void ResetErrorCounters(SyncerStatus status) {
   status.zero_consecutive_errors();
 }
 
-ProcessCommitResponseCommand::ProcessCommitResponseCommand() {}
+ProcessCommitResponseCommand::ProcessCommitResponseCommand(
+    ExtensionsActivityMonitor* monitor) : extensions_monitor_(monitor) {}
 ProcessCommitResponseCommand::~ProcessCommitResponseCommand() {}
 
 void ProcessCommitResponseCommand::ModelChangingExecuteImpl(
     SyncerSession* session) {
+  ProcessCommitResponse(session);
+  if (!session->HadSuccessfulCommits())
+    extensions_monitor_->PutRecords(session->extensions_activity());
+}
+
+void ProcessCommitResponseCommand::ProcessCommitResponse(
+    SyncerSession* session) {
   // TODO(sync): This function returns if it sees problems. We probably want
   // to flag the need for an update or similar.
   ScopedDirLookup dir(session->dirman(), session->account_name());
diff --git a/chrome/browser/sync/engine/process_commit_response_command.h b/chrome/browser/sync/engine/process_commit_response_command.h
index 4e7c94b..9950ba4 100644
--- a/chrome/browser/sync/engine/process_commit_response_command.h
+++ b/chrome/browser/sync/engine/process_commit_response_command.h
@@ -23,7 +23,7 @@ namespace browser_sync {
 class ProcessCommitResponseCommand : public ModelChangingSyncerCommand {
  public:
 
-  ProcessCommitResponseCommand();
+  explicit ProcessCommitResponseCommand(ExtensionsActivityMonitor* monitor);
   virtual ~ProcessCommitResponseCommand();
 
   virtual void ModelChangingExecuteImpl(SyncerSession* session);
@@ -36,6 +36,9 @@ class ProcessCommitResponseCommand : public ModelChangingSyncerCommand {
       std::set<syncable::Id>* deleted_folders,
       SyncerSession* const session);
 
+  // Actually does the work of execute.
+  void ProcessCommitResponse(SyncerSession* session);
+
   void ProcessSuccessfulCommitResponse(syncable::WriteTransaction* trans,
       const CommitResponse_EntryResponse& server_entry,
       const syncable::Id& pre_commit_id, syncable::MutableEntry* local_entry,
@@ -47,6 +50,10 @@ class ProcessCommitResponseCommand : public ModelChangingSyncerCommand {
       const CommitResponse_EntryResponse& server_entry,
       syncable::MutableEntry* local_entry);
 
+  // We may need to update this with records from a commit attempt if the
+  // attempt failed.
+  ExtensionsActivityMonitor* extensions_monitor_;
+
   DISALLOW_COPY_AND_ASSIGN(ProcessCommitResponseCommand);
 };
 
diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc
index de8b1b2..ee56bb9 100644
--- a/chrome/browser/sync/engine/syncer.cc
+++ b/chrome/browser/sync/engine/syncer.cc
@@ -5,6 +5,8 @@
 #include "chrome/browser/sync/engine/syncer.h"
 
 #include "base/format_macros.h"
+#include "base/message_loop.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/browser/sync/engine/apply_updates_command.h"
 #include "chrome/browser/sync/engine/build_and_process_conflict_sets_command.h"
 #include "chrome/browser/sync/engine/build_commit_command.h"
@@ -66,12 +68,21 @@ Syncer::Syncer(
   syncer_event_channel_.reset(new SyncerEventChannel(shutdown));
   shutdown_channel_.reset(new ShutdownChannel(this));
 
+  extensions_monitor_ = new ExtensionsActivityMonitor();
+
   ScopedDirLookup dir(dirman_, account_name_);
   // The directory must be good here.
   CHECK(dir.good());
 }
 
-Syncer::~Syncer() {}
+Syncer::~Syncer() {
+  if (!ChromeThread::DeleteSoon(ChromeThread::UI, FROM_HERE,
+                                extensions_monitor_)) {
+    // In unittests, there may be no UI thread, so the above will fail.
+    delete extensions_monitor_;
+  }
+  extensions_monitor_ = NULL;
+}
 
 void Syncer::RequestNudge(int milliseconds) {
   SyncerEvent event;
@@ -92,6 +103,17 @@ bool Syncer::SyncShare(SyncProcessState* process_state) {
   SyncerSession session(&cycle_state, process_state);
   session.set_source(TestAndSetUpdatesSource());
   session.set_notifications_enabled(notifications_enabled());
+  // This isn't perfect, as we can end up bundling extensions activity
+  // intended for the next session into the current one.  We could do a
+  // test-and-reset as with the source, but note that also falls short if
+  // the commit request fails (due to lost connection, for example), as we will
+  // fall all the way back to the syncer thread main loop in that case, and
+  // wind up creating a new session when a connection is established, losing
+  // the records set here on the original attempt.  This should provide us
+  // with the right data "most of the time", and we're only using this for
+  // analysis purposes, so Law of Large Numbers FTW.
+  extensions_monitor_->GetAndClearRecords(
+      session.mutable_extensions_activity());
   SyncShare(&session, SYNCER_BEGIN, SYNCER_END);
   return session.HasMoreToSync();
 }
@@ -205,7 +227,8 @@ void Syncer::SyncShare(SyncerSession* session,
       }
       case PROCESS_COMMIT_RESPONSE: {
         LOG(INFO) << "Processing the commit response";
-        ProcessCommitResponseCommand process_response_command;
+        ProcessCommitResponseCommand process_response_command(
+            extensions_monitor_);
         process_response_command.Execute(session);
         next_step = BUILD_AND_PROCESS_CONFLICT_SETS;
         break;
diff --git a/chrome/browser/sync/engine/syncer.h b/chrome/browser/sync/engine/syncer.h
index 2d6ba6a..067193b 100644
--- a/chrome/browser/sync/engine/syncer.h
+++ b/chrome/browser/sync/engine/syncer.h
@@ -19,6 +19,7 @@
 #include "chrome/browser/sync/syncable/directory_event.h"
 #include "chrome/browser/sync/util/event_sys-inl.h"
 #include "chrome/browser/sync/util/event_sys.h"
+#include "chrome/browser/sync/util/extensions_activity_monitor.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"  // For FRIEND_TEST
 
 namespace syncable {
@@ -180,6 +181,10 @@ class Syncer {
   // by the SyncerThread that created us.
   ModelSafeWorker* model_safe_worker_;
 
+  // We use this to stuff extensions activity into CommitMessages so the server
+  // can correlate commit traffic with extension-related bookmark mutations.
+  ExtensionsActivityMonitor* extensions_monitor_;
+
   // The source of the last nudge.
   sync_pb::GetUpdatesCallerInfo::GET_UPDATES_SOURCE updates_source_;
 
diff --git a/chrome/browser/sync/engine/syncer_session.h b/chrome/browser/sync/engine/syncer_session.h
index 83643d7..5e4710b 100644
--- a/chrome/browser/sync/engine/syncer_session.h
+++ b/chrome/browser/sync/engine/syncer_session.h
@@ -22,6 +22,7 @@
 #include "chrome/browser/sync/engine/syncer_types.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/util/event_sys.h"
+#include "chrome/browser/sync/util/extensions_activity_monitor.h"
 #include "chrome/browser/sync/util/sync_types.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"  // For FRIEND_TEST
 
@@ -307,6 +308,14 @@ class SyncerSession {
     source_ = source;
   }
 
+  const ExtensionsActivityMonitor::Records& extensions_activity() const {
+    return extensions_activity_;
+  }
+
+  ExtensionsActivityMonitor::Records* mutable_extensions_activity() {
+    return &extensions_activity_;
+  }
+
   bool notifications_enabled() const {
     return notifications_enabled_;
   }
@@ -355,6 +364,9 @@ class SyncerSession {
   // The source for initiating this syncer session.
   sync_pb::GetUpdatesCallerInfo::GET_UPDATES_SOURCE source_;
 
+  // Information about extensions activity since the last successful commit.
+  ExtensionsActivityMonitor::Records extensions_activity_;
+
   // True if notifications are enabled when this session was created.
   bool notifications_enabled_;