Implement base.IPC

remoting/webapp/base/js/ipc.js

Index: remoting/webapp/base/js/ipc.js
diff --git a/remoting/webapp/base/js/ipc.js b/remoting/webapp/base/js/ipc.js
new file mode 100644
index 0000000000000000000000000000000000000000..003b513a2e1a77b628fd4e7ff67102813d76f639
--- /dev/null
+++ b/remoting/webapp/base/js/ipc.js
@@ -0,0 +1,182 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+* @fileoverview
+*
+* In Chrome Apps, some platform APIs can only be called from the background
+* page (e.g. reloading a chrome.app.AppWindow).  Likewise, some chrome API's
+* must be initiated by user interaction, which can only be called from the
+* foreground.
+*
+* This class provides helper functions to invoke methods on different pages
+* using chrome.runtime.sendMessage.  Messages are passed in the following
+* format:
+*     {methodName:{string}, params:{Array}}
+*
+* chrome.runtime.sendMessage allows multiple handlers to be registered on a
+* document, but only one handler can send a response.
+* This class uniquely identifies a method with the |methodName| and enforces
+* that only one handler can be registered per |methodName| in the document.
+*
+* For example, to call method foo() in the background page from the foreground
+* chrome.app.AppWindow, you can do the following.
+* In the background page:
+*     base.IPC.getInstance().register('my.service.name', foo);
+*
+* In the AppWindow document:
+*     base.IPC.invoke('my.service.name', [arg1, arg2]).then(
+*       function(result) {
+*         console.log('The result is ' + result);
+*     });
+*
+* This will invoke foo() with the args [arg1, arg2].

[kelvinp, 2015/01/27 01:27:22]

Jamie: Would it be a lot of work to allow args to be passed without wrapping
them in an
array? It seems a bit more Javascripty.

[kelvinp, 2015/01/27 01:35:00]

Done.

