RefCounted types should not have public destructors

RefCounted types should not have public destructors

BUG=123295
TEST=existing unittests


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

[Ryan Sleevi, 2012-07-09 04:08:04]

jar: base/, chrome/browser/net
brettw: ipc/, ppapi/
piman: content/common/gpu, gpu/
kalman: chrome/browser/extensions

[Ryan Sleevi, 2012-07-09 04:10:42]

As an additional note of context:

While several of these classes may not be base::RefCounted<> themselves (eg:
ipc/), there are RefCounted implementations deriving from these interfaces. This
is similar to the big delegate cleanup of http://crrev.com/144086

These were detected with the following change to the Chromium clang plugin -
http://codereview.chromium.org/10407057/ . Many more issues exist, these were
just some of the low-hanging fruit that could easily be made protected, thereby
avoiding having to redesign the derived classes.

[not at google - send to devlin, 2012-07-09 04:38:32]

extensions lgtm

[piman, 2012-07-09 05:33:30]

lgtm

[jar (doing other things), 2012-07-09 16:51:44]

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe.h
File base/threading/non_thread_safe.h (right):

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe...
base/threading/non_thread_safe.h:65: typedef NonThreadSafeImpl NonThreadSafe;
Can you motivate this change? This opens a tad more functionality than the old
code... so the question is: What methods or slots were you hoping to access on a
typedef'ed NonThreadSafe that you couldn't access when derived?
Perchance you should include the answer in a comment here.

[Ryan Sleevi, 2012-07-09 23:27:54]

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe.h
File base/threading/non_thread_safe.h (right):

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe...
base/threading/non_thread_safe.h:65: typedef NonThreadSafeImpl NonThreadSafe;
On 2012/07/09 16:51:45, jar wrote:
> Can you motivate this change? This opens a tad more functionality than the old
> code... so the question is: What methods or slots were you hoping to access on
a
> typedef'ed NonThreadSafe that you couldn't access when derived?
> Perchance you should include the answer in a comment here.

More aptly, it closes functionality, not opens it up. Specifically, it doesn't
allow the C++ default (constructor, copy operator, destructor) to be created,
since they're all created at public visibility. Instead, it leaves the
visibility as specified by the the lower layer - which includes keeping the
destructor protected (rather than implicitly public)

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe...
base/threading/non_thread_safe.h:67: typedef NonThreadSafeDoNothing
NonThreadsafe;
Incidentally, my shift-key appears to be broken on my home machine, and this
would not have shown up on try bots :-/

[jar (doing other things), 2012-07-09 23:33:51]

LGTM for base

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe.h
File base/threading/non_thread_safe.h (right):

http://codereview.chromium.org/10694111/diff/1/base/threading/non_thread_safe...
base/threading/non_thread_safe.h:65: typedef NonThreadSafeImpl NonThreadSafe;
SGTM (my confusion in my comment).

On 2012/07/09 23:27:54, Ryan Sleevi wrote:
> On 2012/07/09 16:51:45, jar wrote:
> > Can you motivate this change? This opens a tad more functionality than the
old
> > code... so the question is: What methods or slots were you hoping to access
on
> a
> > typedef'ed NonThreadSafe that you couldn't access when derived?
> > Perchance you should include the answer in a comment here.
> 
> More aptly, it closes functionality, not opens it up. Specifically, it doesn't
> allow the C++ default (constructor, copy operator, destructor) to be created,
> since they're all created at public visibility. Instead, it leaves the
> visibility as specified by the the lower layer - which includes keeping the
> destructor protected (rather than implicitly public)

[brettw, 2012-07-10 05:48:42]

ipc/ppapi LGTM

[commit-bot: I haz the power, 2012-07-10 13:07:03]

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

[commit-bot: I haz the power, 2012-07-10 13:21:45]

Try job failure for 10694111-10002 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[Ryan Sleevi, 2012-07-10 22:58:38]

http://codereview.chromium.org/10694111/diff/10002/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

http://codereview.chromium.org/10694111/diff/10002/base/memory/weak_ptr.h#new...
base/memory/weak_ptr.h:167: ~SupportsWeakPtrBase() {}
jar:

Unfortunately, for Windows this requires wrapping SupportsWeakPtr<> with a
NON_EXPORTED_BASE if any of the derived types have _EXPORT/_EXPORT_PRIVATE when
building a shared-library build. We already require this for the other
'attribute' types (like base::NonThreadSafe), but since it means this CL will
require touching a lot more code, I removed it for now, and will ping you about
it later.

[commit-bot: I haz the power, 2012-07-10 22:58:49]

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

[commit-bot: I haz the power, 2012-07-11 00:07:56]

Change committed as 146001