Hangout remote desktop part I - It2Me mode

Hangout remote desktop part I - It2Me mode

This CL enables the user to launch the chrome remote desktop app directly into an immersive it2me helper session.
In the immersive session, the app will
1. hide the home screen
2. create an hrd_helper_session object to communicate with the it2me_background service.

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

[kelvinp, 2014-08-05 01:31:20]

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:542: * @return {boolean} True if the platform is
Mac.
Applying Jamie's change to fix jscompile locally to avoid compiler errors.

[Jamie, 2014-08-05 21:15:14]

https://codereview.chromium.org/439923002/diff/1/remoting/remoting_webapp_fil...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/439923002/diff/1/remoting/remoting_webapp_fil...
remoting/remoting_webapp_files.gypi:50: 'webapp/hrd_helper_session.js',
HRD is not a widely-recognized acronym, and I think the "RD" part is implicit in
the context of Chromoting. I think you can use "hangouts" in place of "hrd"
throughout.

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/client_scree...
remoting/webapp/client_screen.js:93: 'sessionStateChanged',
Can you declare the available session events as an enum, similar to how it's
done for ClientSession? It would eliminate the possibility of a typo.

Come to think of it, why not just fire sessionStateChanged from ClientSession
directly?

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:8: * messages.
To do what? AFAICT, this class does two things:

1. Forwards session state notifications (to where?)
2. Closes the window when the session terminates.

