Move all RefPtr's to CSSValue to oilpan transition types, except for the one in CSSCursorImageValue.

Move all RefPtr's to CSSValue to oilpan transition types, except for the one in CSSCursorImageValue.

This change gets rid of all but one RefPtr to CSSValue. I have introduced a few persistents core/animation/..., core/css/StylePropertySet, and ElementStyleResources. I plan to attack the one in CSSCursorImageValue subsequently, and then change CSSValue from RefCountedGarbageCollected to GarbageCollectedFinalized.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, tkent@chromium.org, vegorov@chromium.org, zerny@chromium.org
BUG=341815
NOTRY=true

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

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

[haraken, 2014-03-04 15:34:24]

LGTM.

https://codereview.chromium.org/184313004/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimatableUnknown.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimatableUnknown.h:40: class AnimatableUnknown FINAL :
public AnimatableValue {

Nit: I'm now moving AnimatableValue to the heap. It's hard :)

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty.h
File Source/core/css/CSSProperty.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty....
Source/core/css/CSSProperty.h:56: ALLOW_ONLY_INLINE_ALLOCATION();

Just to confirm: Now we don't need to manually add the NeedsTracing trait for
CSSProperty, right?

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty....
Source/core/css/CSSProperty.h:196: static const bool canMoveWithMemcpy = true;

Just to confirm: Are you sure that this trait is still correct in the oilpan
world?

[zerny-chromium, 2014-03-05 03:41:44]

lgtm

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSValue.h
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSValue.h#n...
Source/core/css/CSSValue.h:235: const RefPtrWillBeMember<CSSValueType>&
secondPtr = secondVector[i];
Nit: These should in due time (once transition types are eliminated) just be
CSSValueType* firstPtr = ..., no? Maybe add a fixme?

[wibling-chromium, 2014-03-05 08:02:13]

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSValue.h
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSValue.h#n...
Source/core/css/CSSValue.h:235: const RefPtrWillBeMember<CSSValueType>&
secondPtr = secondVector[i];
On 2014/03/05 03:41:44, zerny-chromium wrote:
> Nit: These should in due time (once transition types are eliminated) just be
> CSSValueType* firstPtr = ..., no? Maybe add a fixme?

No, off hand the idea was they should be Member<CSSValueType>&. They are
references directly into the vector's items.

[wibling-chromium, 2014-03-05 08:16:51]

Thanks for the reviews!

https://codereview.chromium.org/184313004/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimatableUnknown.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimatableUnknown.h:40: class AnimatableUnknown FINAL :
public AnimatableValue {
On 2014/03/04 15:34:25, haraken wrote:
> 
> Nit: I'm now moving AnimatableValue to the heap. It's hard :)

Yes! I looked at doing it when I first encountered it, but quickly decided to do
a persistent:)

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty.h
File Source/core/css/CSSProperty.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty....
Source/core/css/CSSProperty.h:56: ALLOW_ONLY_INLINE_ALLOCATION();
On 2014/03/04 15:34:25, haraken wrote:
> 
> Just to confirm: Now we don't need to manually add the NeedsTracing trait for
> CSSProperty, right?

Right. It should just work.

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty....
Source/core/css/CSSProperty.h:196: static const bool canMoveWithMemcpy = true;
On 2014/03/04 15:34:25, haraken wrote:
> 
> Just to confirm: Are you sure that this trait is still correct in the oilpan
> world?

I believe so. I will get Erik to confirm.

[Erik Corry, 2014-03-05 08:41:05]

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty.h
File Source/core/css/CSSProperty.h (right):

https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty....
Source/core/css/CSSProperty.h:196: static const bool canMoveWithMemcpy = true;
On 2014/03/05 08:16:52, wibling-chromium wrote:
> On 2014/03/04 15:34:25, haraken wrote:
> > 
> > Just to confirm: Are you sure that this trait is still correct in the oilpan
> > world?
> 
> I believe so. I will get Erik to confirm.

The trait should still work.

You can't have it set to true for things that are linked together like
Persistent handles, because they would break when memcpy'ed.  Members and
RefPtrs can be memcpy'ed.

