[Chromoting] Create core Application interface for CRD.

[Chromoting] Create core Application interface for CRD.

This adds the a base Application class and defines a few methods
(onConnected, onDisconnected, onError, ...) that remove CRD-specific code
from the base connection code and move it into a CRD-specific file.
app-specific functionality.

BUG=

Committed: https://crrev.com/ccb1b8637c2c16ea42f347f21e61d687cc984844
Cr-Commit-Position: refs/heads/master@{#307153}

[garykac, 2014-12-03 22:59:47]

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

[garykac, 2014-12-04 22:24:17]

Ping!

[Jamie, 2014-12-04 22:56:58]

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:21: * @return {void} Nothing.
It would help to know a bit more about how these callbacks are used. Some (like
init) are relatively obvious, but a bit of documentation for each would be
useful.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_screen.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:99: remoting.app.onDisconnected();
It shouldn't be necessary to call this twice. I think it makes more sense to
call onDisconnected from ClientSession, or perhaps from SessionConnector.
Calling it both when a disconnect is initiated and in response to the state
change event is (or at least should be) redundant.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:134: remoting.app.onError(errorTag);
It might take a bit more untangling, but I would prefer that showConnectError be
removed entirely, and calls to it replaced by calls to remoting.app.onError. As
it stands, this function doesn't provide any clear value beyond logging the
error (which is probably better done in the callback anyway) and clearing the
access code (which can also be done there; I'm not sure if there are bad
consequences to doing so on any of the other call paths).

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:298: remoting.app.onConnected();
What's the rationale for moving some, but not all of this code into
remoting.app?

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:1285: remoting.app.onHostStarted();
The name of this callback is misleading. If the host wasn't started (ie, running
and connected to Talk) then we wouldn't have been able to connect to it in the
first place. Maybe something like onVideoStarted would be a better name?

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/crd_main.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_main.js:14: remoting.RemoteDesktop = function() {
I'm not a fan of this class name, but I'm not sure what to suggest as an
alternative. Maybe DesktopRemoting (as contrasted with AppRemoting)? Also,
please rename this file, since it should be named after the class it defines.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_main.js:77: // opposed to the case where a connection
never succeeded.
When does this get reverted? If we feel that we should show a different message
for connections that fail early on, then we should be resetting this message in
onDisconnected.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_main.js:108: :
remoting.connector.getConnectionMode();
Would it make sense for the SessionConnector and ClientSession instances to be
members of the Application instance? An Application class is typically used to
scope variables that would otherwise be global, but it seems like that's not
quite the intent here, which makes me wonder if Application is the right name
for the base interface.

[garykac, 2014-12-05 19:55:40]

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:21: * @return {void} Nothing.
On 2014/12/04 22:56:57, Jamie wrote:
> It would help to know a bit more about how these callbacks are used. Some
(like
> init) are relatively obvious, but a bit of documentation for each would be
> useful.

Done.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_screen.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:99: remoting.app.onDisconnected();
On 2014/12/04 22:56:57, Jamie wrote:
> It shouldn't be necessary to call this twice. I think it makes more sense to
> call onDisconnected from ClientSession, or perhaps from SessionConnector.
> Calling it both when a disconnect is initiated and in response to the state
> change event is (or at least should be) redundant.

Done.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:134: remoting.app.onError(errorTag);
On 2014/12/04 22:56:57, Jamie wrote:
> It might take a bit more untangling, but I would prefer that showConnectError
be
> removed entirely, and calls to it replaced by calls to remoting.app.onError.
As
> it stands, this function doesn't provide any clear value beyond logging the
> error (which is probably better done in the callback anyway) and clearing the
> access code (which can also be done there; I'm not sure if there are bad
> consequences to doing so on any of the other call paths).

I originally had that change in a follow-up cl, but I've moved it into this cl.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_screen.js:298: remoting.app.onConnected();
On 2014/12/04 22:56:57, Jamie wrote:
> What's the rationale for moving some, but not all of this code into
> remoting.app?

I'm separating out the shared code from the CRD-specific stuff.

The "if (remoting.pairingRequested)" code could be either place, but it's more
closely associated with CRD so I moved it.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:1285: remoting.app.onHostStarted();
On 2014/12/04 22:56:58, Jamie wrote:
> The name of this callback is misleading. If the host wasn't started (ie,
running
> and connected to Talk) then we wouldn't have been able to connect to it in the
> first place. Maybe something like onVideoStarted would be a better name?

