sync: Add UI code for per type debug counters

chrome/browser/ui/webui/sync_internals_message_handler.cc

Index: chrome/browser/ui/webui/sync_internals_message_handler.cc
diff --git a/chrome/browser/ui/webui/sync_internals_message_handler.cc b/chrome/browser/ui/webui/sync_internals_message_handler.cc
index c4612b755bc5c4b38ddffba126d8bf4cd4c278b0..fe4bf2ea0cc6b51b20502a999ebb90da8c74eca4 100644
--- a/chrome/browser/ui/webui/sync_internals_message_handler.cc
+++ b/chrome/browser/ui/webui/sync_internals_message_handler.cc
@@ -14,6 +14,9 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_ui.h"
 #include "sync/internal_api/public/events/protocol_event.h"
+#include "sync/internal_api/public/sessions/commit_counters.h"
+#include "sync/internal_api/public/sessions/status_counters.h"
+#include "sync/internal_api/public/sessions/update_counters.h"
 #include "sync/internal_api/public/util/weak_handle.h"
 #include "sync/js/js_event_details.h"
 
@@ -23,7 +26,9 @@ using syncer::WeakHandle;
 
 SyncInternalsMessageHandler::SyncInternalsMessageHandler()
     : is_registered_(false),
-      weak_ptr_factory_(this) {}
+      is_registered_for_counters_(false),
+      weak_ptr_factory_(this) {
+}
 
 SyncInternalsMessageHandler::~SyncInternalsMessageHandler() {
   if (js_controller_)
@@ -34,6 +39,10 @@ SyncInternalsMessageHandler::~SyncInternalsMessageHandler() {
     service->RemoveObserver(this);
     service->RemoveProtocolEventObserver(this);
   }
+
+  if (service && is_registered_for_counters_) {
+    service->RemoveTypeDebugInfoObserver(this);
+  }
 }
 
 void SyncInternalsMessageHandler::RegisterMessages() {
@@ -45,6 +54,11 @@ void SyncInternalsMessageHandler::RegisterMessages() {
                  base::Unretained(this)));
 
   web_ui()->RegisterMessageCallback(
+      "registerForPerTypeCounters",

[maniscalco, 2014/05/23 00:55:00]

I wonder if we could tie into the existing "registerForEvents"
(HandleRegisterForEvents) callback instead of creating a separate one for per
type counters.  If so, we could drop the new flag.  Does anything prohibit us
from having just one register callback?

[rlarocque, 2014/05/27 20:13:26]

It gets complicated.

The problem is that this registration also implicitly sends a request to
immediately issue a series of events that should initialize the newly-registered
listener's state.

If the registration happens as part of the registerForEvents() call in about.js,
we'd have no guarantee that the listeners in types.js will have been setup and
listening by that time.  I think that may be a risk even if the initialization
request and registration were separate, since (I suppose) types.js could request
initialization before registerForEvents() is called, and I'm not sure what the
semantics of that would be.

===

We could probably refactor around this.  I think the eventual "right" way to fix
this would be to implement an abstraction layer between the chrome.send() calls
and responses and the page display logic.  All data received from Chrome would
be stored in a cache.  Listeners could query it explicitly or request
notifications of updates.  sync_log.js follows this pattern.

On the other hand, this is supposed to be a simple, developer-facing UI for
displaying data exported from sync.  It would be nice if we could keep this page
as dumb as possible.  I'm not sure if implementing the technically "right"
solution would be a good idea in this context.

+      base::Bind(&SyncInternalsMessageHandler::HandleRegisterForPerTypeCounters,
+                 base::Unretained(this)));
+
+  web_ui()->RegisterMessageCallback(
       "requestUpdatedAboutInfo",
       base::Bind(&SyncInternalsMessageHandler::HandleRequestUpdatedAboutInfo,
                  base::Unretained(this)));
@@ -77,6 +91,21 @@ void SyncInternalsMessageHandler::HandleRegisterForEvents(
   }
 }
 
