Initial check-in of Linux virtual Me2Me.

remoting/host/me2me_host.cc

Index: remoting/host/me2me_host.cc
diff --git a/remoting/host/me2me_host.cc b/remoting/host/me2me_host.cc
new file mode 100644
index 0000000000000000000000000000000000000000..14d97e32b9f1c1d380e6ab25a7559364be66ce37
--- /dev/null
+++ b/remoting/host/me2me_host.cc
@@ -0,0 +1,201 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

[Wez, 2011/11/11 00:24:09]

This file should really be remoting_me2me_host.cc, to match the executable name
that it generates.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// This file implements a standalone host process for Me2Me, which is used for
+// the Virtual Me2Me implementation.  Currently, this is Linux-only.

[Sergey Ulanov, 2011/11/11 00:00:43]

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.

[Wez, 2011/11/11 00:24:09]

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."?

[Wez, 2011/11/11 00:33:00]

The paths are Posix-friendly, and might cause issues on Windows?

[Lambros, 2011/11/15 23:27:36]

Fixed comment.

[Lambros, 2011/11/15 23:27:36]

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.

+
+#include <stdlib.h>
+
+#include <iostream>

[Sergey Ulanov, 2011/11/11 00:00:43]

is this used anywhere?

[Lambros, 2011/11/15 23:27:36]

Nope. Removed.

+#include <string>
+
+#include "base/at_exit.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/message_loop.h"
+#include "base/threading/thread.h"
+#include "build/build_config.h"
+#include "crypto/nss_util.h"
+#include "remoting/base/constants.h"
+#include "remoting/host/chromoting_host.h"
+#include "remoting/host/chromoting_host_context.h"
+#include "remoting/host/desktop_environment.h"
+#include "remoting/host/event_executor.h"
+#include "remoting/host/heartbeat_sender.h"
+#include "remoting/host/host_config.h"
+#include "remoting/host/json_host_config.h"
+#include "remoting/host/self_access_verifier.h"
+
+#if defined(TOOLKIT_USES_GTK)
+#include "ui/gfx/gtk_util.h"
+#endif
+
+using remoting::ChromotingHost;

[Sergey Ulanov, 2011/11/11 00:00:43]

Why not put this code in the remoting namespace? That way you would not need
this?

[Lambros, 2011/11/15 23:27:36]

Done.

+using remoting::DesktopEnvironment;
+using remoting::kChromotingTokenDefaultServiceName;
+using remoting::kXmppAuthServiceConfigPath;
+using remoting::kXmppAuthTokenConfigPath;
+using remoting::kXmppLoginConfigPath;

[Wez, 2011/11/11 00:24:09]

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.

[Lambros, 2011/11/15 23:27:36]

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.

+using remoting::protocol::CandidateSessionConfig;
+using remoting::protocol::ChannelConfig;
+
+const char kAuthConfigSwitchName[] = "auth-config";
+const char kHostConfigSwitchName[] = "host-config";
+const FilePath::CharType kDefaultConfigDir[] =
+    FILE_PATH_LITERAL(".config/chrome-remote-desktop");
+const FilePath::CharType kDefaultAuthConfigFile[] =
+    FILE_PATH_LITERAL("auth.json");
+const FilePath::CharType kDefaultHostConfigFile[] =
+    FILE_PATH_LITERAL("host.json");

[Wez, 2011/11/11 00:24:09]

nit: Add a comment preceding these definitions to clarify what they relate to.

[Lambros, 2011/11/15 23:27:36]

Done.

