Bug 223204 – Refactor Pair.h to CSSValuePair.h

• Component: CSS
• Status: NEW

[Tyler Wilcock]

Tracking issue for refactoring Pair.h[1] to CSSValuePair.h.  Pair.h has a couple problems:

1. It's not a sub-class of CSSValue, making it awkward to use.
2. It can only contain CSSPrimitiveValues.

We should make a new pair class that doesn't have these shortcomings.

Some relevant FIXME-NEWPARSER comments:

https://github.com/WebKit/WebKit/blob/878494d4e4784e308ed3c21a5937a2818d5ee27d/Source/WebCore/css/parser/CSSPropertyParser.cpp#L66

> // FIXME-NEWPARSER: Replace Pair and Rect with actual CSSValue subclasses (CSSValuePair and CSSQuadValue).

https://github.com/WebKit/WebKit/blob/878494d4e4784e308ed3c21a5937a2818d5ee27d/Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp#L647#L649

> // FIXME-NEWPARSER: Eventually we'd like this to use CSSCustomIdentValue, but we need
> // to do other plumbing work first (like changing Pair to CSSValuePair and make it not
> // use only primitive values).

[1]: https://github.com/WebKit/WebKit/blob/7e92dc9040eefad3bd0dadf86201eb601dfe82d3/Source/WebCore/css/Pair.h#L33

[Radar WebKit Bug Importer]

<rdar://problem/75702348>

[Tyler Wilcock]

Created attachment 424143 [details]
Patch

[Tyler Wilcock]

Created attachment 424168 [details]
Patch

[Tyler Wilcock]

Created attachment 424205 [details]
Patch

[Sam Weinig]

Antti, is this the direction we want to take things like this? Is there a memory cost / issue we should consider?

[Tyler Wilcock]

Here are the results of Tools/Scripts/dump-class-layout on Pair and CSSValuePair.  There might be more to it than this, but they're equal in this regard at least.  If there's more information that would be useful to gather I'd be happy to do that too.

$ Tools/Scripts/dump-class-layout -c Release WebCore Pair
  +0 < 24> WebCore::Pair
  +0 <  4>     WTF::RefCounted<WebCore::Pair, std::__1::default_delete<WebCore::Pair> > WTF::RefCounted<WebCore::Pair, std::__1::default_delete<WebCore::Pair> >
  +0 <  4>         WTF::RefCountedBase WTF::RefCountedBase
  +0 <  4>           unsigned int m_refCount
  +4 <  1>   WebCore::Pair::IdenticalValueEncoding m_encoding
  +5 <  3>   <PADDING: 3 bytes>
  +8 <  8>     WTF::RefPtr<WebCore::CSSPrimitiveValue, WTF::RawPtrTraits<WebCore::CSSPrimitiveValue>, WTF::DefaultRefDerefTraits<WebCore::CSSPrimitiveValue> > m_first
  +8 <  8>       WTF::RawPtrTraits<WebCore::CSSPrimitiveValue>::StorageType m_ptr
 +16 <  8>     WTF::RefPtr<WebCore::CSSPrimitiveValue, WTF::RawPtrTraits<WebCore::CSSPrimitiveValue>, WTF::DefaultRefDerefTraits<WebCore::CSSPrimitiveValue> > m_second
 +16 <  8>       WTF::RawPtrTraits<WebCore::CSSPrimitiveValue>::StorageType m_ptr
Total byte size: 24
Total pad bytes: 3
Padding percentage: 12.50 %

$ Tools/Scripts/dump-class-layout -c Release WebCore CSSValuePair
  +0 < 24> WebCore::CSSValuePair
  +0 <  8>     WebCore::CSSValue WebCore::CSSValue
  +0 <  4>       unsigned int m_refCount
  +4 < :7>       unsigned int m_primitiveUnitType : 7
  +4 < :1>       unsigned int m_hasCachedCSSText : 1
  +5 < :2>       unsigned int m_valueSeparator : 2
  +5 < :6>       unsigned int m_classType : 6
  +6 <  1>   WebCore::CSSValuePair::IdenticalValueEncoding m_encoding
  +7 <  1>   <PADDING: 1 byte>
  +8 <  8>     WTF::RefPtr<WebCore::CSSValue, WTF::RawPtrTraits<WebCore::CSSValue>, WTF::DefaultRefDerefTraits<WebCore::CSSValue> > m_first
  +8 <  8>       WTF::RawPtrTraits<WebCore::CSSValue>::StorageType m_ptr
 +16 <  8>     WTF::RefPtr<WebCore::CSSValue, WTF::RawPtrTraits<WebCore::CSSValue>, WTF::DefaultRefDerefTraits<WebCore::CSSValue> > m_second
 +16 <  8>       WTF::RawPtrTraits<WebCore::CSSValue>::StorageType m_ptr
Total byte size: 24
Total pad bytes: 1
Padding percentage: 4.17 %

[Antti Koivisto]

Comment on attachment 424205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424205&action=review

I'm going to say r- until it is better understood what the benefits are.

> Source/WebCore/css/CSSBasicShapes.h:141
> -    CSSPrimitiveValue* centerX() const { return m_centerX.get(); }
> -    CSSPrimitiveValue* centerY() const { return m_centerY.get(); }
> +    CSSValue* centerX() const { return m_centerX.get(); }
> +    CSSValue* centerY() const { return m_centerY.get(); }
>      CSSPrimitiveValue* radius() const { return m_radius.get(); }
>  
> -    void setCenterX(Ref<CSSPrimitiveValue>&& centerX) { m_centerX = WTFMove(centerX); }
> -    void setCenterY(Ref<CSSPrimitiveValue>&& centerY) { m_centerY = WTFMove(centerY); }
> +    void setCenterX(Ref<CSSValue>&& centerX) { m_centerX = WTFMove(centerX); }
> +    void setCenterY(Ref<CSSValue>&& centerY) { m_centerY = WTFMove(centerY); }

The practical result of of this refactoring appears to be to weaken typing in many places. This is not a desirable direction. 

Can you explain what concrete benefits there are that would outweigh this regression?

[Antti Koivisto]

I guess that could be avoided by having a common base class for Primitive/Pair/Quad or by starting to use Variant.

[Sam Weinig]

Comment on attachment 424205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424205&action=review

Thanks for looking into this Tyler.

> Source/WebCore/ChangeLog:11
> +        1. It's not a sub-class of CSSValue, making it awkward to use.
> +        2. It can only contain CSSPrimitiveValues.

Would it be possible / practical to have a type, like, CSSPrimitiveValuePair.h which *is* a subclass of CSSValue, but only has CSSPrimitiveValues items for the cases where we don't need to store arbitrary CSSValues as the items? Perhaps even as a subclass of CSSValuePair?

[Tyler Wilcock]

Thank you both for the review!

> The practical result of this refactoring appears to be to weaken typing
> in many places.
You're right, this patch is not worth the weaker typing it brings.

> I guess that could be avoided by having a common base class for Primitive/Pair/Quad or by starting to use Variant.

> Would it be possible / practical to have a type, like, CSSPrimitiveValuePair.h which *is* a subclass of CSSValue, but only has CSSPrimitiveValues items for the cases where we don't need to store arbitrary CSSValues as the items? Perhaps even as a subclass of CSSValuePair?
Thanks for these suggestions.  I'll see if I can come up with something that addresses the original problems without weakening (ideally strengthening) the types.