sync: Buffer Protocol Events for about:sync page

chrome/browser/resources/sync_internals/about.js

Index: chrome/browser/resources/sync_internals/about.js
diff --git a/chrome/browser/resources/sync_internals/about.js b/chrome/browser/resources/sync_internals/about.js
index 172e690030afd562ecef904d9a6e4c51250cc212..61ad8aa2966893749f61d4a2338e254d7000f1d6 100644
--- a/chrome/browser/resources/sync_internals/about.js
+++ b/chrome/browser/resources/sync_internals/about.js
@@ -19,12 +19,23 @@ cr.define('chrome.sync.about_tab', function() {
   /** Container for accumulated sync protocol events. */
   var protocolEvents = [];
 
+  /** We may receive re-delivered events.  Keep a record of ones we've seen. */

[James Hawkins, 2014/03/27 21:01:25]

Can we not fix the issue of re-delivered events?

[rlarocque, 2014/03/27 21:19:54]

It gets ugly no matter how we handle it.

Option #1: Ignore it.  We send any buffered events to all listeners whenever a
new listener registers.  This will be a problem only when someone opens
about:sync twice.

Option #2: Send the buffered only events only to the newly registered listener. 
The problem is that the observation crosses a thread boundary.  The registration
path is

SyncInternalsMessageHandler
  -> ProfileSyncService (observer list is managed here)
  -> SyncBackendHost
   ... across a thread boundary ...
  -> SyncBackendHostCore
  -> SyncManagerImpl (which owns the buffer)

By the time the SyncBackendHostCore returns its response,
 1. the user could have closed the page, thus deleting the observer.
 2. (in theory) the observer could have unregistered itself.

We could handle (1) with a WeakPtr and a Closure.  I think we could handle (2)
if we had some sort of WeakPtr observer interface that got invalidated when the
ProfileSyncService was asked to remove the observer from its list.

This is starting to sound like some kind of generic cross-thread observer list
interface.  I'm pretty sure this is the right solution, but I'm not sure it's
worth building and maintaining just for this use case.

Option #3: The hack you see here.  Make the listeners figure it out.

That's the thinking that led me to this CL.  If you can think of a better
approach, I'd be happy to implement it.

+  var knownEventTimestamps = {};
+
   /**
    * Callback for incoming protocol events.
    * @param {Event} e The protocol event.
    */
   function onReceivedProtocolEvent(e) {
     var details = e.details;
+
+    // Return early if we've seen this event before.  Assumes that timestamps
+    // are sufficiently high resolution to uniquely identify an event.
+    if (knownEventTimestamps.hasOwnProperty(details.time)) {
+      return;
+    }
+
+    knownEventTimestamps[details.time] = true;
     protocolEvents.push(details);
 
     var context = new JsEvalContext({ events: protocolEvents });
@@ -43,29 +54,11 @@ cr.define('chrome.sync.about_tab', function() {
   }
 
   /**
-   * Toggles the given traffic event entry div's "expanded" state.
-   * @param {HTMLElement} element the element to toggle.
-   */
-  function expandListener(element) {
-    element.target.classList.toggle('traffic-event-entry-expanded');
-  }
-
-  /**
-   * Attaches a listener to the given traffic event entry div.
-   * @param {HTMLElement} element the element to attach the listener to.
+   * Initializes listeners for status dump and import UI.
    */
-  function addExpandListener(element) {
-    element.addEventListener('click', expandListener, false);
-  }
-
-  function onLoad() {
+  function initStatusDumpButton() {
     $('status-data').hidden = true;
 
-    chrome.sync.events.addEventListener(
-        'onAboutInfoUpdated',
-        onAboutInfoUpdatedEvent);
-    chrome.sync.requestUpdatedAboutInfo();
-
     var dumpStatusButton = $('dump-status');
     dumpStatusButton.addEventListener('click', function(event) {
       var aboutInfo = chrome.sync.aboutInfo;
@@ -111,8 +104,37 @@ cr.define('chrome.sync.about_tab', function() {
       var aboutInfo = JSON.parse(data);
       refreshAboutInfo(aboutInfo);
     });
+  }
 
+  /**
+   * Toggles the given traffic event entry div's "expanded" state.
+   * @param {HTMLElement} element the element to toggle.
+   */
+  function expandListener(element) {
+    element.target.classList.toggle('traffic-event-entry-expanded');
+  }
+
+  /**
+   * Attaches a listener to the given traffic event entry div.
+   * @param {HTMLElement} element the element to attach the listener to.
+   */
+  function addExpandListener(element) {

[James Hawkins, 2014/03/27 21:01:25]

Who calls this method?

[rlarocque, 2014/03/27 21:19:54]

It's used to attach listeners to the elements created by jstemplate.  See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

+    element.addEventListener('click', expandListener, false);
+  }
+
+  function onLoad() {
+    initStatusDumpButton();
     initProtocolEventLog();
+
+    chrome.sync.events.addEventListener(
+        'onAboutInfoUpdated',
+        onAboutInfoUpdatedEvent);
+
+    // Register to receive a stream of event notifications.
+    chrome.sync.registerForEvents();
+
+    // Request an about info update event to initialize the page.
+    chrome.sync.requestUpdatedAboutInfo();
   }
 
   return {