+
+class Me2MeHost {

[Sergey Ulanov, 2011/11/11 00:00:43]

I suggest that we call it simply HostProcess or something like this.

[Wez, 2011/11/11 00:33:00]

SGTM

[Lambros, 2011/11/15 23:27:36]

Done.

+ public:
+  Me2MeHost() {}
+
+  int Run() {
+    // |message_loop| is declared early so that any code we call into which
+    // requires a current message-loop won't complain.
+    // It needs to be a UI message loop to keep runloops spinning on the Mac.

[Wez, 2011/11/11 00:24:09]

This code isn't built for Mac right now, so do we need this?

[Lambros, 2011/11/15 23:27:36]

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.

[Wez, 2011/11/16 22:26:14]

You mean you're not sure why we need a UI message-loop here?

+    MessageLoop message_loop(MessageLoop::TYPE_UI);
+
+    remoting::ChromotingHostContext context(
+        base::MessageLoopProxy::current());
+    context.Start();
+
+    base::Thread file_io_thread("FileIO");
+    file_io_thread.Start();
+
+    scoped_refptr<remoting::JsonHostConfig> host_config =
+        new remoting::JsonHostConfig(
+            host_config_path_, file_io_thread.message_loop_proxy());
+    scoped_refptr<remoting::JsonHostConfig> auth_config =
+        new remoting::JsonHostConfig(
+            auth_config_path_, file_io_thread.message_loop_proxy());
+
+    std::string failed_path;
+    if (!host_config->Read()) {
+      failed_path = host_config_path_.value();
+    } else if (!auth_config->Read()) {
+      failed_path = auth_config_path_.value();
+    }
+    if (!failed_path.empty()) {
+      LOG(ERROR) << "Failed to read configuration file " << failed_path;
+      context.Stop();
+      return 1;
+    }
+
+    // Copy the needed keys from |auth_config| into |host_config|.
+    std::string value;
+    auth_config->GetString(kXmppAuthTokenConfigPath, &value);

[Sergey Ulanov, 2011/11/11 00:00:43]

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?

[Lambros, 2011/11/15 23:27:36]

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).

[Wez, 2011/11/16 22:26:14]

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.

+    host_config->SetString(kXmppAuthTokenConfigPath, value);
+    auth_config->GetString(kXmppLoginConfigPath, &value);
+    host_config->SetString(kXmppLoginConfigPath, value);

[Wez, 2011/11/11 00:24:09]

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?

[Lambros, 2011/11/15 23:27:36]

Done.

+
+    // For the Me2Me host, we assume we always use the ClientLogin token for
+    // chromiumsync because we do not have an HTTP stack with which we can
+    // easily request an OAuth2 access token even if we had a RefreshToken for
+    // the account.
+    host_config->SetString(kXmppAuthServiceConfigPath,
+                           kChromotingTokenDefaultServiceName);
+
+    // Initialize AccessVerifier.
+    scoped_ptr<remoting::AccessVerifier> access_verifier;

[Wez, 2011/11/11 00:24:09]

Why do we need this separate reference for the access-verifier?

[Lambros, 2011/11/15 23:27:36]

We don't, it was an artifact of copying remoting_simple_host.cc :)  Removed.

+    scoped_ptr<remoting::HeartbeatSender> heartbeat_sender;

[Wez, 2011/11/11 00:24:09]

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.

[Lambros, 2011/11/15 23:27:36]

Done.  Probably was an artifact of the way remoting_simple_host.cc is laid out.

+    scoped_ptr<remoting::SelfAccessVerifier> self_access_verifier(
+        new remoting::SelfAccessVerifier());
+    if (!self_access_verifier->Init(host_config))
+      return 1;

[Wez, 2011/11/11 00:24:09]

Wot no error message? ;)

[Lambros, 2011/11/15 23:27:36]

The Init() method logs if there's an error, so there doesn't seem much point in
adding an extra log here.

[Wez, 2011/11/16 22:26:14]

Yuk.  We should move that logging out and let the calling code log what it
wants, in future.

+    access_verifier.reset(self_access_verifier.release());
+
+    // Construct a chromoting host.

[Wez, 2011/11/11 00:24:09]

nit: the API is Create(), so perhaps "Create the DesktopEnvironment and
ChromotingHost."

[Lambros, 2011/11/15 23:27:36]

Done.

