Hangouts remote desktop part III - It2MeService

Hangouts remote desktop part III - It2MeService

This CL
- Introduces an It2MeService component that listens to incoming connection requests between Hangouts and the webapp and establish a channel between them.
- It enables launching an IT2Me helper session from Hangouts

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289538

[kelvinp, 2014-08-13 00:08:48]

PTAL.

I addresses all comments from the previous CL
https://codereview.chromium.org/450383003/.
I have re-posted the unaddressed feedback as comments.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/a...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/a...
remoting/webapp/background/app_launcher.js:64: resolve(String(tab.id));
tab.id is a number.  Casting to string for API consistency.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:235:
console.log('It2MeHelperChannel id:=' + this.instanceId_ +
This statement will only log to the console of background.html.  IMHO, I think
logging inter-component messages makes it easier to debug protocol errors.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_service.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:43:
remoting.It2MeService.ConnectionTypes = {
These names are not global.  They only needs to be unique within our extension.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:108: switch (port.name) {
Only are web-pages that are white-listed in the manifest can connect to us using
connectExternal.  It is hard to spoof.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:168: 'Cannot start a helper session
while a helpee session is in process.');
Not sure what do you mean by making this error prominent.  In theory the hangout
code should safe guard against this scenario.

[Jamie, 2014-08-13 01:44:54]

I haven't looked in detail at the tests yet since they might need to change with
some of my suggested changes. This is mostly looking good though.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/a...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/a...
remoting/webapp/background/app_launcher.js:64: resolve(String(tab.id));
On 2014/08/13 00:08:48, kelvinp wrote:
> tab.id is a number.  Casting to string for API consistency.

If both v1 and v2 represent a window/tab using a number, why not represent it
internally as a number as well?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/b...
File remoting/webapp/background/background.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/b...
remoting/webapp/background/background.js:59: it2meService.dispose();
Reset remoting.it2meService to null? In fact, it might be clearer if this was
something like:

if (remoting.it2meService) {
  remoting.it2meService.dispose();
  remoting.it2meService = null;
}

ie, get rid of the temporary altogether.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:59: *     Represents an
active connection to Hangouts.
For consistency, param descriptions should start on the same line as the @param,
if possible. The indentation for subsequent lines is correct, however.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:97:
this.onDisconnectCallback_ = onDisconnectCallback;
Unless it fits more logically here, I think it would be better to group it with
the other ctor parameters. It should also have a @private tag (if you can get
away without specifying its type, then that's fine).

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:128: * @return {boolean}
whether the message is handled or not.
@private, or no underscore, here and elsewhere.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:159: // TODO(kepoon): Closing
the v2 app current doesn't disconnect the IT2me
Wrong user id :)

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:159: // TODO(kepoon): Closing
the v2 app current doesn't disconnect the IT2me
s/current/currently/

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:160: // session
crbug.com/402137, so send an explicit notification to the hangout.
Parentheses around the bug.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:166:
this.appLauncher_.close(this.instanceId_);
I'm not sure it makes much difference, but if the explicit notification is to
work around the lack of an onUnload handler, shouldn't it be deferred until the
close() promise fulfills?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:171: * @param
{{method:string, data:Object.<string,*>}} message
Blank line between description and annotations.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:190: this.instanceId_ =
String(instanceId);
Do you need String here?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:191: }.bind(this));
Closing brace should be aligned with 'function'.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:195: function() {
No need for line break I don't think.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:196:
this.appLauncher_.close(this.instanceId_);
In the non-error case, this is going to result in two calls to close(). Since
the interface doesn't mandate that it be idempotent, that could be a problem.

It's probably sufficient to add a comment explaining that this is benign, but I
wonder if there's a cleaner solution?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:202: * @param {string} id
I think these parameters would benefit from documentation: what's on the other
end of the port, and what does id identify?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:216: // If the port got
disconnected while the session is still connected,
Please specify which port.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:235:
console.log('It2MeHelperChannel id:=' + this.instanceId_ +
On 2014/08/13 00:08:48, kelvinp wrote:
> This statement will only log to the console of background.html.  IMHO, I think
> logging inter-component messages makes it easier to debug protocol errors.

As long as the protocol isn't too chatty, that's fine, although I'd go with
either a colon or an equals sign, not both.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:246: throw new Error('Unknown
message method:=' + message.method);
s/:=/: /

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_service.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:9: * It supports multiple concurrent
helper sessions.
Add ", but only a single helpee."

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:34: /** @private */
Type?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:43:
remoting.It2MeService.ConnectionTypes = {
On 2014/08/13 00:08:48, kelvinp wrote:
> These names are not global.  They only needs to be unique within our
extension.

Acknowledged.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:72: * @param {Function} sendResponse
s/Function/function/

Ideally, it should have a prototype as well, but looking at the code, it would
be difficult to express in JSDoc.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:78: var isHandled = false;
You're not using this.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:91: var error = /** @type {Error} */
e;
I think you should log this error.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:93: method: message.method +
'Response',
Does the API guarantee that message is an object? If not, then your catch block
is going to throw if it isn't.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:94: error: error.message
Are you guaranteed that error will be an object with a message field? What if
sendResponse throws?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:96: }
return false

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:108: switch (port.name) {
On 2014/08/13 00:08:48, kelvinp wrote:
> Only are web-pages that are white-listed in the manifest can connect to us
using
> connectExternal.  It is hard to spoof.

Acknowledged.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:116: port.disconnect();
Log the error?

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:117: }
return false

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:168: 'Cannot start a helper session
while a helpee session is in process.');
On 2014/08/13 00:08:48, kelvinp wrote:
> Not sure what do you mean by making this error prominent.  In theory the
hangout
> code should safe guard against this scenario.

I think I meant it should be surfaced in the UI somewhere. If you're confident
it can't happen, optionally consider using an assert instead.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/hangout_sess...
File remoting/webapp/hangout_session.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/hangout_sess...
remoting/webapp/hangout_session.js:55: var error = /** @type{Error} */ e;
Nit: Space after @type.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/js_proto/dom...
File remoting/webapp/js_proto/dom_proto.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/js_proto/dom...
remoting/webapp/js_proto/dom_proto.js:188: * @param {function(*=) :
(Promise|void)=} onReject
This doesn't belong in this CL. I'm surprised that JSCompile hasn't complained
about some of our other uses--plenty of them only pass one parameter to then().

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

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:137: * @return {Promise} A promise that will
resolves to the current id of the
s/resolves/resolve/ (or remove "will").

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:137: * @return {Promise} A promise that will
resolves to the current id of the
Remove "current".

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:155: reject();
I think reject should always specify a reason. I found debugging browser tests
very difficult without it, though that may be in part due to the heavy nesting
of Promises involved.

[kelvinp, 2014-08-13 23:44:11]

PTAL

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/a...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/a...
remoting/webapp/background/app_launcher.js:64: resolve(String(tab.id));
On 2014/08/13 01:44:52, Jamie wrote:
> On 2014/08/13 00:08:48, kelvinp wrote:
> > tab.id is a number.  Casting to string for API consistency.
> 
> If both v1 and v2 represent a window/tab using a number, why not represent it
> internally as a number as well?

The window Id returns in v2 is a string.  I have removed the cast on
V2AppLauncher.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/b...
File remoting/webapp/background/background.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/b...
remoting/webapp/background/background.js:59: it2meService.dispose();
On 2014/08/13 01:44:52, Jamie wrote:
> Reset remoting.it2meService to null? In fact, it might be clearer if this was
> something like:
> 
> if (remoting.it2meService) {
>   remoting.it2meService.dispose();
>   remoting.it2meService = null;
> }
> 
> ie, get rid of the temporary altogether.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:59: *     Represents an
active connection to Hangouts.
On 2014/08/13 01:44:52, Jamie wrote:
> For consistency, param descriptions should start on the same line as the
@param,
> if possible. The indentation for subsequent lines is correct, however.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:97:
this.onDisconnectCallback_ = onDisconnectCallback;
On 2014/08/13 01:44:52, Jamie wrote:
> Unless it fits more logically here, I think it would be better to group it
with
> the other ctor parameters. It should also have a @private tag (if you can get
> away without specifying its type, then that's fine).

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:128: * @return {boolean}
whether the message is handled or not.
On 2014/08/13 01:44:52, Jamie wrote:
> @private, or no underscore, here and elsewhere.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:159: // TODO(kepoon): Closing
the v2 app current doesn't disconnect the IT2me
On 2014/08/13 01:44:52, Jamie wrote:
> Wrong user id :)

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:159: // TODO(kepoon): Closing
the v2 app current doesn't disconnect the IT2me
On 2014/08/13 01:44:52, Jamie wrote:
> s/current/currently/

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:160: // session
crbug.com/402137, so send an explicit notification to the hangout.
On 2014/08/13 01:44:53, Jamie wrote:
> Parentheses around the bug.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:166:
this.appLauncher_.close(this.instanceId_);
On 2014/08/13 01:44:52, Jamie wrote:
> I'm not sure it makes much difference, but if the explicit notification is to
> work around the lack of an onUnload handler, shouldn't it be deferred until
the
> close() promise fulfills?

this.appLauncher_.close will disconnect the port and cause the disconnect
callback to fire.  The background page will thought the webapp has crashed.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:171: * @param
{{method:string, data:Object.<string,*>}} message
On 2014/08/13 01:44:52, Jamie wrote:
> Blank line between description and annotations.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:190: this.instanceId_ =
String(instanceId);
On 2014/08/13 01:44:52, Jamie wrote:
> Do you need String here?

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:191: }.bind(this));
On 2014/08/13 01:44:52, Jamie wrote:
> Closing brace should be aligned with 'function'.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:195: function() {
On 2014/08/13 01:44:52, Jamie wrote:
> No need for line break I don't think.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:196:
this.appLauncher_.close(this.instanceId_);
On 2014/08/13 01:44:52, Jamie wrote:
> In the non-error case, this is going to result in two calls to close(). Since
> the interface doesn't mandate that it be idempotent, that could be a problem.
> 
> It's probably sufficient to add a comment explaining that this is benign, but
I
> wonder if there's a cleaner solution?

This won't need to two calls to close.  The disconnected event will only fire if
the port is disconnected on the remote side.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:202: * @param {string} id
On 2014/08/13 01:44:53, Jamie wrote:
> I think these parameters would benefit from documentation: what's on the other
> end of the port, and what does id identify?

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:216: // If the port got
disconnected while the session is still connected,
On 2014/08/13 01:44:52, Jamie wrote:
> Please specify which port.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:246: throw new Error('Unknown
message method:=' + message.method);
On 2014/08/13 01:44:52, Jamie wrote:
> s/:=/: /

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_service.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:34: /** @private */
On 2014/08/13 01:44:53, Jamie wrote:
> Type?

Will be filled on the next CL

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:72: * @param {Function} sendResponse
On 2014/08/13 01:44:53, Jamie wrote:
> s/Function/function/
> 
> Ideally, it should have a prototype as well, but looking at the code, it would
> be difficult to express in JSDoc.

Acknowledged.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:78: var isHandled = false;
On 2014/08/13 01:44:53, Jamie wrote:
> You're not using this.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:93: method: message.method +
'Response',
On 2014/08/13 01:44:53, Jamie wrote:
> Does the API guarantee that message is an object? If not, then your catch
block
> is going to throw if it isn't.

Yes, the messaging API requires the message object to not be null.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:94: error: error.message
On 2014/08/13 01:44:53, Jamie wrote:
> Are you guaranteed that error will be an object with a message field? What if
> sendResponse throws?

Errors thrown by sendResponse will still have a message field.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:96: }
On 2014/08/13 01:44:53, Jamie wrote:
> return false

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:116: port.disconnect();
On 2014/08/13 01:44:53, Jamie wrote:
> Log the error?

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:117: }
On 2014/08/13 01:44:53, Jamie wrote:
> return false

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_service.js:168: 'Cannot start a helper session
while a helpee session is in process.');
On 2014/08/13 01:44:53, Jamie wrote:
> On 2014/08/13 00:08:48, kelvinp wrote:
> > Not sure what do you mean by making this error prominent.  In theory the
> hangout
> > code should safe guard against this scenario.
> 
> I think I meant it should be surfaced in the UI somewhere. If you're confident
> it can't happen, optionally consider using an assert instead.

Since this is called by external code, I think it is wrong to do assert as the
caller can call it with invalid input.  I will follow up with Dan and see if I
need to post a message to Hangouts if that happens.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/hangout_sess...
File remoting/webapp/hangout_session.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/hangout_sess...
remoting/webapp/hangout_session.js:55: var error = /** @type{Error} */ e;
On 2014/08/13 01:44:53, Jamie wrote:
> Nit: Space after @type.

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/js_proto/dom...
File remoting/webapp/js_proto/dom_proto.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/js_proto/dom...
remoting/webapp/js_proto/dom_proto.js:188: * @param {function(*=) :
(Promise|void)=} onReject
On 2014/08/13 01:44:53, Jamie wrote:
> This doesn't belong in this CL. I'm surprised that JSCompile hasn't complained
> about some of our other uses--plenty of them only pass one parameter to
then().

JSCompile will complain without this change.
Most of our uses are in browserTest which is not JSCompile safe.

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

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:137: * @return {Promise} A promise that will
resolves to the current id of the
On 2014/08/13 01:44:54, Jamie wrote:
> Remove "current".

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:137: * @return {Promise} A promise that will
resolves to the current id of the
On 2014/08/13 01:44:53, Jamie wrote:
> s/resolves/resolve/ (or remove "will").

Done.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:155: reject();
On 2014/08/13 01:44:54, Jamie wrote:
> I think reject should always specify a reason. I found debugging browser tests
> very difficult without it, though that may be in part due to the heavy nesting
> of Promises involved.

Done.

[Jamie, 2014-08-14 00:25:07]

LGTM, but if you decide to make the tests asynchronous and it's a significant
change, I'll take another look.

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/468693002/diff/1/remoting/webapp/background/i...
remoting/webapp/background/it2me_helper_channel.js:97:
this.onDisconnectCallback_ = onDisconnectCallback;
On 2014/08/13 23:44:10, kelvinp wrote:
> On 2014/08/13 01:44:52, Jamie wrote:
> > Unless it fits more logically here, I think it would be better to group it
> with
> > the other ctor parameters. It should also have a @private tag (if you can
get
> > away without specifying its type, then that's fine).
> 
> Done.

You haven't moved it to be with the other parameters? Was that an oversight, or
do you think it makes more sense to put it here? I generally copy the ctor
parameters to member variables at the top of the ctor, in the order they're
declared.

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_helper_channel.js (right):

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:212: * @param
{chrome.runtime.Port} port The port represents a connection to the
I think you can get rid of "port represents a" for something that will fit on
one line.

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

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/remoting...
remoting/webapp/remoting.js:155: reject('Cannot retrieve the current tab.');
new Error?

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/unittest...
File remoting/webapp/unittests/it2me_helper_channel_unittest.js (right):

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:20: return
promiseResolveSynchronous('tabId');
Why does this need to be synchronous? Don't we run the risk of not testing that
the code works in the asynchronous case?

Assuming it is generally useful, would this be worth adding it to base.js, or is
it strictly test-only?

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:20: return
promiseResolveSynchronous('tabId');
'tabId' is used twice; if it's important that the same value is used, I think it
would be clearer to create a named constant, kTabId.

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_helper_channel_unittest.js:122: });
Can you also assert that it's not called with state FAILED?

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/unittest...
File remoting/webapp/unittests/it2me_service_unittest.js (right):

https://codereview.chromium.org/468693002/diff/40001/remoting/webapp/unittest...
remoting/webapp/unittests/it2me_service_unittest.js:105: // verify that hangout
can receive message events from webapp 1
Nit: Move this comment (and the next one) up one block so that they're
commenting the entire post...verify block, otherwise it looks like the first
comment applies to the second sessionStateChanged event, because there's no
intervening comment.

[kelvinp, 2014-08-14 00:38:00]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2014-08-14 00:40:24]

The CQ bit was unchecked by kelvinp@chromium.org

[kelvinp, 2014-08-14 00:59:10]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-08-14 01:01:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/468693002/40001

[kelvinp, 2014-08-14 04:47:30]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-08-14 04:51:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/468693002/60001

[commit-bot: I haz the power, 2014-08-14 12:42:37]

Committed patchset #3 (60001) as 289538