Diff
Modified: trunk/Source/WTF/ChangeLog (235975 => 235976)
--- trunk/Source/WTF/ChangeLog 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WTF/ChangeLog 2018-09-13 17:02:24 UTC (rev 235976)
@@ -1,3 +1,13 @@
+2018-09-13 Alex Christensen <[email protected]>
+
+ Use a Variant instead of a union in CSSSelector
+ https://bugs.webkit.org/show_bug.cgi?id=188559
+
+ Reviewed by Antti Koivisto.
+
+ * wtf/Variant.h:
+ Add packing macros to make it so Variant-containing structures don't always have 7 bytes of padding per Variant.
+
2018-09-12 Guillaume Emont <[email protected]>
Add IGNORE_WARNING_.* macros
Modified: trunk/Source/WTF/wtf/Variant.h (235975 => 235976)
--- trunk/Source/WTF/wtf/Variant.h 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WTF/wtf/Variant.h 2018-09-13 17:02:24 UTC (rev 235976)
@@ -1435,6 +1435,7 @@
__noexcept_variant_swap_impl<__all_swappable<_Types...>::value,_Types...>
{};
+#pragma pack(push, 1)
template<typename ... _Types>
class Variant:
private __variant_base<
@@ -1721,6 +1722,7 @@
}
}
};
+#pragma pack(pop)
template<>
class Variant<>{
Modified: trunk/Source/WebCore/ChangeLog (235975 => 235976)
--- trunk/Source/WebCore/ChangeLog 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WebCore/ChangeLog 2018-09-13 17:02:24 UTC (rev 235976)
@@ -1,3 +1,48 @@
+2018-09-13 Alex Christensen <[email protected]>
+
+ Use a Variant instead of a union in CSSSelector
+ https://bugs.webkit.org/show_bug.cgi?id=188559
+
+ Reviewed by Antti Koivisto.
+
+ No change in behavior. This just makes some of the existing problems more obvious and easy to fix.
+
+ I moved m_caseInsensitiveAttributeValueMatching to RareData because it's only used with RareData.
+ I only have m_isForPage when assertions are enabled because it's only used for an assertion.
+ The rest is pretty straightforward translating union syntax to Variant syntax.
+ I use RefPtr for now where I could use Ref because it's never null to make copying easier, but that's temporary.
+
+ * css/CSSSelector.cpp:
+ (WebCore::CSSSelector::CSSSelector):
+ (WebCore::CSSSelector::createRareData):
+ (WebCore::CSSSelector::setAttribute):
+ (WebCore::CSSSelector::setArgument):
+ (WebCore::CSSSelector::setLangArgumentList):
+ (WebCore::CSSSelector::setSelectorList):
+ (WebCore::CSSSelector::setNth):
+ (WebCore::CSSSelector::matchNth const):
+ (WebCore::CSSSelector::nthA const):
+ (WebCore::CSSSelector::nthB const):
+ (WebCore::CSSSelector::RareData::RareData):
+ * css/CSSSelector.h:
+ (WebCore::CSSSelector::argument const):
+ (WebCore::CSSSelector::langArgumentList const):
+ (WebCore::CSSSelector::selectorList const):
+ (WebCore::CSSSelector::attribute const):
+ (WebCore::CSSSelector::attributeCanonicalLocalName const):
+ (WebCore::CSSSelector::setValue):
+ (WebCore::CSSSelector::CSSSelector):
+ (WebCore::CSSSelector::~CSSSelector):
+ (WebCore::CSSSelector::tagQName const):
+ (WebCore::CSSSelector::tagLowercaseLocalName const):
+ (WebCore::CSSSelector::value const):
+ (WebCore::CSSSelector::serializingValue const):
+ (WebCore::CSSSelector::attributeValueMatchingIsCaseInsensitive const):
+ (WebCore::CSSSelector::RareData::create): Deleted.
+ * css/parser/CSSParserImpl.cpp:
+ (WebCore::CSSParserImpl::parsePageSelector):
+ * css/parser/CSSParserSelector.h:
+
2018-09-13 Fujii Hironori <[email protected]>
[Win][Clang] error: type 'float' cannot be narrowed to 'LONG' (aka 'long') in initializer list in WheelEventWin.cpp
Modified: trunk/Source/WebCore/css/CSSSelector.cpp (235975 => 235976)
--- trunk/Source/WebCore/css/CSSSelector.cpp 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WebCore/css/CSSSelector.cpp 2018-09-13 17:02:24 UTC (rev 235976)
@@ -40,25 +40,19 @@
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(SameSizeAsCSSSelector), "CSSSelector should remain small.");
+static_assert(sizeof(CSSSelector) == sizeof(void*) + sizeof(unsigned), "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_hasRareData(false)
- , m_hasNameWithCase(false)
+ , m_pseudoType(0)
+ , m_tagIsForNamespaceRule(tagIsForNamespaceRule)
+#if !ASSERT_DISABLED
, m_isForPage(false)
- , m_tagIsForNamespaceRule(tagIsForNamespaceRule)
- , m_caseInsensitiveAttributeValueMatching(false)
+#endif
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
, m_destructorHasBeenCalled(false)
#endif
@@ -66,25 +60,21 @@
const AtomicString& tagLocalName = tagQName.localName();
const AtomicString tagLocalNameASCIILowercase = tagLocalName.convertToASCIILowercase();
- 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;
- }
+ if (tagLocalName == tagLocalNameASCIILowercase)
+ m_data = tagQName;
+ else
+ m_data = adoptRef(new NameWithCase(tagQName, tagLocalNameASCIILowercase));
}
void CSSSelector::createRareData()
{
ASSERT(match() != Tag);
- ASSERT(!m_hasNameWithCase);
- if (m_hasRareData)
+ ASSERT(!WTF::holds_alternative<RefPtr<NameWithCase>>(m_data));
+ if (WTF::holds_alternative<RefPtr<RareData>>(m_data))
return;
// Move the value to the rare data stucture.
- AtomicString value { adoptRef(m_data.m_value) };
- m_data.m_rareData = &RareData::create(WTFMove(value)).leakRef();
- m_hasRareData = true;
+ AtomicString value = WTF::get<AtomicString>(m_data);
+ m_data = adoptRef(new RareData(WTFMove(value)));
}
static unsigned simpleSelectorSpecificityInternal(const CSSSelector& simpleSelector, bool isComputingMaximumSpecificity);
@@ -749,61 +739,54 @@
void CSSSelector::setAttribute(const QualifiedName& value, bool convertToLowercase, AttributeMatchType matchType)
{
createRareData();
- m_data.m_rareData->m_attribute = value;
- m_data.m_rareData->m_attributeCanonicalLocalName = convertToLowercase ? value.localName().convertToASCIILowercase() : value.localName();
- m_caseInsensitiveAttributeValueMatching = matchType == CaseInsensitive;
+ 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;
}
void CSSSelector::setArgument(const AtomicString& value)
{
createRareData();
- m_data.m_rareData->m_argument = value;
+ WTF::get<RefPtr<RareData>>(m_data)->m_argument = value;
}
void CSSSelector::setLangArgumentList(std::unique_ptr<Vector<AtomicString>> argumentList)
{
createRareData();
- m_data.m_rareData->m_langArgumentList = WTFMove(argumentList);
+ WTF::get<RefPtr<RareData>>(m_data)->m_langArgumentList = WTFMove(argumentList);
}
void CSSSelector::setSelectorList(std::unique_ptr<CSSSelectorList> selectorList)
{
createRareData();
- m_data.m_rareData->m_selectorList = WTFMove(selectorList);
+ WTF::get<RefPtr<RareData>>(m_data)->m_selectorList = WTFMove(selectorList);
}
void CSSSelector::setNth(int a, int b)
{
createRareData();
- m_data.m_rareData->m_a = a;
- m_data.m_rareData->m_b = b;
+ WTF::get<RefPtr<RareData>>(m_data)->m_a = a;
+ WTF::get<RefPtr<RareData>>(m_data)->m_b = b;
}
bool CSSSelector::matchNth(int count) const
{
- ASSERT(m_hasRareData);
- return m_data.m_rareData->matchNth(count);
+ return WTF::get<RefPtr<RareData>>(m_data)->matchNth(count);
}
int CSSSelector::nthA() const
{
- ASSERT(m_hasRareData);
- return m_data.m_rareData->m_a;
+ return WTF::get<RefPtr<RareData>>(m_data)->m_a;
}
int CSSSelector::nthB() const
{
- ASSERT(m_hasRareData);
- return m_data.m_rareData->m_b;
+ return WTF::get<RefPtr<RareData>>(m_data)->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())
{
}
Modified: trunk/Source/WebCore/css/CSSSelector.h (235975 => 235976)
--- trunk/Source/WebCore/css/CSSSelector.h 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WebCore/css/CSSSelector.h 2018-09-13 17:02:24 UTC (rev 235976)
@@ -23,6 +23,7 @@
#include "QualifiedName.h"
#include "RenderStyleConstants.h"
+#include <wtf/Variant.h>
namespace WebCore {
class CSSSelectorList;
@@ -234,10 +235,10 @@
const AtomicString& serializingValue() const;
const QualifiedName& attribute() const;
const AtomicString& attributeCanonicalLocalName() const;
- const AtomicString& argument() const { return m_hasRareData ? m_data.m_rareData->m_argument : nullAtom(); }
+ const AtomicString& argument() const { return WTF::holds_alternative<RefPtr<RareData>>(m_data) ? WTF::get<RefPtr<RareData>>(m_data)->m_argument : nullAtom(); }
bool attributeValueMatchingIsCaseInsensitive() const;
- 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; }
+ 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; }
void setValue(const AtomicString&, bool matchLowerCase = false);
@@ -314,22 +315,27 @@
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:
- 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;
+ 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
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
- unsigned m_destructorHasBeenCalled : 1;
+ unsigned char m_destructorHasBeenCalled : 1;
#endif
unsigned simpleSelectorSpecificityForPage() const;
@@ -338,7 +344,7 @@
CSSSelector& operator=(const CSSSelector&);
struct RareData : public RefCounted<RareData> {
- static Ref<RareData> create(AtomicString&& value) { return adoptRef(*new RareData(WTFMove(value))); }
+ RareData(AtomicString&& value);
~RareData();
bool matchNth(int count);
@@ -349,16 +355,14 @@
AtomicString m_matchingValue;
AtomicString m_serializingValue;
- int m_a; // Used for :nth-*
- int m_b; // Used for :nth-*
- QualifiedName m_attribute; // used for attribute selector
+ int m_a { 0 }; // Used for :nth-*
+ int m_b { 0 }; // Used for :nth-*
+ QualifiedName m_attribute { anyQName() }; // used for attribute selector
AtomicString m_attributeCanonicalLocalName;
- AtomicString m_argument; // Used for :contains and :nth-*
+ AtomicString m_argument { nullAtom() }; // 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().
-
- private:
- RareData(AtomicString&& value);
+ bool m_caseInsensitiveAttributeValueMatching { false };
};
void createRareData();
@@ -373,28 +377,20 @@
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(m_hasRareData);
- return m_data.m_rareData->m_attribute;
+ ASSERT(WTF::holds_alternative<RefPtr<RareData>>(m_data));
+ return WTF::get<RefPtr<RareData>>(m_data)->m_attribute;
}
inline const AtomicString& CSSSelector::attributeCanonicalLocalName() const
{
ASSERT(isAttributeSelector());
- ASSERT(m_hasRareData);
- return m_data.m_rareData->m_attributeCanonicalLocalName;
+ ASSERT(WTF::holds_alternative<RefPtr<RareData>>(m_data));
+ return WTF::get<RefPtr<RareData>>(m_data)->m_attributeCanonicalLocalName;
}
inline bool CSSSelector::matchesPseudoElement() const
@@ -456,33 +452,28 @@
{
ASSERT(match() != Tag);
AtomicString matchingValue = matchLowerCase ? value.convertToASCIILowercase() : value;
- if (!m_hasRareData && matchingValue != value)
+ if (!WTF::holds_alternative<RefPtr<RareData>>(m_data) && matchingValue != value)
createRareData();
- // 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();
+ if (!WTF::holds_alternative<RefPtr<RareData>>(m_data)) {
+ m_data = value;
return;
}
- m_data.m_rareData->m_matchingValue = WTFMove(matchingValue);
- m_data.m_rareData->m_serializingValue = value;
+ WTF::get<RefPtr<RareData>>(m_data)->m_matchingValue = WTFMove(matchingValue);
+ WTF::get<RefPtr<RareData>>(m_data)->m_serializingValue = value;
}
inline CSSSelector::CSSSelector()
: m_relation(DescendantSpace)
, m_match(Unknown)
- , m_pseudoType(0)
, m_isLastInSelectorList(false)
, m_isLastInTagHistory(true)
- , m_hasRareData(false)
- , m_hasNameWithCase(false)
+ , m_pseudoType(0)
+ , m_tagIsForNamespaceRule(false)
+#if !ASSERT_DISABLED
, m_isForPage(false)
- , m_tagIsForNamespaceRule(false)
- , m_caseInsensitiveAttributeValueMatching(false)
+#endif
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
, m_destructorHasBeenCalled(false)
#endif
@@ -490,33 +481,20 @@
}
inline CSSSelector::CSSSelector(const CSSSelector& o)
- : m_relation(o.m_relation)
+ : m_data(o.m_data)
+ , 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_hasRareData(o.m_hasRareData)
- , m_hasNameWithCase(o.m_hasNameWithCase)
+ , m_pseudoType(o.m_pseudoType)
+ , m_tagIsForNamespaceRule(o.m_tagIsForNamespaceRule)
+#if !ASSERT_DISABLED
, m_isForPage(o.m_isForPage)
- , m_tagIsForNamespaceRule(o.m_tagIsForNamespaceRule)
- , m_caseInsensitiveAttributeValueMatching(o.m_caseInsensitiveAttributeValueMatching)
+#endif
#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()
@@ -525,62 +503,44 @@
#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 (m_hasNameWithCase)
- return m_data.m_nameWithCase->m_originalName;
- return *reinterpret_cast<const QualifiedName*>(&m_data.m_tagQName);
+ if (WTF::holds_alternative<RefPtr<NameWithCase>>(m_data))
+ return WTF::get<RefPtr<NameWithCase>>(m_data)->m_originalName;
+ return WTF::get<QualifiedName>(m_data);
}
inline const AtomicString& CSSSelector::tagLowercaseLocalName() const
{
- if (m_hasNameWithCase)
- return m_data.m_nameWithCase->m_lowercaseLocalName;
- return m_data.m_tagQName->m_localName;
+ if (WTF::holds_alternative<RefPtr<NameWithCase>>(m_data))
+ return WTF::get<RefPtr<NameWithCase>>(m_data)->m_lowercaseLocalName;
+ return WTF::get<QualifiedName>(m_data).localName();
}
inline const AtomicString& CSSSelector::value() const
{
ASSERT(match() != Tag);
- if (m_hasRareData)
- return m_data.m_rareData->m_matchingValue;
+ if (WTF::holds_alternative<RefPtr<RareData>>(m_data))
+ return WTF::get<RefPtr<RareData>>(m_data)->m_matchingValue;
- // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
- return *reinterpret_cast<const AtomicString*>(&m_data.m_value);
+ return WTF::get<AtomicString>(m_data);
}
inline const AtomicString& CSSSelector::serializingValue() const
{
ASSERT(match() != Tag);
- if (m_hasRareData)
- return m_data.m_rareData->m_serializingValue;
+ if (WTF::holds_alternative<RefPtr<RareData>>(m_data))
+ return WTF::get<RefPtr<RareData>>(m_data)->m_serializingValue;
- // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
- return *reinterpret_cast<const AtomicString*>(&m_data.m_value);
+ return WTF::get<AtomicString>(m_data);
}
inline bool CSSSelector::attributeValueMatchingIsCaseInsensitive() const
{
- return m_caseInsensitiveAttributeValueMatching;
+ return WTF::holds_alternative<RefPtr<RareData>>(m_data) && WTF::get<RefPtr<RareData>>(m_data)->m_caseInsensitiveAttributeValueMatching;
}
} // namespace WebCore
Modified: trunk/Source/WebCore/css/parser/CSSParserImpl.cpp (235975 => 235976)
--- trunk/Source/WebCore/css/parser/CSSParserImpl.cpp 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WebCore/css/parser/CSSParserImpl.cpp 2018-09-13 17:02:24 UTC (rev 235976)
@@ -294,7 +294,9 @@
selector->prependTagSelector(QualifiedName(nullAtom(), typeSelector, styleSheet->defaultNamespace()));
}
+#if !ASSERT_DISABLED
selector->setForPage();
+#endif
return CSSSelectorList { Vector<std::unique_ptr<CSSParserSelector>>::from(WTFMove(selector)) };
}
Modified: trunk/Source/WebCore/css/parser/CSSParserSelector.h (235975 => 235976)
--- trunk/Source/WebCore/css/parser/CSSParserSelector.h 2018-09-13 16:43:46 UTC (rev 235975)
+++ trunk/Source/WebCore/css/parser/CSSParserSelector.h 2018-09-13 17:02:24 UTC (rev 235976)
@@ -59,7 +59,9 @@
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(); }
