Refactor about:sync's events framework

chrome/browser/resources/sync_internals/chrome_sync.js

Index: chrome/browser/resources/sync_internals/chrome_sync.js
diff --git a/chrome/browser/resources/sync_internals/chrome_sync.js b/chrome/browser/resources/sync_internals/chrome_sync.js
index fbbada0b70969802f079fd9bfd8c9bb6336c833c..b8a57cb5e3612cad130b06e6300d07f722e1a22b 100644
--- a/chrome/browser/resources/sync_internals/chrome_sync.js
+++ b/chrome/browser/resources/sync_internals/chrome_sync.js
@@ -2,180 +2,98 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-var chrome = chrome || {};
-
-// TODO(akalin): Add mocking code for e.g. chrome.send() so that we
-// can test this without rebuilding chrome.
-
-/**
- * Organize sync event listeners and asynchronous requests.
- * This object is one of a kind; its constructor is not public.
- * @type {Object}
- */
-chrome.sync = chrome.sync || {};
-(function() {
-
-// This Event class is a simplified version of the one from
-// event_bindings.js.
-function Event() {
-  this.listeners_ = [];
-}
-
-Event.prototype.addListener = function(listener) {
-  this.listeners_.push(listener);
-};
-
-Event.prototype.removeListener = function(listener) {
-  var i = this.findListener_(listener);
-  if (i == -1) {
-    return;
+// require cr.js
+// require cr/event_target.js
+// requier cr/util.js
+
+cr.define('chrome.sync', function() {
+  'use strict';
+
+  function makeSyncFunction(name) {
+    var callbacks = [];
+
+    // Calls the function, assuming the last argument is a callback to be
+    // called with the return value.
+    var fn = function() {
+      var args = Array.prototype.slice.call(arguments);
+      callbacks.push(args.pop());
+      chrome.send(name, args);
+    };
+
+    // Handle a reply, assuming that messages are processed in FIFO order.

[Dan Beam, 2014/02/11 23:21:48]

^ this is an odd assumption

[rlarocque, 2014/02/12 00:39:55]

I agree.

Somehow we've been able to get away with this assumption for a long time.  But
I'm taking the sync code in a direction where responses will become less
deterministic.  Which is why I need to refactor the about:sync page to depend
more on events, and less on this async callback interface.

All of the makeSyncFunction()-defined functions will be removed soon.

+    // Called by SyncInternalsUI::HandleJsReply().
+    fn.handleReply = function() {
+      var args = Array.prototype.slice.call(arguments);
+      // Remove the callback before we call it since the callback may
+      // throw.
+      var callback = callbacks.shift();
+      callback.apply(null, args);
+    };
+
+    return fn;
   }
-  this.listeners_.splice(i, 1);
-};
-
-Event.prototype.removeListeners = function() {
-  this.listeners_ = [];
-}
-
-Event.prototype.hasListener = function(listener) {
-  return this.findListener_(listener) > -1;
-};
-
-Event.prototype.hasListeners = function(listener) {
-  return this.listeners_.length > 0;
-};
-
-// Returns the index of the given listener, or -1 if not found.
-Event.prototype.findListener_ = function(listener) {
-  for (var i = 0; i < this.listeners_.length; i++) {
-    if (this.listeners_[i] == listener) {
-      return i;
-    }
-  }
-  return -1;
-};
-
-// Fires the event.  Called by the actual event callback.  Any
-// exceptions thrown by a listener are caught and logged.
-Event.prototype.fire = function() {
-  var args = Array.prototype.slice.call(arguments);
-  for (var i = 0; i < this.listeners_.length; i++) {
-    try {
-      this.listeners_[i].apply(null, args);
-    } catch (e) {
-      if (e instanceof Error) {
-        // Non-standard, but useful.
-        console.error(e.stack);
-      } else {
-        console.error(e);
-      }
-    }
-  }
-};
-
-chrome.sync.events = {
-  'service': [
-    'onServiceStateChanged'
-  ],
-
-  // See chrome/browser/sync/engine/syncapi.h for docs.
-  'notifier': [
-    'onNotificationStateChange',
-    'onIncomingNotification'
-  ],
-
-  'manager': [
-    'onChangesApplied',
-    'onChangesComplete',
-    'onSyncCycleCompleted',
-    'onConnectionStatusChange',
-    'onPassphraseRequired',
-    'onPassphraseAccepted',
-    'onInitializationComplete',
-    'onStopSyncingPermanently',
-    'onClearServerDataSucceeded',
-    'onClearServerDataFailed',
-    'onEncryptedTypesChanged',
-    'onEncryptionComplete',
-    'onActionableError',
-  ],
-
-  'transaction': [
-    'onTransactionWrite',
-  ]
-};
-
-for (var eventType in chrome.sync.events) {
-  var events = chrome.sync.events[eventType];
-  for (var i = 0; i < events.length; ++i) {
-    var event = events[i];
-    chrome.sync[event] = new Event();
-  }
-}
-
-function makeSyncFunction(name) {
-  var callbacks = [];
-
-  // Calls the function, assuming the last argument is a callback to be
-  // called with the return value.
-  var fn = function() {
-    var args = Array.prototype.slice.call(arguments);
-    callbacks.push(args.pop());
-    chrome.send(name, args);
-  };
 
-  // Handle a reply, assuming that messages are processed in FIFO order.
-  // Called by SyncInternalsUI::HandleJsReply().
-  fn.handleReply = function() {
-    var args = Array.prototype.slice.call(arguments);
-    // Remove the callback before we call it since the callback may
-    // throw.
-    var callback = callbacks.shift();
-    callback.apply(null, args);
-  };
+  var syncFunctions = [
+    // Sync service functions.
+    'getAboutInfo',
 
-  return fn;
-}
+    // Notification functions.  See chrome/browser/sync/engine/syncapi.h
+    // for docs.
+    'getNotificationState',
+    'getNotificationInfo',
 
-var syncFunctions = [
-  // Sync service functions.
-  'getAboutInfo',
+    // Get a static list of available data types.
+    'getListOfTypes',
 
-  // Notification functions.  See chrome/browser/sync/engine/syncapi.h
-  // for docs.
-  'getNotificationState',
-  'getNotificationInfo',
+    // Client server communication logging functions.
+    'getClientServerTraffic',
 
-  // Get a static list of available data types.
-  'getListOfTypes',
+    // Get an array containing a JSON representations of all known sync nodes.
+    'getAllNodes',
+  ];
 
-  // Client server communication logging functions.
-  'getClientServerTraffic',
+  for (var i = 0; i < syncFunctions.length; ++i) {
+    var syncFunction = syncFunctions[i];
+    chrome.sync[syncFunction] = makeSyncFunction(syncFunction);
+  }
 
-  // Get an array containing a JSON representations of all known sync nodes.
-  'getAllNodes',
-];
+  /**
+   * Returns an object which measures elapsed time.
+   */
+  chrome.sync.makeTimer = function() {
+    var start = new Date();

[Dan Beam, 2014/02/11 23:21:48]

var start = Date.now();

[rlarocque, 2014/02/12 00:39:55]

Done.

+
+    return {
+      /**
+       * @return {number} The number of seconds since the timer was
+       * created.
+       */
+      get elapsedSeconds() {
+        return ((new Date()).getTime() - start.getTime()) / 1000.0;

[Dan Beam, 2014/02/11 23:21:48]

FYI: all numbers in JavaScript are doubles

return (Date.now() - start) / 1000;

[rlarocque, 2014/02/12 00:39:55]

Done.

+      }
+    };
+  };
 
-for (var i = 0; i < syncFunctions.length; ++i) {
-  var syncFunction = syncFunctions[i];
-  chrome.sync[syncFunction] = makeSyncFunction(syncFunction);
-}
+  /**
+   * @param {string} name The name of the event type.
+   * @param {dictionary} details A collection of event-specific details.

[Dan Beam, 2014/02/11 23:21:48]

  @param {Object} details ...

or

  @param {!Object} details ...

if |details| can't be null

[rlarocque, 2014/02/12 00:39:55]

The C++ code currently guarantees that it will contain at least an empty
dictionary.  I'm not sure if that's better or worse than returning null for that
case.  

Changed to {!Object}.

+   */
+  var handleEvent = function(name, details) {

[Dan Beam, 2014/02/11 23:21:48]

nit: s/handleEvent/dispatchEvent/

[rlarocque, 2014/02/12 00:39:55]

Done.

+    var e = new Event(name);
+    e.details = details;
+    chrome.sync.events.dispatchEvent(e);
+  }

[Dan Beam, 2014/02/11 23:21:48]

};

[rlarocque, 2014/02/12 00:39:55]

Done.

 
-/**
- * Returns an object which measures elapsed time.
- */
-chrome.sync.makeTimer = function() {
-  var start = new Date();
+  /**
+   * Asks the browser to refresh our snapshot of sync state.  Should result
+   * in an onServiceStateChanged event being emitted.
+   */
+  var getAboutInfo = function() {
+    chrome.send('getAboutInfo');
+  }

[Dan Beam, 2014/02/11 23:21:48]

^ where is this actually used?

[rlarocque, 2014/02/12 00:39:55]

That's left over from a bad rebase.  Removed.

 
   return {
-    /**
-     * @return {number} The number of seconds since the timer was
-     * created.
-     */
-    get elapsedSeconds() {
-      return ((new Date()).getTime() - start.getTime()) / 1000.0;
-    }
+    handleEvent: handleEvent,
+    events: new cr.EventTarget(),
   };
-};
-
-})();
+});