Expose ClientSession details to Host Extensions

Add new interface to expose ClientSession details to Host Extensions

Prior to the introduction of HostExtensions, code running in the ClientSession
had access to private state information which it could use to make decisions.
Once this code was moved into a HostExtension, it needed to interact with the
session via the ClientSessionControl interface.  This interface was already
being used for other purposes though so it is too easy to pollute it with
methods/properties which the other consumers of the interface don't care about.

My change adds a new, HostExtension specific interface which wraps the
ClientSessionControl interface.  It also provides a location to add new
methods/properties specifcally for HostExtensions.

This change uses this new interface by adding the session Id property and
exposing it to Host Extensions.  A future CL will update the existing
extensions to ue it.

BUG=591746
Committed: https://crrev.com/4fa09d9b658fe3a79765eaef2fb58f89bbe84cd6
Cr-Commit-Position: refs/heads/master@{#402288}

[Sergey Ulanov, 2016-06-23 22:41:28]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-06-23 22:41:29]

We can just add desktop_session_id() in the ClientSessionControls?
ClientSessionControls interface was added specifically to define what extension
can do with client sessions.

https://codereview.chromium.org/2091553002/diff/20001/remoting/host/host_exte...
File remoting/host/host_extension.h (right):

https://codereview.chromium.org/2091553002/diff/20001/remoting/host/host_exte...
remoting/host/host_extension.h:42: ClientSessionDetails* client_session_details,
Do we really need to have ClientSessionDetails separate from
ClientSessionControl? They are going to point to the same ClientSession object
and I don't see any good reason to separate them.

[joedow, 2016-06-23 23:42:38]

joedow@chromium.org changed reviewers:
+ zijiehe@chromium.org

[joedow, 2016-06-23 23:42:38]

joedow@chromium.org changed required reviewers:
+ sergeyu@chromium.org

[joedow, 2016-06-23 23:51:21]

The question about using ClientSessionControl is totally legit, I went back and
forth a bit on it.

The main reason I chose a new interface was because putting the Windows session
id in ClientSessionControl felt wrong.  ClientSessionControl is used in numerous
different classes in the network and desktop processes and the current methods
on it seem applicable to any of the consumers of it.  It also feels more
action-oriented by name (though it does have one property getter).

Adding a new interface also allows us to provide a set of session specific
information/state to host extensions which the rest of the classes which consume
ClientSessionControl do not require.

[joedow, 2016-06-24 17:15:39]

Discussed with Sergey offline, will incorporate the ClientSessionControl
interface into the ClientSessionDetails interface.  Will send up an update once
that is done.

[joedow, 2016-06-24 18:55:17]

Description was changed from

==========
Expose ClientSession details to Host Extensions

Prior to HostExtensions, code running in the ClientSession has access to
private state information which it could use to make decisions.  With
HostExtensions, this code now has access to some functionality through the
ClientSessionControl interface but everything else is hidden.  I added a simple
interface which can be used for exposing a subset of ClientSession state
information to HostExtensions.  I chose this path as I don't think exposing
all of the ClientSession state makes sense but we still need a way to provide
specific information.

Using this new interface, this CL exposes the current session ID to
HostExtensions, a future CL will update the SecurityKey ExtensionHost to
use this value.

BUG=591746
==========

to

==========
Add new interface to expose ClientSession details to Host Extensions

Prior to the introduction of HostExtensions, code running in the ClientSession
had access to private state information which it could use to make decisions.
Once this code was moved into a HostExtension, it needed to interact with the
session via the ClientSessionControl interface.  This interface was already
being used for other purposes though so it is too easy to pollute it with
methods/properties which the other consumers of the interface don't care about.

My change adds a new, HostExtension specific interface which wraps the
ClientSessionControl interface.  It also provides a location to add new
methods/properties specifcally for HostExtensions.

This change uses this new interface by adding the session Id property and
exposing it to Host Extensions.  A future CL will update the existing
extensions to ue it.

BUG=591746
==========

[joedow, 2016-06-24 20:53:55]

Addressed feedback, PTAL!

Thanks!
Joe

[joedow, 2016-06-27 17:07:14]