+void SyncInternalsMessageHandler::HandleRegisterForPerTypeCounters(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+
+  ProfileSyncService* service = GetProfileSyncService();
+  if (service && !is_registered_for_counters_) {
+    service->AddTypeDebugInfoObserver(this);
+    is_registered_for_counters_ = true;
+  } else {
+    // Re-register to ensure counters get re-emitted.
+    service->RemoveTypeDebugInfoObserver(this);
+    service->AddTypeDebugInfoObserver(this);
+  }
+}
+
 void SyncInternalsMessageHandler::HandleRequestUpdatedAboutInfo(
     const base::ListValue* args) {
   DCHECK(args->empty());
@@ -137,6 +166,42 @@ void SyncInternalsMessageHandler::OnProtocolEvent(
       *value);
 }
 
+void SyncInternalsMessageHandler::OnCommitCountersUpdated(
+    syncer::ModelType type,
+    const syncer::CommitCounters& counters) {
+  scoped_ptr<base::DictionaryValue> details(new base::DictionaryValue());

[maniscalco, 2014/05/23 00:55:00]

Too bad syncer::FooCounters don't have a common base class.  WDYT about using a
function template to reduce duplication?

template <typename T>
scoped_ptr<base::DictionaryValue> CreateDictionaryWithCounters(
    syncer::ModelType type,
    const std::string& description,
    const T& counters) {
  details->SetString("modelType", ModelTypeToString(type));
  details->SetString("counterType", description);
  details->Set("counters", counters.ToValue().release());
}

[rlarocque, 2014/05/27 20:13:26]

I'll admit that seems like a valid use of templates, but I think we have a
pretty strong bias against templates in our code.

I think your proposal still requires three separate listener functions, right? 
What if we pulled the ToValue() call out of proposed template function and into
the type-specific wrapper?  Then CreateDictionaryWithCounters could take a
DictionaryValue instead of a T.

[maniscalco, 2014/05/27 20:31:07]

Yep, you'd still have three listener functions, each with a call to
CreateDictionaryWithCounters.

Pulling the ToValue call up into the listener and passing a DictionaryValue to
the common helper function SGTM.

+  details->SetString("modelType", ModelTypeToString(type));
+  details->SetString("counterType", "commit");
+  details->Set("counters", counters.ToValue().release());
+  web_ui()->CallJavascriptFunction("chrome.sync.dispatchEvent",
+                                   base::StringValue("onCountersUpdated"),
+                                   *details);
+}
+
+void SyncInternalsMessageHandler::OnUpdateCountersUpdated(
+    syncer::ModelType type,
+    const syncer::UpdateCounters& counters) {
+  scoped_ptr<base::DictionaryValue> details(new base::DictionaryValue());
+  details->SetString("modelType", ModelTypeToString(type));
+  details->SetString("counterType", "update");
+  details->Set("counters", counters.ToValue().release());
+  web_ui()->CallJavascriptFunction("chrome.sync.dispatchEvent",
+                                   base::StringValue("onCountersUpdated"),
+                                   *details);
+}
+
+void SyncInternalsMessageHandler::OnStatusCountersUpdated(
+    syncer::ModelType type,
+    const syncer::StatusCounters& counters) {
+  scoped_ptr<base::DictionaryValue> details(new base::DictionaryValue());
+  details->SetString("modelType", ModelTypeToString(type));
+  details->SetString("counterType", "status");
+  details->Set("counters", counters.ToValue().release());
+  web_ui()->CallJavascriptFunction("chrome.sync.dispatchEvent",
+                                   base::StringValue("onCountersUpdated"),
+                                   *details);
+}
+
 void SyncInternalsMessageHandler::HandleJsEvent(
     const std::string& name,
     const JsEventDetails& details) {
@@ -163,4 +228,3 @@ ProfileSyncService* SyncInternalsMessageHandler::GetProfileSyncService() {
   ProfileSyncServiceFactory* factory = ProfileSyncServiceFactory::GetInstance();
   return factory->GetForProfile(profile->GetOriginalProfile());
 }
-