[net/dns] Watch configd notifications to detect DnsConfig changes on Mac.

net/dns/notify_watcher_mac.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.
+

[Mark Mentovai, 2012/04/17 16:27:43]

You need #include guards.

[szym, 2012/04/17 19:39:31]

Oops. 

+#include "base/callback.h"
+#include "base/message_loop.h"
+
+namespace net {
+
+// Receives notifications from notify_register and passes result to a Callback.

[Mark Mentovai, 2012/04/17 16:27:43]

This doesn’t explain what notify_register is, and that word doesn’t appear
elsewhere in the file.

+class NotifyWatcherMac : public MessageLoopForIO::Watcher {
+ public:
+  typedef base::Callback<void(bool succeeded)> CallbackType;
+
+  NotifyWatcherMac();
+  // When deleted, automatically cancels.

[Mark Mentovai, 2012/04/17 16:27:43]

Blank line before this. That helps the reader “see” this as something new and
not just garbage following the constructor on the preceding line.

+  virtual ~NotifyWatcherMac();
+
+  // Registers for notifications for |key|. Returns true if succeeds. If so,
+  // will pass true to |callback| on notification, and false on error.

[Mark Mentovai, 2012/04/17 16:27:43]

This comment is a little bit confusing to me: is the argument to the callback
always the same as the return value from this function? Does the callback even
get called on error?

[szym, 2012/04/17 19:39:31]

The callback is never called synchronously from Watch(), but if in
OnFileCanReadWithoutBlocking the read fails, I take it as an error, and signal
it through the callback.

+  // After failure the watch is cancelled and will have to be restarted.
+  bool Watch(const char* key, const CallbackType& callback);
+
+  // Cancels the watch.
+  void Cancel();
+
+ private:
+  // MessageLoopForIO::Watcher:
+  virtual void OnFileCanReadWithoutBlocking(int /* fd */) OVERRIDE;

[Mark Mentovai, 2012/04/17 16:27:43]

The /* fd */ shouldn’t be necessary, because we don’t ever build with
-Wunused-argument. Same on the next line.

+  virtual void OnFileCanWriteWithoutBlocking(int /* fd */) OVERRIDE {}
+
+  int notify_fd_;
+  int notify_token_;
+  CallbackType callback_;
+  MessageLoopForIO::FileDescriptorWatcher watcher_;
+};

[Mark Mentovai, 2012/04/17 16:27:43]

DISALLOW_COPY_AND_ASSIGN.

+
+}  // namespace net
+

[Mark Mentovai, 2012/04/17 16:27:43]

Get rid of the blank line at the end of the file.

[szym, 2012/04/17 19:39:31]

Ok, although if you look at Patch Set 1, you'll see that Lint complains about
that. Is Lint buggy?

[Mark Mentovai, 2012/04/17 19:47:39]

szym wrote:
The lint on Rietveld is buggy for added files. Patch set 1 was correct. If the
file truly didn’t end with a newline, the patch would have said something like
“\No newline at end of file”. That would be a problem. lint wants to force you
to put TWO newlines at the end of a file, which is one line-ender and one blank
line, and this is wrong.