MergeNetworkList can't group address under the same interface

content/renderer/p2p/socket_dispatcher.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.
 
 // P2PSocketDispatcher is a per-renderer object that dispatchers all
 // P2P messages received from the browser and relays all P2P messages
 // sent to the browser. P2PSocketClient instances register themselves
 // with the dispatcher using RegisterClient() and UnregisterClient().
 //
 // Relationship of classes.
@@ skipping 30 matching lines @@
 class IPEndPoint;
 }  // namespace net
 
 namespace content {
 
 class NetworkListObserver;
 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();
 
  private:
   friend class P2PAsyncAddressResolver;
   friend class P2PSocketClientImpl;
 
   // Send a message asynchronously.
   virtual void Send(IPC::Message* message);
@@ skipping 42 matching lines @@
       network_list_observers_;
 
   IPC::Sender* sender_;
 
   DISALLOW_COPY_AND_ASSIGN(P2PSocketDispatcher);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_P2P_SOCKET_DISPATCHER_H_