Oilpan: Remove Persistent handles from heap allocated CSSRule objects and fix finalization of CSSKe…

Oilpan: Remove Persistent handles from heap allocated CSSRule objects and fix finalization of CSSKeyFramesRule which iterates its children in the destructor. There is no need to do that with oilpan.

R=erik.corry@gmail.com, wibling@chromium.org
BUG=

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

[tkent, 2014-03-03 09:39:17]

lgtm

[Mads Ager (chromium), 2014-03-03 09:40:06]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-03-03 09:40:17]

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

[Erik Corry, 2014-03-03 09:44:28]

LGTM

https://codereview.chromium.org/177423010/diff/1/Source/core/css/CSSPageRule.cpp
File Source/core/css/CSSPageRule.cpp (right):

https://codereview.chromium.org/177423010/diff/1/Source/core/css/CSSPageRule....
Source/core/css/CSSPageRule.cpp:107: CSSRule::trace(visitor);
I think mostly we have done the superclass call first.

https://codereview.chromium.org/177423010/diff/1/Source/core/css/CSSStyleRule...
File Source/core/css/CSSStyleRule.cpp (right):

https://codereview.chromium.org/177423010/diff/1/Source/core/css/CSSStyleRule...
Source/core/css/CSSStyleRule.cpp:135: CSSRule::trace(visitor);
Superclass first?

[wibling-chromium, 2014-03-03 10:08:40]

lgtm

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
File Source/core/css/CSSFilterRule.cpp (right):

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
Source/core/css/CSSFilterRule.cpp:87: CSSRule::trace(visitor);
NIT: Don't we tend to do this last to make it a tail call?

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSKeyfr...
File Source/core/css/CSSKeyframesRule.cpp (right):

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSKeyfr...
Source/core/css/CSSKeyframesRule.cpp:211: #if ENABLE(OILPAN)
NIT: Do we still need the if? I thought we could handle tracing of Vector<RefPtr
<> >?

[zerny-chromium, 2014-03-03 10:25:34]

lgtm.

We have one more Persistent outside the CSSRule case:

../../third_party/WebKit/Source/core/storage/StorageEvent.h:50:1: error:
[blink-gc] GC managed class 'StorageEvent' contains persistent fields.
class StorageEvent FINAL : public Event {
^
../../third_party/WebKit/Source/core/storage/StorageEvent.h:82:5: note:
[blink-gc] Persistent field 'm_storageArea' declared here:
    RefPtrWillBePersistent<Storage> m_storageArea;
    ^

I'll push a fix a new CL.

[haraken, 2014-03-03 10:39:34]

LGTM

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
File Source/core/css/CSSFilterRule.cpp (right):

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
Source/core/css/CSSFilterRule.cpp:87: CSSRule::trace(visitor);
On 2014/03/03 10:08:40, wibling-chromium wrote:
> NIT: Don't we tend to do this last to make it a tail call?

Nit: I want to know this. We sometimes write ParentClass::trace at the head and
sometimes write it at the tail. There is no consistency so far.

[Mads Ager (chromium), 2014-03-03 10:40:39]

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
File Source/core/css/CSSFilterRule.cpp (right):

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
Source/core/css/CSSFilterRule.cpp:87: CSSRule::trace(visitor);
On 2014/03/03 10:39:35, haraken wrote:
> On 2014/03/03 10:08:40, wibling-chromium wrote:
> > NIT: Don't we tend to do this last to make it a tail call?
> 
> Nit: I want to know this. We sometimes write ParentClass::trace at the head
and
> sometimes write it at the tail. There is no consistency so far.

I honestly don't care as long as the call is there. We should do a sweeping
change to put it either first or last. :-)

[commit-bot: I haz the power, 2014-03-03 12:59:42]

Change committed as 168308

[tkent, 2014-03-03 23:55:55]

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
File Source/core/css/CSSFilterRule.cpp (right):

https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
Source/core/css/CSSFilterRule.cpp:87: CSSRule::trace(visitor);
On 2014/03/03 10:40:40, Mads Ager (chromium) wrote:
> On 2014/03/03 10:39:35, haraken wrote:
> > On 2014/03/03 10:08:40, wibling-chromium wrote:
> > > NIT: Don't we tend to do this last to make it a tail call?
> > 
> > Nit: I want to know this. We sometimes write ParentClass::trace at the head
> and
> > sometimes write it at the tail. There is no consistency so far.
> 
> I honestly don't care as long as the call is there. We should do a sweeping
> change to put it either first or last. :-)

As wibling wrote, tail calls are better in general because a compiler can
optimize them to jump instructions.

[haraken, 2014-03-04 00:37:47]

On 2014/03/03 23:55:55, tkent wrote:
>
https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
> File Source/core/css/CSSFilterRule.cpp (right):
> 
>
https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
> Source/core/css/CSSFilterRule.cpp:87: CSSRule::trace(visitor);
> On 2014/03/03 10:40:40, Mads Ager (chromium) wrote:
> > On 2014/03/03 10:39:35, haraken wrote:
> > > On 2014/03/03 10:08:40, wibling-chromium wrote:
> > > > NIT: Don't we tend to do this last to make it a tail call?
> > > 
> > > Nit: I want to know this. We sometimes write ParentClass::trace at the
head
> > and
> > > sometimes write it at the tail. There is no consistency so far.
> > 
> > I honestly don't care as long as the call is there. We should do a sweeping
> > change to put it either first or last. :-)
> 
> As wibling wrote, tail calls are better in general because a compiler can
> optimize them to jump instructions.

In general yes, but I think the point here is whether it's worth caring about or
not.

[wibling-chromium, 2014-03-04 07:10:01]

On 2014/03/04 00:37:47, haraken wrote:
> On 2014/03/03 23:55:55, tkent wrote:
> >
>
https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
> > File Source/core/css/CSSFilterRule.cpp (right):
> > 
> >
>
https://codereview.chromium.org/177423010/diff/20001/Source/core/css/CSSFilte...
> > Source/core/css/CSSFilterRule.cpp:87: CSSRule::trace(visitor);
> > On 2014/03/03 10:40:40, Mads Ager (chromium) wrote:
> > > On 2014/03/03 10:39:35, haraken wrote:
> > > > On 2014/03/03 10:08:40, wibling-chromium wrote:
> > > > > NIT: Don't we tend to do this last to make it a tail call?
> > > > 
> > > > Nit: I want to know this. We sometimes write ParentClass::trace at the
> head
> > > and
> > > > sometimes write it at the tail. There is no consistency so far.
> > > 
> > > I honestly don't care as long as the call is there. We should do a
sweeping
> > > change to put it either first or last. :-)
> > 
> > As wibling wrote, tail calls are better in general because a compiler can
> > optimize them to jump instructions.
> 
> In general yes, but I think the point here is whether it's worth caring about
or
> not.

I don't have a strong preference, but if no one else has a preference either I
would suggest we go with tail calls to at least give the compiler the
opportunity to optimize it if it can. I don't know of a good reason why calling
the parent first would be better.

[haraken, 2014-03-04 07:11:01]

> I don't have a strong preference, but if no one else has a preference either I
> would suggest we go with tail calls to at least give the compiler the
> opportunity to optimize it if it can. I don't know of a good reason why
calling
> the parent first would be better.

Agreed. Sorry for nit-picking.