Remove unneeded destructors and add finalization support to various classes.

Remove unneeded destructors and add finalization support to various classes.

Issues reported by the Blink GC clang plugin.

R=haraken@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168568

[kouhei (in TOK), 2014-03-05 07:05:01]

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
File Source/core/css/CSSBasicShapes.h (left):

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
Source/core/css/CSSBasicShapes.h:64: virtual ~CSSBasicShape() { }
Isn't this needed for RefCounted<CSSBasicType>, built without enable_oilpan=1?

[zerny-chromium, 2014-03-05 07:07:04]

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
File Source/core/css/CSSBasicShapes.h (left):

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
Source/core/css/CSSBasicShapes.h:64: virtual ~CSSBasicShape() { }
On 2014/03/05 07:05:01, kouhei wrote:
> Isn't this needed for RefCounted<CSSBasicType>, built without enable_oilpan=1?

It is a no-op, so the default generated destructor should be sufficient. Is the
default not generated here for some reason?

[haraken, 2014-03-05 07:08:20]

LGTM

[haraken, 2014-03-05 07:09:15]

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
File Source/core/css/CSSBasicShapes.h (left):

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
Source/core/css/CSSBasicShapes.h:64: virtual ~CSSBasicShape() { }
On 2014/03/05 07:07:04, zerny-chromium wrote:
> On 2014/03/05 07:05:01, kouhei wrote:
> > Isn't this needed for RefCounted<CSSBasicType>, built without
enable_oilpan=1?
> 
> It is a no-op, so the default generated destructor should be sufficient. Is
the
> default not generated here for some reason?

Probably the default destructor is not virtual?

[zerny-chromium, 2014-03-05 07:11:33]

On 2014/03/05 07:07:04, zerny-chromium wrote:
>
https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
> File Source/core/css/CSSBasicShapes.h (left):
> 
>
https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
> Source/core/css/CSSBasicShapes.h:64: virtual ~CSSBasicShape() { }
> On 2014/03/05 07:05:01, kouhei wrote:
> > Isn't this needed for RefCounted<CSSBasicType>, built without
enable_oilpan=1?
> 
> It is a no-op, so the default generated destructor should be sufficient. Is
the
> default not generated here for some reason?

Ah, yes. RefCounted and RefCountedBase have non-virtual protected destructors.
Adding an #if !OILPAN_ENABLED

[kouhei (in TOK), 2014-03-05 07:11:55]

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
File Source/core/css/CSSBasicShapes.h (left):

https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
Source/core/css/CSSBasicShapes.h:64: virtual ~CSSBasicShape() { }
On 2014/03/05 07:07:04, zerny-chromium wrote:
> On 2014/03/05 07:05:01, kouhei wrote:
> > Isn't this needed for RefCounted<CSSBasicType>, built without
enable_oilpan=1?
> 
> It is a no-op, so the default generated destructor should be sufficient. Is
the
> default not generated here for some reason?

RefCountedBase's d-tor isn't declared virtual, and there are
RefPtr<CSSBasicShape>, so I thought derived classes' implicitly declared d-tor
like ~CSSBasicShapeRectangle() wouldn't be called without this explicit virtual
decl.

[zerny-chromium, 2014-03-05 07:20:01]

On 2014/03/05 07:11:55, kouhei wrote:
>
https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
> File Source/core/css/CSSBasicShapes.h (left):
> 
>
https://codereview.chromium.org/184853006/diff/1/Source/core/css/CSSBasicShap...
> Source/core/css/CSSBasicShapes.h:64: virtual ~CSSBasicShape() { }
> On 2014/03/05 07:07:04, zerny-chromium wrote:
> > On 2014/03/05 07:05:01, kouhei wrote:
> > > Isn't this needed for RefCounted<CSSBasicType>, built without
> enable_oilpan=1?
> > 
> > It is a no-op, so the default generated destructor should be sufficient. Is
> the
> > default not generated here for some reason?
> 
> RefCountedBase's d-tor isn't declared virtual, and there are
> RefPtr<CSSBasicShape>, so I thought derived classes' implicitly declared d-tor
> like ~CSSBasicShapeRectangle() wouldn't be called without this explicit
virtual
> decl.

And you are right. I've uploaded a new patch using #if's to keep the dtors in
the non-oilpan build.

Thanks for spotting it!

[zerny-chromium, 2014-03-05 07:50:17]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-05 07:50:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/184853006/20001

[zerny-chromium, 2014-03-05 08:02:07]

The CQ bit was unchecked by zerny@chromium.org

[zerny-chromium, 2014-03-05 08:57:05]

Changed the clang plugin to allow destructors with empty bodies when compiling
with oilpan_enabled.

[zerny-chromium, 2014-03-05 09:00:07]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-05 09:01:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/184853006/40001

[commit-bot: I haz the power, 2014-03-05 20:57:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/184853006/40001

[commit-bot: I haz the power, 2014-03-06 04:13:50]

Change committed as 168568

[haraken, 2014-03-06 07:34:21]

On 2014/03/06 04:13:50, I haz the power (commit-bot) wrote:
> Change committed as 168568

This broke oilpan Debug builds with clang. Let me revert it at the moment to
stabilize the tree.

../../third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
1.      <eof> parser at end of file
2.      Per-file LLVM IR generation
3.      ../../third_party/WebKit/Source/heap/Heap.h:904:10: Generating code for
declaration
'WebCore::GarbageCollectedFinalized<WebCore::CSSCalcExpressionNode>::finalize'
clang: error: unable to execute command: Aborted (core dumped)
clang: error: clang frontend command failed due to signal (use -v to see
invocation)

[haraken, 2014-03-06 07:35:21]

On 2014/03/06 07:34:21, haraken wrote:
> On 2014/03/06 04:13:50, I haz the power (commit-bot) wrote:
> > Change committed as 168568
> 
> This broke oilpan Debug builds with clang. Let me revert it at the moment to
> stabilize the tree.
> 
>
../../third_party/WebKit/Source/core/animation/css/CSSAnimatableValueFactory.cpp
> 1.      <eof> parser at end of file
> 2.      Per-file LLVM IR generation
> 3.      ../../third_party/WebKit/Source/heap/Heap.h:904:10: Generating code
for
> declaration
> 'WebCore::GarbageCollectedFinalized<WebCore::CSSCalcExpressionNode>::finalize'
> clang: error: unable to execute command: Aborted (core dumped)
> clang: error: clang frontend command failed due to signal (use -v to see
> invocation)

FYI, this error doesn't happen in g++.

[haraken, 2014-03-06 07:46:53]

Here is more detailed log:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac%20Oilpan%20...

[zerny-chromium, 2014-03-06 10:50:27]

Fix for relanding in https://codereview.chromium.org/188473004