Ping!

[Sergey Ulanov, 2016-06-27 19:51:30]

lgtm

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
remoting/host/client_session.cc:435: return desktop_environment_ ?
desktop_environment_->GetDesktopSessionId()
nit: maybe allow this function to be called only when connection is active? I.e.
add DCHECK(desktop_environment_) here?

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
remoting/host/client_session.h:139: // ClientSessionDetails interface.
nit: move this above ClientSessionControl interface

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/security_...
File remoting/host/security_key/gnubby_extension.cc (right):

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/security_...
remoting/host/security_key/gnubby_extension.cc:32: // TODO(joedow): Update
GnubbyExtensionSession for |client_session_details|.
Not sure what this TODO is about. You did update GnubbyExtensionSession  in this
CL.

[joedow, 2016-06-27 21:07:28]

Thanks for the review!!

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
remoting/host/client_session.cc:435: return desktop_environment_ ?
desktop_environment_->GetDesktopSessionId()
On 2016/06/27 19:51:30, Sergey Ulanov wrote:
> nit: maybe allow this function to be called only when connection is active?
I.e.
> add DCHECK(desktop_environment_) here?

Done.

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/client_se...
remoting/host/client_session.h:139: // ClientSessionDetails interface.
On 2016/06/27 19:51:30, Sergey Ulanov wrote:
> nit: move this above ClientSessionControl interface

Done.

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/security_...
File remoting/host/security_key/gnubby_extension.cc (right):

https://codereview.chromium.org/2091553002/diff/60001/remoting/host/security_...
remoting/host/security_key/gnubby_extension.cc:32: // TODO(joedow): Update
GnubbyExtensionSession for |client_session_details|.
On 2016/06/27 19:51:30, Sergey Ulanov wrote:
> Not sure what this TODO is about. You did update GnubbyExtensionSession  in
this
> CL.

Ah, this CL updated GnubbyExtension, the next CL (mentioned in the TODO) updates
GnubbySessionExtension.  It is kind of confusing as the names are really similar
(like many of the Host classes).

[joedow, 2016-06-27 21:07:36]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-06-27 21:07:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2091553002/#ps80001
(title: "Adressing feedback")

[commit-bot: I haz the power, 2016-06-27 21:08:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 21:26:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-27 21:28:04]

Description was changed from

==========
Add new interface to expose ClientSession details to Host Extensions

Prior to the introduction of HostExtensions, code running in the ClientSession
had access to private state information which it could use to make decisions.
Once this code was moved into a HostExtension, it needed to interact with the
session via the ClientSessionControl interface.  This interface was already
being used for other purposes though so it is too easy to pollute it with
methods/properties which the other consumers of the interface don't care about.

My change adds a new, HostExtension specific interface which wraps the
ClientSessionControl interface.  It also provides a location to add new
methods/properties specifcally for HostExtensions.

This change uses this new interface by adding the session Id property and
exposing it to Host Extensions.  A future CL will update the existing
extensions to ue it.

BUG=591746
==========

to

==========
Add new interface to expose ClientSession details to Host Extensions

Prior to the introduction of HostExtensions, code running in the ClientSession
had access to private state information which it could use to make decisions.
Once this code was moved into a HostExtension, it needed to interact with the
session via the ClientSessionControl interface.  This interface was already
being used for other purposes though so it is too easy to pollute it with
methods/properties which the other consumers of the interface don't care about.

My change adds a new, HostExtension specific interface which wraps the
ClientSessionControl interface.  It also provides a location to add new
methods/properties specifcally for HostExtensions.

This change uses this new interface by adding the session Id property and
exposing it to Host Extensions.  A future CL will update the existing
extensions to ue it.

BUG=591746

Committed: https://crrev.com/4fa09d9b658fe3a79765eaef2fb58f89bbe84cd6
Cr-Commit-Position: refs/heads/master@{#402288}
==========

[commit-bot: I haz the power, 2016-06-27 21:28:05]

Patchset 5 (id:??) landed as
https://crrev.com/4fa09d9b658fe3a79765eaef2fb58f89bbe84cd6
Cr-Commit-Position: refs/heads/master@{#402288}