Massively simplify the NetworkChangeNotifier infrastructure:...

net/base/network_change_notifier_linux.h

Index: net/base/network_change_notifier_linux.h
===================================================================
--- net/base/network_change_notifier_linux.h	(revision 50775)
+++ net/base/network_change_notifier_linux.h	(working copy)
@@ -5,40 +5,35 @@
 #ifndef NET_BASE_NETWORK_CHANGE_NOTIFIER_LINUX_H_
 #define NET_BASE_NETWORK_CHANGE_NOTIFIER_LINUX_H_
 
-#include "base/basictypes.h"
 #include "base/message_loop.h"
-#include "base/non_thread_safe.h"
-#include "base/observer_list.h"
 #include "net/base/network_change_notifier.h"
 
-#if defined(OS_CHROMEOS)
-#include "base/task.h"
-#endif
+namespace base {
+class Thread;
+}
 
 namespace net {
 
-class NetworkChangeNotifierLinux
-    : public NetworkChangeNotifier,
-      public NonThreadSafe,
-      public MessageLoopForIO::Watcher,
-      public MessageLoop::DestructionObserver {
+class NetworkChangeNotifierLinux : public MessageLoop::DestructionObserver,
+                                   public MessageLoopForIO::Watcher,
+                                   public NetworkChangeNotifier {
  public:
   NetworkChangeNotifierLinux();
 
-  // NetworkChangeNotifier methods:
-  virtual void AddObserver(Observer* observer);
-  virtual void RemoveObserver(Observer* observer);
+ private:
+  virtual ~NetworkChangeNotifierLinux();
 
-  // MessageLoopForIO::Watcher methods:
+  // MessageLoop::DestructionObserver:

[willchan no longer on Chromium, 2010/06/25 15:36:22]

There's no clear style consensus on this anywhere, so I won't say you _have_ to
do it, but I think it's best to keep these virtuals public, since they were
public in the base class.  Making them private isn't super effective, since it
can be called via a pointer to the base class anyway.  That said, if you feel
strongly about this (yeah yeah, I know the counterarguments), then it's fine as
is.

[Peter Kasting, 2010/06/25 17:51:33]

I don't feel violently.  It seems like a slight plus to enforce that people only
call interface methods through interface pointers, to prevent misuse of the
class.  Is there a benefit we get from the other way?  Less confusion when
reading the code, maybe?

[willchan no longer on Chromium, 2010/06/25 19:09:29]

Yeah, it's the confusion, because it can be misleading.  It's non-obvious when
reading this header file, and then reading some client code that holds a pointer
to the base class, that this could be called.  Changing the access control in
derived classes makes it non-obvious.

Again, this is personal style.  I'm just advocating my personal preference.  I
leave the final decision to you.

+  virtual void WillDestroyCurrentMessageLoop();
+
+  // MessageLoopForIO::Watcher:
   virtual void OnFileCanReadWithoutBlocking(int fd);
   virtual void OnFileCanWriteWithoutBlocking(int /* fd */);
 
-  // MessageLoop::DestructionObserver methods:
-  virtual void WillDestroyCurrentMessageLoop();
+  // Called on the notifier thread to initialize the notification
+  // implementation.
+  void Init();
 
- private:
-  virtual ~NetworkChangeNotifierLinux();
-
   // Starts listening for netlink messages.  Also handles the messages if there
   // are any available on the netlink socket.
   void ListenForNotifications();
@@ -48,21 +43,14 @@
   // recv() would block.  Otherwise, it returns a net error code.
   int ReadNotificationMessage(char* buf, size_t len);
 
-  // Stops watching the netlink file descriptor.
-  void StopWatching();
+  // The thread used to listen for notifications.  This relays the notification
+  // to the registered observers without posting back to the thread the object
+  // was created on.
+  scoped_ptr<base::Thread> notifier_thread_;

[willchan no longer on Chromium, 2010/06/25 15:36:22]

You are deleting includes and neglecting to include the headers you actually
use.  I'm afraid this is quite common behavior in chrome, but it's against the
style guide.  Please refer to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...
which explicitly says:
"Note: If you use a symbol Foo in your source file, you should bring in a
definition for Foo yourself, either via an #include or via a forward
declaration. Do not depend on the symbol being brought in transitively via
headers not directly included. One exception is if Foo is used in myfile.cc,
it's ok to #include (or forward-declare) Foo in myfile.h, instead of myfile.cc."

[Peter Kasting, 2010/06/25 17:51:33]

Wait, what did I delete here?  base::Thread is declared above.  Do you mean
scoped_ptr.h?  I can certainly include that.  I didn't delete it, though.  It
was never in the include list.  I just didn't notice I needed it because
everything compiled.

I added scoped_ptr.h to both this and the Mac header.

[willchan no longer on Chromium, 2010/06/25 19:09:29]

You've deleted basictypes.h, which IIRC is required for DISALLOW_*

[Peter Kasting, 2010/06/25 20:01:37]

Oh, oops.  Fixed in all the network_change_notifier*.h files.

 
-  void NotifyObserversIPAddressChanged();
+  // The netlink socket descriptor.
+  int netlink_fd_;
 
-  // http://crbug.com/36890.
-  ObserverList<Observer, false> observers_;
-
-  int netlink_fd_;  // This is the netlink socket descriptor.
-
-#if defined(OS_CHROMEOS)
-  ScopedRunnableMethodFactory<NetworkChangeNotifierLinux> factory_;
-#endif
-
-  MessageLoopForIO* loop_;
   MessageLoopForIO::FileDescriptorWatcher netlink_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierLinux);