CSSValue Immediates: Change CSSValue to an object instead of a pointer

CSSValue Immediates: Change CSSValue to an object instead of a pointer

Renamed CSSValue to CSSValueObject, and changed CSSValue to be an object
that holds a CSSValueObject* instead of a pointer itself. Also added
NullableCSSValue for cases where NULL is used as a special return value.
Created a unit test for the new class, and appropriate constructors to
work with all callsites.

This is the first part of a larger refactor of CSSValue to introduce
CSSValue immediates, discussed on the bug.

BUG=486252
COMMIT=false

[sashab, 2015-05-29 08:06:16]

sashab@chromium.org changed reviewers:
+ jochen@chromium.org, timloh@chromium.org

[sashab, 2015-05-29 08:06:18]

This is a fairly large patch, but most of it is mechanical callsite changes.

jochen@chromium.org: Please review changes outside of Source/core

timloh@chromium.org: Please review changes inside of Source/core

Also let me know if there's anything I can do to make reviewing this patch
easier.

[Timothy Loh, 2015-06-02 00:42:04]

I only skimmed a few files, will look at more later.

Why are there test changes? Some of the changes are making PASS expectations
into FAIL expectations, so we're probably breaking something there? We also need
perf results to make sure nothing is going to get slower.

https://codereview.chromium.org/1164573002/diff/1/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/1164573002/diff/1/Source/core/core.gypi#newco...
Source/core/core.gypi:3818:
'css/invalidation/DescendantInvalidationSetTest.cpp',
?? don't move unrelated lines around (also these are ascii-sorted right now)

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

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/CSSValue.h#...
Source/core/css/CSSValue.h:322: CSSValue() { ASSERT_NOT_REACHED(); }
= delete?

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/CSSValue.h#...
Source/core/css/CSSValue.h:323: CSSValue(std::nullptr_t null) {
ASSERT_NOT_REACHED(); }
= delete?

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/resolver/CS...
File Source/core/css/resolver/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/resolver/CS...
Source/core/css/resolver/CSSToStyleMap.cpp:487: void
CSSToStyleMap::mapNinePieceImageSlice(StyleResolverState&, NullableCSSValue
value, NinePieceImage& image)
not nullable?

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/resolver/CS...
Source/core/css/resolver/CSSToStyleMap.cpp:531: BorderImageLengthBox
CSSToStyleMap::mapNinePieceImageQuad(StyleResolverState& state, NullableCSSValue
value)
not nullable?

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/resolver/CS...
Source/core/css/resolver/CSSToStyleMap.cpp:546: void
CSSToStyleMap::mapNinePieceImageRepeat(StyleResolverState&, NullableCSSValue
value, NinePieceImage& image)
not nullable?

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/resolver/El...
File Source/core/css/resolver/ElementStyleResources.h (right):

https://codereview.chromium.org/1164573002/diff/1/Source/core/css/resolver/El...
Source/core/css/resolver/ElementStyleResources.h:46: typedef
WillBeHeapHashMap<CSSPropertyID, NullableCSSValue> PendingImagePropertyMap;
not nullable?

https://codereview.chromium.org/1164573002/diff/1/Source/core/editing/EditorC...
File Source/core/editing/EditorCommand.cpp (right):

https://codereview.chromium.org/1164573002/diff/1/Source/core/editing/EditorC...
Source/core/editing/EditorCommand.cpp:145: String newStyle("none");
looks like the previous code expected this to be non-null, can we make it an
assertion?

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserImpl.cpp (right):

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserImpl.cpp:72:
results.reserveCapacity(parsedProperties.size());
reserveInitialCapacity

[Timothy Loh, 2015-06-02 04:54:04]

So this patch isn't updating all of the places which store CSSValues? I noticed
e.g. that CSSGradientValue still stores RefPtr<CSSPrimitiveValue>s.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSBasi...
File Source/core/css/CSSBasicShapes.cpp (right):

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSBasi...
Source/core/css/CSSBasicShapes.cpp:296: return
compareCSSValueObjectVector(m_values, rhs.m_values);
OK, but you're going to have to make CSSBasicShapePolygon have a vector of
CSSValues before you can make CSSPrimitiveValues into immediates.

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

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:33: 
This stuff should be moved to CSSValueObject.{h,cpp}

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:206: CSSValue(const CSSValueObject* cssValueObject)
This ctor seems wrong, I think we should be creating these from references
instead?

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:220: template <class T>
CSSValue(RefPtrWillBeRawPtr<T> cssValueObject)
extra space after template :|

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:240: m_data->deref();
Should switch order of ref and deref so that assigning a value to itself doesn't
fail.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:298: 
Unused? Should call these in the above functions instead of writing
m_data->ref/deref().

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:317: CSSValue(std::nullptr_t null) {
ASSERT_NOT_REACHED(); }
drop null argument name

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:330: };
drop semicolon

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:332: NullableCSSValue(std::nullptr_t null)
Is this ctor needed?

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:340: if (m_data)
should have (inline) ref/deref functions which do this, instead of having to
explicitly check m_data everywhere

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:367: // CSSValues can be 'auto-cast' to
NullableCSSValues, but not the other way around.
don't think this comment helps (it should be obvious?)

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:387: if (m_data)
swap order of deref/ref

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:402: return *reinterpret_cast<CSSValue*>(this);
Is the reinterpret_cast to avoid ref-churn? If so, a comment would be useful.

