[Chromoting] Move ownership of ClientPlugin into SessionConnector.

[Chromoting] Move ownership of ClientPlugin into SessionConnector.

With this cl, ClientSession and DesktopConnectedView are now created (and owned) by the SessionConnector.
SessionConnector creates the ClientPlugin and passes it to the CS and DCW.

BUG=

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

[garykac, 2015-03-06 00:43:04]

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

[garykac, 2015-03-06 00:45:05]

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

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:127: remoting.clientSession =
clientSession;
This global is managed in SessionConnector.

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_session_connector.js (left):

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_session_connector.js:152:
clientSession.createPluginAndConnect(this.onExtensionMessage_);
This mock is not being used and will be removed soon.

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

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:62: this.uiHandler_ = new
remoting.DesktopConnectedView(
This is created by the sessionconnector

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:273: if (error != remoting.Error.NONE)
{
This error is handled in sessionconnector

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:298:
remoting.ClientSession.prototype.resetWithError_ = function(error) {
This has been replaced with sessionconnector.pluginError.

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

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:411:
remoting.desktopConnectedView.updateClientSessionUi_(this);
Use global (ugh!) to remove dependency on DesktopConnectedView.
This is temp until we can remove the global entirely.

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_connected_view.js (left):

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_connected_view.js:167: this.plugin_ =
remoting.ClientPlugin.factory.createPlugin(
Sessionconnector creates this now.

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_connected_view.js:268: this.plugin_.dispose();
SessionConnector disposes of the plugin when appropriate.

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector_impl.js (left):

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:291: if (this.clientSession_) {
No longer needed - resetConnection does this.

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/981083002/diff/20001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:471:
remoting.SessionConnectorImpl.prototype.onPluginInitialized_ = function(
Most of this came here from DesktopConnectedView

[kelvinp, 2015-03-06 02:58:38]

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (left):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:20: remoting.clientSession = null;
Please also moves it to sessionConnector, if you are goign to initialize the
session there.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:252: remoting.clientSession.dispose();
Previously, this will actually remove the plugin. With this change, it is a
no-op.  We should call removePlugin on the sessionConnector instead.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_session.js (left):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:313: this.uiHandler_.removePlugin();
I think after all, it may still make sense for the clientSession to own the
plugin, due to life time reason.

I think here are the lifetime of the objects
|<----------- Application ---------------->|
|<---------- SessionConnector ------------>|
     |<-----   ClientPlugin  ---------->|
     |<-----   ClientSession ---------->|
        |<--DesktopConnectedView->|

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

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:44: remoting.ClientSession =
function(host, signalStrategy, credentialsProvider,
I think we should create the clientSession only after the plugin is initialized
and passed it (the plugin) in the client session constructor.  In this way, we
don't have to worry about remove the plugin in SessionConnector if plugin
initialization failed.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:266: * Deletes the <embed> element from
the container, without sending a
This comment needs to be updated.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_connected_view.js:37:
remoting.DesktopConnectedView = function(session, container, host, mode,
I think we should create the DesktopConnectedView only after the plugin is
initialized
and passed it (the plugin) in the client session constructor.  In this way, we
don't have to worry about remove the plugin in SessionConnector if plugin
initialization failed.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:116: * second connection. Note
the none of the shared WCS state is reset.
@private

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:445: this.connectedView_ = new
remoting.DesktopConnectedView(
This should be created when the session is connected as the name suggested
unless if it cause problems.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:471:
remoting.SessionConnectorImpl.prototype.onPluginInitialized_ = function(
This should be private.  Annotate and underscore

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:485:
this.connectedView_.onPluginInitialized(this.plugin_);
As mentioned, consider creating the DesktopConnectedView and the ClientSession
here.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:490:
remoting.SessionConnectorImpl.prototype.pluginError = function(error) {
This should be private.  Annotate and underscore

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:496:
remoting.SessionConnectorImpl.prototype.removePlugin = function() {
This should be private.  Annotate and underscore

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:505:
this.clientSession_.dispose();
dispose() is currently poorly named (my apology) and will send an XHR to the
host to issue a disconnect.

For this CL, let's not call dispose here to be consistent with the existing
behavior.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:514:
remoting.SessionConnectorImpl.prototype.resetConnection = function() {
This should be private.  Annotate and underscore

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:519: this.pendingXhr_ = null;
This is already cleared in reset.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:520: this.credentialsProvider_
= null;
This is already cleared in reset.  Do we need to call reset_ here?

[garykac, 2015-03-06 23:38:34]

ptal

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/base/js/...
File remoting/webapp/base/js/application.js (left):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/base/js/...
remoting/webapp/base/js/application.js:20: remoting.clientSession = null;
On 2015/03/06 02:58:37, kelvinp wrote:
> Please also moves it to sessionConnector, if you are goign to initialize the
> session there.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
File remoting/webapp/crd/js/client_session.js (left):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:313: this.uiHandler_.removePlugin();
On 2015/03/06 02:58:37, kelvinp wrote:
> I think after all, it may still make sense for the clientSession to own the
> plugin, due to life time reason.
> 
> I think here are the lifetime of the objects
> |<----------- Application ---------------->|
> |<---------- SessionConnector ------------>|
>      |<-----   ClientPlugin  ---------->|
>      |<-----   ClientSession ---------->|
>         |<--DesktopConnectedView->|

ClientPlugin and ClientSession have similar lifetimes, but the plugin needs to
be shared with the UX code (DesktopConnectedView) and we want to move all the UX
code out of here (into the app-specific code).

Moving the ClientPlugin up into SessionConnector makes it slightly easier to
share the plugin with the app-specific code.

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

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:266: * Deletes the <embed> element from
the container, without sending a
On 2015/03/06 02:58:37, kelvinp wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/d...
File remoting/webapp/crd/js/desktop_connected_view.js (right):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/d...
remoting/webapp/crd/js/desktop_connected_view.js:37:
remoting.DesktopConnectedView = function(session, container, host, mode,
On 2015/03/06 02:58:37, kelvinp wrote:
> I think we should create the DesktopConnectedView only after the plugin is
> initialized
> and passed it (the plugin) in the client session constructor.  In this way, we
> don't have to worry about remove the plugin in SessionConnector if plugin
> initialization failed.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
File remoting/webapp/crd/js/session_connector_impl.js (right):

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:116: * second connection. Note
the none of the shared WCS state is reset.
On 2015/03/06 02:58:38, kelvinp wrote:
> @private

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:445: this.connectedView_ = new
remoting.DesktopConnectedView(
On 2015/03/06 02:58:38, kelvinp wrote:
> This should be created when the session is connected as the name suggested
> unless if it cause problems.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:471:
remoting.SessionConnectorImpl.prototype.onPluginInitialized_ = function(
On 2015/03/06 02:58:38, kelvinp wrote:
> This should be private.  Annotate and underscore

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:485:
this.connectedView_.onPluginInitialized(this.plugin_);
On 2015/03/06 02:58:38, kelvinp wrote:
> As mentioned, consider creating the DesktopConnectedView and the ClientSession
> here.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:490:
remoting.SessionConnectorImpl.prototype.pluginError = function(error) {
On 2015/03/06 02:58:38, kelvinp wrote:
> This should be private.  Annotate and underscore

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:496:
remoting.SessionConnectorImpl.prototype.removePlugin = function() {
On 2015/03/06 02:58:37, kelvinp wrote:
> This should be private.  Annotate and underscore

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:505:
this.clientSession_.dispose();
On 2015/03/06 02:58:38, kelvinp wrote:
> dispose() is currently poorly named (my apology) and will send an XHR to the
> host to issue a disconnect.
> 
> For this CL, let's not call dispose here to be consistent with the existing
> behavior.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:514:
remoting.SessionConnectorImpl.prototype.resetConnection = function() {
On 2015/03/06 02:58:38, kelvinp wrote:
> This should be private.  Annotate and underscore

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:519: this.pendingXhr_ = null;
On 2015/03/06 02:58:38, kelvinp wrote:
> This is already cleared in reset.

Done.

https://codereview.chromium.org/981083002/diff/40001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:520: this.credentialsProvider_
= null;
On 2015/03/06 02:58:38, kelvinp wrote:
> This is already cleared in reset.  Do we need to call reset_ here?

Done.

[kelvinp, 2015-03-07 00:09:28]

Mostly look good, just one comment about null checking the clientSession in
removePlugin.

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

https://codereview.chromium.org/981083002/diff/60001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:43: remoting.ClientSession =
function(plugin, host, signalStrategy, mode) {
Awesome, right now the host is only used in the disconnect method.  Once we move
that to sessionConnector (not in this CL).  We can get rid of the host
altogether!

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

https://codereview.chromium.org/981083002/diff/60001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:517:
this.clientSession_.removePlugin();
Probably want to null check clientSession before removing the plugin or it would
crash if the plugin failed to initialize.  Same for the connectedView.

Once you do that, You can probably get rid of reset_ as it is only called in the
constructor and always call resetConnection_.  Or you can rename
resetConnection_ to reset_ and I am ok with either way.

[garykac, 2015-03-07 01:53:10]

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

https://codereview.chromium.org/981083002/diff/60001/remoting/webapp/crd/js/c...
remoting/webapp/crd/js/client_session.js:43: remoting.ClientSession =
function(plugin, host, signalStrategy, mode) {
On 2015/03/07 00:09:28, kelvinp wrote:
> Awesome, right now the host is only used in the disconnect method.  Once we
move
> that to sessionConnector (not in this CL).  We can get rid of the host
> altogether!

Acknowledged.

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

https://codereview.chromium.org/981083002/diff/60001/remoting/webapp/crd/js/s...
remoting/webapp/crd/js/session_connector_impl.js:517:
this.clientSession_.removePlugin();
On 2015/03/07 00:09:28, kelvinp wrote:
> Probably want to null check clientSession before removing the plugin or it
would
> crash if the plugin failed to initialize.  Same for the connectedView.
> 
> Once you do that, You can probably get rid of reset_ as it is only called in
the
> constructor and always call resetConnection_.  Or you can rename
> resetConnection_ to reset_ and I am ok with either way.

Done.

[kelvinp, 2015-03-07 08:19:41]

lgtm

[garykac, 2015-03-09 14:55:48]

The CQ bit was checked by garykac@chromium.org

[garykac, 2015-03-09 14:55:48]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/981083002/#ps100001
(title: "Merge")

[commit-bot: I haz the power, 2015-03-09 14:55:56]

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

[commit-bot: I haz the power, 2015-03-09 15:34:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-09 15:35:24]

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

[Wez, 2015-03-09 17:33:21]

Sorry, only just spotted this.

Please add the # for the bug that describes the rationale for this change, and
update the CL description to explain why SessionConnector should own
ClientPlugin.