-> onVideoStreamingStarted

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/crd_main.js (right):

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_main.js:14: remoting.RemoteDesktop = function() {
On 2014/12/04 22:56:58, Jamie wrote:
> I'm not a fan of this class name, but I'm not sure what to suggest as an
> alternative. Maybe DesktopRemoting (as contrasted with AppRemoting)? Also,
> please rename this file, since it should be named after the class it defines.

Changed to DesktopRemoting.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_main.js:77: // opposed to the case where a connection
never succeeded.
On 2014/12/04 22:56:58, Jamie wrote:
> When does this get reverted? If we feel that we should show a different
message
> for connections that fail early on, then we should be resetting this message
in
> onDisconnected.

AFAICT, it doesn't get reverted. I've added a TODO.

https://codereview.chromium.org/779613003/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/crd_main.js:108: :
remoting.connector.getConnectionMode();
On 2014/12/04 22:56:58, Jamie wrote:
> Would it make sense for the SessionConnector and ClientSession instances to be
> members of the Application instance? An Application class is typically used to
> scope variables that would otherwise be global, but it seems like that's not
> quite the intent here, which makes me wonder if Application is the right name
> for the base interface.

I was originally doing that in a separate cl, but I've moved the changes into
this one.

[Jamie, 2014-12-06 00:09:24]

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

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:20: * @implements
{remoting.ApplicationDelegate}
Unless you're ever going to want to pass an instance of this class to something
that expects a Delegate, I would remove this; it feels circular. There's no harm
in using the same names without explicitly implementing the interface.

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:61: remoting.clientSession =
clientSession;
Long term, this should also be a member variable, but I suspect that would be a
lot of untangling for this CL. Add a TODO?

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

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/application_delegate.js:22: remoting.ApplicationDelegate
= function() {};
Optional: Make this an inner class of remoting.Application. I don't know if we
do that elsewhere in our JS code, but it's nice to make it explicit what class
this is a delegate for.

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:25: this.app_ = app;
The Application object is only needed to extract the session mode. Assuming
there's no cleaner way of customizing the UI for the two modes (I'm sure we can
do better than having mostly duplicated mark-up for the two "connection closed"
dialogs--they're basically the same--but that's a job for another CL), I would
prefer that the mode be passed in explicitly; it's easier to test and means you
don't need a reference cycle between the app and the delegate.

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector.js (right):

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector.js:102: function(clientContainer, app)
{};
I preferred the previous implementation where onConnected and onError were
passed explicitly. By passing the Application, you're creating a reference cycle
between the app and the connector, which doesn't feel right. Completion/error
callbacks are also very common elsewhere in the JS code-base, so it's more
consistent to express things that way.

[garykac, 2014-12-06 01:39:34]

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

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:20: * @implements
{remoting.ApplicationDelegate}
On 2014/12/06 00:09:23, Jamie wrote:
> Unless you're ever going to want to pass an instance of this class to
something
> that expects a Delegate, I would remove this; it feels circular. There's no
harm
> in using the same names without explicitly implementing the interface.

Done.

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:61: remoting.clientSession =
clientSession;
On 2014/12/06 00:09:23, Jamie wrote:
> Long term, this should also be a member variable, but I suspect that would be
a
> lot of untangling for this CL. Add a TODO?

Added TODO.

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

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/base/js/...
remoting/webapp/base/js/application_delegate.js:22: remoting.ApplicationDelegate
= function() {};
On 2014/12/06 00:09:23, Jamie wrote:
> Optional: Make this an inner class of remoting.Application. I don't know if we
> do that elsewhere in our JS code, but it's nice to make it explicit what class
> this is a delegate for.

