Bug 131093 – Move removeEquivalentProperties functions to EditingStyle

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Zsolt Borbely]

Moved the removeEquivalentProperties functions from StyleProperties to EditingStyle class.

[Zsolt Borbely]

Created attachment 228370 [details]
Proposed patch

[Build Bot]

Comment on attachment 228370 [details]
Proposed patch

Attachment 228370 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5787827161792512

New failing tests:
editing/deleting/4916235-1.html
editing/pasteboard/8145-3.html
editing/pasteboard/3976872.html
editing/spelling/context-menu-suggestions-subword-selection.html
cssom/cssvalue-comparison.html
editing/style/5017613-1.html
editing/style/4230923.html
editing/spelling/context-menu-suggestions-multiword-selection.html
editing/inserting/insert-3659587-fix.html
editing/deleting/5032066.html
editing/inserting/5549929-2.html
editing/inserting/insert-3786362-fix.html
editing/deleting/25322-3.html
editing/spelling/context-menu-suggestions.html
editing/execCommand/15381.html
editing/execCommand/19455.html
editing/selection/crash-on-drag-with-mutation-events.html
editing/selection/applystyle-to-inline-in-block.html
editing/input/reveal-contenteditable-on-paste-vertically.html
editing/selection/5354455-1.html
editing/editability/ignored-content.html
editing/inserting/font-size-clears-from-typing-style.html
editing/selection/contains-boundaries.html
editing/execCommand/19653-1.html
editing/deleting/5115601.html
editing/pasteboard/19644-2.html
editing/pasteboard/19644-1.html
editing/style/4916887.html
editing/execCommand/19403.html
editing/execCommand/16049.html

[Build Bot]

Created attachment 228386 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Andreas Kling]

Comment on attachment 228370 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228370&action=review

Many test failures here.

> Source/WebCore/editing/EditingStyle.cpp:1576
> -    result->removeEquivalentProperties(baseStyle);
> +    RefPtr<EditingStyle> style = EditingStyle::create()->copy();
> +    style->removeEquivalentProperties(baseStyle);

Where's this data supposed to go?

[Ryosuke Niwa]

Comment on attachment 228370 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228370&action=review

>> Source/WebCore/editing/EditingStyle.cpp:1576
>> +    RefPtr<EditingStyle> style = EditingStyle::create()->copy();
>> +    style->removeEquivalentProperties(baseStyle);
> 
> Where's this data supposed to go?

This code doesn't make any sense.  We should be removing baseStyle properties from result here
but the new code is a no-op because there's nothing to be removed from an empty EditingStyle.

> Source/WebCore/editing/EditingStyle.h:149
> +    void removeEquivalentProperties(const StyleProperties*);
> +    void removeEquivalentProperties(const ComputedStyleExtractor*);
> +

These functions should be private.

[Zsolt Borbely]

