Pass the |curtain_activated| flag to IpcDesktopEnvironmentFactory.

Pass the |curtain_activated| flag to IpcDesktopEnvironmentFactory.

In the case of the multi-process host the ability to support curtain mode is a property of the desktop environment representing the screen. This CL hooks up IpcDesktopEnvironmentFactory to receive the request to activate the curtain. This way the factory class will be able to create a different kind of the desktop environment if the curtain was requested.

Other changes:
  - Added remoting_rdp_session% GYP flag that enables RDP integration on Windows.

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

[alexeypa (please no reviews), 2013-03-04 21:18:12]

Renato, please take a look. Jamie, rubber-stamp as needed. Thanks.

[rmsousa, 2013-03-04 21:56:36]

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.cc:102: curtain_activated_ = activated;
This isn't implementing the current curtain mode interface. 

SetActivated isn't a "stateless mode switch", it's a reactive call - it's only
called:
(1) when a new client authenticates;
(2) or when the policy changes to enable curtaining AND someone is already
connected; (kind of a special case)
(very theoretically there's case (3) when a host restarts AND there's someone
already connected, but that's impossible in practice)
And when it's called, it's supposed to do one of two things:
(1) Curtain the console immediately (Mac case); or
(2) Call on_error if this is an uncurtainable desktop (Linux case)

I think that, in this case particularly, the sequence will be that the
DesktopEnvironment is created in the OnIncomingSession event, and THEN the
OnClientAuthenticated event will happen to enable the curtain, i.e. it will be
too late for that desktop environment.

I think what you need to do isn't covered by the current curtain mode interface.
You need to:
Set this mode in the desktop environment factory when the policy changes (in
remoting_me2me_host).
Have SetActivated implementation that immediately rejects (calls on_error) the
session if it's not an RDP session (it somehow needs to have that information).

[alexeypa (please no reviews), 2013-03-04 22:16:03]

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.cc:102: curtain_activated_ = activated;
On 2013/03/04 21:56:36, rmsousa wrote:
> I think that, in this case particularly, the sequence will be that the
> DesktopEnvironment is created in the OnIncomingSession event, and THEN the
> OnClientAuthenticated event will happen to enable the curtain, i.e. it will be
> too late for that desktop environment.

Yes, I know that. I'm preparing a couple of separate CLs that will result in
DesktopEnvironment to be created in
ClientSession::OnConnectionChannelsConnected() - after the curtain was enabled.

I nice catch though!

> You need to:
> Set this mode in the desktop environment factory when the policy changes (in
> remoting_me2me_host).

Agree. I'm trying to keep the changes to curtain mode code to the minimum. There
are way to many other changes in flight right now. It should be relatively
straight-forward refactoring once we have time for it.

> Have SetActivated implementation that immediately rejects (calls on_error) the
> session if it's not an RDP session (it somehow needs to have that
information).

The sequence will be:

1. OnConnectionAuthenticated() causes SetActivated() to be called.
2. OnConnectionChannelsConnected() creates desktop environment, creates event
executor, etc. and frees the desktop environment.
    2.1. This will also cause DesktopSessionProxy to register with the daemon.
3. The daemon will try to attach to a session of requested kind, checking the
curtain flag as well. If it fails it will kill the client session.

[rmsousa, 2013-03-04 23:33:54]

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.cc:102: curtain_activated_ = activated;
> Yes, I know that. I'm preparing a couple of separate CLs that will result in
> DesktopEnvironment to be created in
> ClientSession::OnConnectionChannelsConnected() - after the curtain was
enabled.

Can you add a comment stating something like "This doesn't currently work. TODO:
Have 
DesktopEnvironment be created after the curtain is enabled."
(I would actually prefer to have this go in immediately before or together with
the other changes you mentioned, as this temporarily creates a situation where
the RDP-enabled host doesn't enforce the curtain policy).

> The sequence will be:
> 
> 1. OnConnectionAuthenticated() causes SetActivated() to be called.
> 2. OnConnectionChannelsConnected() creates desktop environment, creates event
> executor, etc. and frees the desktop environment.
>     2.1. This will also cause DesktopSessionProxy to register with the daemon.
> 3. The daemon will try to attach to a session of requested kind, checking the
> curtain flag as well. If it fails it will kill the client session.

This will work (once the other pieces are in), but it's still a bit odd to be
setting/resetting the flag for every connection/disconnection. But it kinda
makes sense if we think about it as activating/deactivating the curtain.

This also won't do anything if the policy is enabled while someone is already
connected to the console (where the other implementations either kick everyone
or turn on the curtain immediately). Currently we restart the host immediately
after that anyway (kicking everyone out in the process), so that's not a
problem, but since the other curtain mode implementations work in this case, I
think we need at least a comment here (or in OnCurtainPolicyUpdated) that RDP
curtaining relies on the host restart, and that we'll need to add code to check
if there are non-RDP sessions and kick everyone out if that assumption ever
changes.

[alexeypa (please no reviews), 2013-03-05 00:19:48]

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/12379084/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.cc:102: curtain_activated_ = activated;
On 2013/03/04 23:33:54, rmsousa wrote:
> Can you add a comment stating something like "This doesn't currently work.
TODO:
> Have 
> DesktopEnvironment be created after the curtain is enabled."

Done.

> (I would actually prefer to have this go in immediately before or together
with
> the other changes you mentioned, as this temporarily creates a situation where
> the RDP-enabled host doesn't enforce the curtain policy).

This should not be a problem because RDP intergation is not enabled unless
remoting_rdp_session% GYP variable is set. It is currently disabled by default.

> This also won't do anything if the policy is enabled while someone is already
> connected to the console (where the other implementations either kick everyone
> or turn on the curtain immediately). Currently we restart the host immediately
> after that anyway (kicking everyone out in the process), so that's not a
> problem, but since the other curtain mode implementations work in this case, I
> think we need at least a comment here (or in OnCurtainPolicyUpdated) that RDP
> curtaining relies on the host restart, and that we'll need to add code to
check
> if there are non-RDP sessions and kick everyone out if that assumption ever
> changes.

Done.

[rmsousa, 2013-03-05 00:35:59]

lgtm

https://chromiumcodereview.appspot.com/12379084/diff/6001/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/12379084/diff/6001/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:858: // The current curtain mode
implementation relies on this code restarting
nit: current windows/RDP curtain mode implementation

[Jamie, 2013-03-05 00:44:20]

Rubber-stamp lgtm for OWNERS.

[alexeypa (please no reviews), 2013-03-05 00:52:10]

https://chromiumcodereview.appspot.com/12379084/diff/6001/remoting/host/remot...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/12379084/diff/6001/remoting/host/remot...
remoting/host/remoting_me2me_host.cc:858: // The current curtain mode
implementation relies on this code restarting
On 2013/03/05 00:36:00, rmsousa wrote:
> nit: current windows/RDP curtain mode implementation

Done.

[commit-bot: I haz the power, 2013-03-05 00:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12379084/11001

[commit-bot: I haz the power, 2013-03-05 02:25:13]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-05 06:30:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12379084/11001

[commit-bot: I haz the power, 2013-03-05 07:47:20]

Change committed as 186129