Should add ASSERT(m_data) at least in these.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:431: static_assert(sizeof(CSSValue) ==
sizeof(CSSValueObject*), "CSSValue must only contain a CSSValueObject*, and no
vtable");
maybe:

== sizeof(void*), "CSSValue should be pointer-sized"

[sashab, 2015-06-05 06:16:30]

Yup; planning on changing all CSSPrimitiveValues to CSSValues in another patch -
this one's already big enough.

This should be ready for you to fully review now - all that's left are to fix
some more small test failures and some introduced leaks in the fast/css layout
tests.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSBasi...
File Source/core/css/CSSBasicShapes.cpp (right):

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSBasi...
Source/core/css/CSSBasicShapes.cpp:296: return
compareCSSValueObjectVector(m_values, rhs.m_values);
On 2015/06/02 at 04:54:02, Timothy Loh wrote:
> OK, but you're going to have to make CSSBasicShapePolygon have a vector of
CSSValues before you can make CSSPrimitiveValues into immediates.

Yup; planning on doing all the primitive values etc in follow-up patches, I want
to keep this one as small as possible. Added a TODO to remove
compareCSSValueObjectVector to remind me about this specific case.

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

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:33: 
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> This stuff should be moved to CSSValueObject.{h,cpp}

Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:206: CSSValue(const CSSValueObject* cssValueObject)
On 2015/06/02 04:54:03, Timothy Loh wrote:
> This ctor seems wrong, I think we should be creating these from references
> instead?

Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:220: template <class T>
CSSValue(RefPtrWillBeRawPtr<T> cssValueObject)
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> extra space after template :|

Ty! Didn't notice

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:240: m_data->deref();
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> Should switch order of ref and deref so that assigning a value to itself
doesn't fail.

Ah, good one!

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:298: 
On 2015/06/02 at 04:54:04, Timothy Loh wrote:
> Unused? Should call these in the above functions instead of writing
m_data->ref/deref().

ref() is used in:
ImmutableStylePropertySet::ImmutableStylePropertySet

deref() is used in:
ImmutableStylePropertySet::~ImmutableStylePropertySet

I'll make a TODO to get rid of them, though, and investigate the listed bug
(http://crbug.com/449032) in a follow-up patch.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:317: CSSValue(std::nullptr_t null) {
ASSERT_NOT_REACHED(); }
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> drop null argument name

Didn't know you could do that. Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:330: };
On 2015/06/02 at 04:54:04, Timothy Loh wrote:
> drop semicolon

Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:332: NullableCSSValue(std::nullptr_t null)
On 2015/06/02 04:54:03, Timothy Loh wrote:
> Is this ctor needed?

Not anymore. Removed.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:340: if (m_data)
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> should have (inline) ref/deref functions which do this, instead of having to
explicitly check m_data everywhere

Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:367: // CSSValues can be 'auto-cast' to
NullableCSSValues, but not the other way around.
On 2015/06/02 at 04:54:04, Timothy Loh wrote:
> don't think this comment helps (it should be obvious?)

Removed.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:387: if (m_data)
On 2015/06/02 at 04:54:04, Timothy Loh wrote:
> swap order of deref/ref

Done, as before.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:402: return *reinterpret_cast<CSSValue*>(this);
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> Is the reinterpret_cast to avoid ref-churn? If so, a comment would be useful.
> 
> Should add ASSERT(m_data) at least in these.

Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/CSSValu...
Source/core/css/CSSValue.h:431: static_assert(sizeof(CSSValue) ==
sizeof(CSSValueObject*), "CSSValue must only contain a CSSValueObject*, and no
vtable");
On 2015/06/02 at 04:54:03, Timothy Loh wrote:
> maybe:
> 
> == sizeof(void*), "CSSValue should be pointer-sized"

Done.

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/parser/...
File Source/core/css/parser/CSSParserImpl.cpp (right):

https://codereview.chromium.org/1164573002/diff/60001/Source/core/css/parser/...
Source/core/css/parser/CSSParserImpl.cpp:72:
results.reserveCapacity(parsedProperties.size());
On 2015/06/02 at 00:42:04, Timothy Loh wrote:
> reserveInitialCapacity

Done.

[jochen (gone - plz use gerrit), 2015-06-05 12:16:23]

(I'm waiting for green trybots and timloh@ to be happy) - will rubberstamp then

[haraken, 2015-06-05 12:35:49]

Looks like the oilpan bots are sad. Would you fix compile issues with
enable_oilpan=1? Then I'll take a look at an oilpan bit.

[sashab, 2015-06-10 00:03:27]

Fixed most of the memory leaks; still 4 failing tests which I'm looking at now
(seems to be ordering issues in the output).

kentaro/timloh: Oilpan is giving me the following compile errors, which I'm not
sure how to address:

    ../../../third_party/WebKit/Source/core/css/CSSProperty.h:56:1: error:
[blink-gc] Class 'CSSProperty' contains invalid fields.
    class CSSProperty {
    ^
    ../../../third_party/WebKit/Source/core/css/CSSProperty.h:90:5: note:
[blink-gc] Part-object field 'm_value' to a GC derived class declared here:
        CSSValue m_value;
        ^

Getting the same error for the CSSValue/NullableCSSValue members in:
- Source/core/css/FontFace.h:59:1: error: [blink-gc] Class 'FontFace'
- Source/core/animation/StringKeyframe.h:42:5: error: [blink-gc] Class
'CSSPropertySpecificKeyframe'

I think there are a few options - we can either make these CSSValues
RefPtrWillBeMember<> objects, or we can make the classes garbage collected. Any
ideas?

Another error I'm not sure about is the vector in CSSValueList (probably related
to above) - it's declared as WillBeHeapVector<CSSValue, 4> m_values and gives
the following errors:

    ../../../third_party/WebKit/Source/wtf/Vector.h:1158:9: error: no matching
function for call to 'operator new'
            new (NotNull, end()) T(*ptr);
            ^~~
    ...
    ../../../third_party/WebKit/Source/platform/heap/HeapAllocator.h:282:5:
error: static_assert failed "HeapVectorBacking doesn't support objects that
cannot be initialized with memset or don't have a vtable"
        static_assert(Traits::canInitializeWithMemset ||
WTF::IsPolymorphic<T>::value, "HeapVectorBacking doesn't support objects that
cannot be initialized with memset or don't have a vtable");
        ^            
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ...
    ../../../third_party/WebKit/Source/platform/heap/TraceTraits.h:364:9: error:
static_assert failed "HeapVectorBacking doesn't support objects that cannot be
initialized with memset."
            static_assert(!ShouldBeTraced<Traits>::value ||
Traits::canInitializeWithMemset || WTF::IsPolymorphic<T>::value,
"HeapVectorBacking doesn't support objects that cannot be initialized with
memset.");
            ^            
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    ...
    ../../../third_party/WebKit/Source/core/css/CSSValueList.h:60:44: note: in
instantiation of function template specialization
'blink::HeapVector<blink::CSSValue, 4>::append<blink::CSSValue>' requested here
        void append(CSSValue value) { m_values.append(value); }
                                               ^

Any advice on how to proceed would be appreciated :)

Thanks,

Sasha

[sashab, 2015-06-10 00:04:49]

Oh, and I should mention - in the meantime I'll try make all these variables
Member<>; looking at other classes that seems to be the way to go. Let me know
if this sounds like the right direction or not.

[esprehn, 2015-06-10 00:11:10]

We don't expose CSSValue to script anymore, I think we should switch them
back to RefCounted and get rid of the Oilpain.

On Tue, Jun 9, 2015 at 5:03 PM, <sashab@chromium.org> wrote:

> Fixed most of the memory leaks; still 4 failing tests which I'm looking at
> now
> (seems to be ordering issues in the output).
>
> kentaro/timloh: Oilpan is giving me the following compile errors, which
> I'm not
> sure how to address:
>
>     ../../../third_party/WebKit/Source/core/css/CSSProperty.h:56:1: error:
> [blink-gc] Class 'CSSProperty' contains invalid fields.
>     class CSSProperty {
>     ^
>     ../../../third_party/WebKit/Source/core/css/CSSProperty.h:90:5: note:
> [blink-gc] Part-object field 'm_value' to a GC derived class declared here:
>         CSSValue m_value;
>         ^
>
> Getting the same error for the CSSValue/NullableCSSValue members in:
> - Source/core/css/FontFace.h:59:1: error: [blink-gc] Class 'FontFace'
> - Source/core/animation/StringKeyframe.h:42:5: error: [blink-gc] Class
> 'CSSPropertySpecificKeyframe'
>
> I think there are a few options - we can either make these CSSValues
> RefPtrWillBeMember<> objects, or we can make the classes garbage
> collected. Any
> ideas?
>
> Another error I'm not sure about is the vector in CSSValueList (probably
> related
> to above) - it's declared as WillBeHeapVector<CSSValue, 4> m_values and
> gives
> the following errors:
>
>     ../../../third_party/WebKit/Source/wtf/Vector.h:1158:9: error: no
> matching
> function for call to 'operator new'
>             new (NotNull, end()) T(*ptr);
>             ^~~
>     ...
>     ../../../third_party/WebKit/Source/platform/heap/HeapAllocator.h:282:5:
> error: static_assert failed "HeapVectorBacking doesn't support objects that
> cannot be initialized with memset or don't have a vtable"
>         static_assert(Traits::canInitializeWithMemset ||
> WTF::IsPolymorphic<T>::value, "HeapVectorBacking doesn't support objects
> that
> cannot be initialized with memset or don't have a vtable");
>         ^
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     ...
>     ../../../third_party/WebKit/Source/platform/heap/TraceTraits.h:364:9:
> error:
> static_assert failed "HeapVectorBacking doesn't support objects that
> cannot be
> initialized with memset."
>             static_assert(!ShouldBeTraced<Traits>::value ||
> Traits::canInitializeWithMemset || WTF::IsPolymorphic<T>::value,
> "HeapVectorBacking doesn't support objects that cannot be initialized with
> memset.");
>             ^
>
>
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     ...
>     ../../../third_party/WebKit/Source/core/css/CSSValueList.h:60:44:
> note: in
> instantiation of function template specialization
> 'blink::HeapVector<blink::CSSValue, 4>::append<blink::CSSValue>' requested
> here
>         void append(CSSValue value) { m_values.append(value); }
>                                                ^
>
> Any advice on how to proceed would be appreciated :)
>
> Thanks,
>
> Sasha
>
>
> https://codereview.chromium.org/1164573002/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to blink-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[haraken, 2015-06-10 00:26:47]

> We don't expose CSSValue to script anymore, I think we should switch them
> back to RefCounted and get rid of the Oilpain.

It even doesn't need to be RefCounted because CSSValues now have an explicit
owner, right? If CSSValues have an explicit lifetime, I think it's a good idea
to move them off Oilpan.

[haraken, 2015-06-10 00:30:09]

On 2015/06/10 00:26:47, haraken wrote:
> > We don't expose CSSValue to script anymore, I think we should switch them
> > back to RefCounted and get rid of the Oilpain.
> 
> It even doesn't need to be RefCounted because CSSValues now have an explicit
> owner, right? If CSSValues have an explicit lifetime, I think it's a good idea
> to move them off Oilpan.

Sorry, I'm confused. It seems that CSSValues still need reference counting, and
at a first glance, this CL doesn't make the lifetime ownership easier.

> We don't expose CSSValue to script anymore

How is this related to Oilpan?

[haraken, 2015-06-10 05:33:41]

On 2015/06/10 00:04:49, sashab wrote:
> Oh, and I should mention - in the meantime I'll try make all these variables
> Member<>; looking at other classes that seems to be the way to go. Let me know
> if this sounds like the right direction or not.

Let me understand what this CL is doing.

After this CL, are you going to mix:

  class A {
    RefPtr<CSSValue> m_cssValue;
  };

and:

  class B {
    CSSValue m_cssValue;
  };

in the code base? Or are you going to completely remove RefPtr<CSSValue>? If
you're going to completely remove RefPtr<CSSValue>, why do we need ref()/deref()
in CSSValue.h?

[esprehn, 2015-06-10 05:49:39]

On 2015/06/10 at 05:33:41, haraken wrote:
>...
> 
> in the code base? Or are you going to completely remove RefPtr<CSSValue>? If
you're going to completely remove RefPtr<CSSValue>, why do we need ref()/deref()
in CSSValue.h?

No one should ever write RefPtr<CSSValue>, in fact CSSValue will not be
RefCounted at all so you can't do that. CSSObjectValue will be, and CSSValue is
an opaque pointer to either a RefCounted CSSObjectValue or an immediate
CSSPrimitiveValue.

I don't think there's any reason for these things to be GC'ed. They're owned by
StylePropertySets, or by a the CSSValuePool cache. There's not much lifetime
complexity there. In the future the CSSParser should be storing the RefCounted
cache of values, and we should probably cache more generically in some kind of
LRU instead of only fonts and colors and such.

ex. Once we switch we want the cache to store the immediate CSSValue for colors
and just return copies to avoid parsing cost.

[haraken, 2015-06-10 07:22:05]

On 2015/06/10 05:49:39, esprehn wrote:
> On 2015/06/10 at 05:33:41, haraken wrote:
> >...
> > 
> > in the code base? Or are you going to completely remove RefPtr<CSSValue>? If
> you're going to completely remove RefPtr<CSSValue>, why do we need
ref()/deref()
> in CSSValue.h?
> 
> No one should ever write RefPtr<CSSValue>, in fact CSSValue will not be
> RefCounted at all so you can't do that. CSSObjectValue will be, and CSSValue
is
> an opaque pointer to either a RefCounted CSSObjectValue or an immediate
> CSSPrimitiveValue.
> 
> I don't think there's any reason for these things to be GC'ed. They're owned
by
> StylePropertySets, or by a the CSSValuePool cache. There's not much lifetime
> complexity there. In the future the CSSParser should be storing the RefCounted
> cache of values, and we should probably cache more generically in some kind of
> LRU instead of only fonts and colors and such.
> 
> ex. Once we switch we want the cache to store the immediate CSSValue for
colors
> and just return copies to avoid parsing cost.

Thanks for the clarification; that sounds reasonable to me. We don't want to put
things that have explicit lifetime into Oilpan's heap. At first we were planning
to put everything into Oilpan's heap, but later we realized that that is not a
good idea because it will unnecessarily increase the marking time (i.e., pause
time). +1 to move CSSValues off the Oilpan's heap.

The problem is how to do the de-oilpanifinication. The most straightforward way
is to drop the WillBeXXX from the CSSValues but it will increase the CL size
even more.

Idea 1:
1) Drop the WillBeXXX from CSSValues.
2) Land this CL.
Problem: You need a lot of work to rebase this CL.

Idea 2:
1) Land this CL.
2) Drop the WillBeXXX from CSSValues.
Problem: You break oilpan builds between 1) and 2).

Idea 3:
1) Drop the WillBeXXX from CSSValues in this CL.
Problem: The CL size explodes.

oilpan-reviews@: Do you have any idea to land this CL and drop WillBeXXX more
efficiently?

[haraken, 2015-06-10 08:53:36]

On 2015/06/10 07:22:05, haraken wrote:
> On 2015/06/10 05:49:39, esprehn wrote:
> > On 2015/06/10 at 05:33:41, haraken wrote:
> > >...
> > > 
> > > in the code base? Or are you going to completely remove RefPtr<CSSValue>?
If
> > you're going to completely remove RefPtr<CSSValue>, why do we need
> ref()/deref()
> > in CSSValue.h?
> > 
> > No one should ever write RefPtr<CSSValue>, in fact CSSValue will not be
> > RefCounted at all so you can't do that. CSSObjectValue will be, and CSSValue
> is
> > an opaque pointer to either a RefCounted CSSObjectValue or an immediate
> > CSSPrimitiveValue.
> > 
> > I don't think there's any reason for these things to be GC'ed. They're owned
> by
> > StylePropertySets, or by a the CSSValuePool cache. There's not much lifetime
> > complexity there. In the future the CSSParser should be storing the
RefCounted
> > cache of values, and we should probably cache more generically in some kind
of
> > LRU instead of only fonts and colors and such.
> > 
> > ex. Once we switch we want the cache to store the immediate CSSValue for
> colors
> > and just return copies to avoid parsing cost.
> 
> Thanks for the clarification; that sounds reasonable to me. We don't want to
put
> things that have explicit lifetime into Oilpan's heap. At first we were
planning
> to put everything into Oilpan's heap, but later we realized that that is not a
> good idea because it will unnecessarily increase the marking time (i.e., pause
> time). +1 to move CSSValues off the Oilpan's heap.
> 
> The problem is how to do the de-oilpanifinication. The most straightforward
way
> is to drop the WillBeXXX from the CSSValues but it will increase the CL size
> even more.
> 
> Idea 1:
> 1) Drop the WillBeXXX from CSSValues.
> 2) Land this CL.
> Problem: You need a lot of work to rebase this CL.
> 
> Idea 2:
> 1) Land this CL.
> 2) Drop the WillBeXXX from CSSValues.
> Problem: You break oilpan builds between 1) and 2).
> 
> Idea 3:
> 1) Drop the WillBeXXX from CSSValues in this CL.
> Problem: The CL size explodes.
> 
> oilpan-reviews@: Do you have any idea to land this CL and drop WillBeXXX more
> efficiently?

