Don't expose implementation details of StylePropertySet storage.
authorkling@webkit.org <kling@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Oct 2012 11:44:31 +0000 (11:44 +0000)
committerkling@webkit.org <kling@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Oct 2012 11:44:31 +0000 (11:44 +0000)
<http://webkit.org/b/100644>

Reviewed by Antti Koivisto.

Add a StylePropertySet::PropertyReference class, now returned by propertyAt(index).
This will allow us to refactor the internal storage of StylePropertySet without
breaking its API.

A PropertyReference is a simple inlinable wrapper around a StylePropertySet&/index pair.

* css/CSSComputedStyleDeclaration.cpp:
* css/CSSParser.cpp:
* css/CSSParser.h:
* css/SVGCSSParser.cpp:
* css/StylePropertySet.cpp:
(WebCore::StylePropertySet::asText):
(WebCore::StylePropertySet::mergeAndOverrideOnConflict):
(WebCore::StylePropertySet::findPropertyWithId):
(WebCore::StylePropertySet::reportMemoryUsage):
* css/StylePropertySet.h:
(StylePropertySet):
(PropertyReference):
(WebCore::StylePropertySet::PropertyReference::PropertyReference):
(WebCore::StylePropertySet::PropertyReference::id):
(WebCore::StylePropertySet::PropertyReference::isImportant):
(WebCore::StylePropertySet::PropertyReference::isInherited):
(WebCore::StylePropertySet::PropertyReference::cssName):
(WebCore::StylePropertySet::PropertyReference::cssText):
(WebCore::StylePropertySet::PropertyReference::value):
(WebCore::StylePropertySet::PropertyReference::propertyInternal):
(WebCore::StylePropertySet::propertyAt):
(WebCore::StylePropertySet::propertyAtInternal):
(WebCore):
* css/StyleResolver.cpp:
(WebCore::attributeStylesEqual):
(WebCore::StyleResolver::applyProperties):
(WebCore::StyleResolver::resolveVariables):
* editing/ApplyStyleCommand.cpp:
* editing/EditingStyle.cpp:
(WebCore::EditingStyle::mergeStyle):
(WebCore::EditingStyle::mergeStyleFromRulesForSerialization):
* editing/Editor.cpp:
* editing/markup.cpp:
* page/Frame.cpp:
* svg/SVGFontFaceElement.cpp:

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

14 files changed:
Source/WebCore/ChangeLog
Source/WebCore/css/CSSComputedStyleDeclaration.cpp
Source/WebCore/css/CSSParser.cpp
Source/WebCore/css/CSSParser.h
Source/WebCore/css/SVGCSSParser.cpp
Source/WebCore/css/StylePropertySet.cpp
Source/WebCore/css/StylePropertySet.h
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/editing/ApplyStyleCommand.cpp
Source/WebCore/editing/EditingStyle.cpp
Source/WebCore/editing/Editor.cpp
Source/WebCore/editing/markup.cpp
Source/WebCore/page/Frame.cpp
Source/WebCore/svg/SVGFontFaceElement.cpp