Done.

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:25: this.app_ = app;
On 2014/12/06 00:09:23, Jamie wrote:
> The Application object is only needed to extract the session mode. Assuming
> there's no cleaner way of customizing the UI for the two modes (I'm sure we
can
> do better than having mostly duplicated mark-up for the two "connection
closed"
> dialogs--they're basically the same--but that's a job for another CL), I would
> prefer that the mode be passed in explicitly; it's easier to test and means
you
> don't need a reference cycle between the app and the delegate.

We can't trivially pass in the mode because it's state that changes in the
Application. We'd have to update it here every time it changed in Application.

But we should get rid of this ref to Application once we're done merging.
I've added a TODO for that.

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector.js (right):

https://codereview.chromium.org/779613003/diff/60001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector.js:102: function(clientContainer, app)
{};
On 2014/12/06 00:09:24, Jamie wrote:
> I preferred the previous implementation where onConnected and onError were
> passed explicitly. By passing the Application, you're creating a reference
cycle
> between the app and the connector, which doesn't feel right. Completion/error
> callbacks are also very common elsewhere in the JS code-base, so it's more
> consistent to express things that way.

Done.

[Jamie, 2014-12-06 01:58:00]

lgtm

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:117: this.session_connector_ =
remoting.SessionConnector.factory.createConnector(
Optional: You could just do this in the ctor.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application_delegate.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/application_delegate.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
This file is no longer needed.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:1285: // remote host has started.
s/remote host/the video stream/

Also, please move this to the end of the function; if nothing else, it removes
the gap between the previous three declarations and their use.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:7: * This class implements the
functionality that is specific to Remote Desktop.
s/Remote Desktop/Chrome Remote Desktop/?

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector.js:108: // needed to pass to the plugin
that is created.
SessionConnector needs to know about onExtensionMessage so that it can pass it
to the ClientSession. However, it looks like all we do is forward it back to the
ClientSession, which begs the question of why the ClientSession needs a callback
in the first place :s

It doesn't need to be fixed in this CL, but there has to be a simpler design
than this...

[garykac, 2014-12-06 02:15:50]

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:117: this.session_connector_ =
remoting.SessionConnector.factory.createConnector(
On 2014/12/06 01:57:59, Jamie wrote:
> Optional: You could just do this in the ctor.

I don't want to change behavior (by moving when things are init'ed) until a
later cl. I've added a TODO.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application_delegate.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/base/js/...
remoting/webapp/base/js/application_delegate.js:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/12/06 01:57:59, Jamie wrote:
> This file is no longer needed.

Done.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_session.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:1285: // remote host has started.
On 2014/12/06 01:57:59, Jamie wrote:
> s/remote host/the video stream/
> 
> Also, please move this to the end of the function; if nothing else, it removes
> the gap between the previous three declarations and their use.

Done.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_remoting.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_remoting.js:7: * This class implements the
functionality that is specific to Remote Desktop.
On 2014/12/06 01:58:00, Jamie wrote:
> s/Remote Desktop/Chrome Remote Desktop/?

Done.

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector.js (right):

https://codereview.chromium.org/779613003/diff/80001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector.js:108: // needed to pass to the plugin
that is created.
On 2014/12/06 01:58:00, Jamie wrote:
> SessionConnector needs to know about onExtensionMessage so that it can pass it
> to the ClientSession. However, it looks like all we do is forward it back to
the
> ClientSession, which begs the question of why the ClientSession needs a
callback
> in the first place :s
> 
> It doesn't need to be fixed in this CL, but there has to be a simpler design
> than this...

Acknowledged.

[garykac, 2014-12-06 02:15:54]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2014-12-06 02:17:05]

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

[commit-bot: I haz the power, 2014-12-06 02:20:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-06 02:20:18]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/90781)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[garykac, 2014-12-06 02:58:09]

The CQ bit was checked by garykac@chromium.org

[commit-bot: I haz the power, 2014-12-06 02:59:22]

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

[commit-bot: I haz the power, 2014-12-06 03:46:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-06 03:47:45]

Patchset 7 (id:??) landed as
https://crrev.com/ccb1b8637c2c16ea42f347f21e61d687cc984844
Cr-Commit-Position: refs/heads/master@{#307153}