I took a look at the CL and it seems that:

- We don't want to put CSSValues on Oilpan's heap;
- We want to put CSSValueObjects on Oilpan's heap.
- Before this CL, CSSValues were on Oilpan's heap.
- This CL renames CSSValues to CSSValueObjects.

Then what we need is just to put CSSValues off the heap (i.e., don't make
CSSValues GarbageCollected). We can keep the WillBe types around the
CSSValueObject hierarchy as is.

[sashab, 2015-06-11 00:47:13]

Thanks all for the detailed comments. I've removed GC from CSSValue, and
left the WillBe parent class for CSSValueObject as it is.

After talking with shans@ about this and thinking about where we want
to eventually head with CSSValues, I think the best way forward is to
replace all ref-counted CSSValueObjects with CSSValues. This means that
callsites which need access to a method specific to some CSSValueObject
subclass will need to call the appropriate toCSSFooValue() function
first.

This has a few advantages:
1. Keeps the code simple; classes store CSSValues and functions take and
   return CSSValues
2. CSSValueObjects may not need oilpan since they can be freed more
   quickly with refcounting because their lifetimes are simple
3. We can easily hide implementation details (such as tagged pointers)
   since all places that use CSSValueObjects are consistent

The problem with this approach is that functions/members can't have
strong type safety when using CSSValueObject types (e.g. a function
can't take a CSSPrimitiveValue, only a CSSValue which could have
CSSPrimitiveValue or any other CSSValueObject inside it). However, why
should it matter to a function whether the input is a CSSPrimitiveValue
or not? CSSValues should just be data containers for storing information
gained from the CSS parser. The real question is whether the CSSValue
has the required data in it or not - currently, we are using subclasses
to store this information, but there's no reason we couldn't do it
another way (e.g. a CSSValue could have all kinds of hasFoo() and
getFoo() methods for all the various CSS values it could store). In the
future, we might consider removing the whole hierarchy and just storing
data in CSSValues, moving the logic elsewhere where it really belongs.