index 94899cf..7bec42a 100644 (file)
@@ -1,3 +1,52 @@
+2012-10-29  Andreas Kling  <kling@webkit.org>
+
+        Don't expose implementation details of StylePropertySet storage.
+        <http://webkit.org/b/100644>
+
+        Reviewed by Antti Koivisto.
+
+        Add a StylePropertySet::PropertyReference class, now returned by propertyAt(index).
+        This will allow us to refactor the internal storage of StylePropertySet without
+        breaking its API.
+
+        A PropertyReference is a simple inlinable wrapper around a StylePropertySet&/index pair.
+
+        * css/CSSComputedStyleDeclaration.cpp:
+        * css/CSSParser.cpp:
+        * css/CSSParser.h:
+        * css/SVGCSSParser.cpp:
+        * css/StylePropertySet.cpp:
+        (WebCore::StylePropertySet::asText):
+        (WebCore::StylePropertySet::mergeAndOverrideOnConflict):
+        (WebCore::StylePropertySet::findPropertyWithId):
+        (WebCore::StylePropertySet::reportMemoryUsage):
+        * css/StylePropertySet.h:
+        (StylePropertySet):
+        (PropertyReference):
+        (WebCore::StylePropertySet::PropertyReference::PropertyReference):
+        (WebCore::StylePropertySet::PropertyReference::id):
+        (WebCore::StylePropertySet::PropertyReference::isImportant):
+        (WebCore::StylePropertySet::PropertyReference::isInherited):
+        (WebCore::StylePropertySet::PropertyReference::cssName):
+        (WebCore::StylePropertySet::PropertyReference::cssText):
+        (WebCore::StylePropertySet::PropertyReference::value):
+        (WebCore::StylePropertySet::PropertyReference::propertyInternal):
+        (WebCore::StylePropertySet::propertyAt):
+        (WebCore::StylePropertySet::propertyAtInternal):
+        (WebCore):
+        * css/StyleResolver.cpp:
+        (WebCore::attributeStylesEqual):
+        (WebCore::StyleResolver::applyProperties):
+        (WebCore::StyleResolver::resolveVariables):
+        * editing/ApplyStyleCommand.cpp:
+        * editing/EditingStyle.cpp:
+        (WebCore::EditingStyle::mergeStyle):
+        (WebCore::EditingStyle::mergeStyleFromRulesForSerialization):
+        * editing/Editor.cpp:
+        * editing/markup.cpp:
+        * page/Frame.cpp:
+        * svg/SVGFontFaceElement.cpp:
+
 2012-10-29  Kent Tamura  <tkent@chromium.org>
 
         Move LocaleWin.{cpp,h} to platform/text/win/
index 8bb0c55..b3a7e52 100644 (file)
@@ -34,7 +34,6 @@
 #include "CSSParser.h"
 #include "CSSPrimitiveValue.h"
 #include "CSSPrimitiveValueMappings.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CSSReflectValue.h"
 #include "CSSSelector.h"
index 95392e9..3094f40 100644 (file)
@@ -43,7 +43,6 @@
 #include "CSSMediaRule.h"
 #include "CSSPageRule.h"
 #include "CSSPrimitiveValue.h"
-#include "CSSProperty.h"
 #include "CSSPropertySourceData.h"
 #include "CSSReflectValue.h"
 #include "CSSSelector.h"
@@ -53,7 +52,6 @@
 #include "CSSValueKeywords.h"
 #include "CSSValueList.h"
 #include "CSSValuePool.h"
-#include "StylePropertyShorthand.h"
 #if ENABLE(CSS_VARIABLES)
 #include "CSSVariableValue.h"
 #endif
