[Chromoting] The daemon process now starts the networking process and passes the host configuration…

remoting/host/config_file_watcher.h

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_DAEMON_CONFIG_WATCHER_H_
+#define REMOTING_HOST_DAEMON_CONFIG_WATCHER_H_
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/timer.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+class Thread;
+
+namespace files {
+class FilePathWatcher;
+}  // namespace files
+}  // namespace base
+
+namespace remoting {
+
+extern const char kHostConfigSwitchName[];
+extern const FilePath::CharType kDefaultHostConfigFile[];
+
+class ConfigFileWatcher {
+ public:
+  class Delegate {
+   public:
+    virtual ~Delegate();

[Wez, 2012/08/30 20:36:04]

nit: Should this be protected, so that only implementations can delete
themselves?

[alexeypa (please no reviews), 2012/08/30 23:15:13]

None of delegate declarations use protected destructors. I'll stick to defacto
standard. 

PS. I actually think this might have some unexpected implications. Other
interfaces also define public destructors. The style guide also does that.

[Wez, 2012/08/31 00:23:04]

It doesn't seem right, does it? Either way, not a debate for this CL. :)

+
+    // Called once after starting watching the configuration file and every time
+    // the file changes.
+    virtual void OnConfigUpdated(const std::string& config) = 0;
+
+    // Called when the configuration file watcher encountered an error.
+    virtual void OnConfigWatcherError() = 0;
+  };
+
+  // Creates a configuration file watcher that lives at the |io_task_runner|
+  // thread but posts config file updates on on |main_task_runner|.
+  ConfigFileWatcher(
+      scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+      const base::WeakPtr<Delegate>& delegate);

[Wez, 2012/08/30 20:36:04]

nit: You're forcing the caller to arrange to provide a WeakPtr<Delegate>; you
can avoid that requirement by instead having the ConfigFileWatcher manage the
WeakPtrFactory<Delegate>, and requiring that the caller call StopAndDelete()
before tearing down the Delegate so that you can invalidate the WeakPtr.

[alexeypa (please no reviews), 2012/08/30 23:15:13]

Done.

+
+  virtual ~ConfigFileWatcher();
+
+  // Starts watching |config_path|.
+  void Watch(const FilePath& config_path);
+
+  // Stops watching the configuration file and delete itself.
+  void StopAndDelete();

[Wez, 2012/08/30 20:36:04]

Should the destructor be public if it's intended that deletion be performed by
StopAndDelete()?

[alexeypa (please no reviews), 2012/08/30 23:15:13]

scoped_ptr<ConfigFileWatcher> will not compile if the destructor is private. I
Added DCHECK to the destructor to validate that the object has shutdown
correctly.

[Wez, 2012/08/31 00:23:04]

If the calling code isn't at liberty to delete the object then it surely
shouldn't be holding it in a scoped_ptr<> at all; presumably we only need that
in case the caller is torn down without ever having called Watch()?

Given that the caller isn't at liberty to destroy the object once Watch() has
been called, you could simply require that they call StopAndDelete() on the
object, and have it delete itself in there if Watch() has never been called?
Then the dtor can be private and the only risk is that the caller leaks it.

[alexeypa (please no reviews), 2012/08/31 21:26:55]

No, the only real need is to detect that StopAndDelete hasn't been called (by
DCHECKing in the destructor).
Leaking the object is pretty dangerous because it will render the host
unstoppable.

I split moved most of ConfigFileWatcher to a private ConfigFileWatcherImpl. What
remained in ConfigFileWatcher now implements fire-and-forget destruction, so
scoped_ptr can be used.

+
+ private:
+  // Called every time the host configuration file is updated.
+  void OnConfigUpdated(const FilePath& path, bool error);
+
+  // Reads the configuration file and passes it to the delegate.
+  void ReloadConfig();
+
+  FilePath config_path_;
+
+  scoped_ptr<base::DelayTimer<ConfigFileWatcher> > config_updated_timer_;
+
+  // Monitors the host configuration file changes.

[Wez, 2012/08/30 20:36:04]

nit: Remove "changes", or add "for" before it.

[alexeypa (please no reviews), 2012/08/30 23:15:13]

Done.

+  scoped_ptr<base::files::FilePathWatcher> config_watcher_;
+
+  base::WeakPtr<Delegate> delegate_;
+
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConfigFileWatcher);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_DAEMON_CONFIG_WATCHER_H_