sync: Buffer Protocol Events for about:sync page

Allows the about:sync page to present the last six network events as
soon as it's opened.

Adds the ProtocolEventBuffer to the SyncManagerImpl.  This class holds
on to a few protocol events and returns them on demand.

Modifies the interface to enable SyncBacknedHost protocol event
forwarding.  By default, it does not forward any events.  There are now
separate registration and unregistration functions.  The registration
function will send the set of buffered notifications to the UI thread,
and also register it to receive incoming events in the future.  It
continues to forward events until the number of registration calls is
matched by the number of un-registration calls.

Makes about:sync's registration to receive events explicit.  This was
a long-time TODO.  If we did not fix this issue, then the about:sync
page could receive the initial set of buffered invalidations before it
had its event listeners defined and registered.

Makes the about:sync page keep track of known events and avoid adding
duplicates to the list.  Since the registration of a new event listener
causes events to be distributed to all listeners, we must add this logic
to ensure that opening one new about:sync tab will not cause previously
opened tabs to display duplicate events.

BUG=329373,349301

Review URL: https://codereview.chromium.org/212603007

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

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. */
+  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) {
+    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 {

chrome/browser/resources/sync_internals/chrome_sync.js

@@ -82,6 +82,14 @@ cr.define('chrome.sync', function() {
     chrome.sync.events.dispatchEvent(e);
   };
 
+  /**
+   * Registers to receive a stream of events through
+   * chrome.sync.dispatchEvent().
+   */
+  var registerForEvents = function() {
+    chrome.send('registerForEvents');
+  };
+
   /**
    * Asks the browser to refresh our snapshot of sync state.  Should result
    * in an onAboutInfoUpdated event being emitted.
@@ -103,6 +111,7 @@ cr.define('chrome.sync', function() {
     dispatchEvent: dispatchEvent,
     events: new cr.EventTarget(),
 
+    registerForEvents: registerForEvents,
     requestUpdatedAboutInfo: requestUpdatedAboutInfo,
     requestListOfTypes: requestListOfTypes,
   };

chrome/browser/sync/glue/sync_backend_host.h

@@ -192,8 +192,14 @@ class SyncBackendHost : public BackendDataTypeConfigurer {
   // Fetches the DeviceInfo tracker.
   virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const = 0;
 
-  // Sets whether or not the frontend will be notified of network events.
-  virtual void SetForwardProtocolEvents(bool forward) = 0;
+  // Requests that the backend forward to the fronent any protocol events in
+  // its buffer and begin forwarding automatically from now on.  Repeated calls
+  // to this function may result in the same events being emitted several
+  // times.
+  virtual void RequestBufferedProtocolEventsAndEnableForwarding() = 0;
+
+  // Disables protocol event forwarding.
+  virtual void DisableProtocolEventForwarding() = 0;
 
   virtual base::MessageLoop* GetSyncLoopForTesting() = 0;
 

chrome/browser/sync/glue/sync_backend_host_core.cc

@@ -104,6 +104,7 @@ SyncBackendHostCore::SyncBackendHostCore(
       sync_loop_(NULL),
       registrar_(NULL),
       has_sync_setup_completed_(has_sync_setup_completed),
+      forward_protocol_events_(false),
       weak_ptr_factory_(this) {
   DCHECK(backend.get());
 }
@@ -605,9 +606,30 @@ void SyncBackendHostCore::DoRetryConfiguration(
              retry_callback);
 }
 
-void SyncBackendHostCore::SetForwardProtocolEvents(bool enabled) {
+void SyncBackendHostCore::SendBufferedProtocolEventsAndEnableForwarding() {
   DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
-  forward_protocol_events_ = enabled;
+  forward_protocol_events_ = true;
+
+  if (sync_manager_) {
+    // Grab our own copy of the buffered events.
+    // The buffer is not modified by this operation.
+    std::vector<syncer::ProtocolEvent*> buffered_events;
+    sync_manager_->GetBufferedProtocolEvents().release(&buffered_events);
+
+    // Send them all over the fence to the host.
+    for (std::vector<syncer::ProtocolEvent*>::iterator it =
+         buffered_events.begin(); it != buffered_events.end(); ++it) {
+      // TODO(rlarocque): Make it explicit that host_ takes ownership.
+      host_.Call(
+          FROM_HERE,
+          &SyncBackendHostImpl::HandleProtocolEventOnFrontendLoop,
+          *it);
+    }
+  }
+}
+
+void SyncBackendHostCore::DisableProtocolEventForwarding() {
+  forward_protocol_events_ = false;
 }
 
 void SyncBackendHostCore::DeleteSyncDataFolder() {

chrome/browser/sync/glue/sync_backend_host_core.h

@@ -209,7 +209,8 @@ class SyncBackendHostCore
     return synced_device_tracker_.get();
   }
 
-  void SetForwardProtocolEvents(bool forward);
+  void SendBufferedProtocolEventsAndEnableForwarding();
+  void DisableProtocolEventForwarding();
 
   // Delete the sync data folder to cleanup backend data.  Happens the first
   // time sync is enabled for a user (to prevent accidentally reusing old

chrome/browser/sync/glue/sync_backend_host_impl.cc

@@ -475,12 +475,22 @@ SyncedDeviceTracker* SyncBackendHostImpl::GetSyncedDeviceTracker() const {
   return core_->synced_device_tracker();
 }
 
-void SyncBackendHostImpl::SetForwardProtocolEvents(bool forward) {
+void SyncBackendHostImpl::RequestBufferedProtocolEventsAndEnableForwarding() {
   DCHECK(initialized());
   registrar_->sync_thread()->message_loop()->PostTask(
       FROM_HERE,
-      base::Bind(&SyncBackendHostCore::SetForwardProtocolEvents,
-                 core_, forward));
+      base::Bind(
+          &SyncBackendHostCore::SendBufferedProtocolEventsAndEnableForwarding,
+          core_));
+}
+
+void SyncBackendHostImpl::DisableProtocolEventForwarding() {
+  DCHECK(initialized());
+  registrar_->sync_thread()->message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &SyncBackendHostCore::DisableProtocolEventForwarding,
+          core_));
 }
 
 void SyncBackendHostImpl::InitCore(scoped_ptr<DoInitializeOptions> options) {

chrome/browser/sync/glue/sync_backend_host_impl.h

@@ -124,7 +124,8 @@ class SyncBackendHostImpl
   virtual void GetModelSafeRoutingInfo(
       syncer::ModelSafeRoutingInfo* out) const OVERRIDE;
   virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const OVERRIDE;
-  virtual void SetForwardProtocolEvents(bool forward) OVERRIDE;
+  virtual void RequestBufferedProtocolEventsAndEnableForwarding() OVERRIDE;
+  virtual void DisableProtocolEventForwarding() OVERRIDE;
   virtual base::MessageLoop* GetSyncLoopForTesting() OVERRIDE;
 
  protected:

chrome/browser/sync/glue/sync_backend_host_mock.cc

@@ -110,7 +110,9 @@ base::MessageLoop* SyncBackendHostMock::GetSyncLoopForTesting() {
   return NULL;
 }
 
-void SyncBackendHostMock::SetForwardProtocolEvents(bool enabled) {}
+void SyncBackendHostMock::RequestBufferedProtocolEventsAndEnableForwarding() {}
+
+void SyncBackendHostMock::DisableProtocolEventForwarding() {}
 
 void SyncBackendHostMock::set_fail_initial_download(bool should_fail) {
   fail_initial_download_ = should_fail;

chrome/browser/sync/glue/sync_backend_host_mock.h

@@ -93,7 +93,8 @@ class SyncBackendHostMock : public SyncBackendHost {
 
   virtual SyncedDeviceTracker* GetSyncedDeviceTracker() const OVERRIDE;
 
-  virtual void SetForwardProtocolEvents(bool enabled) OVERRIDE;
+  virtual void RequestBufferedProtocolEventsAndEnableForwarding() OVERRIDE;
+  virtual void DisableProtocolEventForwarding() OVERRIDE;
 
   virtual base::MessageLoop* GetSyncLoopForTesting() OVERRIDE;
 

chrome/browser/sync/profile_sync_service.cc

@@ -936,7 +936,7 @@ void ProfileSyncService::OnBackendInitialized(
   debug_info_listener_ = debug_info_listener;
 
   if (protocol_event_observers_.might_have_observers()) {
-    backend_->SetForwardProtocolEvents(true);
+    backend_->RequestBufferedProtocolEventsAndEnableForwarding();
   }
 
   // If we have a cached passphrase use it to decrypt/encrypt data now that the
@@ -2067,17 +2067,15 @@ void ProfileSyncService::AddProtocolEventObserver(
     browser_sync::ProtocolEventObserver* observer) {
   protocol_event_observers_.AddObserver(observer);
   if (backend_) {
-    backend_->SetForwardProtocolEvents(
-        protocol_event_observers_.might_have_observers());
+    backend_->RequestBufferedProtocolEventsAndEnableForwarding();
   }
 }
 
 void ProfileSyncService::RemoveProtocolEventObserver(
     browser_sync::ProtocolEventObserver* observer) {
   protocol_event_observers_.RemoveObserver(observer);
-  if (backend_) {
-    backend_->SetForwardProtocolEvents(
-        protocol_event_observers_.might_have_observers());
+  if (backend_ && !protocol_event_observers_.might_have_observers()) {
+    backend_->DisableProtocolEventForwarding();
   }
 }
 

chrome/browser/ui/webui/sync_internals_browsertest.js

@@ -227,6 +227,20 @@ HARD_CODED_ABOUT_INFO = {
   "unrecoverable_error_detected": false
 };
 
+NETWORK_EVENT_DETAILS_1 = {
+  "details":"Notified types: Bookmarks, Autofill",
+  "proto":{},
+  "time":1395874542192.407,
+  "type":"Normal GetUpdate request"
+};
+
+NETWORK_EVENT_DETAILS_2 = {
+  "details":"Received error: SYNC_AUTH_ERROR",
+  "proto":{},
+  "time":1395874542192.837,
+  "type":"GetUpdates Response"
+};
+
 TEST_F('SyncInternalsWebUITest', 'Uninitialized', function() {
   assertNotEquals(null, chrome.sync.aboutInfo);
   expectTrue(this.hasInDetails(true, 'Username', ''));
@@ -246,6 +260,22 @@ TEST_F('SyncInternalsWebUITest', 'LoadPastedAboutInfo', function() {
   expectTrue(this.hasInDetails(true, 'Summary', 'Sync service initialized'));
 });
 
+TEST_F('SyncInternalsWebUITest', 'NetworkEventsTest', function() {
+  networkEvent1 = new Event('onProtocolEvent');
+  networkEvent1.details = NETWORK_EVENT_DETAILS_1;
+  networkEvent2 = new Event('onProtocolEvent');
+  networkEvent2.details = NETWORK_EVENT_DETAILS_2;
+
+  chrome.sync.events.dispatchEvent(networkEvent1);
+  chrome.sync.events.dispatchEvent(networkEvent2);
+
+  expectEquals(2, $('traffic-event-container').children.length);
+
+  // Test that repeated events are not re-displayed.
+  chrome.sync.events.dispatchEvent(networkEvent1);
+  expectEquals(2, $('traffic-event-container').children.length);
+});
+
 TEST_F('SyncInternalsWebUITest', 'SearchTabDoesntChangeOnItemSelect',
        function() {
   // Select the search tab.

chrome/browser/ui/webui/sync_internals_message_handler.cc

@@ -41,14 +41,10 @@ SyncInternalsMessageHandler::~SyncInternalsMessageHandler() {
 void SyncInternalsMessageHandler::RegisterMessages() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  // Register for ProfileSyncService events.
-  ProfileSyncService* service = GetProfileSyncService();
-  if (service) {
-    service->AddObserver(this);
-    service->AddProtocolEventObserver(this);
-    js_controller_ = service->GetJsController();
-    js_controller_->AddJsEventHandler(this);
-  }
+  web_ui()->RegisterMessageCallback(
+      "registerForEvents",
+      base::Bind(&SyncInternalsMessageHandler::HandleRegisterForEvents,
+                 base::Unretained(this)));
 
   web_ui()->RegisterMessageCallback(
       "requestUpdatedAboutInfo",
@@ -64,6 +60,19 @@ void SyncInternalsMessageHandler::RegisterMessages() {
   RegisterJsControllerCallback("getClientServerTraffic");
 }
 
+void SyncInternalsMessageHandler::HandleRegisterForEvents(
+    const base::ListValue* args) {
+  DCHECK(args->empty());
+
+  ProfileSyncService* service = GetProfileSyncService();
+  if (service) {
+    service->AddObserver(this);
+    service->AddProtocolEventObserver(this);
+    js_controller_ = service->GetJsController();
+    js_controller_->AddJsEventHandler(this);
+  }
+}
+
 void SyncInternalsMessageHandler::HandleRequestUpdatedAboutInfo(
     const base::ListValue* args) {
   DCHECK(args->empty());

chrome/browser/ui/webui/sync_internals_message_handler.h

@@ -37,6 +37,9 @@ class SyncInternalsMessageHandler
   // Forwards requests to the JsController.
   void ForwardToJsController(const std::string& name, const base::ListValue*);
 
+  // Sets up observers to receive events and forward them to the UI.
+  void HandleRegisterForEvents(const base::ListValue* args);
+
   // Fires an event to send updated info back to the page.
   void HandleRequestUpdatedAboutInfo(const base::ListValue* args);