Unreviewed, rolling out r235976.
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Sep 2018 21:33:31 +0000 (21:33 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Sep 2018 21:33:31 +0000 (21:33 +0000)
Broke ARM

Reverted changeset:

"Use a Variant instead of a union in CSSSelector"
https://bugs.webkit.org/show_bug.cgi?id=188559
https://trac.webkit.org/changeset/235976

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236284 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/Variant.h
Source/WebCore/ChangeLog
Source/WebCore/css/CSSSelector.cpp
Source/WebCore/css/CSSSelector.h
Source/WebCore/css/parser/CSSParserImpl.cpp
Source/WebCore/css/parser/CSSParserSelector.h

index d308be1..110889c 100644 (file)
@@ -1,3 +1,15 @@
+2018-09-20  Alex Christensen  <achristensen@webkit.org>
+
+        Unreviewed, rolling out r235976.
+
+        Broke ARM
+
+        Reverted changeset:
+
+        "Use a Variant instead of a union in CSSSelector"
+        https://bugs.webkit.org/show_bug.cgi?id=188559
+        https://trac.webkit.org/changeset/235976
+
 2018-09-17  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Use Semaphore and BinarySemaphore instead of dispatch_semaphore_t
index 35fabdb..f6405aa 100644 (file)
@@ -1435,7 +1435,6 @@ struct __noexcept_variant_swap:
 __noexcept_variant_swap_impl<__all_swappable<_Types...>::value,_Types...>
 {};
 
-#pragma pack(push, 1)
 template<typename ... _Types>
 class Variant:
         private __variant_base<
@@ -1722,7 +1721,6 @@ public:
         }
     }
 };
