Use ImmutableStyleProperties for presentational hint style

https://bugs.webkit.org/show_bug.cgi?id=260064 rdar://113741146 Reviewed by Simon Fraser. The mutable style here is never actually mutated after construction. Using immutable style is more memory efficient, allows deduplication and improves matched declarations cache performance. * Source/WebCore/css/ImmutableStyleProperties.h: * Source/WebCore/css/MutableStyleProperties.cpp: (WebCore::MutableStyleProperties::immutableCopy const): * Source/WebCore/css/MutableStyleProperties.h: * Source/WebCore/css/StyleProperties.cpp: (WebCore::StyleProperties::immutableCopyIfNeeded const): * Source/WebCore/dom/ElementData.h: (WebCore::ElementData::presentationalHintStyle const): * Source/WebCore/dom/StyledElement.cpp: (WebCore::StyledElement::presentationalHintStyle const): (WebCore::StyledElement::rebuildPresentationalHintStyle): Make immutable after the attributes have been collected. * Source/WebCore/dom/StyledElement.h: (WebCore::StyledElement::presentationalHintStyle const): Deleted. * Source/WebCore/html/HTMLTextFormControlElement.cpp: * Source/WebCore/inspector/agents/InspectorCSSAgent.cpp: (WebCore::InspectorCSSAgent::buildObjectForAttributesStyle): * Source/WebCore/style/ElementRuleCollector.cpp: (WebCore::Style::ElementRuleCollector::ElementRuleCollector): * Source/WebCore/style/MatchResult.h: (WebCore::Style::MatchResult::MatchResult): (WebCore::Style::operator==): (WebCore::Style::add): Include a bit telling if the match result is for link. Because visited link style handling links and non-links can't share style. HTML <a> has UA sheet rules so this didn't occur easily, however SVG <a> doesn't and this patch reveals the issue in some tests. Canonical link: https://commits.webkit.org/266914@main
WebKit · Aug 15, 2023 · 257a355 · 257a355
1 parent bb3bc0c
commit 257a355
Show file tree

Hide file tree

Showing 10 changed files with 40 additions and 26 deletions.
diff --git a/Source/WebCore/css/MutableStyleProperties.cpp b/Source/WebCore/css/MutableStyleProperties.cpp
@@ -29,6 +29,7 @@
 #include "CSSCustomPropertyValue.h"
 #include "CSSParser.h"
 #include "CSSValuePool.h"
+#include "ImmutableStyleProperties.h"
 #include "PropertySetCSSStyleDeclaration.h"
 #include "StylePropertiesInlines.h"
 #include "StylePropertyShorthand.h"
@@ -82,6 +83,11 @@ Ref<MutableStyleProperties> MutableStyleProperties::createEmpty()
     return adoptRef(*new MutableStyleProperties({ }));
 }
 
