Add deletion observer to P2PSocketDispatcher.

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 34 matching lines @@
 
 namespace webkit_glue {
 class NetworkListObserver;
 }  // webkit_glue
 
 namespace content {
 
 class P2PHostAddressRequest;
 class P2PSocketClient;
 
+// Callback interface that allows the implementor to be notified before a
+// P2PSocketDispatcher is deleted.
+// P2PSocketDispatcher requires that all NetworkListObservers are
+// unregistered when SocketDispatcherGone is called.
+class P2PSocketDispatcherObserver {

[Sergey Ulanov, 2012/05/17 18:20:15]

Can this be defined inside of P2PSocketDispatcher?

[perkj_chrome, 2012/05/18 07:54:25]

Yes, but last time I did it triggered this long discussion a month later:
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

So now the style guide explicitly says this type of classes should be in
separate header files, which to me seems like overkill...

[Sergey Ulanov, 2012/05/21 17:57:19]

Well, that's very arguable rule, not everybody agrees with it, and there are
tons of code that doesn't follow that rule.
In any case, the whole point of that rule is to speed up compilation by
separating class definitions into separate headers. In this particular case,
because there is only a handful of files that include this header, I highly
doubt that this approach can improve compilation time. Nonetheless, if you
decided to go by the letter of the style guide, then you should also move this
class to a separate header. If you keep it in this header, then there is no
reason not to make it nested.

+ public:
+  virtual void SocketDispatcherGone() = 0;

[Sergey Ulanov, 2012/05/17 18:20:15]

OnSocketDispatcherDestroyed()

[perkj_chrome, 2012/05/18 07:54:25]

Done.

+
+ protected:
+  ~P2PSocketDispatcherObserver() {}

[Sergey Ulanov, 2012/05/17 18:20:15]

virtual

[perkj_chrome, 2012/05/18 07:54:25]

ok, this is something that comes up from time to time. Some people claim it
should not be virtual since the implementation should never be deleted through
this interface.

[Sergey Ulanov, 2012/05/21 17:57:19]

The style guide says that ALL interfaces must have a virtual destructor.

+};
+
 class CONTENT_EXPORT P2PSocketDispatcher : public content::RenderViewObserver {
  public:
   explicit P2PSocketDispatcher(RenderViewImpl* render_view);
   virtual ~P2PSocketDispatcher();
 
   // 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(
       webkit_glue::NetworkListObserver* network_list_observer);
 
   // Removes network list observer. Must be called on the thread on
   // which the observer was added.
   void RemoveNetworkListObserver(
       webkit_glue::NetworkListObserver* network_list_observer);
 
+  void AddDeletionObserver(P2PSocketDispatcherObserver* observer);

[Sergey Ulanov, 2012/05/17 18:20:15]

Either the method should be called AddObserver() or the observer interface
should be called P2PSocketDispatcherDeletionObserver. If you move that interface
inside P2PSocketDispatcher then you can call it simply DeletionObserver.

minor-nit: It's more common to call it destruction observer instead of deletion
observer. "Deletion" assumes that the object is allocated on the heap.

[perkj_chrome, 2012/05/18 07:54:25]

Done.

+  void RemoveDeletionObserver(P2PSocketDispatcherObserver* observer);
+
   // RenderViewObserver overrides.
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
  private:
   friend class P2PHostAddressRequest;
   friend class P2PSocketClient;
   class AsyncMessageSender;
 
   base::MessageLoopProxy* message_loop();
 
@@ skipping 21 matching lines @@
   scoped_refptr<base::MessageLoopProxy> message_loop_;
   IDMap<P2PSocketClient> clients_;
 
   IDMap<P2PHostAddressRequest> host_address_requests_;
 
   bool network_notifications_started_;
   scoped_refptr<ObserverListThreadSafe<webkit_glue::NetworkListObserver> >
       network_list_observers_;
 
   scoped_refptr<AsyncMessageSender> async_message_sender_;
+  ObserverList<P2PSocketDispatcherObserver> deletion_observers_;

[Sergey Ulanov, 2012/05/17 18:20:15]

nit: add empty line above this one.

[perkj_chrome, 2012/05/18 07:54:25]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(P2PSocketDispatcher);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_P2P_SOCKET_DISPATCHER_H_