Implement base.IPC

Implement base.IPC

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 CL base.IPC that provides helper functions to invoke methods on
different pages using chrome.runtime.sendMessage.

BUG=452317
Committed: https://crrev.com/6f290c21326a7586347efcf796a9eebf794fc901
Cr-Commit-Position: refs/heads/master@{#313614}

[kelvinp, 2015-01-27 01:18:34]

kelvinp@chromium.org changed reviewers:
+ jamiewalch@chromium.org

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

FYI: Jamie, this is iteration I that with feedback ported from the previous CL.
I will send out iteration II with my comments on your feedback.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc.js
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:34: * This will invoke foo() with the args [arg1,
arg2].
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.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:49: base.IPC = function() {
Jamie: I suggest base.Ipc as a name instead.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:78: base.IPC.Request_ = function(methodName,
params) {
Jamie: If this is a private type, does it need to be exposed on
base.RemoteMethods?

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:101: * @param {string} methodName
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.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:143: * @return A Promise that would resolves to
the return value of the handler or
Jamie: @return?

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:150: [null, new base.IPC.Request_(methodName,
params)]);
Jamie: I don't think you need the leading null, since the docs say the
extensionId is

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:156: return Promise.reject(response.error);
Jamie: Don't these reject/resolves need to be returned?

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:165: var instance_ = null;
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:29:43]

Patchset #2 (id:20001) has been deleted

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

PTAL

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc.js
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:34: * This will invoke foo() with the args [arg1,
arg2].
On 2015/01/27 01:27:22, kelvinp wrote:
> 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.

Done.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:49: base.IPC = function() {
On 2015/01/27 01:27:21, kelvinp wrote:
> Jamie: I suggest base.Ipc as a name instead.

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

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:78: base.IPC.Request_ = function(methodName,
params) {
On 2015/01/27 01:27:21, kelvinp wrote:
> Jamie: If this is a private type, does it need to be exposed on
> base.RemoteMethods?

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

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:86: * @param {Function} handler The handler can
be invoked by calling
Jamie: s/Function/function(*):void/

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:86: * @param {Function} handler The handler can
be invoked by calling
I think function(*):void is a bit misleading as it conveys that the handler
cannot return anything.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:101: * @param {string} methodName
On 2015/01/27 01:27:22, kelvinp wrote:
> 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.

Done.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:139: * Invokes a method on a remote page
Jamie: Blank (comment) line after method description.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:139: * Invokes a method on a remote page
Done.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:143: * @return A Promise that would resolves to
the return value of the handler or
On 2015/01/27 01:27:21, kelvinp wrote:
> Jamie: @return?

Done.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:150: [null, new base.IPC.Request_(methodName,
params)]);
On 2015/01/27 01:27:21, kelvinp wrote:
> Jamie: I don't think you need the leading null, since the docs say the
> extensionId is

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.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:156: return Promise.reject(response.error);
On 2015/01/27 01:27:22, kelvinp wrote:
> Jamie: Don't these reject/resolves need to be returned?

Good catch.  Done.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:165: var instance_ = null;
On 2015/01/27 01:27:22, kelvinp wrote:
> 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.

Done.

[Jamie, 2015-01-27 22:52:25]

Mostly looks good, with the exception of one of the tests that looks a bit
suspect. Maybe I'm just reading it wrong though.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc.js
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:49: base.IPC = function() {
On 2015/01/27 01:34:59, kelvinp wrote:
> On 2015/01/27 01:27:21, kelvinp wrote:
> > Jamie: I suggest base.Ipc as a name instead.
> 
> I use base.IPC instead as IPC is an abbreviation. Let me know if we have
naming
> rules for naming abbreviations.

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.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:69: 'base.IPC only accept incoming requests from
the same extension.'
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.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:78: base.IPC.Request_ = function(methodName,
params) {
On 2015/01/27 01:34:59, kelvinp wrote:
> On 2015/01/27 01:27:21, kelvinp wrote:
> > Jamie: If this is a private type, does it need to be exposed on
> > base.RemoteMethods?
> 
> JSCompile cannot recognize the type unless I put it in a public namespace.

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?

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:86: * @param {Function} handler The handler can
be invoked by calling
On 2015/01/27 01:34:59, kelvinp wrote:
> I think function(*):void is a bit misleading as it conveys that the handler
> cannot return anything.

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).

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:150: [null, new base.IPC.Request_(methodName,
params)]);
On 2015/01/27 01:34:59, kelvinp wrote:
> On 2015/01/27 01:27:21, kelvinp wrote:
> > Jamie: I don't think you need the leading null, since the docs say the
> > extensionId is
> 
> 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.

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?

https://codereview.chromium.org/877993002/diff/40001/remoting/webapp/base/js/...
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/40001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:34: * This will invoke foo() with the arg1, arg2
and ... .
"with arg1, arg2, ...."

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:47: * @constructor
Maybe declare this @private?

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:93: console.error('service :' + methodName + ' is
already registered.');
Nit: No colon needed.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:113: sendResponse) {
Indentation.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:120: sendResponse({error :
base.IPC.Error.INVALID_REQUEST_ORIGIN});
Nit: No space before colon, here and below.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/chrome_mocks.js (right):

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/chrome_mocks.js:54: if (!extensionId) {
This should not be conditional, I don't think. When calling mock$fire, the
extension id should be that of the sender, whereas when calling sendMessage it's
the id of the receiver. As I noted earlier, I think you should assert here that
extensionId is null, since I think that's the only value that makes sense for
testing (when testing communication with an external extension, you'd probably
need to write a custom mock anyway).

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/chrome_mocks.js:59:
chromeMocks.runtime.onMessage.mock$fire(message, {id: extensionId},
Maybe convert |message| to JSON and back, since that better matches what the
real API does. For example, it would let you test code that either modifies
|message| before it is processed by the recipient or tries to include
non-serializable fields.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/ipc_unittest.js (right):

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/ipc_unittest.js:47: var argJson = {
s/argJson/argDict/?

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/ipc_unittest.js:94: chrome.runtime.id =
'some-other-extension';
I don't understand how this manipulation of chrome.runtime.id works. It feels
back-to-front: you're setting it to "our-extension", before sending invoking,
and expecting it to fail. I would expect to see something more like:

  var oldId = chrome.runtime.id;
  chrome.runtime.id = 'foreign-extension-id';
  invoke(...);
  chrome.runtime.id = oldId;

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/ipc_unittest.js:114: var returnValue = {};
You should probably include some values of various types to make sure that
they're marshalled correctly.

[kelvinp, 2015-01-28 01:28:35]

Patchset #4 (id:80001) has been deleted

[kelvinp, 2015-01-28 01:31:52]

PTAL

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc.js
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:49: base.IPC = function() {
On 2015/01/27 22:52:24, Jamie wrote:
> On 2015/01/27 01:34:59, kelvinp wrote:
> > On 2015/01/27 01:27:21, kelvinp wrote:
> > > Jamie: I suggest base.Ipc as a name instead.
> > 
> > I use base.IPC instead as IPC is an abbreviation. Let me know if we have
> naming
> > rules for naming abbreviations.
> 
> 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.

Done.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:69: 'base.IPC only accept incoming requests from
the same extension.'
On 2015/01/27 22:52:24, Jamie wrote:
> 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.

The are only logged in the console.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:78: base.IPC.Request_ = function(methodName,
params) {
On 2015/01/27 22:52:24, Jamie wrote:
> On 2015/01/27 01:34:59, kelvinp wrote:
> > On 2015/01/27 01:27:21, kelvinp wrote:
> > > Jamie: If this is a private type, does it need to be exposed on
> > > base.RemoteMethods?
> > 
> > JSCompile cannot recognize the type unless I put it in a public namespace.
> 
> 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?

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.

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:86: * @param {Function} handler The handler can
be invoked by calling
On 2015/01/27 22:52:24, Jamie wrote:
> On 2015/01/27 01:34:59, kelvinp wrote:
> > I think function(*):void is a bit misleading as it conveys that the handler
> > cannot return anything.
> 
> 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).

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

https://codereview.chromium.org/877993002/diff/1/remoting/webapp/base/js/ipc....
remoting/webapp/base/js/ipc.js:150: [null, new base.IPC.Request_(methodName,
params)]);
On 2015/01/27 22:52:24, Jamie wrote:
> On 2015/01/27 01:34:59, kelvinp wrote:
> > On 2015/01/27 01:27:21, kelvinp wrote:
> > > Jamie: I don't think you need the leading null, since the docs say the
> > > extensionId is
> > 
> > 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.
> 
> 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?

Done.

https://codereview.chromium.org/877993002/diff/40001/remoting/webapp/base/js/...
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/40001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:34: * This will invoke foo() with the arg1, arg2
and ... .
On 2015/01/27 22:52:24, Jamie wrote:
> "with arg1, arg2, ...."

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
File remoting/webapp/base/js/ipc.js (right):

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:47: * @constructor
On 2015/01/27 22:52:24, Jamie wrote:
> Maybe declare this @private?

Done. JSCompile doesn't throw any error if you try to create it in another file
though.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:93: console.error('service :' + methodName + ' is
already registered.');
On 2015/01/27 22:52:24, Jamie wrote:
> Nit: No colon needed.

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:113: sendResponse) {
On 2015/01/27 22:52:25, Jamie wrote:
> Indentation.

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/ipc.js:120: sendResponse({error :
base.IPC.Error.INVALID_REQUEST_ORIGIN});
On 2015/01/27 22:52:25, Jamie wrote:
> Nit: No space before colon, here and below.

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/chrome_mocks.js (right):

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/chrome_mocks.js:54: if (!extensionId) {
On 2015/01/27 22:52:25, Jamie wrote:
> This should not be conditional, I don't think. When calling mock$fire, the
> extension id should be that of the sender, whereas when calling sendMessage
it's
> the id of the receiver. As I noted earlier, I think you should assert here
that
> extensionId is null, since I think that's the only value that makes sense for
> testing (when testing communication with an external extension, you'd probably
> need to write a custom mock anyway).

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/chrome_mocks.js:59:
chromeMocks.runtime.onMessage.mock$fire(message, {id: extensionId},
On 2015/01/27 22:52:25, Jamie wrote:
> Maybe convert |message| to JSON and back, since that better matches what the
> real API does. For example, it would let you test code that either modifies
> |message| before it is processed by the recipient or tries to include
> non-serializable fields.

Good idea :) Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/ipc_unittest.js (right):

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/ipc_unittest.js:47: var argJson = {
On 2015/01/27 22:52:25, Jamie wrote:
> s/argJson/argDict/?

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/ipc_unittest.js:94: chrome.runtime.id =
'some-other-extension';
On 2015/01/27 22:52:25, Jamie wrote:
> I don't understand how this manipulation of chrome.runtime.id works. It feels
> back-to-front: you're setting it to "our-extension", before sending invoking,
> and expecting it to fail. I would expect to see something more like:
> 
>   var oldId = chrome.runtime.id;
>   chrome.runtime.id = 'foreign-extension-id';
>   invoke(...);
>   chrome.runtime.id = oldId;

Done.

https://codereview.chromium.org/877993002/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/ipc_unittest.js:114: var returnValue = {};
On 2015/01/27 22:52:25, Jamie wrote:
> You should probably include some values of various types to make sure that
> they're marshalled correctly.

Done.

[Jamie, 2015-01-28 18:57:47]

LGTM as long as my question about deepCopy doesn't raise any concerns.

https://codereview.chromium.org/877993002/diff/100001/remoting/webapp/unittes...
File remoting/webapp/unittests/chrome_mocks.js (right):

https://codereview.chromium.org/877993002/diff/100001/remoting/webapp/unittes...
remoting/webapp/unittests/chrome_mocks.js:59: var message_copy =
deepCopy(message);
Where is deepCopy defined? I don't see it in trunk or in this CL, and I get an
error trying to call it from the current v2 app, so I don't think it's
pre-defined either.

[kelvinp, 2015-01-28 22:19:18]

Patchset #5 (id:120001) has been deleted

[kelvinp, 2015-01-28 22:19:25]

FYI

[kelvinp, 2015-01-28 22:19:32]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-01-28 22:20:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877993002/140001

[commit-bot: I haz the power, 2015-01-28 23:30:07]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-01-28 23:31:11]

Patchset 5 (id:??) landed as
https://crrev.com/6f290c21326a7586347efcf796a9eebf794fc901
Cr-Commit-Position: refs/heads/master@{#313614}