Add deletion observer to P2PSocketDispatcher.

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 8cebd5190f221f9eb26568fb7e6ac149482da7f3..7c54d6b9035d0c296349580bab70de25ed4cb039 100644
--- a/content/renderer/p2p/socket_dispatcher.h
+++ b/content/renderer/p2p/socket_dispatcher.h
@@ -52,6 +52,18 @@ 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);
@@ -69,6 +81,9 @@ class CONTENT_EXPORT P2PSocketDispatcher : public content::RenderViewObserver {
   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;
 
@@ -110,6 +125,7 @@ class CONTENT_EXPORT P2PSocketDispatcher : public content::RenderViewObserver {
       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);
 };