RefCounted types should not have public destructors, content/browser part 2

RefCounted types should not have public destructors, content/browser part 2

BUG=123295
TEST=none
TBR=brettw

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134446

[Ryan Sleevi, 2012-04-13 09:26:25]

jamesr: PTAL

[jamesr, 2012-04-13 18:29:28]

LGTM but as we discussed in IRC I wonder if virtual d'tors should really be
protected, since if a class has virtuals then that normally means there might be
subclasses any potential subclass will definitely need access to the d'tor.  Use
your best judgement.

I also don't have OWNERS for most of this patch, FYI

[Ryan Sleevi, 2012-04-27 20:06:52]

scherkus: Since jamesr raised OWNERs concerns, content/renderer/media
piman: content/common/gpu & content/gpu

Note, I'm in process of addressing James' concerns by making the dtors
protected, rather than private, and addressing pkasting's concerns by making
sure ordering matches. This is mostly just stamp collecting for any concerns.

[Ryan Sleevi, 2012-04-27 20:07:22]

er, +scherkus, +piman for real this time

(Previous message below)

scherkus: Since jamesr raised OWNERs concerns, content/renderer/media
piman: content/common/gpu & content/gpu

Note, I'm in process of addressing James' concerns by making the dtors
protected, rather than private, and addressing pkasting's concerns by making
sure ordering matches. This is mostly just stamp collecting for any concerns.

[piman, 2012-04-27 20:11:01]

LGTM from my part, if that's really what we want (I let C++ gurus weigh in on
that).

[scherkus (not reviewing), 2012-04-27 20:59:48]

LGTM w/ some nits

the patch seems to be inconsistent in its usage between:

private:
  friend class base::RefCountedThreadSafe<T>;
  friend class SomeMockClassThatExtendsT;
  virtual ~T();

-- and --

protected:
  friend class base::RefCountedThreadSafe<T>;
  virtual ~T();

private:
  // other crap



However I don't really mind which one you go with :)

http://codereview.chromium.org/10071038/diff/1/content/renderer/media/audio_i...
File content/renderer/media/audio_input_message_filter.h (right):

http://codereview.chromium.org/10071038/diff/1/content/renderer/media/audio_i...
content/renderer/media/audio_input_message_filter.h:23: : public
IPC::ChannelProxy::MessageFilter {
nit: want to indent this by 2 more spaces?

http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_impl_manager.h (right):

http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_impl_manager.h:49: friend class
MockVideoCaptureImplManager;
is this needed because the dtor isn't protected?

http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
File content/renderer/media/video_capture_message_filter.h (right):

http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
content/renderer/media/video_capture_message_filter.h:63: friend class
MockVideoCaptureMessageFilter;
nit: is this needed because dtor isn't protected?

[Ryan Sleevi, 2012-04-27 23:33:16]

On 2012/04/27 20:59:48, scherkus wrote:
> LGTM w/ some nits
> 
> the patch seems to be inconsistent in its usage between:
> 
> private:
>   friend class base::RefCountedThreadSafe<T>;
>   friend class SomeMockClassThatExtendsT;
>   virtual ~T();
> 
> -- and --
> 
> protected:
>   friend class base::RefCountedThreadSafe<T>;
>   virtual ~T();
> 
> private:
>   // other crap
> 

Yeah, that's what was (and now is) being fixed.

If it implements an interface, that interface is invariably protected, and so
the protected-ness was kept. This resolves the Mock issues you pointed out
further.

If it was RCTS itself, however, and not implementing an interface, it was kept
private, unless it was exposing virtuals.

> 
> 
> However I don't really mind which one you go with :)
> 
>
http://codereview.chromium.org/10071038/diff/1/content/renderer/media/audio_i...
> File content/renderer/media/audio_input_message_filter.h (right):
> 
>
http://codereview.chromium.org/10071038/diff/1/content/renderer/media/audio_i...
> content/renderer/media/audio_input_message_filter.h:23: : public
> IPC::ChannelProxy::MessageFilter {
> nit: want to indent this by 2 more spaces?

Done

> 
>
http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
> File content/renderer/media/video_capture_impl_manager.h (right):
> 
>
http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
> content/renderer/media/video_capture_impl_manager.h:49: friend class
> MockVideoCaptureImplManager;
> is this needed because the dtor isn't protected?
> 
>
http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
> File content/renderer/media/video_capture_message_filter.h (right):
> 
>
http://codereview.chromium.org/10071038/diff/1/content/renderer/media/video_c...
> content/renderer/media/video_capture_message_filter.h:63: friend class
> MockVideoCaptureMessageFilter;
> nit: is this needed because dtor isn't protected?

[Ryan Sleevi, 2012-04-28 04:10:06]

TBR brettw for the few scattered files that are bubbling up / one offs (3 files
in content/common, 2 in content/gpu, 2 in content/common/indexed_db)

[commit-bot: I haz the power, 2012-04-28 04:10:18]

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

[commit-bot: I haz the power, 2012-04-28 04:10:42]

Presubmit check for 10071038-14004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
New code should not use ScopedAllowIO. Post a task to the blocking pool or the
FILE thread instead.
    content/renderer/media/audio_input_device.cc:267

Presubmit checks took 2.8s to calculate.