So, the way I see it, the plan is to do the following:
1. Land this patch.
2. Change all RefPtrWillBeMember<>s and RefPtr<CSSPrimitiveValue>s to
   CSSValues (separate patch to keep this one manageable)
3. Decide whether or not to change CSSValueObject to ref-counted.
4. Tagged pointers.
5. ??? Further optimizations, such as removing the CSSValueObject
   hierarchy altogether.

Let me know how this sounds.

Thanks all,

Sasha

[haraken, 2015-06-11 01:58:53]

On 2015/06/11 00:47:13, sashab wrote:
> Thanks all for the detailed comments. I've removed GC from CSSValue, and
> left the WillBe parent class for CSSValueObject as it is.
> 
> After talking with shans@ about this and thinking about where we want
> to eventually head with CSSValues, I think the best way forward is to
> replace all ref-counted CSSValueObjects with CSSValues. This means that
> callsites which need access to a method specific to some CSSValueObject
> subclass will need to call the appropriate toCSSFooValue() function
> first.
> 
> This has a few advantages:
> 1. Keeps the code simple; classes store CSSValues and functions take and
>    return CSSValues
> 2. CSSValueObjects may not need oilpan since they can be freed more
>    quickly with refcounting because their lifetimes are simple
> 3. We can easily hide implementation details (such as tagged pointers)
>    since all places that use CSSValueObjects are consistent
> 
> The problem with this approach is that functions/members can't have
> strong type safety when using CSSValueObject types (e.g. a function
> can't take a CSSPrimitiveValue, only a CSSValue which could have
> CSSPrimitiveValue or any other CSSValueObject inside it). However, why
> should it matter to a function whether the input is a CSSPrimitiveValue
> or not? CSSValues should just be data containers for storing information
> gained from the CSS parser. The real question is whether the CSSValue
> has the required data in it or not - currently, we are using subclasses
> to store this information, but there's no reason we couldn't do it
> another way (e.g. a CSSValue could have all kinds of hasFoo() and
> getFoo() methods for all the various CSS values it could store). In the
> future, we might consider removing the whole hierarchy and just storing
> data in CSSValues, moving the logic elsewhere where it really belongs.
> 
> So, the way I see it, the plan is to do the following:
> 1. Land this patch.
> 2. Change all RefPtrWillBeMember<>s and RefPtr<CSSPrimitiveValue>s to
>    CSSValues (separate patch to keep this one manageable)
> 3. Decide whether or not to change CSSValueObject to ref-counted.
> 4. Tagged pointers.
> 5. ??? Further optimizations, such as removing the CSSValueObject
>    hierarchy altogether.
> 
> Let me know how this sounds.

