Hangout remote desktop part II - background.html and AppLauncher

Hangout remote desktop part II - background.html and AppLauncher

This CL:
- Moves background.js to background.html
- Introduces remoting.appLauncher that allows the caller to launch and close the webapp without knowing the implementation difference between a v1 app and a v2 app.

NOTRY=true
R=jamiewalch@chromium.org

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

[kelvinp, 2014-08-11 18:37:57]

PTAL

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/js_proto...
File remoting/webapp/js_proto/chrome_proto.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/js_proto...
remoting/webapp/js_proto/chrome_proto.js:19: this.listeners_.push(callback);
I provide a simple implementation of chrome.events so that they can be used in
unit-test, as this file is also loaded to provide stubs in unit-test.  If you
think they are better off living in a helper file, I can move them there.

[Jamie, 2014-08-12 02:24:59]

I think this needs to split into smaller CLs. I suggest one CL moving
background.js -> background.html (I'm almost ready to sign off on that one); one
CL introducing the AppLauncher class (could be combined with the first CL if you
like); one introducing the IT2Me service and helper channel classes and unit
tests (this is the meat of the CL, so I'd strongly advise separating it out);
and finally one CL integrating them with the existing web-app.

https://codereview.chromium.org/450383003/diff/60001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/remoting.gyp#ne...
remoting/remoting.gyp:168: 'webapp/background/background.js',
Shouldn't the new files under background/ also be in this list?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:9: * implementation difference
between launching a v1 app and a v2 app.
Can you clean up this comment? Specifically, I don't know what "app launching
closing implementations" means, and I think you mean "allows... without needing
to know", not "nows... without knowing".

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:22: * @return {Promise}
The methods of this interface need descriptions, or perhaps some pseudo-code in
the fileoverview might be more appropriate, showing how they are typically
called.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:22: * @return {Promise}
Make it clear that when the Promise resolves, it will provide the caller with
the tab id.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:24:
remoting.AppLauncher.prototype.launch = function(opt_launchArgs) { };
Nit: No space between braces.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:37: remoting.AppLauncher.urlJoin =
function(url, opt_params) {
Interfaces should not have concrete methods, and in any case, this doesn't feel
like it belongs on an AppLauncher class. It should be moved to a separate
utility suite.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:63: chrome.tabs.create({ url: url,
selected: true},
Nit: No space after '{' (or add one after 'true').

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:67: reject(chrome.runtime.lastError);
Is it better to use new Error to get a stack trace, or does that only work with
the test frameworks?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:84:
reject(chrome.runtime.lastError.message);
Why .message here but not in launch?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:86: chrome.tabs.remove(tab.id,
function(){
Do you need the wrapper function?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:101:
remoting.V2AppLauncher.nextWindowId_ = 0;
@type and @private annotations, please.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:115: 'id': "hangout" +
remoting.V2AppLauncher.nextWindowId_++
I think the id would be better managed by the caller so that this class can be
completely agnostic about its environment (ie, not specific to Hangouts). Come
to think of it, why is an explicit id needed?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/background.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:8: (function(){
We don't use this construct elsewhere, and IIRC, it doesn't work well with
JSCompile.

Speaking of JSCompile, the background page has enough complexity that I think it
needs to be run through it. The right way to do that is probably to modify the
gyp rule that runs jscompile to operate at the level of an html page to verify
that not only does our Javascript compile, we're not missing any <script> tags.
That doesn't need to be in this CL, but I think it's a high priority work item.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:35: title:
chrome.i18n.getMessage(/*i18n-content*/'NEW_WINDOW')
Please verify that moving this string to a new file doesn't introduce any build
warnings.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:41:
chrome.app.runtime.onLaunched.addListener(appLauncher.launch);
Does appLauncher.launch need a bind()?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:46: * requests from the hangout page
and the webapp.
s/hangout/Hangouts/ (branding)

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

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:8: * It2MeHelperChannel relay
messages between the hangout web page (hangout) and
s/relay/relays/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:8: * It2MeHelperChannel relay
messages between the hangout web page (hangout) and
s/hangout/Hangouts/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:9: * the chrome remote
desktop (webapp) for the helper (The person who is giving
Chrome Remote Desktop (capitalization).

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:9: * the chrome remote
desktop (webapp) for the helper (The person who is giving
s/The/the/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:10: * remoting assistance).
s/remoting/remote/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:13: * between the background
script and the webapp/hangout.
I think this whole paragraph can be more succinctly summarised by:

"It runs in the background page and contains two chrome.runtime.Port objects,
respresenting connections to the webapp and hangout, respectively."

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:15: * Connection is always
initiated from Hangout.
s/Hangout/the hangout/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:19: *    |------connect
message------->|---------launch----> Chrome remote desktop
Even though CRD is not running until this step, I think it would be clearer to
move it up to the same header line as the hangout and helper channel.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:28: *    c)
It2MeHelperChannel- window.close() -----------> Webapp
Can these diagrams be made to fit the layout above? In other words, hangout on
the left, channel in the middle and CRD on the right?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:44: *
Nit: Unnecessary blank line.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:54: * @param
{chrome.runtime.Port} hangoutPort
Please add a description for this parameter.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:56: *     Callback to notify
when the
Formatting.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:57: *     connection is torn
down.  IT2MeServices uses this callback to dispose the
s/dispose/dispose of/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:57: *     connection is torn
down.  IT2MeServices uses this callback to dispose the
s/Services/Service/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:65: * @private
Nit: @private should come after the type (for consistency with the rest of the
code-base).

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:86: this.instanceId_ = "";
Nit: Single-quotes for strings.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:98:
this.onDisconnectCallback_ = onDisconnectCallback;
I think it's clearer to save all the ctor parameters together at the top of the
ctor, then initialize any other members.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:101:
remoting.It2MeHelperChannel.HangoutMessageTypes = {
Add @enum annotation?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:110:
remoting.It2MeHelperChannel.prototype.init = function() {
Please add some doc comments to this and other methods.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:161: // session
crbug.com/402137.
It's not clear what this comment is trying to say. I suggest something like:

TODO(kelvinp): Closing the v2 app current doesn't disconnect the IT2me session
(crbug.com/402137), so send an explicit notification to the hangout.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:177: throw new
Error('AccessCode is missing');
s/Access/access/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:196: // Runtime port do not
fire disconnect events synchronously so
s/Runtime port do/chrome.runtime.Port does/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:197: // appLauncher_.close
won't invoke onWebappDiscconect
s/Discconect/Disconnect/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:197: // appLauncher_.close
won't invoke onWebappDiscconect
I don't understand what this comment is trying to tell me. Will
onWebappDisconnect never be called? Do we need the call to unhookPorts_ as a
consequence?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:208:
base.debug.assert(Boolean(this.hangoutPort_));
I think "this.hangoutPort_ != null" would be clearer.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:238: ' incoming message
method:=' + message.method);
Can this be removed? Console spam makes it harder to diagnose problems.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:248: throw new Error('Unknown
messages.');
s/messages/message/

Also, include message.method in the error.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:257: 
Nit: Unnecessary blank line.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_service.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:7: * It2MeService listens for
incoming connections request from the hangout
s/connections request/connection requests/

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:45: HELPER_WEBAPP:
'it2me.helper.webapp'
Are these names global? If so, then they should probably something com.google.*
or org.chromium.*

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:65: * @param {{method:string,
data:Object.<string,*>}} message
Nit: Blank line between function comment and type annotations.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:78: }
I don't think this helper actually helps much, since you only call it twice;
once with only a value and once with only an error. Also, do you need a separate
error field on every message, as opposed to a dedicated "error" message?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:103: switch (port.name) {
Presumably port.name can be spoofed. Can/should we have a more robust check? For
example, the origin of the other end?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:148:
remoting.It2MeService.prototype.onHelperChannelDisconnected = function (helper){
Nit: Space before '{'

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:148:
remoting.It2MeService.prototype.onHelperChannelDisconnected = function (helper){
Is this a public method? If so, it should be moved above the private methods. If
not, it should end with an underscore.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:162: console.error('An existing
helpee connection is in process.');
This error should probably be more prominent. Is anything displayed to the user
in this case?

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/js_proto...
File remoting/webapp/js_proto/chrome_proto.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/js_proto...
remoting/webapp/js_proto/chrome_proto.js:19: this.listeners_.push(callback);
On 2014/08/11 18:37:56, kelvinp wrote:
> I provide a simple implementation of chrome.events so that they can be used in
> unit-test, as this file is also loaded to provide stubs in unit-test.  If you
> think they are better off living in a helper file, I can move them there.

Yes, please do that. Files under js_proto/ should only be used to help the
compile with type-checking. Using it for stubs in unit tests is not ideal, but
does at least mean we're not duplicating stub definitions. However, we shouldn't
be providing actual functionality here.

[kelvinp, 2014-08-12 18:22:44]

PTAL. 

I have kept background.js and appLauncher and some chrome_proto changes in the
current file.  I will move the rest of the change to a different CL. Thank you
for reviewing.

https://codereview.chromium.org/450383003/diff/60001/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/remoting.gyp#ne...
remoting/remoting.gyp:168: 'webapp/background/background.js',
On 2014/08/12 02:24:56, Jamie wrote:
> Shouldn't the new files under background/ also be in this list?

The action verifyResources.py basically scans through all the files in sources
to look for references to resource strings and make sure those strings are
present in remoting_strings.grd.  So only files that references
chrome.i18n.getMessage should be in the list.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:9: * implementation difference
between launching a v1 app and a v2 app.
On 2014/08/12 02:24:56, Jamie wrote:
> Can you clean up this comment? Specifically, I don't know what "app launching
> closing implementations" means, and I think you mean "allows... without
needing
> to know", not "nows... without knowing".

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:22: * @return {Promise}
On 2014/08/12 02:24:56, Jamie wrote:
> Make it clear that when the Promise resolves, it will provide the caller with
> the tab id.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:24:
remoting.AppLauncher.prototype.launch = function(opt_launchArgs) { };
On 2014/08/12 02:24:56, Jamie wrote:
> Nit: No space between braces.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:37: remoting.AppLauncher.urlJoin =
function(url, opt_params) {
On 2014/08/12 02:24:56, Jamie wrote:
> Interfaces should not have concrete methods, and in any case, this doesn't
feel
> like it belongs on an AppLauncher class. It should be moved to a separate
> utility suite.

Moved to base.js

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:63: chrome.tabs.create({ url: url,
selected: true},
On 2014/08/12 02:24:56, Jamie wrote:
> Nit: No space after '{' (or add one after 'true').

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:84:
reject(chrome.runtime.lastError.message);
On 2014/08/12 02:24:56, Jamie wrote:
> Why .message here but not in launch?

Good catch. Typo in launch

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:86: chrome.tabs.remove(tab.id,
function(){
On 2014/08/12 02:24:56, Jamie wrote:
> Do you need the wrapper function?

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/app_launcher.js:115: 'id': "hangout" +
remoting.V2AppLauncher.nextWindowId_++
On 2014/08/12 02:24:56, Jamie wrote:
> I think the id would be better managed by the caller so that this class can be
> completely agnostic about its environment (ie, not specific to Hangouts). Come
> to think of it, why is an explicit id needed?

You are right.  Prefix removed.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/background.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:8: (function(){
On 2014/08/12 02:24:57, Jamie wrote:
> We don't use this construct elsewhere, and IIRC, it doesn't work well with
> JSCompile.
> 
> Speaking of JSCompile, the background page has enough complexity that I think
it
> needs to be run through it. The right way to do that is probably to modify the
> gyp rule that runs jscompile to operate at the level of an html page to verify
> that not only does our Javascript compile, we're not missing any <script>
tags.
> That doesn't need to be in this CL, but I think it's a high priority work
item.

Before this CL, JSCompile is not run on background.js, but now it does!

This construct known as IIFE (immediately invoked function execution) prevents
us from leaking variables and functions into the global namespace.  Consider the
scenario when two JavaScript files declares a helper function with the same
name, logDebugInfo.  Without wrapping the definitions in the IIFE, the later
definition will override the previous one, which will lead to bugs. 

IIFE is an industrial wide practice.  You will find this practice in every major
JavaScript library (case and point, the polymer project source code! See
https://github.com/Polymer/polymer-dev/blob/master/src/lib/dom.js.)  I would
argue that as long as JSCompile permit, we should use it everywhere in our code
base.

You may wonder.  Does it break JSCompile in this case?  No, JSCompile cannot
extract type information inside an IIFE.   As long as our file, background.js,
does not define new types, but only uses it, we can use this construct.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:35: title:
chrome.i18n.getMessage(/*i18n-content*/'NEW_WINDOW')
On 2014/08/12 02:24:57, Jamie wrote:
> Please verify that moving this string to a new file doesn't introduce any
build
> warnings.

Hence the renaming of the file in remoting.gyp.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:41:
chrome.app.runtime.onLaunched.addListener(appLauncher.launch);
On 2014/08/12 02:24:57, Jamie wrote:
> Does appLauncher.launch need a bind()?

No, AppLauncher.launch does not refer to any local state.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:46: * requests from the hangout page
and the webapp.
On 2014/08/12 02:24:57, Jamie wrote:
> s/hangout/Hangouts/ (branding)

Done.

[Jamie, 2014-08-12 20:42:55]

Thanks for splitting this up. It's very nearly there, but some of my suggested
changes are non-trivial so I'd like to take another look.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/background.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:8: (function(){
On 2014/08/12 18:22:44, kelvinp wrote:
> On 2014/08/12 02:24:57, Jamie wrote:
> > We don't use this construct elsewhere, and IIRC, it doesn't work well with
> > JSCompile.
> > 
> > Speaking of JSCompile, the background page has enough complexity that I
think
> it
> > needs to be run through it. The right way to do that is probably to modify
the
> > gyp rule that runs jscompile to operate at the level of an html page to
verify
> > that not only does our Javascript compile, we're not missing any <script>
> tags.
> > That doesn't need to be in this CL, but I think it's a high priority work
> item.
> 
> Before this CL, JSCompile is not run on background.js, but now it does!
> 
> This construct known as IIFE (immediately invoked function execution) prevents
> us from leaking variables and functions into the global namespace.  Consider
the
> scenario when two JavaScript files declares a helper function with the same
> name, logDebugInfo.  Without wrapping the definitions in the IIFE, the later
> definition will override the previous one, which will lead to bugs. 
> 
> IIFE is an industrial wide practice.  You will find this practice in every
major
> JavaScript library (case and point, the polymer project source code! See
> https://github.com/Polymer/polymer-dev/blob/master/src/lib/dom.js.)  I would
> argue that as long as JSCompile permit, we should use it everywhere in our
code
> base.
> 
> You may wonder.  Does it break JSCompile in this case?  No, JSCompile cannot
> extract type information inside an IIFE.   As long as our file, background.js,
> does not define new types, but only uses it, we can use this construct.

Acknowledged.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/background.js:41:
chrome.app.runtime.onLaunched.addListener(appLauncher.launch);
On 2014/08/12 18:22:44, kelvinp wrote:
> On 2014/08/12 02:24:57, Jamie wrote:
> > Does appLauncher.launch need a bind()?
> 
> No, AppLauncher.launch does not refer to any local state.

The question is whether it's a static function or not. Functions in a classes
prototype are non-static and need to be bound--the fact that the current
implementation doesn't access 'this' is an implementation detail that this
initializeContextMenu can't (and shouldn't) know about.

https://codereview.chromium.org/450383003/diff/140001/remoting/remoting_webap...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/remoting_webap...
remoting/remoting_webapp_files.gypi:175: 'webapp/client_session.js',
We shouldn't be pulling this into the background page. I think it's okay in the
short term, but if there are components of this file that are needed in the
background page, they are good candidates for refactoring.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/backgro...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/backgro...
remoting/webapp/background/app_launcher.js:19: * appLauncher.close(appId);
Nit: Code samples stand out better if they're indented relative to the parent
paragraph.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/html/te...
File remoting/webapp/html/template_background.html (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/html/te...
remoting/webapp/html/template_background.html:14: <script
src='background.js'></script>
Can't the same thing be achieved without order-dependency using onLoad, like we
do for main.html?

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
File remoting/webapp/js_proto/chrome_proto.js (left):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
remoting/webapp/js_proto/chrome_proto.js:238:
chrome.Event.prototype.removeListener = function(callback) {};
This looks like useful clean-up, but does it belong in this CL?

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
File remoting/webapp/js_proto/chrome_proto.js (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
remoting/webapp/js_proto/chrome_proto.js:339: this.id = "";
Single-quotes for strings, please.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/manifes...
File remoting/webapp/manifest.json.jinja2 (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/manifes...
remoting/webapp/manifest.json.jinja2:25: {% endif %}
Is this needed for this part of the CL? I don't think you've added any v1
functionality to the background page yet, right?

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/manifes...
remoting/webapp/manifest.json.jinja2:68: },
As above, I think this probably belongs in one of the follow-up CLs.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/unittes...
File remoting/webapp/unittests/base_unittest.js (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/unittes...
remoting/webapp/unittests/base_unittest.js:69: foo: 'foo='
Can you add a few more escape-requiring characters here, just to be on the safe
side?

[kelvinp, 2014-08-12 21:42:43]

PTAL

I also updated comments for the 2nd part of the CL. Please ignore those for now.

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

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:9: * the chrome remote
desktop (webapp) for the helper (The person who is giving
On 2014/08/12 02:24:58, Jamie wrote:
> Chrome Remote Desktop (capitalization).

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:9: * the chrome remote
desktop (webapp) for the helper (The person who is giving
On 2014/08/12 02:24:58, Jamie wrote:
> s/The/the/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:10: * remoting assistance).
On 2014/08/12 02:24:58, Jamie wrote:
> s/remoting/remote/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:15: * Connection is always
initiated from Hangout.
On 2014/08/12 02:24:58, Jamie wrote:
> s/Hangout/the hangout/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:19: *    |------connect
message------->|---------launch----> Chrome remote desktop
On 2014/08/12 02:24:58, Jamie wrote:
> Even though CRD is not running until this step, I think it would be clearer to
> move it up to the same header line as the hangout and helper channel.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:28: *    c)
It2MeHelperChannel- window.close() -----------> Webapp
On 2014/08/12 02:24:58, Jamie wrote:
> Can these diagrams be made to fit the layout above? In other words, hangout on
> the left, channel in the middle and CRD on the right?

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:44: *
On 2014/08/12 02:24:57, Jamie wrote:
> Nit: Unnecessary blank line.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:54: * @param
{chrome.runtime.Port} hangoutPort
On 2014/08/12 02:24:57, Jamie wrote:
> Please add a description for this parameter.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:56: *     Callback to notify
when the
On 2014/08/12 02:24:58, Jamie wrote:
> Formatting.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:57: *     connection is torn
down.  IT2MeServices uses this callback to dispose the
On 2014/08/12 02:24:58, Jamie wrote:
> s/dispose/dispose of/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:65: * @private
On 2014/08/12 02:24:57, Jamie wrote:
> Nit: @private should come after the type (for consistency with the rest of the
> code-base).

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:98:
this.onDisconnectCallback_ = onDisconnectCallback;
On 2014/08/12 02:24:57, Jamie wrote:
> I think it's clearer to save all the ctor parameters together at the top of
the
> ctor, then initialize any other members.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:101:
remoting.It2MeHelperChannel.HangoutMessageTypes = {
On 2014/08/12 02:24:58, Jamie wrote:
> Add @enum annotation?

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:161: // session
crbug.com/402137.
On 2014/08/12 02:24:58, Jamie wrote:
> It's not clear what this comment is trying to say. I suggest something like:
> 
> TODO(kelvinp): Closing the v2 app current doesn't disconnect the IT2me session
> (crbug.com/402137), so send an explicit notification to the hangout.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:177: throw new
Error('AccessCode is missing');
On 2014/08/12 02:24:58, Jamie wrote:
> s/Access/access/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:196: // Runtime port do not
fire disconnect events synchronously so
On 2014/08/12 02:24:57, Jamie wrote:
> s/Runtime port do/chrome.runtime.Port does/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:197: // appLauncher_.close
won't invoke onWebappDiscconect
On 2014/08/12 02:24:57, Jamie wrote:
> s/Discconect/Disconnect/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:197: // appLauncher_.close
won't invoke onWebappDiscconect
On 2014/08/12 02:24:58, Jamie wrote:
> I don't understand what this comment is trying to tell me. Will
> onWebappDisconnect never be called? Do we need the call to unhookPorts_ as a
> consequence?
Comments removed

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:208:
base.debug.assert(Boolean(this.hangoutPort_));
On 2014/08/12 02:24:57, Jamie wrote:
> I think "this.hangoutPort_ != null" would be clearer.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:238: ' incoming message
method:=' + message.method);
On 2014/08/12 02:24:58, Jamie wrote:
> Can this be removed? Console spam makes it harder to diagnose problems.

This will only log to the console of the background page.  I think logging
inter-component messages actually helps debugging.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:248: throw new Error('Unknown
messages.');
On 2014/08/12 02:24:57, Jamie wrote:
> s/messages/message/
> 
> Also, include message.method in the error.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_helper_channel.js:257: 
On 2014/08/12 02:24:57, Jamie wrote:
> Nit: Unnecessary blank line.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
File remoting/webapp/background/it2me_service.js (right):

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:7: * It2MeService listens for
incoming connections request from the hangout
On 2014/08/12 02:24:59, Jamie wrote:
> s/connections request/connection requests/

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:45: HELPER_WEBAPP:
'it2me.helper.webapp'
On 2014/08/12 02:24:59, Jamie wrote:
> Are these names global? If so, then they should probably something
com.google.*
> or org.chromium.*

Those names are only valid within our extension.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:65: * @param {{method:string,
data:Object.<string,*>}} message
On 2014/08/12 02:24:59, Jamie wrote:
> Nit: Blank line between function comment and type annotations.

Acknowledged.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:65: * @param {{method:string,
data:Object.<string,*>}} message
On 2014/08/12 02:24:59, Jamie wrote:
> Nit: Blank line between function comment and type annotations.

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:103: switch (port.name) {
On 2014/08/12 02:24:58, Jamie wrote:
> Presumably port.name can be spoofed. Can/should we have a more robust check?
For
> example, the origin of the other end.

Yes, not every webpage can connect to our list.  Only the apps white-listed in
the manifest can connect.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:148:
remoting.It2MeService.prototype.onHelperChannelDisconnected = function (helper){
On 2014/08/12 02:24:59, Jamie wrote:
> Nit: Space before '{'

Done.

https://codereview.chromium.org/450383003/diff/60001/remoting/webapp/backgrou...
remoting/webapp/background/it2me_service.js:162: console.error('An existing
helpee connection is in process.');
On 2014/08/12 02:24:59, Jamie wrote:
> This error should probably be more prominent. Is anything displayed to the
user
> in this case?

The hangout code should guarantee this should never happens.

https://codereview.chromium.org/450383003/diff/140001/remoting/remoting_webap...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/remoting_webap...
remoting/remoting_webapp_files.gypi:175: 'webapp/client_session.js',
On 2014/08/12 20:42:53, Jamie wrote:
> We shouldn't be pulling this into the background page. I think it's okay in
the
> short term, but if there are components of this file that are needed in the
> background page, they are good candidates for refactoring.

This is because we would like to re-use events from the clientSession, which is
defined in client_session.js.  We can probably pull out those events in its own
file.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/backgro...
File remoting/webapp/background/app_launcher.js (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/backgro...
remoting/webapp/background/app_launcher.js:19: * appLauncher.close(appId);
On 2014/08/12 20:42:54, Jamie wrote:
> Nit: Code samples stand out better if they're indented relative to the parent
> paragraph.

Done.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/html/te...
File remoting/webapp/html/template_background.html (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/html/te...
remoting/webapp/html/template_background.html:14: <script
src='background.js'></script>
On 2014/08/12 20:42:54, Jamie wrote:
> Can't the same thing be achieved without order-dependency using onLoad, like
we
> do for main.html?

Done.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
File remoting/webapp/js_proto/chrome_proto.js (left):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
remoting/webapp/js_proto/chrome_proto.js:238:
chrome.Event.prototype.removeListener = function(callback) {};
On 2014/08/12 20:42:54, Jamie wrote:
> This looks like useful clean-up, but does it belong in this CL?

It doesn't have to be in this CL.  But since I am already changing this file.
Might as well lest I would forget.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
File remoting/webapp/js_proto/chrome_proto.js (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/js_prot...
remoting/webapp/js_proto/chrome_proto.js:339: this.id = "";
On 2014/08/12 20:42:54, Jamie wrote:
> Single-quotes for strings, please.

Done.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/manifes...
File remoting/webapp/manifest.json.jinja2 (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/manifes...
remoting/webapp/manifest.json.jinja2:25: {% endif %}
On 2014/08/12 20:42:54, Jamie wrote:
> Is this needed for this part of the CL? I don't think you've added any v1
> functionality to the background page yet, right?

Done.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/manifes...
remoting/webapp/manifest.json.jinja2:68: },
On 2014/08/12 20:42:55, Jamie wrote:
> As above, I think this probably belongs in one of the follow-up CLs.

Done.

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/unittes...
File remoting/webapp/unittests/base_unittest.js (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/webapp/unittes...
remoting/webapp/unittests/base_unittest.js:69: foo: 'foo='
On 2014/08/12 20:42:55, Jamie wrote:
> Can you add a few more escape-requiring characters here, just to be on the
safe
> side?

Done.

[Jamie, 2014-08-12 21:54:56]

LGTM!

https://codereview.chromium.org/450383003/diff/140001/remoting/remoting_webap...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/450383003/diff/140001/remoting/remoting_webap...
remoting/remoting_webapp_files.gypi:175: 'webapp/client_session.js',
On 2014/08/12 21:42:42, kelvinp wrote:
> On 2014/08/12 20:42:53, Jamie wrote:
> > We shouldn't be pulling this into the background page. I think it's okay in
> the
> > short term, but if there are components of this file that are needed in the
> > background page, they are good candidates for refactoring.
> 
> This is because we would like to re-use events from the clientSession, which
is
> defined in client_session.js.  We can probably pull out those events in its
own
> file.

Acknowledged.

[kelvinp, 2014-08-12 22:09:42]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-08-12 22:22:12]

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

[kelvinp, 2014-08-12 22:33:03]

The CQ bit was unchecked by kelvinp@chromium.org

[kelvinp, 2014-08-12 22:47:22]

Committed patchset #3 manually as 289096 (presubmit successful).