DnsConfigService and a posix implementation

net/dns/dns_config_service_posix.h

+// Copyright (c) 2011 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.
+
+#include <resolv.h>
+
+#include "base/compiler_specific.h"
+#include "base/files/file_path_watcher.h"
+#include "base/memory/ref_counted.h"
+#include "base/observer_list.h"
+#include "base/scoped_ptr.h"
+#include "net/dns/dns_config_service.h"
+
+namespace net {
+
+// Fills in |dns_config| from |res|. Exposed for tests.
+bool ConvertResToConfig(const struct __res_state& res, DnsConfig* dns_config);
+
+// Allows mocking res_ninit.
+struct ResolverLib {

[cbentzel, 2011/08/15 18:16:34]

struct's are intended for buckets-of-bits only. Once you add behavioral stuff
like virtuals it should be a class.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

[cbentzel, 2011/08/15 18:16:34]

You may also want to make this listed as NonThreadSafe to help check that it's
only called from one worker-thread. It's a little complicated in this case
because you will have to do a DetachFromThread each time you post to the worker
thread, and again each time that you come back from the worker thread.

[szym, 2011/08/15 22:02:01]

Done.

+  virtual ~ResolverLib() {}
+  virtual int ninit(res_state statp) {
+    return ::res_ninit(statp);
+  }
+  virtual void nclose(res_state statp) {
+    return ::res_nclose(statp);
+  }

[cbentzel, 2011/08/15 18:16:34]

DISALLOW_COPY_AND_ASSIGN

[szym, 2011/08/15 22:02:01]

Done.

+};
+
+// Allows mocking FilePathWatcher
+class FilePathWatcherShim {

[cbentzel, 2011/08/15 18:16:34]

I'm wondering if these should be nested classes of DnsConfigServicePosix, since
the names are fairly generic and I'm not sure if they should be part of the net
namespace.

[szym, 2011/08/15 22:02:01]

Done.

+ public:
+  FilePathWatcherShim();
+  virtual ~FilePathWatcherShim() {}
+
+  virtual bool Watch(
+      const FilePath& path,
+      base::files::FilePathWatcher::Delegate* delegate) WARN_UNUSED_RESULT;
+ private:
+  scoped_ptr<base::files::FilePathWatcher> watcher_;

[cbentzel, 2011/08/15 18:16:34]

DISALLOW_COPY_AND_ASSIGN

[szym, 2011/08/15 22:02:01]

Done.

+};
+
+struct FilePathWatcherFactory {

[cbentzel, 2011/08/15 18:16:34]

Ditto here.

[szym, 2011/08/15 22:02:01]

Done.

+  virtual ~FilePathWatcherFactory() {}
+  virtual FilePathWatcherShim* CreateFilePathWatcher() {
+    return new FilePathWatcherShim();
+  }
+};
+
+class WatcherDelegate;
+
+class DnsConfigServicePosix : public DnsConfigService {

[cbentzel, 2011/08/15 18:16:34]

Could this inherit from NonThreadSafe [from base/threading/non_thread_safe] and
add appropriate DCHECK's in the functions? This is intended to be used on only
one thread. 

[szym, 2011/08/15 22:02:01]

Done.

+ public:
+  DnsConfigServicePosix();
+  virtual ~DnsConfigServicePosix();
+
+  virtual void AddObserver(Observer* observer) OVERRIDE {

[cbentzel, 2011/08/15 18:16:34]

Don't inline virtual methods. There's a number of rules about when inline'ing is
generally OK in Chrome, see
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts for details.
I'm not going to mention each one in this CL, so please do a clean up pass.

[cbentzel, 2011/08/15 18:16:34]

Thanks for adding OVERRIDE.

[szym, 2011/08/15 22:02:01]

Done. Although only in .h files.

+    observers_.AddObserver(observer);
+    if (have_config_) {

[cbentzel, 2011/08/15 18:16:34]

Good idea, cool. 

+      observer->OnConfigChanged(dns_config_);
+    }
+  }
+
+  virtual void RemoveObserver(Observer* observer) OVERRIDE {
+    observers_.RemoveObserver(observer);
+  }
+
+  virtual void Watch() OVERRIDE;
+
+  // Takes ownership of |lib|. Must be set before Watch.
+  void set_resolver_lib(ResolverLib* lib) {
+    DCHECK(!watcher_delegate_.get());
+    resolver_lib_.reset(lib);
+  }
+
+  // Takes ownership of |factory|. Must be set before Watch.
+  void set_watcher_factory(FilePathWatcherFactory* factory) {
+    DCHECK(!watcher_delegate_.get());
+    watcher_factory_.reset(factory);
+  }
+
+ private:
+  friend class WatcherDelegate;
+
+  void OnConfigRead(const DnsConfig& config) {
+    if (!config.Equals(dns_config_)) {
+      dns_config_ = config;
+      have_config_ = true;
+      FOR_EACH_OBSERVER(Observer, observers_, OnConfigChanged(config));
+    }
+  }
+
+  // Configure a FilePathWatcher and install watcher_delegate_. Executed each
+  // time FilePathWatcher fails.
+  void StartWatch();
+
+  DnsConfig dns_config_;
+  // True after first OnConfigChanged, that is, dns_config_ is valid.
+  bool have_config_;
+
+  scoped_ptr<ResolverLib> resolver_lib_;
+  scoped_ptr<FilePathWatcherFactory> watcher_factory_;
+  scoped_ptr<FilePathWatcherShim> resolv_file_watcher_;
+  scoped_refptr<WatcherDelegate> watcher_delegate_;
+  ObserverList<Observer> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(DnsConfigServicePosix);
+};
+
+

[cbentzel, 2011/08/15 18:16:34]

Nit: extra new line

[szym, 2011/08/15 22:02:01]

Done.

+}  // namespace net
+