I'm not familiar with 2 - 5 (you should be an expert), but at least I agree with
1. Landing this CL would be a good step forward anyway. I'll review the CL from
the oilpan perspective.

Just to confirm: Regarding 2, one of the motivations of moving CSSValues to
Oilpan was to make the script-exposed lifetime correct. However, we no longer
need to worry about it since CSSValues will be completely removed from scripts,
right? For example, we can remove the following tests, right?

# With Oilpan, the parent pointer in the CSSRule hierarchy is
# strong. This is the intended behavior.
crbug.com/345630 fast/dom/StyleSheet/gc-parent-rule.html [ Failure ]
crbug.com/345630 virtual/slimmingpaint/fast/dom/StyleSheet/gc-parent-rule.html [
Failure ]
crbug.com/345630 fast/dom/StyleSheet/gc-parent-stylesheet.html [ Failure ]
crbug.com/345630
virtual/slimmingpaint/fast/dom/StyleSheet/gc-parent-stylesheet.html [ Failure ]
crbug.com/345655 fast/dom/StyleSheet/detached-parent-rule-without-wrapper.html [
Failure ]
crbug.com/345655
virtual/slimmingpaint/fast/dom/StyleSheet/detached-parent-rule-without-wrapper.html
[ Failure ]
crbug.com/345655 fast/dom/StyleSheet/detached-stylesheet-without-wrapper.html [
Failure ]
crbug.com/345655
virtual/slimmingpaint/fast/dom/StyleSheet/detached-stylesheet-without-wrapper.html
[ Failure ]
crbug.com/370906 fast/dom/StyleSheet/gc-declaration-parent-rule.html [ Failure ]
crbug.com/370906
virtual/slimmingpaint/fast/dom/StyleSheet/gc-declaration-parent-rule.html [
Failure ]

[haraken, 2015-06-11 02:48:50]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-06-11 02:48:53]

A couple of comments from the oilpan perspective. Mostly looks good.

- Confirm that the CL compiles with 'enable_oilpan=1 blink_gc_plugin=1'. This is
called an oilpan build.

