RefCounted types should not have public destructors, media/ and gpu/ edition

RefCounted types should not have public destructors, media/ and gpu/ edition

BUG=123295
TEST=none


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

[Ryan Sleevi, 2012-04-13 09:40:00]

gman: gpu/
fischman: media/

http://codereview.chromium.org/10067035/diff/1/gpu/command_buffer/client/ref_...
File gpu/command_buffer/client/ref_counted.h (right):

http://codereview.chromium.org/10067035/diff/1/gpu/command_buffer/client/ref_...
gpu/command_buffer/client/ref_counted.h:12: class RefCountedThreadSafe : public
nacl::RefCounted<T> {
I don't suppose there's any chance this can be renamed? ;-)

I suspect this is going to break down in a pure-NaCL compile. This could be
worked around by having RefCounted<> take a Traits class, much like
base::RefCountedThreadSafe<> does. Then we do the exact same trick
base::RefCountedThreadSafe<> does to specialize the default traits so that
friending "gpu::RefCountedThreadSafe" in a NaCL build does the right thing.

When building on Mac, however, I have no clue how to build this target.

[greggman, 2012-04-13 17:19:24]

lgtm

http://codereview.chromium.org/10067035/diff/1/gpu/command_buffer/client/ref_...
File gpu/command_buffer/client/ref_counted.h (right):

http://codereview.chromium.org/10067035/diff/1/gpu/command_buffer/client/ref_...
gpu/command_buffer/client/ref_counted.h:12: class RefCountedThreadSafe : public
nacl::RefCounted<T> {
On 2012/04/13 09:40:00, Ryan Sleevi wrote:
> I don't suppose there's any chance this can be renamed? ;-)
> 
> I suspect this is going to break down in a pure-NaCL compile. 

This is already compiling in NaCl. What am I missing?

Feel free to change it though.

> This could be
> worked around by having RefCounted<> take a Traits class, much like
> base::RefCountedThreadSafe<> does. Then we do the exact same trick
> base::RefCountedThreadSafe<> does to specialize the default traits so that
> friending "gpu::RefCountedThreadSafe" in a NaCL build does the right thing.
> 
> When building on Mac, however, I have no clue how to build this target.

[Ryan Sleevi, 2012-04-13 17:30:01]

http://codereview.chromium.org/10067035/diff/1/gpu/command_buffer/client/ref_...
File gpu/command_buffer/client/ref_counted.h (right):

http://codereview.chromium.org/10067035/diff/1/gpu/command_buffer/client/ref_...
gpu/command_buffer/client/ref_counted.h:12: class RefCountedThreadSafe : public
nacl::RefCounted<T> {
On 2012/04/13 17:19:24, greggman wrote:
> On 2012/04/13 09:40:00, Ryan Sleevi wrote:
> > I don't suppose there's any chance this can be renamed? ;-)
> > 
> > I suspect this is going to break down in a pure-NaCL compile. 
> 
> This is already compiling in NaCl. What am I missing?
> 
> Feel free to change it though.
> 
> > This could be
> > worked around by having RefCounted<> take a Traits class, much like
> > base::RefCountedThreadSafe<> does. Then we do the exact same trick
> > base::RefCountedThreadSafe<> does to specialize the default traits so that
> > friending "gpu::RefCountedThreadSafe" in a NaCL build does the right thing.
> > 
> > When building on Mac, however, I have no clue how to build this target.
> 

It compiles because the destructors are public today.

In the GPU code, the pattern is "friend class
gpu::RefCountedThreadSafe<MyClassHere>;". Prior to this change, it worked for
both base::RCTS and nacl::RC<>, because the destructors were public.

However, when making the destructors protected/private, it doesn't, because
gpu::RCTS is not the one doing the actual deleting, so the friend specifier
gives no access to the base class, only the derived (here or on line 25). That
is, "gpu::RCTS<T>" does not delete T - the (unfriend) base::RCTS<>/nacl::RT<>
do.

This is easily worked around for the base::RCTS by making gpu::RCTS an alias via
using (since you can't typedef templates). But for nacl::RC<>, I suspect this is
going to break.

The 'trick' is the same as base::RCTS - define a traits parameter for
nacl::RC<>, have nacl::RC<> call the traits to do the delete, have the traits
call the (derived) gpu::RCTS, so that the effective "friend class gpu::RCTS<T>"
works and exposes the destructor (which is still invisible to nacl::RC<>).

The question is: What steps are needed to build and test this to work it out?

[scherkus (not reviewing), 2012-04-13 18:06:53]

fischman is pool side in mexico somewhere

lgtm w/ one nit

thanks!!

http://codereview.chromium.org/10067035/diff/1/media/audio/audio_output_dispa...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/10067035/diff/1/media/audio/audio_output_dispa...
media/audio/audio_output_dispatcher.h:77: ~AudioOutputDispatcher();
+virtual

[commit-bot: I haz the power, 2012-05-18 20:11:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10067035/13003

[commit-bot: I haz the power, 2012-05-18 21:37:07]

Change committed as 137966