Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12544020:
Remote RDP sessions, rather than the console, if curtain-mode is configured. (Closed)
|
Created:
7 years, 9 months ago by alexeypa (please no reviews) Modified:
7 years, 9 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemote RDP sessions, rather than the console, if curtain-mode is configured.
This CL adds a new DesktopSession implementation that starts an RDP terminal and attaches to it. Attaching to the terminal launches the DesktopSessionAgent into the corresponding RDP session, and a new agent will be launched each time the terminal's active session is switched, e.g. if the user logs in to an existing session, in the same way as when remoting the host's physical console.
The terminal will be configured to match client dimensions as reported by the Chromoting host.
BUG=137696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188283
Patch Set 1 #
Total comments: 26
Patch Set 2 : CR feedback. #
Total comments: 17
Patch Set 3 : CR feedback. #Patch Set 4 : rebased #Patch Set 5 : fixing remoting_unittests #
Messages
Total messages: 15 (0 generated)
Wez, PTAL. Justin, PTAL at RdpSession::Initialize(const DesktopSessionParams& params) in desktop_session_win.cc. |params| is received via IPC from a less privileged process.
On 2013/03/08 18:49:38, alexeypa wrote: > Wez, PTAL. First impression: The CL description is really confusing. I'd suggest something like: "Remote RDP sessions, rather than the console, if curtain-mode is configured. This CL adds a new DesktopSession implementation that starts an RDP terminal and attaches to it. Attaching to the terminal launches the DesktopSessionAgent into the corresponding RDP session, and a new agent will be launched each time the terminal's active session is switched, e.g. if the user logs in to an existing session, in the same way as when remoting the host's physical console. The terminal will be configured to match client dimensions as reported by the Chromoting host."
https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:214: // Update the expected terminal ID. nit: Consider updating the comment, e.g. The expected next terminal-ID increases even if session creation fails. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_win.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:60: // RDC 6.1 (W2K8) supports resolution up to 4096x2048. nit: resolution -> dimensions of https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:64: const int kMinRdpScreenHeight = 600; nit: Comment on why we have a minimum. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:67: const int kDefaultDpiY = 96; nit: Comment on what these are used for. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:79: // session |id| and the interface for monitoring console session attach/detach nit: console session -> session https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:85: int id, nit: |session_id|? or |desktop_session_id|? https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:92: virtual void OnPermanentError() OVERRIDE; nit: Both of these interfaces can move to private, I think. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:106: // Start monitoring session attach/detach notification for the given endpoint. nit: Start -> Starts e.g. Starts monitoring for session attach/detach events for |client_endpoint|. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:112: // ChromotingDesktopDaemonMsg_InjectSas handler. nit: OnInjectSas() -> InjectSas() w/ comment "Injects a secure attention sequence into the session."? https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:125: // Message loop used by the IPC channel. nit: Does the IPC channel use it, or do we use it when working w/ the IPC channel. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:128: // Handle of the desktop process. nit: Is the desktop process the DesktopSessionAgent, or something else? https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_win.h (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.h:20: scoped_refptr<AutoThreadTaskRunner> caller_task_runner, I think it's cleaner to keep separate CreateForConsole and CreateForVirtualTerminal constructors here, for clarity. If you're doing that then you can also move the DesktopSessionWin implementation back here, move the ctor, dtor and interface overrides to be private or protected, and have a pair of static CreateForX members.
https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:214: // Update the expected terminal ID. On 2013/03/12 01:21:11, Wez wrote: > nit: Consider updating the comment, e.g. The expected next terminal-ID increases > even if session creation fails. Done. Also moved this up because |next_terminal_id_| once we are in this function. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_win.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:60: // RDC 6.1 (W2K8) supports resolution up to 4096x2048. On 2013/03/12 01:21:11, Wez wrote: > nit: resolution -> dimensions of Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:64: const int kMinRdpScreenHeight = 600; On 2013/03/12 01:21:11, Wez wrote: > nit: Comment on why we have a minimum. Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:67: const int kDefaultDpiY = 96; On 2013/03/12 01:21:11, Wez wrote: > nit: Comment on what these are used for. Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:79: // session |id| and the interface for monitoring console session attach/detach On 2013/03/12 01:21:11, Wez wrote: > nit: console session -> session Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:85: int id, On 2013/03/12 01:21:11, Wez wrote: > nit: |session_id|? or |desktop_session_id|? I prefer |id| without any prefix because this is the ID of |this|, not some other external ID. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:92: virtual void OnPermanentError() OVERRIDE; On 2013/03/12 01:21:11, Wez wrote: > nit: Both of these interfaces can move to private, I think. Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:106: // Start monitoring session attach/detach notification for the given endpoint. On 2013/03/12 01:21:11, Wez wrote: > nit: Start -> Starts > > e.g. Starts monitoring for session attach/detach events for |client_endpoint|. Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:112: // ChromotingDesktopDaemonMsg_InjectSas handler. On 2013/03/12 01:21:11, Wez wrote: > nit: OnInjectSas() -> InjectSas() w/ comment "Injects a secure attention > sequence into the session."? Done. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:125: // Message loop used by the IPC channel. On 2013/03/12 01:21:11, Wez wrote: > nit: Does the IPC channel use it, or do we use it when working w/ the IPC > channel. It is used by the IPC channel. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:128: // Handle of the desktop process. On 2013/03/12 01:21:11, Wez wrote: > nit: Is the desktop process the DesktopSessionAgent, or something else? The desktop process is the process running in the user's session whether it runs DesktopSessionAgent or not. https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_win.h (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.h:20: scoped_refptr<AutoThreadTaskRunner> caller_task_runner, On 2013/03/12 01:21:11, Wez wrote: > I think it's cleaner to keep separate CreateForConsole and > CreateForVirtualTerminal constructors here, for clarity. > > If you're doing that then you can also move the DesktopSessionWin implementation > back here, move the ctor, dtor and interface overrides to be private or > protected, and have a pair of static CreateForX members. Done.
ping.
On 2013/03/13 20:03:29, alexeypa wrote: > ping. I was waiting for wez to finish. :)
On 2013/03/13 21:23:53, Justin Schuh wrote: > On 2013/03/13 20:03:29, alexeypa wrote: > > ping. > > I was waiting for wez to finish. :) Understood. :-)
LGTM - looks great. :) https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/daemon_p... File remoting/host/daemon_process.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/daemon_p... remoting/host/daemon_process.cc:214: // Update the expected terminal ID. On 2013/03/12 20:21:15, alexeypa wrote: > On 2013/03/12 01:21:11, Wez wrote: > > nit: Consider updating the comment, e.g. The expected next terminal-ID > increases > > even if session creation fails. > > Done. Also moved this up because |next_terminal_id_| once we are in this > function. Much nicer. :) https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... File remoting/host/desktop_session_win.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/1/remoting/host/desktop_... remoting/host/desktop_session_win.cc:128: // Handle of the desktop process. On 2013/03/12 20:21:15, alexeypa wrote: > On 2013/03/12 01:21:11, Wez wrote: > > nit: Is the desktop process the DesktopSessionAgent, or something else? > > The desktop process is the process running in the user's session whether it runs > DesktopSessionAgent or not. What I'm really looking for is clarity as to what the process this handle refers to actually does, since "desktop process" might mean different things in different contexts. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... File remoting/host/desktop_session_win.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:64: // The minimum effective screen resolution supported by Windows is 800x600. nit: resolution -> dimensions https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:68: // Default dots per inch setting is 96 DPI. nit: No need for "setting" here. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:407: LOG(ERROR) << "Session attach notification hasn't arrived within " nit: "didn't arrive within" or "hasn't arrived after" https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:442: // process so it would be able to duplicate handles of shared memory objects nit: "so it would be able to" -> "to use to" https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:487: // Get the desktop binary name. nit: Suggest "Get the name of the executable the desktop process will run." https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:496: scoped_ptr<CommandLine> target(new CommandLine(desktop_binary)); nit: Add a comment to explain why we're copying switches, and why we only copy some of them. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:502: // Create a delegate to launch a process into the session. nit: "to launch the process into the session" ? https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... File remoting/host/desktop_session_win.h (right): https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.h:115: // Pointer used to unsubscribe from session attach and detach events. nit: No need for "Pointer" here.
Done. Justin, now it is your turn. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... File remoting/host/desktop_session_win.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:64: // The minimum effective screen resolution supported by Windows is 800x600. On 2013/03/13 22:30:27, Wez wrote: > nit: resolution -> dimensions Done. BTW, that was a quote directly from an MSDN page... https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:68: // Default dots per inch setting is 96 DPI. On 2013/03/13 22:30:27, Wez wrote: > nit: No need for "setting" here. Done. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:407: LOG(ERROR) << "Session attach notification hasn't arrived within " On 2013/03/13 22:30:27, Wez wrote: > nit: "didn't arrive within" or "hasn't arrived after" Done. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:442: // process so it would be able to duplicate handles of shared memory objects On 2013/03/13 22:30:27, Wez wrote: > nit: "so it would be able to" -> "to use to" Done. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:487: // Get the desktop binary name. On 2013/03/13 22:30:27, Wez wrote: > nit: Suggest "Get the name of the executable the desktop process will run." Done. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:496: scoped_ptr<CommandLine> target(new CommandLine(desktop_binary)); On 2013/03/13 22:30:27, Wez wrote: > nit: Add a comment to explain why we're copying switches, and why we only copy > some of them. Done. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:502: // Create a delegate to launch a process into the session. On 2013/03/13 22:30:27, Wez wrote: > nit: "to launch the process into the session" ? Done. https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... File remoting/host/desktop_session_win.h (right): https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.h:115: // Pointer used to unsubscribe from session attach and detach events. On 2013/03/13 22:30:27, Wez wrote: > nit: No need for "Pointer" here. Done.
https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... File remoting/host/desktop_session_win.cc (right): https://chromiumcodereview.appspot.com/12544020/diff/10001/remoting/host/desk... remoting/host/desktop_session_win.cc:64: // The minimum effective screen resolution supported by Windows is 800x600. On 2013/03/13 22:51:50, alexeypa wrote: > On 2013/03/13 22:30:27, Wez wrote: > > nit: resolution -> dimensions > > Done. > > BTW, that was a quote directly from an MSDN page... Understood. Elsewhere resolution gets used interchangeably to refer to the number of pixels, and to the pixel density, or both, so our codebase I've adopted "dimensions" for the size and "resolution" for size+density, for clarity.
I'm assuming you just wanted a high-level sanity check on things like the ACL for IPC channel and the curtain temrinal creationhandling. If so, lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12544020/26001
Retried try job too often on mac_rel for step(s) remoting_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/12544020/33002
Message was sent while issue was closed.
Change committed as 188283 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