index 8b0967a..b62d88f 100644 (file)
@@ -47,7 +47,6 @@ namespace WebCore {
 
 class CSSBorderImageSliceValue;
 class CSSPrimitiveValue;
-class CSSProperty;
 class CSSSelectorList;
 class CSSValue;
 class CSSValueList;
index 26d44e4..7200f21 100644 (file)
@@ -26,7 +26,6 @@
 #include "CSSInheritedValue.h"
 #include "CSSInitialValue.h"
 #include "CSSParser.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CSSValueKeywords.h"
 #include "CSSValueList.h"
index b4e42d0..1dceae3 100644 (file)
@@ -635,10 +635,10 @@ String StylePropertySet::asText() const
 {
     StringBuilder result;
 
-    const CSSProperty* positionXProp = 0;
-    const CSSProperty* positionYProp = 0;
-    const CSSProperty* repeatXProp = 0;
-    const CSSProperty* repeatYProp = 0;
+    int positionXPropertyIndex = -1;
+    int positionYPropertyIndex = -1;
+    int repeatXPropertyIndex = -1;
+    int repeatYPropertyIndex = -1;
 
     BitArray<numCSSProperties> shorthandPropertyUsed;
     BitArray<numCSSProperties> shorthandPropertyAppeared;
@@ -646,8 +646,8 @@ String StylePropertySet::asText() const
     unsigned size = propertyCount();
     unsigned numDecls = 0;
     for (unsigned n = 0; n < size; ++n) {
-        const CSSProperty& prop = propertyAt(n);
-        CSSPropertyID propertyID = prop.id();
+        PropertyReference property = propertyAt(n);
+        CSSPropertyID propertyID = property.id();
         CSSPropertyID shorthandPropertyID = CSSPropertyInvalid;
         CSSPropertyID borderFallbackShorthandProperty = CSSPropertyInvalid;
         String value;
@@ -657,20 +657,20 @@ String StylePropertySet::asText() const
         case CSSPropertyVariable:
             if (numDecls++)
                 result.append(' ');
-            result.append(prop.cssText());
+            result.append(property.cssText());
             continue;
 #endif
         case CSSPropertyBackgroundPositionX:
-            positionXProp = &prop;
+            positionXPropertyIndex = n;
             continue;
         case CSSPropertyBackgroundPositionY:
-            positionYProp = &prop;
+            positionYPropertyIndex = n;
             continue;
         case CSSPropertyBackgroundRepeatX:
-            repeatXProp = &prop;
+            repeatXPropertyIndex = n;
             continue;
         case CSSPropertyBackgroundRepeatY:
-            repeatYProp = &prop;
+            repeatYPropertyIndex = n;
             continue;
         case CSSPropertyBorderTopWidth:
         case CSSPropertyBorderRightWidth:
@@ -806,7 +806,7 @@ String StylePropertySet::asText() const
             propertyID = shorthandPropertyID;
             shorthandPropertyUsed.set(shortPropertyIndex);
         } else
-            value = prop.value()->cssText();
+            value = property.value()->cssText();
 
         if (value == "initial" && !CSSProperty::isInheritedProperty(propertyID))
             continue;
@@ -816,7 +816,7 @@ String StylePropertySet::asText() const
         result.append(getPropertyName(propertyID));
         result.appendLiteral(": ");
         result.append(value);
-        if (prop.isImportant())
+        if (property.isImportant())
             result.appendLiteral(" !important");
         result.append(';');
     }
@@ -825,58 +825,64 @@ String StylePropertySet::asText() const
     // It is required because background-position-x/y are non-standard properties and WebKit generated output
     // would not work in Firefox (<rdar://problem/5143183>)
     // It would be a better solution if background-position was CSS_PAIR.
-    if (positionXProp && positionYProp && positionXProp->isImportant() == positionYProp->isImportant()) {
+    if (positionXPropertyIndex != -1 && positionYPropertyIndex != -1 && propertyAt(positionXPropertyIndex).isImportant() == propertyAt(positionYPropertyIndex).isImportant()) {
+        PropertyReference positionXProperty = propertyAt(positionXPropertyIndex);
+        PropertyReference positionYProperty = propertyAt(positionYPropertyIndex);
+
         if (numDecls++)
             result.append(' ');
         result.appendLiteral("background-position: ");
-        if (positionXProp->value()->isValueList() || positionYProp->value()->isValueList())
+        if (positionXProperty.value()->isValueList() || positionYProperty.value()->isValueList())
             result.append(getLayeredShorthandValue(backgroundPositionShorthand()));
         else {
-            result.append(positionXProp->value()->cssText());
+            result.append(positionXProperty.value()->cssText());
             result.append(' ');
-            result.append(positionYProp->value()->cssText());
+            result.append(positionYProperty.value()->cssText());
         }
-        if (positionXProp->isImportant())
+        if (positionXProperty.isImportant())
             result.appendLiteral(" !important");
         result.append(';');
     } else {
-        if (positionXProp) {
+        if (positionXPropertyIndex != -1) {
             if (numDecls++)
                 result.append(' ');
-            result.append(positionXProp->cssText());
+            result.append(propertyAt(positionXPropertyIndex).cssText());
         }
-        if (positionYProp) {
+        if (positionYPropertyIndex != -1) {
             if (numDecls++)
                 result.append(' ');
-            result.append(positionYProp->cssText());
+            result.append(propertyAt(positionYPropertyIndex).cssText());
         }
     }
 
     // FIXME: We need to do the same for background-repeat.
-    if (repeatXProp && repeatYProp && repeatXProp->isImportant() == repeatYProp->isImportant()) {
+    if (repeatXPropertyIndex != -1 && repeatYPropertyIndex != -1 && propertyAt(repeatXPropertyIndex).isImportant() == propertyAt(repeatYPropertyIndex).isImportant()) {
+        PropertyReference repeatXProperty = propertyAt(repeatXPropertyIndex);
+        PropertyReference repeatYProperty = propertyAt(repeatYPropertyIndex);
+
         if (numDecls++)
             result.append(' ');
         result.appendLiteral("background-repeat: ");
-        if (repeatXProp->value()->isValueList() || repeatYProp->value()->isValueList())
+        if (repeatXProperty.value()->isValueList() || repeatYProperty.value()->isValueList())
             result.append(getLayeredShorthandValue(backgroundRepeatShorthand()));
         else {
-            result.append(repeatXProp->value()->cssText());
+            result.append(repeatXProperty.value()->cssText());
             result.append(' ');
-            result.append(repeatYProp->value()->cssText());
+            result.append(repeatYProperty.value()->cssText());
         }
-        if (repeatXProp->isImportant())
+        if (repeatXProperty.isImportant())
             result.appendLiteral(" !important");
         result.append(';');
     } else {
-        if (repeatXProp) {
+        if (repeatXPropertyIndex != -1) {
             if (numDecls++)
                 result.append(' ');
-            result.append(repeatXProp->cssText());
+            result.append(propertyAt(repeatXPropertyIndex).cssText());
         }
-        if (repeatYProp) {
+        if (repeatYPropertyIndex != -1) {
             if (numDecls++)
                 result.append(' ');
-            result.append(repeatYProp->cssText());
+            result.append(propertyAt(repeatYPropertyIndex).cssText());
         }
     }
 
@@ -889,7 +895,7 @@ void StylePropertySet::mergeAndOverrideOnConflict(const StylePropertySet* other)
     ASSERT(isMutable());
     unsigned size = other->propertyCount();
     for (unsigned n = 0; n < size; ++n) {
-        const CSSProperty& toMerge = other->propertyAt(n);
+        const CSSProperty& toMerge = other->propertyAtInternal(n);
         CSSProperty* old = findPropertyWithId(toMerge.id());
         if (old)
             setProperty(toMerge, old);
@@ -989,7 +995,7 @@ const CSSProperty* StylePropertySet::findPropertyWithId(CSSPropertyID propertyID
 {
     for (int n = propertyCount() - 1 ; n >= 0; --n) {
         if (propertyID == propertyAt(n).id())
-            return &propertyAt(n);
+            return &propertyAtInternal(n);
     }
     return 0;
 }
@@ -999,7 +1005,7 @@ CSSProperty* StylePropertySet::findPropertyWithId(CSSPropertyID propertyID)
     ASSERT(isMutable());
     for (int n = propertyCount() - 1 ; n >= 0; --n) {
         if (propertyID == propertyAt(n).id())
-            return &propertyAt(n);
+            return &propertyAtInternal(n);
     }
     return 0;
 }
@@ -1109,7 +1115,7 @@ void StylePropertySet::reportMemoryUsage(MemoryObjectInfo* memoryObjectInfo) con
 
     unsigned count = propertyCount();
     for (unsigned i = 0; i < count; ++i)
-        info.addMember(propertyAt(i));
+        info.addMember(propertyAtInternal(i));
 }
 
 // See the function above if you need to update this.
index 44b6b19..51e5975 100644 (file)
@@ -42,6 +42,7 @@ class StylePropertyShorthand;
 class StyleSheetContents;
 
 class StylePropertySet : public RefCounted<StylePropertySet> {
+    friend class PropertyReference;
 public:
     ~StylePropertySet();
 
@@ -53,10 +54,34 @@ public:
     static PassRefPtr<StylePropertySet> create(const CSSProperty* properties, unsigned count);
     static PassRefPtr<StylePropertySet> createImmutable(const CSSProperty* properties, unsigned count, CSSParserMode);
 
+    class PropertyReference {
+    public:
+        PropertyReference(const StylePropertySet& propertySet, unsigned index)
+            : m_propertySet(propertySet)
+            , m_index(index)
+        { }
+
+        CSSPropertyID id() const { return propertyInternal().id(); }
+        bool isImportant() const { return propertyInternal().isImportant(); }
+        bool isInherited() const { return propertyInternal().isInherited(); }
+
+        String cssName() const { return propertyInternal().cssName(); }
+        String cssText() const { return propertyInternal().cssText(); }
+
+        const CSSValue* value() const { return propertyInternal().value(); }
+        // FIXME: We should try to remove this mutable overload.
+        CSSValue* value() { return const_cast<CSSValue*>(propertyInternal().value()); }
+
+    private:
+        const CSSProperty& propertyInternal() const { return m_propertySet.propertyAtInternal(m_index); }
+
+        const StylePropertySet& m_propertySet;
+        unsigned m_index;
+    };
+
     unsigned propertyCount() const;
     bool isEmpty() const;
-    const CSSProperty& propertyAt(unsigned index) const;
-    CSSProperty& propertyAt(unsigned index);
+    PropertyReference propertyAt(unsigned index) const { return PropertyReference(*this, index); }
 
     PassRefPtr<CSSValue> getPropertyCSSValue(CSSPropertyID) const;
     String getPropertyValue(CSSPropertyID) const;
@@ -161,6 +186,9 @@ private:
 
     void append(const CSSProperty&);
 
+    CSSProperty& propertyAtInternal(unsigned index);
+    const CSSProperty& propertyAtInternal(unsigned index) const;
+
     friend class PropertySetCSSStyleDeclaration;
 };
 
@@ -183,6 +211,20 @@ public:
     Vector<CSSProperty, 4> m_propertyVector;
 };
 
+inline CSSProperty& StylePropertySet::propertyAtInternal(unsigned index)
+{
+    if (m_isMutable)
+        return mutablePropertyVector().at(index);
+    return const_cast<CSSProperty*>(immutablePropertyArray())[index];
+}
+
+inline const CSSProperty& StylePropertySet::propertyAtInternal(unsigned index) const
+{
+    if (m_isMutable)
+        return mutablePropertyVector().at(index);
+    return immutablePropertyArray()[index];
+}
+
 inline Vector<CSSProperty, 4>& StylePropertySet::mutablePropertyVector()
 {
     ASSERT(m_isMutable);
@@ -201,18 +243,6 @@ inline const CSSProperty* StylePropertySet::immutablePropertyArray() const
     return reinterpret_cast<const CSSProperty*>(&static_cast<const ImmutableStylePropertySet*>(this)->m_propertyArray);
 }
 
-inline CSSProperty& StylePropertySet::propertyAt(unsigned index)
-{
-    return mutablePropertyVector().at(index);
-}
-
-inline const CSSProperty& StylePropertySet::propertyAt(unsigned index) const
-{
-    if (m_isMutable)
-        return mutablePropertyVector().at(index);
-    return immutablePropertyArray()[index];
-}
-
 inline unsigned StylePropertySet::propertyCount() const
 {
     if (m_isMutable)
index 0bbace2..b4b86ce 100644 (file)
@@ -1143,10 +1143,10 @@ static inline bool attributeStylesEqual(const StylePropertySet* a, const StylePr
         return false;
     unsigned propertyCount = a->propertyCount();
     for (unsigned i = 0; i < propertyCount; ++i) {
-        const CSSProperty& aProperty = a->propertyAt(i);
+        StylePropertySet::PropertyReference aProperty = a->propertyAt(i);
         unsigned j;
         for (j = 0; j < propertyCount; ++j) {
-            const CSSProperty& bProperty = b->propertyAt(j);
+            StylePropertySet::PropertyReference bProperty = b->propertyAt(j);
             if (aProperty.id() != bProperty.id())
                 continue;
             // We could get a few more hits by comparing cssText() here, but that gets expensive quickly.
@@ -2248,7 +2248,7 @@ void StyleResolver::applyProperties(const StylePropertySet* properties, StyleRul
 
     unsigned propertyCount = properties->propertyCount();
     for (unsigned i = 0; i < propertyCount; ++i) {
-        const CSSProperty& current = properties->propertyAt(i);
+        StylePropertySet::PropertyReference current = properties->propertyAt(i);
         if (isImportant != current.isImportant())
             continue;
         if (inheritedOnly && !current.isInherited()) {
@@ -2801,7 +2801,7 @@ void StyleResolver::resolveVariables(CSSPropertyID id, CSSValue* value, Vector<s
         return; // expression failed to parse.
 
     for (unsigned i = 0; i < resultSet->propertyCount(); i++) {
-        const CSSProperty& property = resultSet->propertyAt(i);
+        StylePropertySet::PropertyReference property = resultSet->propertyAt(i);
         if (property.id() != CSSPropertyVariable && hasVariableReference(property.value()))
             resolveVariables(property.id(), property.value(), knownExpressions);
         else
index 98b00dd..71fb60d 100644 (file)
@@ -28,7 +28,6 @@
 
 #include "CSSComputedStyleDeclaration.h"
 #include "CSSParser.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CSSValueKeywords.h"
 #include "CSSValuePool.h"
index f01a513..0373bba 100644 (file)
@@ -1058,7 +1058,7 @@ void EditingStyle::mergeStyle(const StylePropertySet* style, CSSPropertyOverride
 
     unsigned propertyCount = style->propertyCount();
     for (unsigned i = 0; i < propertyCount; ++i) {
-        const CSSProperty& property = style->propertyAt(i);
+        StylePropertySet::PropertyReference property = style->propertyAt(i);
         RefPtr<CSSValue> value = m_mutableStyle->getPropertyCSSValue(property.id());
 
         // text decorations never override values
@@ -1114,7 +1114,7 @@ void EditingStyle::mergeStyleFromRulesForSerialization(StyledElement* element)
     {
         unsigned propertyCount = m_mutableStyle->propertyCount();
         for (unsigned i = 0; i < propertyCount; ++i) {
-            const CSSProperty& property = m_mutableStyle->propertyAt(i);
+            StylePropertySet::PropertyReference property = m_mutableStyle->propertyAt(i);
             CSSValue* value = property.value();
             if (!value->isPrimitiveValue())
                 continue;
index 499c5fd..5071d10 100644 (file)
@@ -31,7 +31,6 @@
 #include "AlternativeTextController.h"
 #include "ApplyStyleCommand.h"
 #include "CSSComputedStyleDeclaration.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CSSValueKeywords.h"
 #include "CachedResourceLoader.h"
index 46ee497..f6bf970 100644 (file)
@@ -31,7 +31,6 @@
 
 #include "CDATASection.h"
 #include "CSSPrimitiveValue.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CSSRule.h"
 #include "CSSRuleList.h"
index ed25477..56dcc28 100644 (file)
@@ -33,7 +33,6 @@
 #include "ApplyStyleCommand.h"
 #include "BackForwardController.h"
 #include "CSSComputedStyleDeclaration.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CachedCSSStyleSheet.h"
 #include "Chrome.h"
index 7c67648..249ebce 100644 (file)
@@ -27,7 +27,6 @@
 #include "Attribute.h"
 #include "CSSFontFaceSrcValue.h"
 #include "CSSParser.h"
-#include "CSSProperty.h"
 #include "CSSPropertyNames.h"
 #include "CSSStyleSheet.h"
 #include "CSSValueKeywords.h"