(In reply to comment #5)
> (From update of attachment 228370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228370&action=review
> 
> >> Source/WebCore/editing/EditingStyle.cpp:1576
> >> +    RefPtr<EditingStyle> style = EditingStyle::create()->copy();
> >> +    style->removeEquivalentProperties(baseStyle);
> > 
> > Where's this data supposed to go?
> 
> This code doesn't make any sense.  We should be removing baseStyle properties from result here
> but the new code is a no-op because there's nothing to be removed from an empty EditingStyle.

You are right.

> > Source/WebCore/editing/EditingStyle.h:149
> > +    void removeEquivalentProperties(const StyleProperties*);
> > +    void removeEquivalentProperties(const ComputedStyleExtractor*);
> > +
> 
> These functions should be private.

If I move these functions to private I can't use them in the extractPropertiesNotIn(). 
Should I find a solution to use them as private?

[Zsolt Borbely]

Created attachment 230297 [details]
Proposed patch

[Darin Adler]

Comment on attachment 230297 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230297&action=review

> Source/WebCore/editing/EditingStyle.cpp:609
> +    RefPtr<MutableStyleProperties> style = nodeStyle->style();

This doesn’t need to be a RefPtr. Just a raw pointer would do. In fact, I suggest a reference rather than a pointer for this local variable.

> Source/WebCore/editing/EditingStyle.cpp:1195
> +void EditingStyle::removeEquivalentProperties(const StyleProperties* style)

This should take a reference, not a pointer, since a null pointer is not allowed. The callers should pass *style rather than style.get().

> Source/WebCore/editing/EditingStyle.cpp:1200
> +        StyleProperties::PropertyReference property = m_mutableStyle->propertyAt(i);

I think this would read better with auto.

> Source/WebCore/editing/EditingStyle.cpp:1206
> +    for (unsigned i = 0; i < propertiesToRemove.size(); ++i)
> +        m_mutableStyle->removeProperty(propertiesToRemove[i]);

This should be a modern C++ for loop.

    for (auto& property : propertiesToRemove)
        m_mutableStyle->removeProperty(property);

> Source/WebCore/editing/EditingStyle.cpp:1209
> +void EditingStyle::removeEquivalentProperties(const ComputedStyleExtractor* computedStyle)

This should take a reference, not a pointer, since a null pointer is not allowed. The callers should pass *style rather than style.get().

> Source/WebCore/editing/EditingStyle.cpp:1220
> +    for (unsigned i = 0; i < propertiesToRemove.size(); ++i)
> +        m_mutableStyle->removeProperty(propertiesToRemove[i]);

Ditto.

[Zsolt Borbely]

Comment on attachment 230297 [details]
Proposed patch

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 6c4749e..9a8531e 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,24 @@
[phone]  Zsolt Borbely  <[e-mail]>
+
+        Move removeEquivalentProperties functions to EditingStyle
+        https://bugs.webkit.org/show_bug.cgi?id=131093
+
+        Reviewed by NOBODY (OOPS!).
+
+        Moved the removeEquivalentProperties functions
+        from StyleProperties to EditingStyle class.
+
+        * css/StyleProperties.cpp:
+        (WebCore::MutableStyleProperties::removeEquivalentProperties): Deleted.
+        * css/StyleProperties.h:
+        * editing/EditingStyle.cpp:
+        (WebCore::EditingStyle::removeStyleAddedByNode):
+        (WebCore::EditingStyle::removeStyleConflictingWithStyleOfNode):
+        (WebCore::EditingStyle::prepareToApplyAt):
+        (WebCore::EditingStyle::removeEquivalentProperties):
+        (WebCore::extractPropertiesNotIn):
+        * editing/EditingStyle.h:
+
 2014-04-28  Carlos Garcia Campos  <[e-mail]>
 
         Unreviewed. Update GObject DOM bindings symbols file.
diff --git a/Source/WebCore/css/StyleProperties.cpp b/Source/WebCore/css/StyleProperties.cpp
index 0fd8d16..c06784a 100644
--- a/Source/WebCore/css/StyleProperties.cpp
+++ b/Source/WebCore/css/StyleProperties.cpp
@@ -1173,34 +1173,6 @@ bool StyleProperties::propertyMatches(CSSPropertyID propertyID, const CSSValue*
     return propertyAt(foundPropertyIndex).value()->equals(*propertyValue);
 }
 
-void MutableStyleProperties::removeEquivalentProperties(const StyleProperties* style)
-{
-    Vector<CSSPropertyID> propertiesToRemove;
-    unsigned size = m_propertyVector.size();
-    for (unsigned i = 0; i < size; ++i) {
-        PropertyReference property = propertyAt(i);
-        if (style->propertyMatches(property.id(), property.value()))
-            propertiesToRemove.append(property.id());
-    }    
-    // FIXME: This should use mass removal.
-    for (unsigned i = 0; i < propertiesToRemove.size(); ++i)
-        removeProperty(propertiesToRemove[i]);
-}
-
-void MutableStyleProperties::removeEquivalentProperties(const ComputedStyleExtractor* computedStyle)
-{
-    Vector<CSSPropertyID> propertiesToRemove;
-    unsigned size = m_propertyVector.size();
-    for (unsigned i = 0; i < size; ++i) {
-        PropertyReference property = propertyAt(i);
-        if (computedStyle->propertyMatches(property.id(), property.value()))
-            propertiesToRemove.append(property.id());
-    }    
-    // FIXME: This should use mass removal.
-    for (unsigned i = 0; i < propertiesToRemove.size(); ++i)
-        removeProperty(propertiesToRemove[i]);
-}
-
 PassRef<MutableStyleProperties> StyleProperties::mutableCopy() const
 {
     return adoptRef(*new MutableStyleProperties(*this));
diff --git a/Source/WebCore/css/StyleProperties.h b/Source/WebCore/css/StyleProperties.h
index f958c84..c9d7c57 100644
--- a/Source/WebCore/css/StyleProperties.h
+++ b/Source/WebCore/css/StyleProperties.h
@@ -211,10 +211,6 @@ public:
     void removeBlockProperties();
     bool removePropertiesInSet(const CSSPropertyID* set, unsigned length);
 
-    // FIXME: These two can be moved to EditingStyle.cpp
-    void removeEquivalentProperties(const StyleProperties*);
-    void removeEquivalentProperties(const ComputedStyleExtractor*);
-
     void mergeAndOverrideOnConflict(const StyleProperties&);
 
     void clear();
diff --git a/Source/WebCore/editing/EditingStyle.cpp b/Source/WebCore/editing/EditingStyle.cpp
index 000cdbd..a4f6334 100644
--- a/Source/WebCore/editing/EditingStyle.cpp
+++ b/Source/WebCore/editing/EditingStyle.cpp
@@ -593,8 +593,8 @@ void EditingStyle::removeStyleAddedByNode(Node* node)
         return;
     RefPtr<MutableStyleProperties> parentStyle = copyPropertiesFromComputedStyle(node->parentNode(), EditingPropertiesInEffect);
     RefPtr<MutableStyleProperties> nodeStyle = copyPropertiesFromComputedStyle(node, EditingPropertiesInEffect);
-    nodeStyle->removeEquivalentProperties(parentStyle.get());
-    m_mutableStyle->removeEquivalentProperties(nodeStyle.get());
+    removeEquivalentProperties(*parentStyle);
+    removeEquivalentProperties(*nodeStyle);
 }
 
 void EditingStyle::removeStyleConflictingWithStyleOfNode(Node* node)
@@ -603,12 +603,13 @@ void EditingStyle::removeStyleConflictingWithStyleOfNode(Node* node)
         return;
 
     RefPtr<MutableStyleProperties> parentStyle = copyPropertiesFromComputedStyle(node->parentNode(), EditingPropertiesInEffect);
-    RefPtr<MutableStyleProperties> nodeStyle = copyPropertiesFromComputedStyle(node, EditingPropertiesInEffect);
-    nodeStyle->removeEquivalentProperties(parentStyle.get());
+    RefPtr<EditingStyle> nodeStyle = EditingStyle::create(node, EditingPropertiesInEffect);
+    nodeStyle->removeEquivalentProperties(*parentStyle);
 
-    unsigned propertyCount = nodeStyle->propertyCount();
+    MutableStyleProperties* style = nodeStyle->style();
+    unsigned propertyCount = style->propertyCount();
     for (unsigned i = 0; i < propertyCount; ++i)
-        m_mutableStyle->removeProperty(nodeStyle->propertyAt(i).id());
+        m_mutableStyle->removeProperty(style->propertyAt(i).id());
 }
 
 void EditingStyle::collapseTextDecorationProperties()
@@ -916,7 +917,7 @@ void EditingStyle::prepareToApplyAt(const Position& position, ShouldPreserveWrit
         direction = m_mutableStyle->getPropertyCSSValue(CSSPropertyDirection);
     }
 
-    m_mutableStyle->removeEquivalentProperties(styleAtPosition);
+    removeEquivalentProperties(*styleAtPosition);
 
     if (textAlignResolvingStartAndEnd(m_mutableStyle.get()) == textAlignResolvingStartAndEnd(styleAtPosition))
         m_mutableStyle->removeProperty(CSSPropertyTextAlign);
@@ -1191,6 +1192,19 @@ void EditingStyle::removePropertiesInElementDefaultStyle(Element* element)
     removePropertiesInStyle(m_mutableStyle.get(), defaultStyle.get());
 }
 
