Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8511029:
Initial check-in of Linux virtual Me2Me. (Closed)
|
Created:
9 years, 1 month ago by Lambros Modified:
9 years, 1 month ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, dmaclach+watch_chromium.org, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, pam+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial check-in of Linux virtual Me2Me.
This CL includes a Python script, remoting.py, that sets up and runs a virtual X session and host process. It also includes a remoting_me2me_host executable target, which is a simplified version of remoting_simple_host, using the config files that remoting.py sets up.
remoting.py uses Xvfb which needs to be installed (sudo apt-get install xvfb).
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110597
Patch Set 1 #Patch Set 2 : Use "xmpp_login" rather than "email" in config file format. #Patch Set 3 : Add remoting_me2me_host, and use that instead of remoting_simple_host. #
Total comments: 88
Patch Set 4 : Addressed comments for me2me.cc. Python changes to follow. #
Total comments: 6
Patch Set 5 : Address Python comments. #Patch Set 6 : Address .cc comments. #
Messages
Total messages: 17 (0 generated)
Merged other CL into here, and will discard http://codereview.chromium.org/8469013/
On 2011/11/10 22:58:33, Lambros wrote: > Merged other CL into here, and will discard > http://codereview.chromium.org/8469013/ It would be helpful to have a summary of what the CL contains in the CL description.
On 2011/11/10 23:28:42, Wez wrote: > On 2011/11/10 22:58:33, Lambros wrote: > > Merged other CL into here, and will discard > > http://codereview.chromium.org/8469013/ > > It would be helpful to have a summary of what the CL contains in the CL > description. Done
Some drive-by suggestions. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:6: // the Virtual Me2Me implementation. Currently, this is Linux-only. I don't think it is necessary to mention Virtual Me2Me here - we will probably use this code later for a non-virtual implementation too. Also there is nothing linux-specific in this particular file, so the last sentence can go too. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:10: #include <iostream> is this used anywhere? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:38: using remoting::ChromotingHost; Why not put this code in the remoting namespace? That way you would not need this? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:56: class Me2MeHost { I suggest that we call it simply HostProcess or something like this. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:94: auth_config->GetString(kXmppAuthTokenConfigPath, &value); This looks like a hack to me. Since auth and host configs are separate, would it make sense to pass both of them to the host? http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp#newcod... remoting/remoting.gyp:206: 'host/capturer_fake_ascii.cc', Do we need this here? Maybe make it part of remoting_host target? http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp#newcod... remoting/remoting.gyp:209: 'host/continue_window_linux.cc', same here
Some comments on remoting_me2me_host. remoting.py comments to follow... http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. This file should really be remoting_me2me_host.cc, to match the executable name that it generates. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:6: // the Virtual Me2Me implementation. Currently, this is Linux-only. nit: This host isn't intrinsically tied to the virtual implementation, so perhaps say "which is current used for the Linux-only Virtual Me2Me build."? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:43: using remoting::kXmppLoginConfigPath; The names of these are confusing; "path" implies that they are probably filesystem paths, but in fact they are names within the config dictionary, so they should probably be *Name, not *Path. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:54: FILE_PATH_LITERAL("host.json"); nit: Add a comment preceding these definitions to clarify what they relate to. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:63: // It needs to be a UI message loop to keep runloops spinning on the Mac. This code isn't built for Mac right now, so do we need this? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:97: host_config->SetString(kXmppLoginConfigPath, value); Past this point |auth_config| isn't used, I don't think - all we're doing to this point is loading both files and merging their content to generate the run-time config. Can that be moved into a LoadHostConfig() member function? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:107: scoped_ptr<remoting::AccessVerifier> access_verifier; Why do we need this separate reference for the access-verifier? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:108: scoped_ptr<remoting::HeartbeatSender> heartbeat_sender; Define this further down, where it is created. If there is a constraint on tear-down order that means it must appear earlier then add a comment to clarify that. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:112: return 1; Wot no error message? ;) http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:115: // Construct a chromoting host. nit: the API is Create(), so perhaps "Create the DesktopEnvironment and ChromotingHost." http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:124: host_->set_protocol_config(protocol_config_.release()); Don't think we ever use this. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:135: // Let the chromoting host run until the shutdown task is executed. nit: "Run the ChromotingHost until the shutdown task is executed". Although I don't see any way in this file for shutdown to be triggered? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:156: void set_protocol_config(CandidateSessionConfig* protocol_config) { Don't think we ever use this? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:164: scoped_ptr<CandidateSessionConfig> protocol_config_; I don't think we ever use this? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:177: gfx::GtkInitFromCommandLine(*cmd_line); nit: Add a comment to explain why we need this, and the exit manager and crypto init, since they are not used directly in this file. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:190: default_config_dir.Append(kDefaultAuthConfigFile)); nit: Default the config dir in Me2MeHost's ctor? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:197: default_config_dir.Append(kDefaultHostConfigFile)); nit: as above? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:198: } nit: Consider adding a comment before default_config_dir is defined, indicating that the subsequent chunk of code is processing command-line configuration arguments. Also consider moving that processing to a separate method, possibly on Me2MeHost itself. http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp#newcod... remoting/remoting.gyp:206: 'host/capturer_fake_ascii.cc', On 2011/11/11 00:00:43, sergeyu wrote: > Do we need this here? Maybe make it part of remoting_host target? We shouldn't need these! http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp#newcod... remoting/remoting.gyp:209: 'host/continue_window_linux.cc', On 2011/11/11 00:00:43, sergeyu wrote: > same here These are required by ChromotingHost when running IT2Me; we need to separate the IT2Me specific bits out of that to resolve these. Perhaps add a TODO() with a crbug.com number for that work.
http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:6: // the Virtual Me2Me implementation. Currently, this is Linux-only. On 2011/11/11 00:00:43, sergeyu wrote: > I don't think it is necessary to mention Virtual Me2Me here - we will probably > use this code later for a non-virtual implementation too. Also there is nothing > linux-specific in this particular file, so the last sentence can go too. The paths are Posix-friendly, and might cause issues on Windows? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:56: class Me2MeHost { On 2011/11/11 00:00:43, sergeyu wrote: > I suggest that we call it simply HostProcess or something like this. SGTM
http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py File remoting/tools/remoting.py (right): http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:5: Add a description of what this script is for. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:5: I think the script name should describe what it does, e.g me2me_virtual_host.py http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:53: def load_config(self): nit: Why load/write_config rather than read/write_config or load/save_config? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:71: os.umask(0066) # Set permission mask for created file. Does this fail with an error return code, or an exception? What happens then? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:78: """Create a new, or manage an existing, host registration.""" Surely this object effectively _is_ a virtual Me2Me Host configuration? Creating one of these doesn't actually create a registration until you call register_new(), for example. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:89: tuple(map(lambda x: random.randrange(0, 0x10000), range(8)))) Replace this with uuid.uuid1()? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:91: def register_new(self, auth): Split this method down into generate_config(), register(), etc? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:91: def register_new(self, auth): Rename this method create_host() or create_config(), and add a short description of how the caller uses it? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:128: def load_config(self): load vs read / save vs write http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:201: self.child_env[key] = os.environ[key] Are these really all we need for a valid environment? Will session startup source their .profile for us? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:235: atexit.register(cleanup) This means at present that killing the script will kill the desktop(s). We previously discussed having this script effectively "manage" the desktops, so the terminal you run it on doesn't need to linger - is that planned for a future CL? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:240: ): Could this be a single line? It looks strange laid out like this. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:256: host.write_config() Support for multiple Host configurations, and re-starting failed Desktops, is for a follow-up CL? http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:263: desktop.launch_host() If one of these fails, it looks like you'll get error return codes, which you'll ignore?
http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py File remoting/tools/remoting.py (right): http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:23: REMOTING_COMMAND = ["../../out/Debug/remoting_me2me_host"] Can we detect this path similar to how keygen module detects path for chromoting_host_keygen? Currently this script works only when the current directory is set to src/remoting/tools .
Fixed up me2me.cc - Python changes coming soon. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:6: // the Virtual Me2Me implementation. Currently, this is Linux-only. On 2011/11/11 00:24:09, Wez wrote: > nit: This host isn't intrinsically tied to the virtual implementation, so > perhaps say "which is current used for the Linux-only Virtual Me2Me build."? Fixed comment. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:6: // the Virtual Me2Me implementation. Currently, this is Linux-only. On 2011/11/11 00:33:00, Wez wrote: > On 2011/11/11 00:00:43, sergeyu wrote: > > I don't think it is necessary to mention Virtual Me2Me here - we will probably > > use this code later for a non-virtual implementation too. Also there is > nothing > > linux-specific in this particular file, so the last sentence can go too. > > The paths are Posix-friendly, and might cause issues on Windows? Windows will need a different config-file location. I wanted to keep this small and simple for now, by focusing on Linux and not worrying too much about Win/Mac. We can add support for other platforms when we need it. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:10: #include <iostream> On 2011/11/11 00:00:43, sergeyu wrote: > is this used anywhere? Nope. Removed. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:38: using remoting::ChromotingHost; On 2011/11/11 00:00:43, sergeyu wrote: > Why not put this code in the remoting namespace? That way you would not need > this? Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:43: using remoting::kXmppLoginConfigPath; On 2011/11/11 00:24:09, Wez wrote: > The names of these are confusing; "path" implies that they are probably > filesystem paths, but in fact they are names within the config dictionary, so > they should probably be *Name, not *Path. I didn't choose those names - they were already defined in host_config.h. I'd prefer to rename these in a separate CL, if that's what we want. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:54: FILE_PATH_LITERAL("host.json"); On 2011/11/11 00:24:09, Wez wrote: > nit: Add a comment preceding these definitions to clarify what they relate to. Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:56: class Me2MeHost { On 2011/11/11 00:00:43, sergeyu wrote: > I suggest that we call it simply HostProcess or something like this. Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:63: // It needs to be a UI message loop to keep runloops spinning on the Mac. On 2011/11/11 00:24:09, Wez wrote: > This code isn't built for Mac right now, so do we need this? Without this, the host starts and immediately shuts down. Comment and code were copied from simple_host_process.cc, and I'm not sure of the correct thing to do here. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:94: auth_config->GetString(kXmppAuthTokenConfigPath, &value); On 2011/11/11 00:00:43, sergeyu wrote: > This looks like a hack to me. Since auth and host configs are separate, would it > make sense to pass both of them to the host? I'm still working out how we can do this. There are lots of places where HostConfig objects get passed around, and it's not clear to me how we can manage two of them. I figured it would be simpler to stick with a single HostConfig, but have it loaded from two files. Also, remoting_simple_host.cc still wants to load everything from one file (unless we change that, in which case we'd have to update the remoting/tools scripts as well). http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:97: host_config->SetString(kXmppLoginConfigPath, value); On 2011/11/11 00:24:09, Wez wrote: > Past this point |auth_config| isn't used, I don't think - all we're doing to > this point is loading both files and merging their content to generate the > run-time config. Can that be moved into a LoadHostConfig() member function? Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:107: scoped_ptr<remoting::AccessVerifier> access_verifier; On 2011/11/11 00:24:09, Wez wrote: > Why do we need this separate reference for the access-verifier? We don't, it was an artifact of copying remoting_simple_host.cc :) Removed. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:108: scoped_ptr<remoting::HeartbeatSender> heartbeat_sender; On 2011/11/11 00:24:09, Wez wrote: > Define this further down, where it is created. If there is a constraint on > tear-down order that means it must appear earlier then add a comment to clarify > that. Done. Probably was an artifact of the way remoting_simple_host.cc is laid out. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:112: return 1; On 2011/11/11 00:24:09, Wez wrote: > Wot no error message? ;) The Init() method logs if there's an error, so there doesn't seem much point in adding an extra log here. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:115: // Construct a chromoting host. On 2011/11/11 00:24:09, Wez wrote: > nit: the API is Create(), so perhaps "Create the DesktopEnvironment and > ChromotingHost." Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:124: host_->set_protocol_config(protocol_config_.release()); On 2011/11/11 00:24:09, Wez wrote: > Don't think we ever use this. Done (removed all protocol_config stuff). http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:135: // Let the chromoting host run until the shutdown task is executed. On 2011/11/11 00:24:09, Wez wrote: > nit: "Run the ChromotingHost until the shutdown task is executed". Done. > > Although I don't see any way in this file for shutdown to be triggered? I think you're right, it just runs forever until it's killed (or it crashes :) http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:156: void set_protocol_config(CandidateSessionConfig* protocol_config) { On 2011/11/11 00:24:09, Wez wrote: > Don't think we ever use this? Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:164: scoped_ptr<CandidateSessionConfig> protocol_config_; On 2011/11/11 00:24:09, Wez wrote: > I don't think we ever use this? Done. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:177: gfx::GtkInitFromCommandLine(*cmd_line); On 2011/11/11 00:24:09, Wez wrote: > nit: Add a comment to explain why we need this, and the exit manager and crypto > init, since they are not used directly in this file. I've explained GtkInit and AtExitManager, but I don't know if we need to bother with the crypto init - it seems to work fine without it, so I've removed it. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:190: default_config_dir.Append(kDefaultAuthConfigFile)); On 2011/11/11 00:24:09, Wez wrote: > nit: Default the config dir in Me2MeHost's ctor? Nah, there's no need for a member here. The default_config_dir object is only used to set defaults for host/auth config, and isn't needed beyond that. Let's keep locals local? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:198: } On 2011/11/11 00:24:09, Wez wrote: > nit: Consider adding a comment before default_config_dir is defined, indicating > that the subsequent chunk of code is processing command-line configuration > arguments. Also consider moving that processing to a separate method, possibly > on Me2MeHost itself. Added ProcessCommandLine() method. Hopefully that makes things a little clearer, so I don't think a comment is needed. http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp#newcod... remoting/remoting.gyp:206: 'host/capturer_fake_ascii.cc', On 2011/11/11 00:24:09, Wez wrote: > On 2011/11/11 00:00:43, sergeyu wrote: > > Do we need this here? Maybe make it part of remoting_host target? > > We shouldn't need these! Removed. http://codereview.chromium.org/8511029/diff/3001/remoting/remoting.gyp#newcod... remoting/remoting.gyp:209: 'host/continue_window_linux.cc', On 2011/11/11 00:24:09, Wez wrote: > On 2011/11/11 00:00:43, sergeyu wrote: > > same here > > These are required by ChromotingHost when running IT2Me; we need to separate the > IT2Me specific bits out of that to resolve these. Perhaps add a TODO() with a > http://crbug.com number for that work. Done (added TODO with bug-report).
http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:63: // It needs to be a UI message loop to keep runloops spinning on the Mac. On 2011/11/15 23:27:36, Lambros wrote: > On 2011/11/11 00:24:09, Wez wrote: > > This code isn't built for Mac right now, so do we need this? > Without this, the host starts and immediately shuts down. Comment and code were > copied from simple_host_process.cc, and I'm not sure of the correct thing to do > here. You mean you're not sure why we need a UI message-loop here? http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:94: auth_config->GetString(kXmppAuthTokenConfigPath, &value); On 2011/11/15 23:27:36, Lambros wrote: > On 2011/11/11 00:00:43, sergeyu wrote: > > This looks like a hack to me. Since auth and host configs are separate, would > it > > make sense to pass both of them to the host? > > I'm still working out how we can do this. There are lots of places where > HostConfig objects get passed around, and it's not clear to me how we can manage > two of them. I figured it would be simpler to stick with a single HostConfig, > but have it loaded from two files. > > Also, remoting_simple_host.cc still wants to load everything from one file > (unless we change that, in which case we'd have to update the remoting/tools > scripts as well). I think it's reasonable for the Host to be configured with a single configuration structure. Perhaps the user authentication details could move to be a sub-object within the HostConfig, to make it easier to load one to configure a Host, but that sounds like a separate CL to me. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:112: return 1; On 2011/11/15 23:27:36, Lambros wrote: > On 2011/11/11 00:24:09, Wez wrote: > > Wot no error message? ;) > The Init() method logs if there's an error, so there doesn't seem much point in > adding an extra log here. Yuk. We should move that logging out and let the calling code log what it wants, in future. http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:177: gfx::GtkInitFromCommandLine(*cmd_line); On 2011/11/15 23:27:36, Lambros wrote: > On 2011/11/11 00:24:09, Wez wrote: > > nit: Add a comment to explain why we need this, and the exit manager and > crypto > > init, since they are not used directly in this file. > I've explained GtkInit and AtExitManager, but I don't know if we need to bother > with the crypto init - it seems to work fine without it, so I've removed it. EnsureNSPRInit() starts the Netscape Portable Runtime, which is required by NSS. I'm not sure whether we can get away without the call because initializing NSS does this for us? http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:70: } With this code moved to ProcessCommandLine(), if a caller just creates a HostProcess instance and calls Run(), they will get HostProcess with no defaults. If they call ProcessCommandLine() on it, passing in an empty command-line, it will work. That doesn't seem right! Either initializing the defaults in the ctor, or renaming this method InitWithCommandLine() would address that. http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:74: bool LoadConfig(base::MessageLoopProxy* message_loop_proxy) { Doesn't need to be public any more? http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:170: } Since the only thing that sets these is ProcessCommandLine(), you don't really need public setters any more.
Please have a look at my Python fixes, while I fix up the .cc file again :) http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py File remoting/tools/remoting.py (right): http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:5: On 2011/11/11 01:32:32, Wez wrote: > Add a description of what this script is for. Done. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:5: On 2011/11/11 01:32:32, Wez wrote: > I think the script name should describe what it does, e.g me2me_virtual_host.py Done. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:23: REMOTING_COMMAND = ["../../out/Debug/remoting_me2me_host"] On 2011/11/11 05:33:02, sergeyu wrote: > Can we detect this path similar to how keygen module detects path for > chromoting_host_keygen? Currently this script works only when the current > directory is set to src/remoting/tools . Done. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:53: def load_config(self): On 2011/11/11 01:32:32, Wez wrote: > nit: Why load/write_config rather than read/write_config or load/save_config? Done (load/save). http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:71: os.umask(0066) # Set permission mask for created file. On 2011/11/11 01:32:32, Wez wrote: > Does this fail with an error return code, or an exception? What happens then? umask() cannot fail, but the open/write/close calls can throw. In that case, the script would exit and print a Python stack trace. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:78: """Create a new, or manage an existing, host registration.""" On 2011/11/11 01:32:32, Wez wrote: > Surely this object effectively _is_ a virtual Me2Me Host configuration? > Creating one of these doesn't actually create a registration until you call > register_new(), for example. Reworded comment. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:89: tuple(map(lambda x: random.randrange(0, 0x10000), range(8)))) On 2011/11/11 01:32:32, Wez wrote: > Replace this with uuid.uuid1()? Done. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:91: def register_new(self, auth): On 2011/11/11 01:32:32, Wez wrote: > Rename this method create_host() or create_config(), and add a short description > of how the caller uses it? Done. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:128: def load_config(self): On 2011/11/11 01:32:32, Wez wrote: > load vs read / save vs write Done. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:201: self.child_env[key] = os.environ[key] On 2011/11/11 01:32:32, Wez wrote: > Are these really all we need for a valid environment? Will session startup > source their .profile for us? This is essentially what GNOME Display Manager does, except with a few more variables. I've added these here. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:235: atexit.register(cleanup) On 2011/11/11 01:32:32, Wez wrote: > This means at present that killing the script will kill the desktop(s). We > previously discussed having this script effectively "manage" the desktops, so > the terminal you run it on doesn't need to linger - is that planned for a future > CL? Future CL - a bit complex to include in here. Note that self-daemonizing is not necessarily a good thing - read http://mywiki.wooledge.org/ProcessManagement and search that page for "self-daemonizing". :) http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:240: ): On 2011/11/11 01:32:32, Wez wrote: > Could this be a single line? It looks strange laid out like this. Done. I think that list is exhaustive. Trapping things like SIGSEGV, SIGILL etc in a Python-interpreted script is silly! :) http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:256: host.write_config() On 2011/11/11 01:32:32, Wez wrote: > Support for multiple Host configurations, and re-starting failed Desktops, is > for a follow-up CL? I think so. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:263: desktop.launch_host() On 2011/11/11 01:32:32, Wez wrote: > If one of these fails, it looks like you'll get error return codes, which you'll > ignore? Turned these into Exceptions, which will kill the script and show a stack trace. I've also added a bit of error-handling in the case of authentication failure when entering email/password.
http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... File remoting/host/remoting_me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:70: } On 2011/11/16 22:26:14, Wez wrote: > With this code moved to ProcessCommandLine(), if a caller just creates a > HostProcess instance and calls Run(), they will get HostProcess with no > defaults. If they call ProcessCommandLine() on it, passing in an empty > command-line, it will work. That doesn't seem right! > > Either initializing the defaults in the ctor, or renaming this method > InitWithCommandLine() would address that. Done. I like both these suggestions, but the former might put a bit too much code in the ctor (I know, it's borderline :) So I've gone with the latter. http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:74: bool LoadConfig(base::MessageLoopProxy* message_loop_proxy) { On 2011/11/16 22:26:14, Wez wrote: > Doesn't need to be public any more? Moved to private section. http://codereview.chromium.org/8511029/diff/11001/remoting/host/remoting_me2m... remoting/host/remoting_me2me_host.cc:170: } On 2011/11/16 22:26:14, Wez wrote: > Since the only thing that sets these is ProcessCommandLine(), you don't really > need public setters any more. Done.
http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:177: gfx::GtkInitFromCommandLine(*cmd_line); On 2011/11/16 22:26:14, Wez wrote: > On 2011/11/15 23:27:36, Lambros wrote: > > On 2011/11/11 00:24:09, Wez wrote: > > > nit: Add a comment to explain why we need this, and the exit manager and > > crypto > > > init, since they are not used directly in this file. > > I've explained GtkInit and AtExitManager, but I don't know if we need to > bother > > with the crypto init - it seems to work fine without it, so I've removed it. > > EnsureNSPRInit() starts the Netscape Portable Runtime, which is required by NSS. > I'm not sure whether we can get away without the call because initializing NSS > does this for us? The (LazyInstance-created) NSSInitSingleton ctor does this for us (in crypto/nss_util.cc), so I'm confident it's safe for us to not bother. I conjecture the reason that Chrome calls EnsureNSPRInit() (content/browser/browser_main_loop.cc) is to ensure it gets executed before the sandbox kicks in.
Gary, please have a look at the Python script for any obvious style problems, or anything you think could be done better.
lgtm http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc File remoting/host/me2me_host.cc (right): http://codereview.chromium.org/8511029/diff/3001/remoting/host/me2me_host.cc#... remoting/host/me2me_host.cc:177: gfx::GtkInitFromCommandLine(*cmd_line); On 2011/11/17 01:47:07, Lambros wrote: > On 2011/11/16 22:26:14, Wez wrote: > > On 2011/11/15 23:27:36, Lambros wrote: > > > On 2011/11/11 00:24:09, Wez wrote: > > > > nit: Add a comment to explain why we need this, and the exit manager and > > > crypto > > > > init, since they are not used directly in this file. > > > I've explained GtkInit and AtExitManager, but I don't know if we need to > > bother > > > with the crypto init - it seems to work fine without it, so I've removed it. > > > > EnsureNSPRInit() starts the Netscape Portable Runtime, which is required by > NSS. > > I'm not sure whether we can get away without the call because initializing > NSS > > does this for us? > > The (LazyInstance-created) NSSInitSingleton ctor does this for us (in > crypto/nss_util.cc), so I'm confident it's safe for us to not bother. I > conjecture the reason that Chrome calls EnsureNSPRInit() > (content/browser/browser_main_loop.cc) is to ensure it gets executed before the > sandbox kicks in. The main browser process doesn't run a sandbox, so I doubt that that's it. ;) The comment above that line says "make sure we init NSPR on the main thread", so I suspect we should be calling it too, since we're multi-threaded. http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py File remoting/tools/remoting.py (right): http://codereview.chromium.org/8511029/diff/3001/remoting/tools/remoting.py#n... remoting/tools/remoting.py:235: atexit.register(cleanup) On 2011/11/17 01:07:05, Lambros wrote: > On 2011/11/11 01:32:32, Wez wrote: > > This means at present that killing the script will kill the desktop(s). We > > previously discussed having this script effectively "manage" the desktops, so > > the terminal you run it on doesn't need to linger - is that planned for a > future > > CL? > Future CL - a bit complex to include in here. Note that self-daemonizing is not > necessarily a good thing - read http://mywiki.wooledge.org/ProcessManagement > and search that page for "self-daemonizing". > :) That page is really talking about how self-daemonizing makes it hard for a system-wide, long-lived daemon-management process to manage daemons, which is a valid point but doesn't apply to a daemon the user runs themself, for which there is no "manager" process. Of course if you went on to make this script run as a daemon on Linux, and spawn the user's desktop for them as soon as the machine starts up, I'm sure folks wouldn't complain!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/8511029/15004
Change committed as 110597 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
