[css3-text] Implement text-decoration property shorthand

Source/core/editing/EditingStyle.cpp

Index: Source/core/editing/EditingStyle.cpp
diff --git a/Source/core/editing/EditingStyle.cpp b/Source/core/editing/EditingStyle.cpp
index 94915548ce017a13b75a80f91cfc8d52354b65eb..7b2b646959e6497242ae01ac7a9ca554d268ede8 100644
--- a/Source/core/editing/EditingStyle.cpp
+++ b/Source/core/editing/EditingStyle.cpp
@@ -55,16 +55,27 @@
 
 namespace WebCore {
 
+// FIXME: Text decoration property to be used based on runtime feature check.
+// Needs to be removed when CSS3 Text Decoration is no longer experimental.

[Julien - ping for review, 2013/08/02 23:22:13]

I don't think this comment adds much value: it's fairly obvious that we should
remove this function once it's trivially returning one value.
Again, this is a 'what' comment.

[abinader, 2013/08/05 22:51:14]

Ack.

+static const CSSPropertyID& runtimeTextDecorationProperty()

[Julien - ping for review, 2013/08/02 23:22:13]

How about textDecorationLine()? (this matches what we expect to support in the
future and you can explain *why* we return CSSPropertyDecoration for CSS 2.1)

Alternative naming: textDecorationPropertyForEditing()

[abinader, 2013/08/05 22:51:14]

Right, textDecorationPropertyForEditing() sounds good.

+{
+    static const CSSPropertyID property = RuntimeEnabledFeatures::css3TextDecorationsEnabled() ? CSSPropertyTextDecorationLine : CSSPropertyTextDecoration;
+    return property;
+}
+
 // Editing style properties must be preserved during editing operation.
 // e.g. when a user inserts a new paragraph, all properties listed here must be copied to the new paragraph.
 // NOTE: Use editingProperties() to respect runtime enabling of properties.
 static const unsigned nonInheritedStaticPropertiesCount = 2;
 
 static const CSSPropertyID staticEditingProperties[] = {
-    // NOTE: inheritableEditingProperties depends on these two properties being first.
+    // NOTE: inheritableEditingProperties depends on these properties being first.
     // If you change this list, make sure to update nonInheritedPropertyCount.

[Julien - ping for review, 2013/08/02 23:22:13]

I missed that: nonInheritPropertyCount should still be updated as
text-decoration-line is not inherited per the specification.

Is this tested somewhere as it seems like a bad idea to include non-inherited
properties into the inherited ones?

[abinader, 2013/08/05 22:51:14]

I wasn't planning on modifying the nonInheritedStaticPropertiesCount from 2 to
3, but I'm seeing now this is really needed for inheritableEditingProperties()
sanity - I erroneously assumed that both static functions shared the same
"properties" vector, and I was only making the proper sanity check on
allEditingProperties(). Nice catch!

     CSSPropertyBackgroundColor,
+    // FIXME: CSSPropertyTextDecoration needs to be removed when CSS3 Text
+    // Decoration feature is no longer experimental.
     CSSPropertyTextDecoration,
+    CSSPropertyTextDecorationLine,
 
     // CSS inheritable properties
     CSSPropertyColor,
@@ -93,8 +104,16 @@ enum EditingPropertiesType { OnlyInheritableEditingProperties, AllEditingPropert
 static const Vector<CSSPropertyID>& allEditingProperties()
 {
     DEFINE_STATIC_LOCAL(Vector<CSSPropertyID>, properties, ());
-    if (properties.isEmpty())
+    if (properties.isEmpty()) {
         RuntimeCSSEnabled::filterEnabledCSSPropertiesIntoVector(staticEditingProperties, WTF_ARRAY_LENGTH(staticEditingProperties), properties);
+        // This ensures we have only one of these properties on the vector.
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+            properties.remove(properties.find(CSSPropertyTextDecoration));
+        else
+            properties.remove(properties.find(CSSPropertyTextDecorationLine));
+    }
+    ASSERT((RuntimeEnabledFeatures::css3TextDecorationsEnabled() && !properties.contains(CSSPropertyTextDecoration))
+        || (!RuntimeEnabledFeatures::css3TextDecorationsEnabled() && properties.contains(CSSPropertyTextDecoration)));

[Julien - ping for review, 2013/08/02 23:22:13]

This would be better off written:

#ifndef NDEBUG
    if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
        ASSERT(!properties.contains(CSSPropertyTextDecoration));
    else
        ASSERT(properties.contains(CSSPropertyTextDecoration));
#endif

Also, isn't it redundant with the code above anyway as we can trivially see what
you remove?

[abinader, 2013/08/05 22:51:14]

I guess you're right, this looks too trivial and thus unnecessary.

     return properties;
 }
 
@@ -203,21 +222,22 @@ private:
 };
 
 HTMLTextDecorationEquivalent::HTMLTextDecorationEquivalent(CSSValueID primitiveValue, const QualifiedName& tagName)
-    : HTMLElementEquivalent(CSSPropertyTextDecoration, primitiveValue, tagName)
+    : HTMLElementEquivalent(runtimeTextDecorationProperty(), primitiveValue, tagName)
     // m_propertyID is used in HTMLElementEquivalent::addToStyle
 {
 }
 
 bool HTMLTextDecorationEquivalent::propertyExistsInStyle(const StylePropertySet* style) const
 {
-    return style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    return style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect)
+        || style->getPropertyCSSValue(runtimeTextDecorationProperty());
 }
 
 bool HTMLTextDecorationEquivalent::valueIsPresentInStyle(Element* element, StylePropertySet* style) const
 {
     RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
     if (!styleValue)
-        styleValue = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+        styleValue = style->getPropertyCSSValue(runtimeTextDecorationProperty());
     return matches(element) && styleValue && styleValue->isValueList() && static_cast<CSSValueList*>(styleValue.get())->hasValue(m_primitiveValue.get());
 }
 
@@ -651,15 +671,18 @@ void EditingStyle::collapseTextDecorationProperties()
         return;
 
     if (textDecorationsInEffect->isValueList())
-        m_mutableStyle->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText(), m_mutableStyle->propertyIsImportant(CSSPropertyTextDecoration));
+        m_mutableStyle->setProperty(runtimeTextDecorationProperty(), textDecorationsInEffect->cssText(), m_mutableStyle->propertyIsImportant(runtimeTextDecorationProperty()));
     else
