Oilpan: Move more code of RenderObject destructors to destroy().

Oilpan: Move more code of RenderObject destructors to destroy().

Like [1] and [2]. We should do it in general.

[1] http://src.chromium.org/viewvc/blink?view=revision&revision=180023
[2] http://src.chromium.org/viewvc/blink?view=revision&revision=180029

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

[tkent, 2014-08-12 07:53:05]

Please review this.

[haraken, 2014-08-12 08:08:16]

LGTM

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:232: removeFromGlobalMaps();

Can't we call removeFromGlobalMaps() in destroy() in non-oilpan builds as well?
We don't want to have different call paths between oilpan builds and non-oilpan
builds if possible.

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:238: RenderBox::destroy();

Nit: Make the destroy call to the parent class a tail call (just for consistency
with other destroy methods).

[tkent, 2014-08-12 08:37:51]

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:232: removeFromGlobalMaps();
On 2014/08/12 08:08:15, haraken wrote:
> 
> Can't we call removeFromGlobalMaps() in destroy() in non-oilpan builds as
well?
> We don't want to have different call paths between oilpan builds and
non-oilpan
> builds if possible.

We can't.  |this| is deleted in destroy().

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:238: RenderBox::destroy();
On 2014/08/12 08:08:15, haraken wrote:
> 
> Nit: Make the destroy call to the parent class a tail call (just for
consistency
> with other destroy methods).

We can't do it.  destroy() depends on the global maps.

[tkent, 2014-08-12 08:37:56]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-08-12 08:38:16]

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

[haraken, 2014-08-12 08:47:00]

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/459353002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:238: RenderBox::destroy();
On 2014/08/12 08:37:51, tkent wrote:
> On 2014/08/12 08:08:15, haraken wrote:
> > 
> > Nit: Make the destroy call to the parent class a tail call (just for
> consistency
> > with other destroy methods).
> 
> We can't do it.  destroy() depends on the global maps.

I understand what's going on. Then it seems to me that it would be better to
keep removeFromGlobalMaps() in ~RenderBlock(), because conceptually
removeFromGlobalMaps() must be called after the RenderBlock becomes unreachable.

With your CL, once we have the following code, we'll hit a crash:

class RenderSomething : public RenderBlock {
  void destroy() {
    RenderBlock::destroy();
    doSomething();
  }
};

(In the future, we should replace those global maps with weak maps of
RenderObjects.)

[commit-bot: I haz the power, 2014-08-12 08:48:15]

Change committed as 180046

[tkent, 2014-08-18 05:14:43]

A revert of this CL (patchset #1) has been created in
https://codereview.chromium.org/478923005/ by tkent@chromium.org.

The reason for reverting is: Caused crashes in
ImageQualityController::highQualityRepaintTimerFired().
.