+* The return value of foo() will be passed back to the caller in the
+* form of a promise.
+*/
+
+/** @suppress {duplicate} */
+var base = base || {};
+
+(function() {
+
+'use strict';
+
+/**
+ * @constructor
+ */
+base.IPC = function() {

[kelvinp, 2015/01/27 01:27:21]

Jamie: I suggest base.Ipc as a name instead.

[kelvinp, 2015/01/27 01:34:59]

I use base.IPC instead as IPC is an abbreviation. Let me know if we have naming
rules for naming abbreviations.

[Jamie, 2015/01/27 22:52:24]

I tend to treat them as if they weren't abbreviations (see the various Wcs*
classes for example). I find CamelCase names much easier to parse if that
convention is adopted, and although that's less of an issue where (since IPC is
the entire name, not just a part of it), consistency is always good.

[kelvinp, 2015/01/28 01:31:51]

Done.

+  base.debug.assert(instance_ === null);
+  /**
+   * @type {!Object.<Function>}
+   * @private
+   */
+  this.handlers_ = {};
+  this.onMessageHandler_ = this.onMessage_.bind(this);
+  chrome.runtime.onMessage.addListener(this.onMessageHandler_);
+};
+
+/** @private */
+base.IPC.prototype.dispose_ = function() {
+  chrome.runtime.onMessage.removeListener(this.onMessageHandler_);
+};
+
+/** @enum {string} */
+base.IPC.Error = {
+  UNSUPPORTED_REQUEST_TYPE: 'Unsupported method name.',
+  INVALID_REQUEST_ORIGIN:
+      'base.IPC only accept incoming requests from the same extension.'

[Jamie, 2015/01/27 22:52:24]

Are these string ever displayed in the UI? If so then they should be localized.
If they're only ever logged via the console, then this is fine.

[kelvinp, 2015/01/28 01:31:51]

The are only logged in the console.

+};
+
+/**
+ * @constructor
+ * @param {string} methodName
+ * @param {?Array} params
+ * @struct
+ */
+base.IPC.Request_ = function(methodName, params) {

[kelvinp, 2015/01/27 01:27:21]

Jamie: If this is a private type, does it need to be exposed on
base.RemoteMethods?

[kelvinp, 2015/01/27 01:34:59]

JSCompile cannot recognize the type unless I put it in a public namespace.

[Jamie, 2015/01/27 22:52:24]

Okay, in that case please annotate it as @private with a comment. However, I
thought the latest jscompile was fine with locally-defined types? ISTR we've
used them recently as enums; does it not work for classes?

[kelvinp, 2015/01/28 01:31:51]

JSCompile can understand classes that is attached to a namespace declared inside
a IEFE (immediate execute function expression) but unfortunately it can't handle
anonymous classes.

+  this.methodName = methodName;
+  this.params = params;
+};
+
+
+/**
+ * @param {string} methodName
+ * @param {Function} handler The handler can be invoked by calling

[kelvinp, 2015/01/27 01:34:59]

Jamie: s/Function/function(*):void/

[kelvinp, 2015/01/27 01:34:59]

I think function(*):void is a bit misleading as it conveys that the handler
cannot return anything.

[Jamie, 2015/01/27 22:52:24]

How about function(*) then? I don't think we use Function with a capital F
anywhere else (in fact, I didn't realize it was valid).

[kelvinp, 2015/01/28 01:31:51]

Done. Function is the JavaScript type name for all function
i.e. var foo = function() {}; 
     foo instanceof Function would return true

+ *   base.IPC.invoke(|methodName|, arg1, arg2, ...)
+ * Async handlers that return promises are currently not supported.
+ * @return {boolean} Whether the handler is successfully registered.
+ */
+base.IPC.prototype.register = function(methodName, handler) {
+  if (methodName in this.handlers_) {
+    console.error('service :' + methodName + ' is already registered.');
+    return false;
+  }
+  this.handlers_[methodName] = handler;
+  return true;
+};
+
+/**
+ * @param {string} methodName

[kelvinp, 2015/01/27 01:27:22]

Jamie: Since we're talking about methods, I think methodName would be a more
suitable
parameter name. A brief description for the first two parameters would also be
useful, particularly explaining how the method parameters are passed to the
handler.

[kelvinp, 2015/01/27 01:35:00]

Done.

+ */
+base.IPC.prototype.unregister = function(methodName) {
+  delete this.handlers_[methodName];
+};
+
+/**
+ * @param {base.IPC.Request_} message
+ * @param {chrome.runtime.MessageSender} sender
+ * @param {function(*): void} sendResponse
+ */
+base.IPC.prototype.onMessage_ = function(message, sender,
+                                                   sendResponse) {
+  var methodName = message.methodName;
+  if (typeof methodName !== 'string') {
+    return;
+  }
+
+  if (sender.id !== chrome.runtime.id) {
+    sendResponse({error : base.IPC.Error.INVALID_REQUEST_ORIGIN});
+    return;
+  }
+
+  var remoteMethod =
+      /** @type {function(*):void} */ (this.handlers_[methodName]);
+  if (!remoteMethod) {
+    sendResponse({error : base.IPC.Error.UNSUPPORTED_REQUEST_TYPE});
+    return;
+  }
+
+  try {
+    sendResponse(remoteMethod.apply(null, message.params));
+  } catch (/** @type {Error} */ e) {
+    sendResponse({error: e.message});
+  }
+};
+
+/**
+ * Invokes a method on a remote page

[kelvinp, 2015/01/27 01:34:59]

Done.

[kelvinp, 2015/01/27 01:34:59]

Jamie: Blank (comment) line after method description.

+ *
+ * @param {string} methodName
+ * @param {...} var_args
+ * @return A Promise that would resolves to the return value of the handler or

[kelvinp, 2015/01/27 01:27:21]

Jamie: @return?

[kelvinp, 2015/01/27 01:34:59]

Done.

+ *   rejects if the handler throws an exception.
+ */
+base.IPC.invoke = function(methodName, var_args) {
+  var params = Array.prototype.slice.call(arguments, 1);
+  var sendMessage = base.Promise.as(
+      chrome.runtime.sendMessage,
+      [null, new base.IPC.Request_(methodName, params)]);

[kelvinp, 2015/01/27 01:27:21]

Jamie: I don't think you need the leading null, since the docs say the
extensionId is

[kelvinp, 2015/01/27 01:34:59]

Good point.  The leading null is there to reduce the mocking effort.  Otherwise,
i would need to do some fancy type check to figure out whether the first
argument is the extension or the message itself.

[Jamie, 2015/01/27 22:52:24]

Fair enough, but I worry about the robustness of the mock if all code has to
pass this leading null. Is there a way we can make a mock fail-safe if it's run
against code that forgets to do that? I think we only pass non-null in the
Gnubby and Feedback code, neither of which have tests, so it might be worthwhile
asserting that the first parameter is null in the mock. That way, if the mock is
ever used by code that doesn't include the leading null, the test will fail and
the fix will be obvious. WDYT?

[kelvinp, 2015/01/28 01:31:51]

Done.

+
+  return sendMessage.then(
+    /** @param {?{error: Error}} response */
+    function(response) {
+      if (response && response.error) {
+        return Promise.reject(response.error);

[kelvinp, 2015/01/27 01:27:22]

Jamie: Don't these reject/resolves need to be returned?

[kelvinp, 2015/01/27 01:34:59]

Good catch.  Done.

+      } else {
+        return Promise.resolve(response);
+      }
+  });
+};
+
+
+/** @type {base.IPC} */
+var instance_ = null;

[kelvinp, 2015/01/27 01:27:22]

Jamie: Although we have too many globals in our code, I think this is a good
example of
a class that should be a singleton, since the duplicate-checking in register
needs to be done globally. I suggest you create an instance here.

[kelvinp, 2015/01/27 01:34:59]

Done.

+
+/** @return {base.IPC} */
+base.IPC.getInstance = function() {
+  if (!instance_) {
+    instance_ = new base.IPC();
+  }
+  return instance_;
+};
+
+base.IPC.deleteInstance = function() {
+  if (instance_) {
+    instance_.dispose_();
+    instance_ = null;
+  }
+};
+
+})();