+    scoped_ptr<DesktopEnvironment> desktop_environment(
+        DesktopEnvironment::Create(&context));
+
+    host_ = ChromotingHost::Create(&context, host_config,
+                                   desktop_environment.get(),
+                                   access_verifier.release(), false);
+
+    if (protocol_config_.get()) {
+      host_->set_protocol_config(protocol_config_.release());

[Wez, 2011/11/11 00:24:09]

Don't think we ever use this.

[Lambros, 2011/11/15 23:27:36]

Done (removed all protocol_config stuff).

+    }
+
+    // Initialize HeartbeatSender.
+    heartbeat_sender.reset(
+        new remoting::HeartbeatSender(context.network_message_loop(),
+                                      host_config));
+    if (!heartbeat_sender->Init())
+      return 1;
+    host_->AddStatusObserver(heartbeat_sender.get());
+
+    // Let the chromoting host run until the shutdown task is executed.

[Wez, 2011/11/11 00:24:09]

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?

[Lambros, 2011/11/15 23:27:36]

Done.
I think you're right, it just runs forever until it's killed (or it crashes :)

+    host_->Start();
+    message_loop.MessageLoop::Run();
+
+    // And then stop the chromoting context.
+    context.Stop();
+    file_io_thread.Stop();
+
+    host_ = NULL;
+
+    return 0;
+  }
+
+  void set_auth_config_path(const FilePath& config_path) {
+    auth_config_path_ = config_path;
+  }
+
+  void set_host_config_path(const FilePath& config_path) {
+    host_config_path_ = config_path;
+  }
+
+  void set_protocol_config(CandidateSessionConfig* protocol_config) {

[Wez, 2011/11/11 00:24:09]

Don't think we ever use this?

[Lambros, 2011/11/15 23:27:36]

Done.

+    protocol_config_.reset(protocol_config);
+  }
+
+ private:
+  FilePath auth_config_path_;
+  FilePath host_config_path_;
+
+  scoped_ptr<CandidateSessionConfig> protocol_config_;

[Wez, 2011/11/11 00:24:09]

I don't think we ever use this?

[Lambros, 2011/11/15 23:27:36]

Done.

+
+  scoped_refptr<ChromotingHost> host_;
+};
+
+int main(int argc, char** argv) {
+  CommandLine::Init(argc, argv);
+  const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
+
+  base::AtExitManager exit_manager;
+  crypto::EnsureNSPRInit();
+
+#if defined(TOOLKIT_USES_GTK)
+  gfx::GtkInitFromCommandLine(*cmd_line);

[Wez, 2011/11/11 00:24:09]

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.

[Lambros, 2011/11/15 23:27:36]

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.

[Wez, 2011/11/16 22:26:14]

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?

[Lambros, 2011/11/17 01:47:07]

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.

[Wez, 2011/11/17 02:19:28]

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.

+#endif  // TOOLKIT_USES_GTK
+
+  Me2MeHost me2me_host;
+
+  FilePath default_config_dir =
+      file_util::GetHomeDir().Append(kDefaultConfigDir);
+
+  if (cmd_line->HasSwitch(kAuthConfigSwitchName)) {
+    me2me_host.set_auth_config_path(
+        cmd_line->GetSwitchValuePath(kAuthConfigSwitchName));
+  } else {
+    me2me_host.set_auth_config_path(
+        default_config_dir.Append(kDefaultAuthConfigFile));

[Wez, 2011/11/11 00:24:09]

nit: Default the config dir in Me2MeHost's ctor?

[Lambros, 2011/11/15 23:27:36]

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?

+  }
+  if (cmd_line->HasSwitch(kHostConfigSwitchName)) {
+    me2me_host.set_host_config_path(
+        cmd_line->GetSwitchValuePath(kHostConfigSwitchName));
+  } else {
+    me2me_host.set_host_config_path(
+        default_config_dir.Append(kDefaultHostConfigFile));

[Wez, 2011/11/11 00:24:09]

nit: as above?

+  }

[Wez, 2011/11/11 00:24:09]

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.

[Lambros, 2011/11/15 23:27:36]

Added ProcessCommandLine() method. Hopefully that makes things a little clearer,
so I don't think a comment is needed.

+
+  return me2me_host.Run();
+}