MergeNetworkList can't group address under the same interface

content/renderer/p2p/socket_dispatcher.h

Index: content/renderer/p2p/socket_dispatcher.h
diff --git a/content/renderer/p2p/socket_dispatcher.h b/content/renderer/p2p/socket_dispatcher.h
index 6ad48c39c29e8d64f82bc5b66a14c57bc50ba9d8..ca97413062b1dd623b4b14198069e161a38a8ada 100644
--- a/content/renderer/p2p/socket_dispatcher.h
+++ b/content/renderer/p2p/socket_dispatcher.h
@@ -48,19 +48,37 @@ class P2PAsyncAddressResolver;
 class P2PSocketClientImpl;
 class RenderViewImpl;
 
-class CONTENT_EXPORT P2PSocketDispatcher : public IPC::MessageFilter {
+class P2PSocketDispatcherInterface {

[Sergey Ulanov, 2014/09/10 18:05:32]

We don't normally use Interface suffix for interfaces. Usage of interface
instead of implementation should not be penalized. Instead you can use Impl
suffix for the implementation. 

But in this case it's probably better to give this interface a different name. I
suggest NetworkListManager

[guoweis_webrtc, 2014/09/10 19:56:33]

Since it's the manager of NetworkListObserver, I name it
NetworkListObserverManager, hope it's ok.

[Sergey Ulanov, 2014/09/10 22:28:25]

This doesn't look like a good name. It doesn't manage NetworkListObserver. It's
an object that keeps current list of network interfaces and allows other object
to get notified when the list changes. Storing list of observers is something a
correct implementation has to do, but it's not the core responsibility of
objects that implement this insterface.

[guoweis_webrtc, 2014/09/10 23:22:25]

Changed to NetworkListManager

+ public:
+  // Add a new network list observer.
+  virtual void AddNetworkListObserver(
+    NetworkListObserver* network_list_observer) = 0;
+
+  // Removes network list observer.
+  virtual void RemoveNetworkListObserver(
+    NetworkListObserver* network_list_observer) = 0;
+
+ protected:
+  // Marked as protected to prevent explicit deletion, as
+  // P2PSocketDispatcher is not owned by IpcNetworkManager.
+  virtual ~P2PSocketDispatcherInterface() {}
+};
+
+class CONTENT_EXPORT P2PSocketDispatcher
+ : public IPC::MessageFilter, public P2PSocketDispatcherInterface {

[Sergey Ulanov, 2014/09/10 18:05:32]

indentation.

[guoweis_webrtc, 2014/09/10 19:56:33]

Done.

  public:
   explicit P2PSocketDispatcher(base::MessageLoopProxy* ipc_message_loop);
 
-  // Add a new network list observer. Each observer is called
-  // immidiately after it is registered and then later whenever
-  // network configuration changes. Can be called on any thread. The
-  // observer is always called on the thread it was added.
-  void AddNetworkListObserver(NetworkListObserver* network_list_observer);
+  // Each observer is called immidiately after it is registered and

[Sergey Ulanov, 2014/09/10 18:05:32]

This comment should be in the interface definition. In the implementation you
just need one like like this:
  // P2PSocketDispatcher interface:

[guoweis_webrtc, 2014/09/10 19:56:33]

I feel how it should be called is really an implementation detail such that I
put it here. For example, the comment doesn't really apply with the Mock one
that I created

[Sergey Ulanov, 2014/09/10 22:28:25]

These are not implementation details - they are part of the interface contract
that should be documented in the interface. Not knowing these details about
interface makes it impossible to use it correctly. Even though your Mock doesn't
really abide by that contract the test overall still does - it still calls the
observer after AddNetworkListObserver() .

[guoweis_webrtc, 2014/09/10 23:22:25]

Done.

+  // then later whenever network configuration changes. Can be called
+  // on any thread. The observer is always called on the thread it was
+  // added.
+  virtual void AddNetworkListObserver(
+    NetworkListObserver* network_list_observer) OVERRIDE;
 
-  // Removes network list observer. Must be called on the thread on
-  // which the observer was added.
-  void RemoveNetworkListObserver(NetworkListObserver* network_list_observer);
+  // Must be called on the thread on which the observer was added.
+  virtual void RemoveNetworkListObserver(
+    NetworkListObserver* network_list_observer) OVERRIDE;
 
  protected:
   virtual ~P2PSocketDispatcher();