+template<typename T>
+void EditingStyle::removeEquivalentProperties(const T& style)
+{
+    Vector<CSSPropertyID> propertiesToRemove;
+    for (auto& property : m_mutableStyle->m_propertyVector) {
+        if (style.propertyMatches(property.id(), property.value()))
+            propertiesToRemove.append(property.id());
+    }
+    // FIXME: This should use mass removal.
+    for (auto& property : propertiesToRemove)
+        m_mutableStyle->removeProperty(property);
+}
+
 void EditingStyle::forceInline()
 {
     if (!m_mutableStyle)
@@ -1542,28 +1556,28 @@ template<typename T>
 static PassRefPtr<MutableStyleProperties> extractPropertiesNotIn(StyleProperties* styleWithRedundantProperties, T* baseStyle)
 {
     ASSERT(styleWithRedundantProperties);
-    RefPtr<MutableStyleProperties> result = styleWithRedundantProperties->mutableCopy();
-
-    result->removeEquivalentProperties(baseStyle);
+    RefPtr<EditingStyle> result = EditingStyle::create(styleWithRedundantProperties);
+    result->removeEquivalentProperties(*baseStyle);
+    RefPtr<MutableStyleProperties> mutableStyle = result->style();
 
     RefPtr<CSSValue> baseTextDecorationsInEffect = extractPropertyValue(baseStyle, CSSPropertyWebkitTextDecorationsInEffect);
-    diffTextDecorations(result.get(), CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
-    diffTextDecorations(result.get(), CSSPropertyWebkitTextDecorationsInEffect, baseTextDecorationsInEffect.get());
+    diffTextDecorations(mutableStyle.get(), CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
+    diffTextDecorations(mutableStyle.get(), CSSPropertyWebkitTextDecorationsInEffect, baseTextDecorationsInEffect.get());
 
-    if (extractPropertyValue(baseStyle, CSSPropertyFontWeight) && fontWeightIsBold(result.get()) == fontWeightIsBold(baseStyle))
-        result->removeProperty(CSSPropertyFontWeight);
+    if (extractPropertyValue(baseStyle, CSSPropertyFontWeight) && fontWeightIsBold(mutableStyle.get()) == fontWeightIsBold(baseStyle))
+        mutableStyle->removeProperty(CSSPropertyFontWeight);
 
-    if (extractPropertyValue(baseStyle, CSSPropertyColor) && textColorFromStyle(result.get()) == textColorFromStyle(baseStyle))
-        result->removeProperty(CSSPropertyColor);
+    if (extractPropertyValue(baseStyle, CSSPropertyColor) && textColorFromStyle(mutableStyle.get()) == textColorFromStyle(baseStyle))
+        mutableStyle->removeProperty(CSSPropertyColor);
 
     if (extractPropertyValue(baseStyle, CSSPropertyTextAlign)
-        && textAlignResolvingStartAndEnd(result.get()) == textAlignResolvingStartAndEnd(baseStyle))
-        result->removeProperty(CSSPropertyTextAlign);
+        && textAlignResolvingStartAndEnd(mutableStyle.get()) == textAlignResolvingStartAndEnd(baseStyle))
+        mutableStyle->removeProperty(CSSPropertyTextAlign);
 
-    if (extractPropertyValue(baseStyle, CSSPropertyBackgroundColor) && backgroundColorFromStyle(result.get()) == backgroundColorFromStyle(baseStyle))
-        result->removeProperty(CSSPropertyBackgroundColor);
+    if (extractPropertyValue(baseStyle, CSSPropertyBackgroundColor) && backgroundColorFromStyle(mutableStyle.get()) == backgroundColorFromStyle(baseStyle))
+        mutableStyle->removeProperty(CSSPropertyBackgroundColor);
 
-    return result.release();
+    return mutableStyle.release();
 }
 
 template<typename T>
diff --git a/Source/WebCore/editing/EditingStyle.h b/Source/WebCore/editing/EditingStyle.h
index 8200903..bde713e 100644
--- a/Source/WebCore/editing/EditingStyle.h
+++ b/Source/WebCore/editing/EditingStyle.h
@@ -48,6 +48,7 @@ class CSSStyleDeclaration;
 class CSSComputedStyleDeclaration;
 class CSSPrimitiveValue;
 class CSSValue;
+class ComputedStyleExtractor;
 class Document;
 class Element;
 class HTMLElement;
@@ -108,6 +109,8 @@ public:
     void removeBlockProperties();
     void removeStyleAddedByNode(Node*);
     void removeStyleConflictingWithStyleOfNode(Node*);
+    template<typename T>
+    void removeEquivalentProperties(const T&);
     void collapseTextDecorationProperties();
     enum ShouldIgnoreTextOnlyProperties { IgnoreTextOnlyProperties, DoNotIgnoreTextOnlyProperties };
     TriState triStateOfStyle(EditingStyle*) const;

[Zsolt Borbely]

Oops, "Edit Attachment As Comment" in the Details does not work as I thought.

[Zsolt Borbely]

Created attachment 230388 [details]
Patch

[Darin Adler]

Comment on attachment 230388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230388&action=review

> Source/WebCore/editing/EditingStyle.h:113
> +    template<typename T>
> +    void removeEquivalentProperties(const T&);

I’d format this on a single line. It’s misleading formatted like this.

[WebKit Commit Bot]

Comment on attachment 230388 [details]
Patch

Clearing flags on attachment: 230388

Committed r167967: <http://trac.webkit.org/changeset/167967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zsolt Borbely]

(In reply to comment #12)
> (From update of attachment 230388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230388&action=review
> 
> > Source/WebCore/editing/EditingStyle.h:113
> > +    template<typename T>
> > +    void removeEquivalentProperties(const T&);
> 
> I’d format this on a single line. It’s misleading formatted like this.

You are right.
I checked that there is an other template definition in the same file and I just copied its align.

[Zsolt Borbely]

Fixed in r168011.