| Summary: | Move removeEquivalentProperties functions to EditingStyle | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zsolt Borbely <zsborbely.u-szeged> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, rniwa | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Zsolt Borbely
2014-04-02 01:07:56 PDT
Created attachment 228370 [details]
Proposed patch
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 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
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? 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. (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? Created attachment 230297 [details]
Proposed patch
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. 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 @@ +2014-04-29 Zsolt Borbely <zsborbely.u-szeged@partner.samsung.com> + + 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 <cgarcia@igalia.com> 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; Oops, "Edit Attachment As Comment" in the Details does not work as I thought. Created attachment 230388 [details]
Patch
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. Comment on attachment 230388 [details] Patch Clearing flags on attachment: 230388 Committed r167967: <http://trac.webkit.org/changeset/167967> All reviewed patches have been landed. Closing bug. (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. Fixed in r168011. |