[Mads Ager (chromium), 2014-03-05 09:05:32]

LGTM

[wibling-chromium, 2014-03-05 09:22:46]

On 2014/03/05 08:41:05, Erik Corry wrote:
> https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty.h
> File Source/core/css/CSSProperty.h (right):
> 
>
https://codereview.chromium.org/184313004/diff/1/Source/core/css/CSSProperty....
> Source/core/css/CSSProperty.h:196: static const bool canMoveWithMemcpy = true;
> On 2014/03/05 08:16:52, wibling-chromium wrote:
> > On 2014/03/04 15:34:25, haraken wrote:
> > > 
> > > Just to confirm: Are you sure that this trait is still correct in the
oilpan
> > > world?
> > 
> > I believe so. I will get Erik to confirm.
> 
> The trait should still work.
> 
> You can't have it set to true for things that are linked together like
> Persistent handles, because they would break when memcpy'ed.  Members and
> RefPtrs can be memcpy'ed.

Thanks for the clarification!

[wibling-chromium, 2014-03-05 09:51:06]

The CQ bit was checked by wibling@chromium.org

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

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

[wibling-chromium, 2014-03-05 11:20:37]

The CQ bit was unchecked by wibling@chromium.org

[wibling-chromium, 2014-03-05 11:21:28]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-05 11:21:46]

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

[commit-bot: I haz the power, 2014-03-05 11:22:45]

Change committed as 168466

[wibling-chromium, 2014-03-05 15:52:56]

A revert of this CL has been created in
https://codereview.chromium.org/184853009/ by wibling@chromium.org.

The reason for reverting is: It seems to break some of our layout tests..

[wibling-chromium, 2014-03-06 13:04:29]

This should now work without breaking any tests in oilpan. The issue was
twofold. I had missed a Vector<CSSProperty> -> WillBeHeapVector<...> in
CSSComputedStyleDeclaration.cpp and also a bit more tricky the m_storage of the
ImmutableStylePropertySet which needed to be traced since it contained
CSSValues.

PTAL

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

LGTM with the finalization fixed.

https://codereview.chromium.org/184313004/diff/40001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/184313004/diff/40001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:53: void deref();
I think you need to have a finalize method then? It looks like the
MutableStylePropertySet has an OwnPtr and therefore needs its destructor
dispatched.

[wibling-chromium, 2014-03-06 13:34:35]

https://codereview.chromium.org/184313004/diff/40001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/184313004/diff/40001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:53: void deref();
On 2014/03/06 13:30:10, Mads Ager (chromium) wrote:
> I think you need to have a finalize method then? It looks like the
> MutableStylePropertySet has an OwnPtr and therefore needs its destructor
> dispatched.

Thanks for catching that. I had missed the OwnPtr. I will upload a new patch.

[Mads Ager (chromium), 2014-03-06 13:41:45]

LGTM

https://codereview.chromium.org/184313004/diff/60001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/184313004/diff/60001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:50: // destructor. This can be removed once
the ImmutableStylePropertySet's OwnPtr is moved
MutableStylePropertySet's

[wibling-chromium, 2014-03-06 13:47:40]

https://codereview.chromium.org/184313004/diff/60001/Source/core/css/StylePro...
File Source/core/css/StylePropertySet.h (right):

https://codereview.chromium.org/184313004/diff/60001/Source/core/css/StylePro...
Source/core/css/StylePropertySet.h:50: // destructor. This can be removed once
the ImmutableStylePropertySet's OwnPtr is moved
On 2014/03/06 13:41:46, Mads Ager (chromium) wrote:
> MutableStylePropertySet's

Done.

[wibling-chromium, 2014-03-06 13:48:36]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-03-06 13:48:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wibling@chromium.org/184313004/80001

[commit-bot: I haz the power, 2014-03-06 14:05:52]

Change committed as 168635

[wibling-chromium, 2014-03-06 14:19:44]

A revert of this CL has been created in
https://codereview.chromium.org/188693002/ by wibling@chromium.org.

The reason for reverting is: Broke the build..