Add deletion observer to P2PSocketDispatcher.

Add deletion observer to P2PSocketDispatcher.

This is an alternative solution to http://codereview.chromium.org/10332114/

RenderViewObserver is actually destroyed in the same order as they are created. 
Ie 
P2PSocketDispatcher::P2PSocketDispatcher 
MediaStreamImpl::MediaStreamImpl 
P2PSocketDispatcher::~P2PSocketDispatcher 
MediaStreamImpl::~MediaStreamImpl 

Since MediaStreamImpl holds an observer object to P2PSocketDispatcher and the P2PSocketDispatcher require all observers to have unregistered before it is deleted, it is required that the MediaStreamImpl is either destroyed before P2PSocketDispatcher or that it is notified that the 
PSocketDispatcher will be destroyed. 

BUG=127782
TEST= Please see the defect


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138066

[perkj_chrome, 2012-05-15 16:18:16]

Hej, here comes an alternative to http://codereview.chromium.org/10332114/

Lets see tomorrow which one to pursue.

[perkj_chrome, 2012-05-17 11:36:20]

Hi, sergeyu, can you please review this cl? 
Please also see the discussion in by first attempt to fix the same problem
http://codereview.chromium.org/10332114/

Thanks
Per

[tommi (sloooow) - chröme, 2012-05-17 11:58:16]

lgtm. looks hacky but I don't see an alternative right now besides redesigning
how the whole thing hangs together.

[piman, 2012-05-17 16:40:30]

LGTM. I like this approach a lot better where dependencies are explicit.

[Sergey Ulanov, 2012-05-17 18:20:14]

I have some style nits. LGTM when they are addressed.

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
File content/renderer/p2p/socket_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:59: class P2PSocketDispatcherObserver {
Can this be defined inside of P2PSocketDispatcher?

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:61: virtual void SocketDispatcherGone()
= 0;
OnSocketDispatcherDestroyed()

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:64: ~P2PSocketDispatcherObserver() {}
virtual

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:84: void
AddDeletionObserver(P2PSocketDispatcherObserver* observer);
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.

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:128:
ObserverList<P2PSocketDispatcherObserver> deletion_observers_;
nit: add empty line above this one.

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

Thanks, sergeyu, can you have another look?

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
File content/renderer/p2p/socket_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:59: class P2PSocketDispatcherObserver {
On 2012/05/17 18:20:15, sergeyu wrote:
> Can this be defined inside of P2PSocketDispatcher?

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...

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:61: virtual void SocketDispatcherGone()
= 0;
On 2012/05/17 18:20:15, sergeyu wrote:
> OnSocketDispatcherDestroyed()

Done.

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:64: ~P2PSocketDispatcherObserver() {}
On 2012/05/17 18:20:15, sergeyu wrote:
> virtual
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.

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:84: void
AddDeletionObserver(P2PSocketDispatcherObserver* observer);
On 2012/05/17 18:20:15, sergeyu wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:128:
ObserverList<P2PSocketDispatcherObserver> deletion_observers_;
On 2012/05/17 18:20:15, sergeyu wrote:
> nit: add empty line above this one.

Done.

[piman, 2012-05-18 20:11:23]

On Fri, May 18, 2012 at 12:54 AM, <perkj@chromium.org> wrote:

> Thanks, sergeyu, can you have another look?
>
>
>
> https://chromiumcodereview.**appspot.com/10387135/diff/**
>
3001/content/renderer/p2p/**socket_dispatcher.h<https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2p/socket_dispatcher.h>
> File content/renderer/p2p/socket_**dispatcher.h (right):
>
> https://chromiumcodereview.**appspot.com/10387135/diff/**
>
3001/content/renderer/p2p/**socket_dispatcher.h#newcode59<https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2p/socket_dispatcher.h#newcode59>
> content/renderer/p2p/socket_**dispatcher.h:59: class
> P2PSocketDispatcherObserver {
> On 2012/05/17 18:20:15, sergeyu wrote:
>
>> Can this be defined inside of P2PSocketDispatcher?
>>
>
> 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/thread/**d54af6cd391a0af9/**942e237d1ffc077a?lnk=st&q=**
>
fixing+vs2005+build&pli=1#<https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/d54af6cd391a0af9/942e237d1ffc077a?lnk=st&q=fixing+vs2005+build&pli=1#>
>
> So now the style guide explicitly says this type of classes should be in
> separate header files, which to me seems like overkill...
>
>
> https://chromiumcodereview.**appspot.com/10387135/diff/**
>
3001/content/renderer/p2p/**socket_dispatcher.h#newcode61<https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2p/socket_dispatcher.h#newcode61>
> content/renderer/p2p/socket_**dispatcher.h:61: virtual void
> SocketDispatcherGone() = 0;
> On 2012/05/17 18:20:15, sergeyu wrote:
>
>> OnSocketDispatcherDestroyed()
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10387135/diff/**
>
3001/content/renderer/p2p/**socket_dispatcher.h#newcode64<https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2p/socket_dispatcher.h#newcode64>
> content/renderer/p2p/socket_**dispatcher.h:64:
> ~P2PSocketDispatcherObserver() {}
> On 2012/05/17 18:20:15, sergeyu wrote:
>
>> virtual
>>
> 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.


The style guide s pretty clear about that:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
"If your class has virtual methods, its destructor should be virtual."


>
> https://chromiumcodereview.**appspot.com/10387135/diff/**
>
3001/content/renderer/p2p/**socket_dispatcher.h#newcode84<https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2p/socket_dispatcher.h#newcode84>
> content/renderer/p2p/socket_**dispatcher.h:84: void
> AddDeletionObserver(**P2PSocketDispatcherObserver* observer);
> On 2012/05/17 18:20:15, sergeyu wrote:
>
>> Either the method should be called AddObserver() or the observer
>>
> interface
>
>> should be called P2PSocketDispatcherDeletionObs**erver. 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.
>>
>
> Done.
>
>
> https://chromiumcodereview.**appspot.com/10387135/diff/**
>
3001/content/renderer/p2p/**socket_dispatcher.h#newcode128<https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2p/socket_dispatcher.h#newcode128>
> content/renderer/p2p/socket_**dispatcher.h:128:
> ObserverList<**P2PSocketDispatcherObserver> deletion_observers_;
> On 2012/05/17 18:20:15, sergeyu wrote:
>
>> nit: add empty line above this one.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/10387135/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-05-20 07:34:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10387135/6003

[commit-bot: I haz the power, 2012-05-20 08:17:29]

Try job failure for 10387135-6003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-20 15:43:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10387135/10004

[commit-bot: I haz the power, 2012-05-20 16:57:01]

Change committed as 138066

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

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
File content/renderer/p2p/socket_dispatcher.h (right):

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:59: class P2PSocketDispatcherObserver {
On 2012/05/18 07:54:25, perkj wrote:
> On 2012/05/17 18:20:15, sergeyu wrote:
> > Can this be defined inside of P2PSocketDispatcher?
> 
> 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...
> 

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.

https://chromiumcodereview.appspot.com/10387135/diff/3001/content/renderer/p2...
content/renderer/p2p/socket_dispatcher.h:64: ~P2PSocketDispatcherObserver() {}
On 2012/05/18 07:54:25, perkj wrote:
> On 2012/05/17 18:20:15, sergeyu wrote:
> > virtual
> 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.

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