-#pragma pack(pop)
 
 template<>
 class Variant<>{
index 3e42c2b..cf3d57d 100644 (file)
@@ -1,3 +1,15 @@
+2018-09-20  Alex Christensen  <achristensen@webkit.org>
+
+        Unreviewed, rolling out r235976.
+
+        Broke ARM
+
+        Reverted changeset:
+
+        "Use a Variant instead of a union in CSSSelector"
+        https://bugs.webkit.org/show_bug.cgi?id=188559
+        https://trac.webkit.org/changeset/235976
+
 2018-09-20  Oriol Brufau  <obrufau@igalia.com>
 
         Fix 'border' serialization with both common and uncommon values
index e547200..b347735 100644 (file)
@@ -40,19 +40,25 @@ namespace WebCore {
 
 using namespace HTMLNames;
 
+struct SameSizeAsCSSSelector {
+    unsigned flags;
+    void* unionPointer;
+};
+
 static_assert(CSSSelector::RelationType::Subselector == 0, "Subselector must be 0 for consumeCombinator.");
-static_assert(sizeof(CSSSelector) == sizeof(void*) + sizeof(unsigned), "CSSSelector should remain small.");
+static_assert(sizeof(CSSSelector) == sizeof(SameSizeAsCSSSelector), "CSSSelector should remain small.");
 
 CSSSelector::CSSSelector(const QualifiedName& tagQName, bool tagIsForNamespaceRule)
     : m_relation(DescendantSpace)
     , m_match(Tag)
+    , m_pseudoType(0)
     , m_isLastInSelectorList(false)
     , m_isLastInTagHistory(true)
-    , m_pseudoType(0)
-    , m_tagIsForNamespaceRule(tagIsForNamespaceRule)
-#if !ASSERT_DISABLED
+    , m_hasRareData(false)
+    , m_hasNameWithCase(false)
     , m_isForPage(false)
-#endif
+    , m_tagIsForNamespaceRule(tagIsForNamespaceRule)
+    , m_caseInsensitiveAttributeValueMatching(false)
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
     , m_destructorHasBeenCalled(false)
 #endif
@@ -60,21 +66,25 @@ CSSSelector::CSSSelector(const QualifiedName& tagQName, bool tagIsForNamespaceRu
     const AtomicString& tagLocalName = tagQName.localName();
     const AtomicString tagLocalNameASCIILowercase = tagLocalName.convertToASCIILowercase();
 
-    if (tagLocalName == tagLocalNameASCIILowercase)
-        m_data = tagQName;
-    else
-        m_data = adoptRef(new NameWithCase(tagQName, tagLocalNameASCIILowercase));
+    if (tagLocalName == tagLocalNameASCIILowercase) {
+        m_data.m_tagQName = tagQName.impl();
+        m_data.m_tagQName->ref();
+    } else {
+        m_data.m_nameWithCase = adoptRef(new NameWithCase(tagQName, tagLocalNameASCIILowercase)).leakRef();
+        m_hasNameWithCase = true;
+    }
 }
 
 void CSSSelector::createRareData()
 {
     ASSERT(match() != Tag);
-    ASSERT(!WTF::holds_alternative<RefPtr<NameWithCase>>(m_data));
-    if (WTF::holds_alternative<RefPtr<RareData>>(m_data))
+    ASSERT(!m_hasNameWithCase);
+    if (m_hasRareData)
         return;
     // Move the value to the rare data stucture.
-    AtomicString value = WTF::get<AtomicString>(m_data);
-    m_data = adoptRef(new RareData(WTFMove(value)));
+    AtomicString value { adoptRef(m_data.m_value) };
+    m_data.m_rareData = &RareData::create(WTFMove(value)).leakRef();
+    m_hasRareData = true;
 }
 
 static unsigned simpleSelectorSpecificityInternal(const CSSSelector& simpleSelector, bool isComputingMaximumSpecificity);
@@ -739,54 +749,61 @@ String CSSSelector::selectorText(const String& rightSide) const
 void CSSSelector::setAttribute(const QualifiedName& value, bool convertToLowercase, AttributeMatchType matchType)
 {
     createRareData();
-    WTF::get<RefPtr<RareData>>(m_data)->m_attribute = value;
-    WTF::get<RefPtr<RareData>>(m_data)->m_attributeCanonicalLocalName = convertToLowercase ? value.localName().convertToASCIILowercase() : value.localName();
-    WTF::get<RefPtr<RareData>>(m_data)->m_caseInsensitiveAttributeValueMatching = matchType == CaseInsensitive;
+    m_data.m_rareData->m_attribute = value;
+    m_data.m_rareData->m_attributeCanonicalLocalName = convertToLowercase ? value.localName().convertToASCIILowercase() : value.localName();
+    m_caseInsensitiveAttributeValueMatching = matchType == CaseInsensitive;
 }
     
 void CSSSelector::setArgument(const AtomicString& value)
 {
     createRareData();
-    WTF::get<RefPtr<RareData>>(m_data)->m_argument = value;
+    m_data.m_rareData->m_argument = value;
 }
 
 void CSSSelector::setLangArgumentList(std::unique_ptr<Vector<AtomicString>> argumentList)
 {
     createRareData();
-    WTF::get<RefPtr<RareData>>(m_data)->m_langArgumentList = WTFMove(argumentList);
+    m_data.m_rareData->m_langArgumentList = WTFMove(argumentList);
 }
 
 void CSSSelector::setSelectorList(std::unique_ptr<CSSSelectorList> selectorList)
 {
     createRareData();
-    WTF::get<RefPtr<RareData>>(m_data)->m_selectorList = WTFMove(selectorList);
+    m_data.m_rareData->m_selectorList = WTFMove(selectorList);
 }
 
 void CSSSelector::setNth(int a, int b)
 {
     createRareData();
-    WTF::get<RefPtr<RareData>>(m_data)->m_a = a;
-    WTF::get<RefPtr<RareData>>(m_data)->m_b = b;
+    m_data.m_rareData->m_a = a;
+    m_data.m_rareData->m_b = b;
 }
 
 bool CSSSelector::matchNth(int count) const
 {
-    return WTF::get<RefPtr<RareData>>(m_data)->matchNth(count);
+    ASSERT(m_hasRareData);
+    return m_data.m_rareData->matchNth(count);
 }
 
 int CSSSelector::nthA() const
 {
-    return WTF::get<RefPtr<RareData>>(m_data)->m_a;
+    ASSERT(m_hasRareData);
+    return m_data.m_rareData->m_a;
 }
 
 int CSSSelector::nthB() const
 {
-    return WTF::get<RefPtr<RareData>>(m_data)->m_b;
+    ASSERT(m_hasRareData);
+    return m_data.m_rareData->m_b;
 }
 
 CSSSelector::RareData::RareData(AtomicString&& value)
     : m_matchingValue(value)
     , m_serializingValue(value)
+    , m_a(0)
+    , m_b(0)
+    , m_attribute(anyQName())
+    , m_argument(nullAtom())
 {
 }
 
index 2d44410..0ec5870 100644 (file)
@@ -23,7 +23,6 @@
 
 #include "QualifiedName.h"
 #include "RenderStyleConstants.h"
-#include <wtf/Variant.h>
 
 namespace WebCore {
     class CSSSelectorList;
@@ -235,10 +234,10 @@ namespace WebCore {
         const AtomicString& serializingValue() const;
         const QualifiedName& attribute() const;
         const AtomicString& attributeCanonicalLocalName() const;
-        const AtomicString& argument() const { return WTF::holds_alternative<RefPtr<RareData>>(m_data) ? WTF::get<RefPtr<RareData>>(m_data)->m_argument : nullAtom(); }
+        const AtomicString& argument() const { return m_hasRareData ? m_data.m_rareData->m_argument : nullAtom(); }
         bool attributeValueMatchingIsCaseInsensitive() const;
-        const Vector<AtomicString>* langArgumentList() const { return WTF::holds_alternative<RefPtr<RareData>>(m_data) ? WTF::get<RefPtr<RareData>>(m_data)->m_langArgumentList.get() : nullptr; }
-        const CSSSelectorList* selectorList() const { return WTF::holds_alternative<RefPtr<RareData>>(m_data) ? WTF::get<RefPtr<RareData>>(m_data)->m_selectorList.get() : nullptr; }
+        const Vector<AtomicString>* langArgumentList() const { return m_hasRareData ? m_data.m_rareData->m_langArgumentList.get() : nullptr; }
+        const CSSSelectorList* selectorList() const { return m_hasRareData ? m_data.m_rareData->m_selectorList.get() : nullptr; }
 
         void setValue(const AtomicString&, bool matchLowerCase = false);
         
@@ -315,27 +314,22 @@ namespace WebCore {
         bool isLastInTagHistory() const { return m_isLastInTagHistory; }
         void setNotLastInTagHistory() { m_isLastInTagHistory = false; }
 
-#if !ASSERT_DISABLED
         bool isForPage() const { return m_isForPage; }
         void setForPage() { m_isForPage = true; }
-#endif
 
     private:
-        struct RareData;
-        struct NameWithCase;
-        Variant<AtomicString, QualifiedName, RefPtr<RareData>, RefPtr<NameWithCase>> m_data;
-
-        unsigned char m_relation               : 3; // enum RelationType.
-        mutable unsigned char m_match          : 4; // enum Match.
-        unsigned char m_isLastInSelectorList   : 1;
-        unsigned char m_isLastInTagHistory     : 1;
-        mutable unsigned char m_pseudoType     : 7; // enum PseudoClassType.
-        unsigned char m_tagIsForNamespaceRule  : 1;
-#if !ASSERT_DISABLED
-        unsigned char m_isForPage              : 1;
-#endif
+        unsigned m_relation              : 4; // enum RelationType.
+        mutable unsigned m_match         : 4; // enum Match.
+        mutable unsigned m_pseudoType    : 8; // PseudoType.
+        unsigned m_isLastInSelectorList  : 1;
+        unsigned m_isLastInTagHistory    : 1;
+        unsigned m_hasRareData           : 1;
+        unsigned m_hasNameWithCase       : 1;
+        unsigned m_isForPage             : 1;
+        unsigned m_tagIsForNamespaceRule : 1;
+        unsigned m_caseInsensitiveAttributeValueMatching : 1;
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
-        unsigned char m_destructorHasBeenCalled : 1;
+        unsigned m_destructorHasBeenCalled : 1;
 #endif
 
         unsigned simpleSelectorSpecificityForPage() const;
@@ -344,7 +338,7 @@ namespace WebCore {
         CSSSelector& operator=(const CSSSelector&);
 
         struct RareData : public RefCounted<RareData> {
-            RareData(AtomicString&& value);
+            static Ref<RareData> create(AtomicString&& value) { return adoptRef(*new RareData(WTFMove(value))); }
             ~RareData();
 
             bool matchNth(int count);
@@ -355,14 +349,16 @@ namespace WebCore {
             AtomicString m_matchingValue;
             AtomicString m_serializingValue;
             
-            int m_a { 0 }; // Used for :nth-*
-            int m_b { 0 }; // Used for :nth-*
-            QualifiedName m_attribute { anyQName() }; // used for attribute selector
+            int m_a; // Used for :nth-*
+            int m_b; // Used for :nth-*
+            QualifiedName m_attribute; // used for attribute selector
             AtomicString m_attributeCanonicalLocalName;
-            AtomicString m_argument { nullAtom() }; // Used for :contains and :nth-*
+            AtomicString m_argument; // Used for :contains and :nth-*
             std::unique_ptr<Vector<AtomicString>> m_langArgumentList; // Used for :lang arguments.
             std::unique_ptr<CSSSelectorList> m_selectorList; // Used for :matches() and :not().
-            bool m_caseInsensitiveAttributeValueMatching { false };
+        
+        private:
+            RareData(AtomicString&& value);
         };
         void createRareData();
 
@@ -377,20 +373,28 @@ namespace WebCore {
             const QualifiedName m_originalName;
             const AtomicString m_lowercaseLocalName;
         };
+
+        union DataUnion {
+            DataUnion() : m_value(0) { }
+            AtomicStringImpl* m_value;
+            QualifiedName::QualifiedNameImpl* m_tagQName;
+            RareData* m_rareData;
+            NameWithCase* m_nameWithCase;
+        } m_data;
     };
 
 inline const QualifiedName& CSSSelector::attribute() const
 {
     ASSERT(isAttributeSelector());
-    ASSERT(WTF::holds_alternative<RefPtr<RareData>>(m_data));
-    return WTF::get<RefPtr<RareData>>(m_data)->m_attribute;
+    ASSERT(m_hasRareData);
+    return m_data.m_rareData->m_attribute;
 }
 
 inline const AtomicString& CSSSelector::attributeCanonicalLocalName() const
 {
     ASSERT(isAttributeSelector());
-    ASSERT(WTF::holds_alternative<RefPtr<RareData>>(m_data));
-    return WTF::get<RefPtr<RareData>>(m_data)->m_attributeCanonicalLocalName;
+    ASSERT(m_hasRareData);
+    return m_data.m_rareData->m_attributeCanonicalLocalName;
 }
 
 inline bool CSSSelector::matchesPseudoElement() const
@@ -452,28 +456,33 @@ inline void CSSSelector::setValue(const AtomicString& value, bool matchLowerCase
 {
     ASSERT(match() != Tag);
     AtomicString matchingValue = matchLowerCase ? value.convertToASCIILowercase() : value;
-    if (!WTF::holds_alternative<RefPtr<RareData>>(m_data) && matchingValue != value)
+    if (!m_hasRareData && matchingValue != value)
         createRareData();
     
-    if (!WTF::holds_alternative<RefPtr<RareData>>(m_data)) {
-        m_data = value;
+    // Need to do ref counting manually for the union.
+    if (!m_hasRareData) {
+        if (m_data.m_value)
+            m_data.m_value->deref();
+        m_data.m_value = value.impl();
+        m_data.m_value->ref();
         return;
     }
 
-    WTF::get<RefPtr<RareData>>(m_data)->m_matchingValue = WTFMove(matchingValue);
-    WTF::get<RefPtr<RareData>>(m_data)->m_serializingValue = value;
+    m_data.m_rareData->m_matchingValue = WTFMove(matchingValue);
+    m_data.m_rareData->m_serializingValue = value;
 }
 
 inline CSSSelector::CSSSelector()
     : m_relation(DescendantSpace)
     , m_match(Unknown)
+    , m_pseudoType(0)
     , m_isLastInSelectorList(false)
     , m_isLastInTagHistory(true)
-    , m_pseudoType(0)
-    , m_tagIsForNamespaceRule(false)
-#if !ASSERT_DISABLED
+    , m_hasRareData(false)
+    , m_hasNameWithCase(false)
     , m_isForPage(false)
-#endif
+    , m_tagIsForNamespaceRule(false)
+    , m_caseInsensitiveAttributeValueMatching(false)
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
     , m_destructorHasBeenCalled(false)
 #endif
@@ -481,20 +490,33 @@ inline CSSSelector::CSSSelector()
 }
 
 inline CSSSelector::CSSSelector(const CSSSelector& o)
-    : m_data(o.m_data)
-    , m_relation(o.m_relation)
+    : m_relation(o.m_relation)
     , m_match(o.m_match)
+    , m_pseudoType(o.m_pseudoType)
     , m_isLastInSelectorList(o.m_isLastInSelectorList)
     , m_isLastInTagHistory(o.m_isLastInTagHistory)
-    , m_pseudoType(o.m_pseudoType)
-    , m_tagIsForNamespaceRule(o.m_tagIsForNamespaceRule)
-#if !ASSERT_DISABLED
+    , m_hasRareData(o.m_hasRareData)
+    , m_hasNameWithCase(o.m_hasNameWithCase)
     , m_isForPage(o.m_isForPage)
-#endif
+    , m_tagIsForNamespaceRule(o.m_tagIsForNamespaceRule)
+    , m_caseInsensitiveAttributeValueMatching(o.m_caseInsensitiveAttributeValueMatching)
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
     , m_destructorHasBeenCalled(false)
 #endif
 {
+    if (o.m_hasRareData) {
+        m_data.m_rareData = o.m_data.m_rareData;
+        m_data.m_rareData->ref();
+    } else if (o.m_hasNameWithCase) {
+        m_data.m_nameWithCase = o.m_data.m_nameWithCase;
+        m_data.m_nameWithCase->ref();
+    } if (o.match() == Tag) {
+        m_data.m_tagQName = o.m_data.m_tagQName;
+        m_data.m_tagQName->ref();
+    } else if (o.m_data.m_value) {
+        m_data.m_value = o.m_data.m_value;
+        m_data.m_value->ref();
+    }
 }
 
 inline CSSSelector::~CSSSelector()
@@ -503,44 +525,62 @@ inline CSSSelector::~CSSSelector()
 #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
     m_destructorHasBeenCalled = true;
 #endif
+    if (m_hasRareData) {
+        m_data.m_rareData->deref();
+        m_data.m_rareData = nullptr;
+        m_hasRareData = false;
+    } else if (m_hasNameWithCase) {
+        m_data.m_nameWithCase->deref();
+        m_data.m_nameWithCase = nullptr;
+        m_hasNameWithCase = false;
+    } else if (match() == Tag) {
+        m_data.m_tagQName->deref();
+        m_data.m_tagQName = nullptr;
+        m_match = Unknown;
+    } else if (m_data.m_value) {
+        m_data.m_value->deref();
+        m_data.m_value = nullptr;
+    }
 }
 
 inline const QualifiedName& CSSSelector::tagQName() const
 {
     ASSERT(match() == Tag);
-    if (WTF::holds_alternative<RefPtr<NameWithCase>>(m_data))
-        return WTF::get<RefPtr<NameWithCase>>(m_data)->m_originalName;
-    return WTF::get<QualifiedName>(m_data);
+    if (m_hasNameWithCase)
+        return m_data.m_nameWithCase->m_originalName;
+    return *reinterpret_cast<const QualifiedName*>(&m_data.m_tagQName);
 }
 
 inline const AtomicString& CSSSelector::tagLowercaseLocalName() const
 {
-    if (WTF::holds_alternative<RefPtr<NameWithCase>>(m_data))
-        return WTF::get<RefPtr<NameWithCase>>(m_data)->m_lowercaseLocalName;
-    return WTF::get<QualifiedName>(m_data).localName();
+    if (m_hasNameWithCase)
+        return m_data.m_nameWithCase->m_lowercaseLocalName;
+    return m_data.m_tagQName->m_localName;
 }
 
 inline const AtomicString& CSSSelector::value() const
 {
     ASSERT(match() != Tag);
-    if (WTF::holds_alternative<RefPtr<RareData>>(m_data))
-        return WTF::get<RefPtr<RareData>>(m_data)->m_matchingValue;
+    if (m_hasRareData)
+        return m_data.m_rareData->m_matchingValue;
 
-    return WTF::get<AtomicString>(m_data);
+    // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
+    return *reinterpret_cast<const AtomicString*>(&m_data.m_value);
 }
 
 inline const AtomicString& CSSSelector::serializingValue() const
 {
     ASSERT(match() != Tag);
-    if (WTF::holds_alternative<RefPtr<RareData>>(m_data))
-        return WTF::get<RefPtr<RareData>>(m_data)->m_serializingValue;
+    if (m_hasRareData)
+        return m_data.m_rareData->m_serializingValue;
     
-    return WTF::get<AtomicString>(m_data);
+    // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
+    return *reinterpret_cast<const AtomicString*>(&m_data.m_value);
 }
     
 inline bool CSSSelector::attributeValueMatchingIsCaseInsensitive() const
 {
-    return WTF::holds_alternative<RefPtr<RareData>>(m_data) && WTF::get<RefPtr<RareData>>(m_data)->m_caseInsensitiveAttributeValueMatching;
+    return m_caseInsensitiveAttributeValueMatching;
 }
 
 } // namespace WebCore
index 0f7bf19..4110cd4 100644 (file)
@@ -294,9 +294,7 @@ CSSSelectorList CSSParserImpl::parsePageSelector(CSSParserTokenRange range, Styl
             selector->prependTagSelector(QualifiedName(nullAtom(), typeSelector, styleSheet->defaultNamespace()));
     }
 
-#if !ASSERT_DISABLED
     selector->setForPage();
-#endif
     return CSSSelectorList { Vector<std::unique_ptr<CSSParserSelector>>::from(WTFMove(selector)) };
 }
 
index b09abfe..860fda1 100644 (file)
@@ -59,9 +59,7 @@ public:
     void setNth(int a, int b) { m_selector->setNth(a, b); }
     void setMatch(CSSSelector::Match value) { m_selector->setMatch(value); }
     void setRelation(CSSSelector::RelationType value) { m_selector->setRelation(value); }
-#if !ASSERT_DISABLED
     void setForPage() { m_selector->setForPage(); }
-#endif
 
     CSSSelector::Match match() const { return m_selector->match(); }
     CSSSelector::PseudoElementType pseudoElementType() const { return m_selector->pseudoElementType(); }