The me2me host is now configurable from the web UI on Windows.

remoting/host/plugin/daemon_controller_win.cc

Index: remoting/host/plugin/daemon_controller_win.cc
diff --git a/remoting/host/plugin/daemon_controller_win.cc b/remoting/host/plugin/daemon_controller_win.cc
index 901fe9594c9b7ff156b0f182e7e81ffb5ee8b211..49fe259c8791963d189225050f3340acfe9e0d3a 100644
--- a/remoting/host/plugin/daemon_controller_win.cc
+++ b/remoting/host/plugin/daemon_controller_win.cc
@@ -4,18 +4,63 @@
 
 #include "remoting/host/plugin/daemon_controller.h"
 
+#include <objbase.h>
+
 #include "base/basictypes.h"
+#include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/logging.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread.h"
+#include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "remoting/base/scoped_sc_handle_win.h"
+#include "remoting/host/branding.h"
+
+// MIDL-generated declarations and definitions.
+#include "elevated_controller.h"
+#include "elevated_controller_i.c"
 
 namespace remoting {
 
 namespace {
 
+// The COM elevation moniker for the elevated controller.

[Wez, 2012/03/30 22:11:01]

nit: Consider adding a sentence to explain what this is actually used for.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

This is typical RTFM. I.e. one sentence is too little. More text is overkill. 
"The COM elevation moniker" is a unique phase interested folks can use to read
about it.

+const char kElevationMoniker[] = "Elevation:Administrator!new:"
+    "{430a9403-8176-4733-afdc-0b325a8fda84}";
+
+// Name of the worker thread.

[Wez, 2012/03/30 22:11:01]

nit: Similarly, what worker thread? What does the thread actually do?  Why is
the constant not called kDaemonControllerThreadName?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+const char kWorkerThreadName[] = "Daemon Controller thread";
+
+// A simple wrapper around base::Thread making sure that COM is initialized on
+// the owner thread.

[Wez, 2012/03/30 22:11:01]

nit: I think you mean "A base::Thread implementation that initializes COM on the
new thread"?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+class ComThread : public base::Thread {
+ public:
+  explicit ComThread(const char* name);
+
+  // Activates an elevated instance of elevated controller and returns

[Wez, 2012/03/30 22:11:01]

nit: ... elevated instance of the controller ...

or  ... elevated instance of ElevatedController

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  // the pointer to the control interface in |control_out|. This routine keeps

[Wez, 2012/03/30 22:11:01]

nit: The routine can't keep ownership; do you mean the ComThread does?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  // the ownership of the pointer so the caller should not call call AddRef() or
+  // Release().
+  HRESULT ActivateElevatedController(IDaemonControl** control_out);
+
+ protected:
+  virtual void Init() OVERRIDE;
+  virtual void CleanUp() OVERRIDE;
+
+  IDaemonControl* control_;

[Wez, 2012/03/30 22:11:01]

nit: Perhaps we should have a ScopedIPtr... ;)

[alexeypa (please no reviews), 2012/03/30 23:47:09]

It is not going to help in this case.

+
+  DISALLOW_COPY_AND_ASSIGN(ComThread);
+};
+
 class DaemonControllerWin : public remoting::DaemonController {
  public:
   DaemonControllerWin();
+  virtual ~DaemonControllerWin();
 
   virtual State GetState() OVERRIDE;
   virtual void GetConfig(const GetConfigCallback& callback) OVERRIDE;
@@ -25,23 +70,170 @@ class DaemonControllerWin : public remoting::DaemonController {
   virtual void Stop() OVERRIDE;
 
  private:
+  // Activates an elevated instance of elevated controller and returns

[Wez, 2012/03/30 22:11:01]

nit: See above.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Stale code. Removed.

+  // the pointer to the control interface in |control_out|.
+  HRESULT ActivateElevatedController(IDaemonControl** control_out);
+
+  // Opens the controlled service handle.

[Wez, 2012/03/30 22:11:01]

nit: What is the controlled service handle?  You mean it opens a handle to the
Daemon service?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Reworded.

+  DWORD OpenService(ScopedScHandle* service_out);
+
+  // Worker functions called in the context of the worker thread.

[Wez, 2012/03/30 22:11:01]

nit: Double use of the term "worker" makes this comment less helpful!

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  void DoGetConfig(const GetConfigCallback& callback);
+  void DoSetConfigAndStart(scoped_ptr<base::DictionaryValue> config);
+  void DoStop();
+
+  // Converts SERVICE_XXX contants representing the service state (i.e.
+  // SERVICE_RUNNING, SERVICE_STOPPED) to the daemon state.

[Wez, 2012/03/30 22:11:01]

nit: "Converts a Windows service status code to a Daemon state."

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  static State ConvertToDaemonState(DWORD service_state);
+
+  // Service status change notification callback.

[Wez, 2012/03/30 22:11:01]

nit: Suggest rewording e.g. "Status change callback used to monitor start/stop
progress".

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Stale code.

+  static VOID CALLBACK OnServiceStatusChange(PVOID context);
+
+  // The worker thread used for servicing long running operations.
+  ComThread worker_thread_;
+
+  // The lock protecting access to all data members below.
+  base::Lock lock_;
+
+  // The error occurred during the last transition.
+  HRESULT last_error_;
+
+  // Cached daemon state.

[Wez, 2012/03/30 22:11:01]

nit: This is the state as of the most recent status-change callback?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

No, it is not. Reworded.

+  State state_;
+
+  // The state that should never be reported to JS unless there is an error.
+  // For instance, when Start() is called, the state of the service doesn't
+  // switch to "starting" immediately. This could lead to JS interpreting
+  // "stopped" as a failure to start the service.

[Wez, 2012/03/30 22:11:01]

This comment is very confusing, as is the name |forbidden_state_|.

The idea is that the actual state of the service doesn't necessarily reflect
transitions accurately, so we only report it to callers in case of errors, and
otherwise maintain an "official" state the rest of the time?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

forbidden_state_ is a hack and it is confusing because it is a hack!
No. The state of the service reflects transitions accurately. It is just JS code
expectation are not synchronized with the service status changes.

+  // TODO(alexeypa): remove this variable once JS interafce supports callbacks.

[Wez, 2012/03/30 22:11:01]

typo: interafce

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  State forbidden_state_;
+
   DISALLOW_COPY_AND_ASSIGN(DaemonControllerWin);
 };
 
-DaemonControllerWin::DaemonControllerWin() {
+ComThread::ComThread(const char* name) : base::Thread(name), control_(NULL) {
+}
+
+void ComThread::Init() {
+  CoInitialize(NULL);
+}
+
+void ComThread::CleanUp() {
+  if (control_ != NULL) {
+    control_->Release();
+  }
+

[Wez, 2012/03/30 22:11:01]

nit: Personally I'd lose the {} on the if, and this blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  CoUninitialize();
+}
+
+HRESULT ComThread::ActivateElevatedController(
+    IDaemonControl** control_out) {
+

[Wez, 2012/03/30 22:11:01]

nit: No need for blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

I replaced it with a comment. 

+  if (control_ == NULL) {
+    BIND_OPTS3 bind_options;
+    memset(&bind_options, 0, sizeof(bind_options));
+    bind_options.cbStruct = sizeof(bind_options);
+    bind_options.hwnd = NULL;
+    bind_options.dwClassContext  = CLSCTX_LOCAL_SERVER;
+
+    IDaemonControl* control = NULL;

[Wez, 2012/03/30 22:11:01]

This doesn't seem to be used?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+    HRESULT hr = ::CoGetObject(ASCIIToUTF16(kElevationMoniker).c_str(),
+                               &bind_options,
+                               IID_IDaemonControl,
+                               reinterpret_cast<void**>(&control_));
+    if (FAILED(hr)) {
+      LOG(ERROR) << "Failed to create the elevated controller (error: 0x"
+          << std::hex << hr << std::dec << ").";
+      return hr;
+    }
+  }
+
+  *control_out = control_;
+  return S_OK;
+}
+
+DaemonControllerWin::DaemonControllerWin()
+    : last_error_(S_OK),
+      state_(STATE_UNKNOWN),
+      forbidden_state_(STATE_UNKNOWN),
+      worker_thread_(kWorkerThreadName) {
+  // N.B. The UI message loop is required for the single threaded COM apartment.

[Wez, 2012/03/30 22:11:01]

typo: The UI ... is required for ...  -> COM must be run on a UI message loop
...

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  base::Thread::Options thread_options(MessageLoop::TYPE_UI, 0);
+  if (!worker_thread_.StartWithOptions(thread_options)) {

[Wez, 2012/03/30 22:11:01]

Since ComThread doesn't make sense unless started with a UI message loop, can we
make it default to one instead of passing options here?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+    // N.B. StartWithOptions() does not report the error code returned by
+    // the system.
+    last_error_ = E_FAIL;
+  }
 }
 
-DaemonController::State DaemonControllerWin::GetState() {
-  return DaemonController::STATE_NOT_IMPLEMENTED;
+DaemonControllerWin::~DaemonControllerWin() {
+  worker_thread_.Stop();
+}
+
+remoting::DaemonController::State DaemonControllerWin::GetState() {
+  // TODO(alexeypa): convert polling to async callbacks once there is a thread
+  // that can receive APC callbacks.

[Wez, 2012/03/30 22:11:01]

nit: Suggest "Make the thread alertable, so we can switch to APC notifications
rather than polling, "?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  ScopedScHandle service;
+  DWORD error = OpenService(&service);
+
+  if (error == ERROR_SUCCESS) {
+    SERVICE_STATUS status;
+    if (::QueryServiceStatus(service, &status)) {
+      State new_state = ConvertToDaemonState(status.dwCurrentState);
+
+      base::AutoLock lock(lock_);
+      // TODO(alexeypa): remove |forbidden_state_| hack once JS interface
+      // supports callbacks.

[Wez, 2012/03/30 22:11:01]

nit: remove ... -> Remove ...

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+      if (forbidden_state_ != new_state || FAILED(last_error_)) {
+        state_ = new_state;
+      }
+
+      // TODO(alexeypa): remove this hack once JS nicely reports errors.

[Wez, 2012/03/30 22:11:01]

nit: It's not immediately obvious what the hack is here. :)

[alexeypa (please no reviews), 2012/03/30 23:47:09]

I'm sorry. It is there. Look more closely. :-)

+      if (FAILED(last_error_)) {
+        state_ = STATE_START_FAILED;
+      }
+
+      return state_;
+    } else {
+      error = GetLastError();
+      LOG_GETLASTERROR(ERROR)
+          << "Failed to query the state of the '" << kWindowsServiceName
+          << "' service";
+    }
+  }
+
+  base::AutoLock lock(lock_);
+  if (error == ERROR_SERVICE_DOES_NOT_EXIST) {
+    state_ = STATE_NOT_IMPLEMENTED;
+  } else {
+    last_error_ = HRESULT_FROM_WIN32(error);
+    state_ = STATE_UNKNOWN;
+  }
+
+  return state_;
 }
 
 void DaemonControllerWin::GetConfig(const GetConfigCallback& callback) {
-  NOTIMPLEMENTED();
+  worker_thread_.message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&DaemonControllerWin::DoGetConfig,
+                 base::Unretained(this), callback));
 }
 
 void DaemonControllerWin::SetConfigAndStart(
     scoped_ptr<base::DictionaryValue> config) {
-  NOTIMPLEMENTED();
+  base::AutoLock lock(lock_);
+
+  // TODO(alexeypa): implement on-demand installation.

[Wez, 2012/03/30 22:11:01]

typo: implement ... -> Implement ...

[Wez, 2012/03/30 22:11:01]

nit: We generally prefer to create TODOs with an associated bug, if possible,
and refer to the bug-Id in the TODO.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

While being a good practice (I presume) I haven't seen many TODO comments
mentioning any bug number.

+  if (state_ == STATE_STOPPED) {
+    last_error_ = S_OK;
+    forbidden_state_ = STATE_STOPPED;
+    state_ = STATE_STARTING;
+    worker_thread_.message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&DaemonControllerWin::DoSetConfigAndStart,
+                   base::Unretained(this), base::Passed(&config)));
+  }
 }
 
 void DaemonControllerWin::SetPin(const std::string& pin) {
@@ -49,7 +241,167 @@ void DaemonControllerWin::SetPin(const std::string& pin) {
 }
 
 void DaemonControllerWin::Stop() {
-  NOTIMPLEMENTED();
+  base::AutoLock lock(lock_);
+
+  if (state_ == STATE_STARTING ||
+      state_ == STATE_STARTED ||
+      state_ == STATE_STOPPING) {

[Wez, 2012/03/30 22:11:01]

These are cached states; isn't there a risk that something has started the
service since we read them?  Is there any danger in posting a stop control when
the service isn't in one of these states?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

There is no danger. The state is inherently out-on-sync. This is just a
precaution making JS logic a little bit more reliable.

+

[Wez, 2012/03/30 22:11:01]

nit: Errant blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+    last_error_ = S_OK;
+    forbidden_state_ = STATE_STARTED;
+    state_ = STATE_STOPPING;
+    worker_thread_.message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&DaemonControllerWin::DoStop, base::Unretained(this)));
+  }
+}
+
+DWORD DaemonControllerWin::OpenService(ScopedScHandle* service_out) {

[Wez, 2012/03/30 22:11:01]

nit: This function looks oddly familiar from ElevatedController... ;)

