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
<rdar://problem/75702348>
Created attachment 424143 [details] Patch
Created attachment 424168 [details] Patch
Created attachment 424205 [details] Patch
Antti, is this the direction we want to take things like this? Is there a memory cost / issue we should consider?
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 %
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?
I guess that could be avoided by having a common base class for Primitive/Pair/Quad or by starting to use Variant.
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?
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.