-        m_mutableStyle->removeProperty(CSSPropertyTextDecoration);
+        m_mutableStyle->removeProperty(runtimeTextDecorationProperty());
     m_mutableStyle->removeProperty(CSSPropertyWebkitTextDecorationsInEffect);
 }
 
 // CSS properties that create a visual difference only when applied to text.
 static const CSSPropertyID textOnlyProperties[] = {
+    // FIXME: CSSPropertyTextDecoration needs to be removed when CSS3 Text
+    // Decoration feature is no longer experimental.
     CSSPropertyTextDecoration,
+    CSSPropertyTextDecorationLine,
     CSSPropertyWebkitTextDecorationsInEffect,
     CSSPropertyFontStyle,
     CSSPropertyFontWeight,
@@ -736,12 +759,16 @@ bool EditingStyle::conflictsWithInlineStyleOfElement(Element* element, EditingSt
         if (propertyID == CSSPropertyWhiteSpace && isTabSpanNode(element))
             continue;
 
-        if (propertyID == CSSPropertyWebkitTextDecorationsInEffect && inlineStyle->getPropertyCSSValue(CSSPropertyTextDecoration)) {
+        if (propertyID == CSSPropertyWebkitTextDecorationsInEffect && inlineStyle->getPropertyCSSValue(runtimeTextDecorationProperty())) {
             if (!conflictingProperties)
                 return true;
             conflictingProperties->append(CSSPropertyTextDecoration);
+            // Because text-decoration expands to text-decoration-line when CSS3
+            // Text Decoration is enabled, we also state it as conflicting.
+            if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+                conflictingProperties->append(CSSPropertyTextDecorationLine);
             if (extractedStyle)
-                extractedStyle->setProperty(CSSPropertyTextDecoration, inlineStyle->getPropertyValue(CSSPropertyTextDecoration), inlineStyle->propertyIsImportant(CSSPropertyTextDecoration));
+                extractedStyle->setProperty(runtimeTextDecorationProperty(), inlineStyle->getPropertyValue(runtimeTextDecorationProperty()), inlineStyle->propertyIsImportant(runtimeTextDecorationProperty()));
             continue;
         }
 
@@ -1082,7 +1109,7 @@ void EditingStyle::mergeStyle(const StylePropertySet* style, CSSPropertyOverride
         RefPtr<CSSValue> value = m_mutableStyle->getPropertyCSSValue(property.id());
 
         // text decorations never override values
-        if ((property.id() == CSSPropertyTextDecoration || property.id() == CSSPropertyWebkitTextDecorationsInEffect) && property.value()->isValueList() && value) {
+        if ((property.id() == runtimeTextDecorationProperty() || property.id() == CSSPropertyWebkitTextDecorationsInEffect) && property.value()->isValueList() && value) {
             if (value->isValueList()) {
                 mergeTextDecorationValues(static_cast<CSSValueList*>(value.get()), static_cast<CSSValueList*>(property.value()));
                 continue;
@@ -1337,18 +1364,18 @@ WritingDirection EditingStyle::textDirectionForSelection(const VisibleSelection&
 static void reconcileTextDecorationProperties(MutableStylePropertySet* style)
 {
     RefPtr<CSSValue> textDecorationsInEffect = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
-    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(runtimeTextDecorationProperty());
     // We shouldn't have both text-decoration and -webkit-text-decorations-in-effect because that wouldn't make sense.
     ASSERT(!textDecorationsInEffect || !textDecoration);
     if (textDecorationsInEffect) {
-        style->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText());
+        style->setProperty(runtimeTextDecorationProperty(), textDecorationsInEffect->cssText());
         style->removeProperty(CSSPropertyWebkitTextDecorationsInEffect);
         textDecoration = textDecorationsInEffect;
     }
 
     // If text-decoration is set to "none", remove the property because we don't want to add redundant "text-decoration: none".
     if (textDecoration && !textDecoration->isValueList())
-        style->removeProperty(CSSPropertyTextDecoration);
+        style->removeProperty(runtimeTextDecorationProperty());
 }
 
 StyleChange::StyleChange(EditingStyle* style, const Position& position)
@@ -1412,7 +1439,7 @@ void StyleChange::extractTextStyles(Document* document, MutableStylePropertySet*
 
     // Assuming reconcileTextDecorationProperties has been called, there should not be -webkit-text-decorations-in-effect
     // Furthermore, text-decoration: none has been trimmed so that text-decoration property is always a CSSValueList.
-    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(runtimeTextDecorationProperty());
     if (textDecoration && textDecoration->isValueList()) {
         DEFINE_STATIC_LOCAL(RefPtr<CSSPrimitiveValue>, underline, (CSSPrimitiveValue::createIdentifier(CSSValueUnderline)));
         DEFINE_STATIC_LOCAL(RefPtr<CSSPrimitiveValue>, lineThrough, (CSSPrimitiveValue::createIdentifier(CSSValueLineThrough)));
@@ -1424,7 +1451,7 @@ void StyleChange::extractTextStyles(Document* document, MutableStylePropertySet*
             m_applyLineThrough = true;
 
         // If trimTextDecorations, delete underline and line-through
-        setTextDecorationProperty(style, newTextDecoration.get(), CSSPropertyTextDecoration);
+        setTextDecorationProperty(style, newTextDecoration.get(), runtimeTextDecorationProperty());
     }
 
     int verticalAlign = getIdentifierValue(style, CSSPropertyVerticalAlign);
@@ -1529,7 +1556,7 @@ PassRefPtr<MutableStylePropertySet> getPropertiesNotIn(StylePropertySet* styleWi
     result->removeEquivalentProperties(baseStyle);
 
     RefPtr<CSSValue> baseTextDecorationsInEffect = baseStyle->getPropertyCSSValueInternal(CSSPropertyWebkitTextDecorationsInEffect);
-    diffTextDecorations(result.get(), CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
+    diffTextDecorations(result.get(), runtimeTextDecorationProperty(), baseTextDecorationsInEffect.get());
     diffTextDecorations(result.get(), CSSPropertyWebkitTextDecorationsInEffect, baseTextDecorationsInEffect.get());
 
     if (baseStyle->getPropertyCSSValueInternal(CSSPropertyFontWeight) && fontWeightIsBold(result.get()) == fontWeightIsBold(baseStyle))