[alexeypa (please no reviews), 2012/03/30 23:47:09]

There is little benefit in sharing the same implementation of the function
between those two places at the moment. I don't really want to introduce a whole
file for them. Not until the code is a little bit more stable.

+  // Open the service and query its current state.
+  ScopedScHandle scmanager(
+      ::OpenSCManagerW(NULL, SERVICES_ACTIVE_DATABASE,
+                       SC_MANAGER_CONNECT | SC_MANAGER_ENUMERATE_SERVICE));
+  if (!scmanager.IsValid()) {
+    DWORD error = GetLastError();
+    LOG_GETLASTERROR(ERROR)
+        << "Failed to connect to the service control manager";
+    return error;
+  }
+
+  ScopedScHandle service(
+      ::OpenServiceW(scmanager, ASCIIToUTF16(kWindowsServiceName).c_str(),
+                     SERVICE_QUERY_STATUS));
+  if (!service.IsValid()) {
+    DWORD error = GetLastError();
+    if (error != ERROR_SERVICE_DOES_NOT_EXIST) {
+      LOG_GETLASTERROR(ERROR)
+          << "Failed to open to the '" << kWindowsServiceName << "' service";
+    }
+    return error;
+  }
+
+  service_out->Set(service.Take());
+  return ERROR_SUCCESS;
+}
+
+void DaemonControllerWin::DoGetConfig(const GetConfigCallback& callback) {
+  IDaemonControl* control = NULL;
+  HRESULT hr = worker_thread_.ActivateElevatedController(&control);
+  if (FAILED(hr)) {
+    callback.Run(scoped_ptr<base::DictionaryValue>());
+    return;
+  }
+
+  // Get the host configuration.
+  BSTR host_config = NULL;

[Wez, 2012/03/30 22:11:01]

Is there no managed container for BSTR?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

There is. It drags ATL. We don't want it here.

+  hr = control->GetConfig(&host_config);
+  if (FAILED(hr)) {
+    callback.Run(scoped_ptr<base::DictionaryValue>());
+    return;
+  }
+
+  string16 file_content(static_cast<char16*>(host_config),
+                        ::SysStringLen(host_config));
+  SysFreeString(host_config);
+
+  // Parse the string into a dictionary.
+  scoped_ptr<base::Value> config(
+      base::JSONReader::Read(UTF16ToUTF8(file_content), true));
+
+  base::DictionaryValue* dictionary;
+  if (config.get() == NULL || !config->GetAsDictionary(&dictionary)) {
+    callback.Run(scoped_ptr<base::DictionaryValue>());
+    return;
+  }
+
+  config.release();
+  callback.Run(scoped_ptr<base::DictionaryValue>(dictionary));

[Wez, 2012/03/30 22:11:01]

Yup. Still gross ... :P

+}
+
+void DaemonControllerWin::DoSetConfigAndStart(
+    scoped_ptr<base::DictionaryValue> config) {
+

[Wez, 2012/03/30 22:11:01]

nit: Errant blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  IDaemonControl* control = NULL;
+  HRESULT hr = worker_thread_.ActivateElevatedController(&control);
+  if (FAILED(hr)) {
+    base::AutoLock lock(lock_);
+    last_error_ = hr;

[Wez, 2012/03/30 22:11:01]

Why do we lock when setting |last_error_|? Surely if someone is querying
asynchronously then there's going to be a race in the states they see?

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Race is fine. Corruption (state_ is not synched with last_error_ for instance)
is not fine.

+    return;
+  }
+
+  // Set the configuration file

[Wez, 2012/03/30 22:11:01]

Suggest "Store the configuration."

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  std::string file_content;
+  base::JSONWriter::Write(config.get(), &file_content);
+
+  BSTR host_config = ::SysAllocString(UTF8ToUTF16(file_content).c_str());
+  if (host_config == NULL) {
+    base::AutoLock lock(lock_);
+    last_error_ = E_OUTOFMEMORY;
+    return;
+  }
+
+  hr = control->SetConfig(host_config);
+  ::SysFreeString(host_config);
+

[Wez, 2012/03/30 22:11:01]

nit: No need for blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  if (FAILED(hr)) {
+    base::AutoLock lock(lock_);
+    last_error_ = hr;
+    return;
+  }
+
+  // Start daemon.
+  hr = control->StartDaemon();
+

[Wez, 2012/03/30 22:11:01]

nit: No need for blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  if (FAILED(hr)) {
+    base::AutoLock lock(lock_);
+    last_error_ = hr;
+  }
+}
+
+void DaemonControllerWin::DoStop() {
+  IDaemonControl* control = NULL;
+  HRESULT hr = worker_thread_.ActivateElevatedController(&control);
+  if (FAILED(hr)) {
+    base::AutoLock lock(lock_);
+    last_error_ = hr;
+    return;
+  }
+
+  hr = control->StopDaemon();
+

[Wez, 2012/03/30 22:11:01]

nit: Blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  if (FAILED(hr)) {
+    base::AutoLock lock(lock_);
+    last_error_ = hr;
+  }
+}
+
+// static
+remoting::DaemonController::State DaemonControllerWin::ConvertToDaemonState(
+    DWORD service_state) {
+

[Wez, 2012/03/30 22:11:01]

nit: Blank line.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

Done.

+  switch (service_state) {
+  case SERVICE_RUNNING:
+    return STATE_STARTED;
+

[Wez, 2012/03/30 22:11:01]

nit: No need for these blank lines.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

My eyes hurt. It is unreadable.

+  case SERVICE_CONTINUE_PENDING:
+  case SERVICE_START_PENDING:
+    return STATE_STARTING;
+    break;

[Wez, 2012/03/30 22:11:01]

nit: Nor for this break, or the ones below.

[alexeypa (please no reviews), 2012/03/30 23:47:09]

See above.

+
+  case SERVICE_PAUSE_PENDING:
+  case SERVICE_STOP_PENDING:
+    return STATE_STOPPING;
+    break;
+
+  case SERVICE_PAUSED:
+  case SERVICE_STOPPED:
+    return STATE_STOPPED;
+    break;
+
+  default:
+    NOTREACHED();
+    return STATE_UNKNOWN;
+  }
 }
 
 }  // namespace