- Confirm that the oilpan build passes all tests. You can use the oilpan try
bots.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableUnknownTest.cpp (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
Source/core/animation/animatable/AnimatableUnknownTest.cpp:60: CSSValue
otherCSSValue;

This looks wrong. What you're doing here is:

class A {  // Not garbage collected
  Persistent<CSSValue> cssValue;
};

to:

class A {  // Not garbage collected
  CSSValueObject* m_cssValueObject; 
};

This is unsafe because the m_cssValueObject is not traced. In general:

- it is safe to replace Member<CSSValue> to CSSValue.
- but it is unsafe to replace Persistent<CSSValue> to CSSValue.

As far as I scan the CL, this is the only place where you replaced
Persistent<CSSValue> with CSSValue, so I think we can work around the issue here
instead of inventing some general solution. Specifically, you can do something
like:

class PersistentCSSValue : public GarbageCollected<PersistentCSSValue> {
  DEFINE_INLINE_TRACE()
  {
    visitor->trace(cssValue);
  }
  CSSValue cssValue;
};

class AnimationAnimatableUnknownTest {
  ...;
  PersistentCSSValue persistentCSSValue;
};

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
Source/core/animation/animatable/AnimatableUnknownTest.cpp:60: CSSValue
otherCSSValue;

oilpan-reviews@: The GC plugin must detect this error.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.cpp (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.cpp:32: DEFINE_TRACE(CSSValue)

You can inline this by using DEFINE_INLINE_TRACE. Then you can remove this file.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.cpp:35: visitor->trace(*m_data);

Nit: Just visitor->trace(m_data) will work.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.cpp:40: if (m_data)

Nit: This check won't be needed.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:161: CSSValueObject* m_data;

You need to use RawPtrWillBeMember<CSSValueObject>. I guess the GC plugin will
complain about this with oilpan builds, but didn't it?

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:292: CSSValueObject* m_data;

Ditto. You need to use RawPtrWillBeMember<CSSValueObject>.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
File Source/core/css/CSSValueObject.h (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValueObject.h:31: class CORE_EXPORT CSSValueObject : public
RefCountedWillBeGarbageCollectedFinalized<CSSValueObject> {

CSSValue is allocated on a dedicated heap in Oilpan (for performance reasons).
Now that you renamed it to CSSValueObject, you need to make the corresponding
change in the heap. Would you open platform/heap/ThreadState.h and change
'H(CSSValue)' to 'H(CSSValueObject)'?

[sashab, 2015-06-11 23:05:35]

Thanks for the feedback haraken; I've responded inline.

Progress update: still stuck on these 6 failing tests which I'm working through
now. Finding it difficult to narrow down the cause of the issue. Next is fixing
the build on other bots (android?), and perf regression tests.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableUnknownTest.cpp (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
Source/core/animation/animatable/AnimatableUnknownTest.cpp:60: CSSValue
otherCSSValue;
On 2015/06/11 02:48:53, haraken wrote:
> 
> oilpan-reviews@: The GC plugin must detect this error.

In this case I could just remove the test fixture altogether; the values are
only really needed in the last test anyway. Good to know about Persistent
though.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.cpp (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.cpp:32: DEFINE_TRACE(CSSValue)
On 2015/06/11 02:48:53, haraken wrote:
> 
> You can inline this by using DEFINE_INLINE_TRACE. Then you can remove this
file.

Great! Done.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.cpp:35: visitor->trace(*m_data);
On 2015/06/11 02:48:53, haraken wrote:
> 
> Nit: Just visitor->trace(m_data) will work.

Done.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.cpp:40: if (m_data)
On 2015/06/11 02:48:53, haraken wrote:
> 
> Nit: This check won't be needed.

Done; traced m_data instead of *m_data.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:161: CSSValueObject* m_data;
On 2015/06/11 02:48:53, haraken wrote:
> 
> You need to use RawPtrWillBeMember<CSSValueObject>. I guess the GC plugin will
> complain about this with oilpan builds, but didn't it?

I wasn't compiling with blink_gc_plugin=1, so maybe I didn't get it :) Thanks
though; changed it now.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:292: CSSValueObject* m_data;
On 2015/06/11 02:48:53, haraken wrote:
> 
> Ditto. You need to use RawPtrWillBeMember<CSSValueObject>.

Done.

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
File Source/core/css/CSSValueObject.h (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/css/CSSVal...
Source/core/css/CSSValueObject.h:31: class CORE_EXPORT CSSValueObject : public
RefCountedWillBeGarbageCollectedFinalized<CSSValueObject> {
On 2015/06/11 02:48:53, haraken wrote:
> 
> CSSValue is allocated on a dedicated heap in Oilpan (for performance reasons).
> Now that you renamed it to CSSValueObject, you need to make the corresponding
> change in the heap. Would you open platform/heap/ThreadState.h and change
> 'H(CSSValue)' to 'H(CSSValueObject)'?

Ah, didn't know about this. Thanks; done.

[haraken, 2015-06-12 00:02:17]

From the oilpan's perspective, looks good.

Also it would be helpful to run run-webkit-tests with the
'--enable-leak-detection' option on non-oilpan builds. It will detect leaks if
any. You can confirm that the reference counting you introduced to CSSValue is
not leaking anything.

https://codereview.chromium.org/1164573002/diff/180001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/180001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:138: visitor->trace(m_data);

Add ASSERT(m_data).

https://codereview.chromium.org/1164573002/diff/180001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:340:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::NullableCSSValue);

As discussed offline, shall we add a comment and mention why it's safe to mark
CSSValue as canInitializeWithMemset? (I'm sorry that oilpan's vector support is
not sufficient.)

[sof, 2015-06-12 05:10:01]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-06-12 05:10:04]

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableUnknownTest.cpp (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
Source/core/animation/animatable/AnimatableUnknownTest.cpp:60: CSSValue
otherCSSValue;
On 2015/06/11 02:48:53, haraken wrote:
> 
> oilpan-reviews@: The GC plugin must detect this error.

This is an anonymous namespace, where the GC plugin isn't activated.

[sof, 2015-06-12 10:48:08]

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
File Source/core/animation/animatable/AnimatableUnknownTest.cpp (right):

https://codereview.chromium.org/1164573002/diff/160001/Source/core/animation/...
Source/core/animation/animatable/AnimatableUnknownTest.cpp:60: CSSValue
otherCSSValue;
On 2015/06/12 05:10:04, sof wrote:
> On 2015/06/11 02:48:53, haraken wrote:
> > 
> > oilpan-reviews@: The GC plugin must detect this error.
> 
> This is an anonymous namespace, where the GC plugin isn't activated.

That quirk has now been removed, http://crbug.com/499738

[sof, 2015-06-15 06:06:05]

https://codereview.chromium.org/1164573002/diff/220001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/220001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:344:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::CSSValue);
WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS() can now be used here instead,
see https://codereview.chromium.org/1180383002

[sashab, 2015-06-16 21:26:38]

Thanks for the oilpan feedback; applied those fixes so it's now compiling and
working on the oilpan bots. All tests are passing except two - having trouble
finding the root cause of these, maybe someone more familiar than me with the
code could help a bit with the codepath:
  editing/style/make-text-writing-direction-inline-mac.html
  editing/style/make-text-writing-direction-inline-win.html

The only other thing I'd like to do (once these tests are fixed) is run leak
tests with oilpan, and some perf tests to make sure there are no regressions.
Then we should be set to land. :)

https://codereview.chromium.org/1164573002/diff/180001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/180001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:138: visitor->trace(m_data);
On 2015/06/12 00:02:16, haraken wrote:
> 
> Add ASSERT(m_data).

oh, sorry about that, I misinterpreted your last comment. Done.

https://codereview.chromium.org/1164573002/diff/180001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:340:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::NullableCSSValue);
On 2015/06/12 00:02:16, haraken wrote:
> 
> As discussed offline, shall we add a comment and mention why it's safe to mark
> CSSValue as canInitializeWithMemset? (I'm sorry that oilpan's vector support
is
> not sufficient.)
> 

Done.

https://codereview.chromium.org/1164573002/diff/220001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/220001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:344:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::CSSValue);
On 2015/06/15 06:06:05, sof wrote:
> WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS() can now be used here
instead,
> see https://codereview.chromium.org/1180383002

Oh nice; done. I guess the comment/todos aren't needed anymore because this is
now valid behavior?

[haraken, 2015-06-16 22:10:00]

Oilpan part looks good.

https://codereview.chromium.org/1164573002/diff/220001/Source/core/css/CSSVal...
File Source/core/css/CSSValue.h (right):

https://codereview.chromium.org/1164573002/diff/220001/Source/core/css/CSSVal...
Source/core/css/CSSValue.h:344:
WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS(blink::CSSValue);
On 2015/06/16 21:26:38, sashab wrote:
> On 2015/06/15 06:06:05, sof wrote:
> > WTF_ALLOW_CLEAR_UNUSED_SLOTS_WITH_MEM_FUNCTIONS() can now be used here
> instead,
> > see https://codereview.chromium.org/1180383002
> 
> Oh nice; done. I guess the comment/todos aren't needed anymore because this is
> now valid behavior?

Yes, you can now remove the comment.

[sashab, 2015-06-18 20:39:01]

Patchset #17 (id:320001) has been deleted

[sashab, 2015-06-18 20:39:20]

Patchset #16 (id:300001) has been deleted

[sashab, 2015-06-18 21:01:45]

Patchset #16 (id:340001) has been deleted

[sashab, 2015-06-19 17:10:23]

Patchset #17 (id:380001) has been deleted

[sashab, 2015-06-24 00:48:28]

Patchset #20 (id:460001) has been deleted

[sashab, 2015-06-24 00:48:48]

Patchset #19 (id:440001) has been deleted

[sashab, 2015-06-24 04:27:11]

Patchset #22 (id:540001) has been deleted

[sashab, 2015-06-25 08:05:40]

timloh: Patch ready for review.

1. Passes on all trybots
2. No leaks detected in the fast/css tests with --enable-leak-detection with
both oilpan turned on and off
3. No noticeable performance differences in a local blink.top_25 test with a
Nexus 7 (parse_css_large_tokens 3.03% worse, parse_css_regular 2.77% worse,
tokenize_css_large_tokens 9.16% better, tokenize_css_regular 6.83% better,
update_style 1.67% worse, update_style_cold 2.04% worse). Will keep trying to
get this to work on the trybots so we can get better output over multiple runs.

Some remaining ideas/open questions for future patches, let me know if you agree
with each one:
1. I believe we should remove toFooCSSValue(NullableCSSValue&). Callers should
be forced to dereference a NullableCSSValue before they can cast it.
NullableCSSValue should have nothing except the * and -> operators. We should
probably also remove the stuff generated by DEFINE_TYPE_CASTS (converting from
CSSValueObjects to the various subclasses) - these should no longer be needed
now that we use CSSValue everywhere. So all that's left is toFooCSSValue.
2. I believe we should move toFooCSSValue(CSSValue&) and the isFooCSSValue()
methods into the CSSValue class, as the template methods to<Type> and
isValid<Type> (like the WebNode class). This has two advantages: one, we can
remove the public get() method from CSSValue and no callsites can get a
CSSValueObject* (only a CSSFooValue&); two, it makes it clearer in the code that
it's returning a *view* of the object, not a copy of it:
value.to<CSSPrimitiveValue>().doSomething() as opposed to
toCSSPrimitiveValue(value).doSomething().
3. I'm not sure whether NullableCSSValue should be in a separate file. Either
way, we should probably have a comment on CSSValue explaining that you should
only ever pass these around by value, and if you need a null value use
NullableCSSValue.
4. I'm in two minds about removing oilpan from CSSValue. Removing it would
certainly make a lot of code simpler. If we want to do it, we should do it
sooner rather than later to make these patches simpler.

The next big patch will change CSSPrimitiveValue to be a CSSValue-like class
that supports tagged pointers (although I won't implement tagged pointers in the
next patch, just update all the callsites). It will store a
CSSLargePrimitiveValue*, which will then be optimized to sometimes be a tagged
ptr. But I'll work on any/all of the above 4 tasks first if you think they're a
good idea.

Thanks,

Sasha

[sashab, 2015-07-09 04:11:10]

sashab@chromium.org changed reviewers:
+ esprehn@chromium.org