+Ref<ImmutableStyleProperties> MutableStyleProperties::immutableCopy() const
+{
+    return ImmutableStyleProperties::createDeduplicating(m_propertyVector.data(), m_propertyVector.size(), cssParserMode());
+}
+
 // FIXME: Change StylePropertyShorthand::properties to return a Span and delete this.
 static inline std::span<const CSSPropertyID> span(const StylePropertyShorthand& shorthand)
 {

diff --git a/Source/WebCore/css/MutableStyleProperties.h b/Source/WebCore/css/MutableStyleProperties.h
@@ -46,6 +46,8 @@ class MutableStyleProperties final : public StyleProperties {
 
     WEBCORE_EXPORT ~MutableStyleProperties();
 
+    Ref<ImmutableStyleProperties> immutableCopy() const;
+
     unsigned propertyCount() const { return m_propertyVector.size(); }
     bool isEmpty() const { return !propertyCount(); }
     PropertyReference propertyAt(unsigned index) const;

diff --git a/Source/WebCore/css/StyleProperties.cpp b/Source/WebCore/css/StyleProperties.cpp
@@ -54,8 +54,7 @@ Ref<ImmutableStyleProperties> StyleProperties::immutableCopyIfNeeded() const
 {
     if (is<ImmutableStyleProperties>(*this))
         return downcast<ImmutableStyleProperties>(const_cast<StyleProperties&>(*this));
-    const MutableStyleProperties& mutableThis = downcast<MutableStyleProperties>(*this);
-    return ImmutableStyleProperties::createDeduplicating(mutableThis.m_propertyVector.data(), mutableThis.m_propertyVector.size(), cssParserMode());
+    return downcast<MutableStyleProperties>(*this).immutableCopy();
 }
 
 String serializeLonghandValue(CSSPropertyID property, const CSSValue& value)

diff --git a/Source/WebCore/dom/ElementData.h b/Source/WebCore/dom/ElementData.h
@@ -32,7 +32,7 @@
 namespace WebCore {
 
 class Attr;
-class MutableStyleProperties;
+class ImmutableStyleProperties;
 class ShareableElementData;
 class StyleProperties;
 class UniqueElementData;
@@ -93,7 +93,7 @@ class ElementData : public RefCounted<ElementData> {
     void setIdForStyleResolution(const AtomString& newId) const { m_idForStyleResolution = newId; }
 
     const StyleProperties* inlineStyle() const { return m_inlineStyle.get(); }
-    const MutableStyleProperties* presentationalHintStyle() const;
+    const ImmutableStyleProperties* presentationalHintStyle() const;
 
     unsigned length() const;
     bool isEmpty() const { return !length(); }
@@ -221,7 +221,7 @@ class UniqueElementData : public ElementData {
 
     static ptrdiff_t attributeVectorMemoryOffset() { return OBJECT_OFFSETOF(UniqueElementData, m_attributeVector); }
 
-    mutable RefPtr<MutableStyleProperties> m_presentationalHintStyle;
+    mutable RefPtr<ImmutableStyleProperties> m_presentationalHintStyle;
     typedef Vector<Attribute, 4> AttributeVector;
     AttributeVector m_attributeVector;
 };
@@ -247,7 +247,7 @@ inline const Attribute* ElementData::attributeBase() const
     return downcast<ShareableElementData>(*this).m_attributeArray;
 }
 
-inline const MutableStyleProperties* ElementData::presentationalHintStyle() const
+inline const ImmutableStyleProperties* ElementData::presentationalHintStyle() const
 {
     if (!is<UniqueElementData>(*this))
         return nullptr;

diff --git a/Source/WebCore/dom/StyledElement.cpp b/Source/WebCore/dom/StyledElement.cpp
@@ -276,6 +276,15 @@ void StyledElement::addSubresourceAttributeURLs(ListHashSet<URL>& urls) const
     });
 }
 
+const ImmutableStyleProperties* StyledElement::presentationalHintStyle() const
+{
+    if (!elementData())
+        return nullptr;
+    if (elementData()->presentationalHintStyleIsDirty())
+        const_cast<StyledElement&>(*this).rebuildPresentationalHintStyle();
+    return elementData()->presentationalHintStyle();
+}
+
 void StyledElement::rebuildPresentationalHintStyle()
 {
     auto style = MutableStyleProperties::create(isSVGElement() ? SVGAttributeMode : HTMLQuirksMode);
@@ -292,7 +301,7 @@ void StyledElement::rebuildPresentationalHintStyle()
     if (style->isEmpty())
         elementData.m_presentationalHintStyle = nullptr;
     else
-        elementData.m_presentationalHintStyle = WTFMove(style);
+        elementData.m_presentationalHintStyle = style->immutableCopy();
 }
 
 void StyledElement::addPropertyToPresentationalHintStyle(MutableStyleProperties& style, CSSPropertyID propertyID, CSSValueID identifier)

diff --git a/Source/WebCore/dom/StyledElement.h b/Source/WebCore/dom/StyledElement.h
@@ -33,6 +33,7 @@
 namespace WebCore {
 
 class Attribute;
+class ImmutableStyleProperties;
 class MutableStyleProperties;
 class PropertySetCSSStyleDeclaration;
 class StyleProperties;
@@ -64,7 +65,7 @@ class StyledElement : public Element {
     StylePropertyMap& ensureAttributeStyleMap();
 
     // https://html.spec.whatwg.org/#presentational-hints
-    const MutableStyleProperties* presentationalHintStyle() const;
+    const ImmutableStyleProperties* presentationalHintStyle() const;
     virtual void collectPresentationalHintsForAttribute(const QualifiedName&, const AtomString&, MutableStyleProperties&) { }
     virtual const MutableStyleProperties* additionalPresentationalHintStyle() const { return nullptr; }
     virtual void collectExtraStyleForPresentationalHints(MutableStyleProperties&) { }
@@ -97,15 +98,6 @@ class StyledElement : public Element {
     void rebuildPresentationalHintStyle();
 };
 
-inline const MutableStyleProperties* StyledElement::presentationalHintStyle() const
-{
-    if (!elementData())
-        return nullptr;
-    if (elementData()->presentationalHintStyleIsDirty())
-        const_cast<StyledElement&>(*this).rebuildPresentationalHintStyle();
-    return elementData()->presentationalHintStyle();
-}
-
 } // namespace WebCore
 
 SPECIALIZE_TYPE_TRAITS_BEGIN(WebCore::StyledElement)

diff --git a/Source/WebCore/html/HTMLTextFormControlElement.cpp b/Source/WebCore/html/HTMLTextFormControlElement.cpp
@@ -44,12 +44,12 @@
 #include "HTMLInputElement.h"
 #include "HTMLNames.h"
 #include "HTMLParserIdioms.h"
+#include "ImmutableStyleProperties.h"
 #include "InlineIteratorBox.h"
 #include "InlineIteratorLineBox.h"
 #include "LayoutDisallowedScope.h"
 #include "LocalFrame.h"
 #include "Logging.h"
-#include "MutableStyleProperties.h"
 #include "NodeTraversal.h"
 #include "Page.h"
 #include "PseudoClassChangeInvalidation.h"

diff --git a/Source/WebCore/inspector/agents/InspectorCSSAgent.cpp b/Source/WebCore/inspector/agents/InspectorCSSAgent.cpp
@@ -1296,12 +1296,13 @@ Ref<JSON::ArrayOf<Protocol::CSS::RuleMatch>> InspectorCSSAgent::buildArrayForMat
 
 RefPtr<Protocol::CSS::CSSStyle> InspectorCSSAgent::buildObjectForAttributesStyle(StyledElement& element)
 {
-    // FIXME: Ugliness below.
-    auto* attributeStyle = const_cast<MutableStyleProperties*>(element.presentationalHintStyle());
-    if (!attributeStyle)
+    auto* presentationalHintStyle = element.presentationalHintStyle();
+    if (!presentationalHintStyle)
         return nullptr;
 
-    auto inspectorStyle = InspectorStyle::create(InspectorCSSId(), attributeStyle->ensureCSSStyleDeclaration(), nullptr);
+    auto mutableStyle = presentationalHintStyle->mutableCopy();
+
+    auto inspectorStyle = InspectorStyle::create(InspectorCSSId(), mutableStyle->ensureCSSStyleDeclaration(), nullptr);
     return inspectorStyle->buildObjectForStyle();
 }
 

diff --git a/Source/WebCore/style/ElementRuleCollector.cpp b/Source/WebCore/style/ElementRuleCollector.cpp
@@ -96,7 +96,7 @@ ElementRuleCollector::ElementRuleCollector(const Element& element, const ScopeRu
     , m_userStyle(ruleSets.userStyle())
     , m_userAgentMediaQueryStyle(ruleSets.userAgentMediaQueryStyle())
     , m_selectorMatchingState(selectorMatchingState)
-    , m_result(makeUnique<MatchResult>())
+    , m_result(makeUnique<MatchResult>(element.isLink()))
 {
     ASSERT(!m_selectorMatchingState || m_selectorMatchingState->selectorFilter.parentStackIsConsistent(element.parentNode()));
 }
@@ -105,7 +105,7 @@ ElementRuleCollector::ElementRuleCollector(const Element& element, const RuleSet
     : m_element(element)
     , m_authorStyle(authorStyle)
     , m_selectorMatchingState(selectorMatchingState)
-    , m_result(makeUnique<MatchResult>())
+    , m_result(makeUnique<MatchResult>(element.isLink()))
 {
     ASSERT(!m_selectorMatchingState || m_selectorMatchingState->selectorFilter.parentStackIsConsistent(element.parentNode()));
 }

diff --git a/Source/WebCore/style/MatchResult.h b/Source/WebCore/style/MatchResult.h
@@ -46,7 +46,11 @@ struct MatchedProperties {
 
 struct MatchResult {
     WTF_MAKE_STRUCT_FAST_ALLOCATED;
+    MatchResult(bool isForLink = false)
+        : isForLink(isForLink)
+    { }
 
+    bool isForLink { false };
     bool isCacheable { true };
     Vector<MatchedProperties> userAgentDeclarations;
     Vector<MatchedProperties> userDeclarations;
@@ -57,7 +61,8 @@ struct MatchResult {
 
 inline bool operator==(const MatchResult& a, const MatchResult& b)
 {
-    return a.isCacheable == b.isCacheable
+    return a.isForLink == b.isForLink
+        && a.isCacheable == b.isCacheable
         && a.userAgentDeclarations == b.userAgentDeclarations
         && a.userDeclarations == b.userDeclarations
         && a.authorDeclarations == b.authorDeclarations;
@@ -87,7 +92,7 @@ inline void add(Hasher& hasher, const MatchedProperties& matchedProperties)
 
 inline void add(Hasher& hasher, const MatchResult& matchResult)
 {
-    add(hasher, matchResult.userAgentDeclarations, matchResult.userDeclarations, matchResult.authorDeclarations);
+    add(hasher, matchResult.isForLink, matchResult.userAgentDeclarations, matchResult.userDeclarations, matchResult.authorDeclarations);
 }
 
 }