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..0bdde0717f8d7d018fbcc32f02308fac76dff822 100644
--- a/chrome/browser/resources/sync_internals/chrome_sync.js
+++ b/chrome/browser/resources/sync_internals/chrome_sync.js
@@ -2,180 +2,100 @@
 // 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.
+    // 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);
-  };
+  var syncFunctions = [
+    // Sync service functions.
+    'getAboutInfo',
 
-  // 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);
-  };
+    // Notification functions.  See chrome/browser/sync/engine/syncapi.h
+    // for docs.
+    'getNotificationState',
+    'getNotificationInfo',
 
-  return fn;
-}
+    // Get a static list of available data types.
+    'getListOfTypes',
 
-var syncFunctions = [
-  // Sync service functions.
-  'getAboutInfo',
+    // Client server communication logging functions.
+    'getClientServerTraffic',
 
-  // Notification functions.  See chrome/browser/sync/engine/syncapi.h
-  // for docs.
-  'getNotificationState',
-  'getNotificationInfo',
+    // Get an array containing a JSON representations of all known sync nodes.
+    'getAllNodes',
+  ];
 
-  // Get a static list of available data types.
-  'getListOfTypes',
+  for (var i = 0; i < syncFunctions.length; ++i) {
+    var syncFunction = syncFunctions[i];
+    chrome.sync[syncFunction] = makeSyncFunction(syncFunction);
+  }
 
-  // Client server communication logging functions.
-  'getClientServerTraffic',
+  /**
+   * A simple timer to measure elapsed time.
+   * @constructor
+   */
+  function Timer() {
+    /**
+     * The time that this Timer was created.
+     * @type {number}
+     * @private
+     * @const
+     */
+    this.start_ = Date.now();
+  }
 
-  // Get an array containing a JSON representations of all known sync nodes.
-  'getAllNodes',
-];
+  /**
+   * @return {number} The elapsed seconds since this Timer was created.
+   */
+  Timer.prototype.getElapsedSeconds = function() {
+    return (Date.now() - this.start_) / 1000;
+  }

[Dan Beam, 2014/02/12 03:27:33]

nit: whenever you say

  var blah = function() {};

you're using a function expression.  this is similar to:

  var blah = 1;

if you write this as:

  var blah = 1

in most cases it'll work.  however, it relies on some pretty wonky rules.  for
instance:

  return {
    makeTimer: makeTimer,
  };

works as you'd expect whereas:

  return
  {
    makeTimer: makeTimer,
  };

will throw no errors but silently return undefined as it's interpreted more as:

  return;
  // useless code ignored

additionally, a function specific bug I've seen before:

  var a = function(arg) {
    return {doSomething: function(){ alert(1); }};
  }
  (new Class).doSomething();

will immediately invoke the function with (new Class) as |arg| and then call
#doSomething on the result.

function *declarations* like |Timer| don't need a ; because they're slightly
different than function *expressions*.

tl;dr - please add a ; whenever doing "= function(){...};"

 
-for (var i = 0; i < syncFunctions.length; ++i) {
-  var syncFunction = syncFunctions[i];
-  chrome.sync[syncFunction] = makeSyncFunction(syncFunction);
-}
+  /** @return {!Timer} An object which measures elapsed time. */
+  var makeTimer = function() {
+    return new Timer;
+  }

[Dan Beam, 2014/02/12 03:27:33]

^ and here

 
-/**
- * Returns an object which measures elapsed time.
- */
-chrome.sync.makeTimer = function() {
-  var start = new Date();
+  /**
+   * @param {string} name The name of the event type.
+   * @param {!Object} details A collection of event-specific details.
+   */
+  var dispatchEvent = function(name, details) {
+    var e = new Event(name);
+    e.details = details;
+    chrome.sync.events.dispatchEvent(e);
+  };
 
   return {
-    /**
-     * @return {number} The number of seconds since the timer was
-     * created.
-     */
-    get elapsedSeconds() {
-      return ((new Date()).getTime() - start.getTime()) / 1000.0;
-    }
+    makeTimer: makeTimer,
+    dispatchEvent: dispatchEvent,
+    events: new cr.EventTarget(),
   };
-};
-
-})();
+});