I think that should be made explicit in the fileoverview.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:19: remoting.HRDHelperSession = function()
{
Can you come up with something more descriptive than "Helper"? It's not clear
from the code or comments what this class does.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:20: /** @type {chrome.extension.Port} */
Add @private?

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:22: this.onSessionStateChangedHandler_ =
this.onSessionStateChanged_.bind(this);
You only use this in one place, so there's no need to save it to a member
variable.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:26: this.port_ =
chrome.runtime.connect({name: 'it2me.helper.webapp'});
Are you missing an extension id here? Without one, you're forwarding state
change notifications to yourself, which doesn't seem right.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:41: } catch (e) {
This ignores any errors. Is that what you want? If so, then I think a comment
here explaining why that's safe would be useful.

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_session_...
remoting/webapp/hrd_session_events.js:18: };
I don't think you need this file at all. sessionStateChanged can be fired by
ClientSession, and we already have an client state enum.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/main.css
File remoting/webapp/main.css (right):

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/main.css#new...
remoting/webapp/main.css:787: .hrd .home-screen {
Please use a more descriptive class name. Maybe hangouts-session? Also, I've
conventionally scoped this sort of global class elsewhere (e.g. body.fullscreen,
html.apps-v2); it helps makes the rule more specific and documents that you only
expect this class to apply at top-level.

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/remoting.js#...
remoting/webapp/remoting.js:542: * @return {boolean} True if the platform is
Mac.
On 2014/08/05 01:31:20, kelvinp wrote:
> Applying Jamie's change to fix jscompile locally to avoid compiler errors.

This has now landed on trunk, so you'll need to rebase.

[kelvinp, 2014-08-07 18:03:25]

https://codereview.chromium.org/439923002/diff/1/remoting/remoting_webapp_fil...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/439923002/diff/1/remoting/remoting_webapp_fil...
remoting/remoting_webapp_files.gypi:50: 'webapp/hrd_helper_session.js',
On 2014/08/05 21:15:13, Jamie wrote:
> HRD is not a widely-recognized acronym, and I think the "RD" part is implicit
in
> the context of Chromoting. I think you can use "hangouts" in place of "hrd"
> throughout.

Done.

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/client_scree...
remoting/webapp/client_screen.js:93: 'sessionStateChanged',
On 2014/08/05 21:15:13, Jamie wrote:
> Can you declare the available session events as an enum, similar to how it's
> done for ClientSession? It would eliminate the possibility of a typo.
> 
> Come to think of it, why not just fire sessionStateChanged from ClientSession
> directly?

I can make the event name into a constant.
We can't use clientSession directly as remoting.clientSession does not exist
until the connection is established.  As a result, we would not be able to
detect the connected event and connection failure.

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:8: * messages.
On 2014/08/05 21:15:14, Jamie wrote:
> To do what? AFAICT, this class does two things:
> 
> 1. Forwards session state notifications (to where?)
> 2. Closes the window when the session terminates.
> 
> I think that should be made explicit in the fileoverview.

Done.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:19: remoting.HRDHelperSession = function()
{
On 2014/08/05 21:15:14, Jamie wrote:
> Can you come up with something more descriptive than "Helper"? It's not clear
> from the code or comments what this class does.

Done.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:20: /** @type {chrome.extension.Port} */
On 2014/08/05 21:15:14, Jamie wrote:
> Add @private?

Done.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:22: this.onSessionStateChangedHandler_ =
this.onSessionStateChanged_.bind(this);
On 2014/08/05 21:15:13, Jamie wrote:
> You only use this in one place, so there's no need to save it to a member
> variable.

Done.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:26: this.port_ =
chrome.runtime.connect({name: 'it2me.helper.webapp'});
On 2014/08/05 21:15:13, Jamie wrote:
> Are you missing an extension id here? Without one, you're forwarding state
> change notifications to yourself, which doesn't seem right.

This is intended.  According to the documentation, when the extension id is
omitted.  The message will be broadcasted to all pages (background/other window)
within the app.  This is the recommended way for an app to communicate with its
background page.

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_helper_s...
remoting/webapp/hrd_helper_session.js:41: } catch (e) {
On 2014/08/05 21:15:13, Jamie wrote:
> This ignores any errors. Is that what you want? If so, then I think a comment
> here explaining why that's safe would be useful.

Done.

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

https://codereview.chromium.org/439923002/diff/1/remoting/webapp/hrd_session_...
remoting/webapp/hrd_session_events.js:18: };
On 2014/08/05 21:15:14, Jamie wrote:
> I don't think you need this file at all. sessionStateChanged can be fired by
> ClientSession, and we already have an client state enum.

See comments in hrd_helper_session.js

[kelvinp, 2014-08-07 18:03:50]

PTAL

[Jamie, 2014-08-07 23:12:36]

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/client_s...
File remoting/webapp/client_screen.js (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_screen.js:93: 'sessionStateChanged',
You've missed this and a few other places where this string is still hard-coded.

If you take my advice about re-using ClientSession.State, this becomes a lot
simpler, because you can raise the event outside the switch statement. You'll
fire more events that way, but since the recipient has to check the state
anyway, I don't think that matters.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
File remoting/webapp/hangout_session.js (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
remoting/webapp/hangout_session.js:33: var HangoutSessionEvents =
remoting.HangoutSessionEvents;
No need for this assignment.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
File remoting/webapp/hangout_session_events.js (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
remoting/webapp/hangout_session_events.js:9: remoting.HangoutSessionEvents = new
base.EventSource();
s/Hangout/hangout/ (initial capital is just for classes).

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
remoting/webapp/hangout_session_events.js:22: ERROR: 'ERROR'
We aready have ClientSession.State, which I think can be re-used here. That will
help when writing the recipient as well, since there's a fromString method that
will validate the string.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/main.css...
remoting/webapp/main.css:787: body.hangout-remote-assistance .home-screen {
The product is called Hangouts Remote Desktop, so I think
hangouts-remote-desktop would be a more suitable class name.

[kelvinp, 2014-08-08 01:14:22]

PTAL

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/client_s...
File remoting/webapp/client_screen.js (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/client_s...
remoting/webapp/client_screen.js:93: 'sessionStateChanged',
On 2014/08/07 23:12:35, Jamie wrote:
> You've missed this and a few other places where this string is still
hard-coded.
> 
> If you take my advice about re-using ClientSession.State, this becomes a lot
> simpler, because you can raise the event outside the switch statement. You'll
> fire more events that way, but since the recipient has to check the state
> anyway, I don't think that matters.

Done.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
File remoting/webapp/hangout_session.js (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
remoting/webapp/hangout_session.js:33: var HangoutSessionEvents =
remoting.HangoutSessionEvents;
On 2014/08/07 23:12:35, Jamie wrote:
> No need for this assignment.

Done.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
File remoting/webapp/hangout_session_events.js (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
remoting/webapp/hangout_session_events.js:9: remoting.HangoutSessionEvents = new
base.EventSource();
On 2014/08/07 23:12:36, Jamie wrote:
> s/Hangout/hangout/ (initial capital is just for classes).

Done.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/hangout_...
remoting/webapp/hangout_session_events.js:22: ERROR: 'ERROR'
On 2014/08/07 23:12:36, Jamie wrote:
> We aready have ClientSession.State, which I think can be re-used here. That
will
> help when writing the recipient as well, since there's a fromString method
that
> will validate the string.

Done.

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/main.css
File remoting/webapp/main.css (right):

https://codereview.chromium.org/439923002/diff/20001/remoting/webapp/main.css...
remoting/webapp/main.css:787: body.hangout-remote-assistance .home-screen {
On 2014/08/07 23:12:36, Jamie wrote:
> The product is called Hangouts Remote Desktop, so I think
> hangouts-remote-desktop would be a more suitable class name.

Hangout-remote-assistance is actually the name that we show to the user in
hangouts.  But I think we refer it as HRD within our team.  I will change it to
make the terminology more consistent.

Done.

[Jamie, 2014-08-08 01:24:12]

lgtm

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

https://codereview.chromium.org/439923002/diff/40001/remoting/webapp/hangout_...
remoting/webapp/hangout_session.js:60: 
Three blank lines is overkill :) I suggest two, since there's a substantial
context switch taking place.

[kelvinp, 2014-08-08 18:58:43]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2014-08-08 18:58:50]

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

https://codereview.chromium.org/439923002/diff/40001/remoting/webapp/hangout_...
remoting/webapp/hangout_session.js:60: 
On 2014/08/08 01:24:12, Jamie wrote:
> Three blank lines is overkill :) I suggest two, since there's a substantial
> context switch taking place.

Done.

[commit-bot: I haz the power, 2014-08-08 19:02:17]

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

[commit-bot: I haz the power, 2014-08-08 23:24:44]

Change committed as 288466