Store and propagate a list of applied text decorations.

Source/core/rendering/style/RenderStyle.cpp

Index: Source/core/rendering/style/RenderStyle.cpp
diff --git a/Source/core/rendering/style/RenderStyle.cpp b/Source/core/rendering/style/RenderStyle.cpp
index dfeb3bed61f5b2ce8ec111e0e8193140b5c768cd..553405cf723f756786f2d533c4ac4344d3e17363 100644
--- a/Source/core/rendering/style/RenderStyle.cpp
+++ b/Source/core/rendering/style/RenderStyle.cpp
@@ -28,6 +28,7 @@
 #include "core/css/resolver/StyleResolver.h"
 #include "core/rendering/RenderTheme.h"
 #include "core/rendering/TextAutosizer.h"
+#include "core/rendering/style/AppliedTextDecoration.h"
 #include "core/rendering/style/ContentData.h"
 #include "core/rendering/style/CursorList.h"
 #include "core/rendering/style/QuotesData.h"
@@ -688,7 +689,7 @@ unsigned RenderStyle::computeChangedContextSensitiveProperties(const RenderStyle
 
     if (!diff.needsRepaint()) {
         if (inherited->color != other.inherited->color
-            || inherited_flags._text_decorations != other.inherited_flags._text_decorations
+            || inherited_flags.m_textUnderline != other.inherited_flags.m_textUnderline
             || visual->textDecoration != other.visual->textDecoration) {
             changedContextSensitiveProperties |= ContextSensitivePropertyTextOrColor;
         } else if (rareNonInheritedData.get() != other.rareNonInheritedData.get()) {
@@ -699,7 +700,8 @@ unsigned RenderStyle::computeChangedContextSensitiveProperties(const RenderStyle
             if (rareInheritedData->textFillColor() != other.rareInheritedData->textFillColor()
                 || rareInheritedData->textStrokeColor() != other.rareInheritedData->textStrokeColor()
                 || rareInheritedData->textEmphasisColor() != other.rareInheritedData->textEmphasisColor()
-                || rareInheritedData->textEmphasisFill != other.rareInheritedData->textEmphasisFill)
+                || rareInheritedData->textEmphasisFill != other.rareInheritedData->textEmphasisFill
+                || rareInheritedData->appliedTextDecorations != other.rareInheritedData->appliedTextDecorations)
                 changedContextSensitiveProperties |= ContextSensitivePropertyTextOrColor;
         }
     }
@@ -1204,6 +1206,32 @@ float RenderStyle::computedFontSize() const { return fontDescription().computedS
 int RenderStyle::fontSize() const { return fontDescription().computedPixelSize(); }
 FontWeight RenderStyle::fontWeight() const { return fontDescription().weight(); }
 
+TextDecoration RenderStyle::textDecorationsInEffect() const
+{
+    int decorations = 0;
+
+    const Vector<AppliedTextDecoration>& applied = appliedTextDecorations();
+
+    for (size_t i = 0; i < applied.size(); ++i)
+        decorations |= applied[i].line();
+
+    return static_cast<TextDecoration>(decorations);
+}
+
+const Vector<AppliedTextDecoration>& RenderStyle::appliedTextDecorations() const
+{
+    if (!inherited_flags.m_textUnderline && !rareInheritedData->appliedTextDecorations) {
+        DEFINE_STATIC_LOCAL(Vector<AppliedTextDecoration>, empty, ());
+        return empty;
+    }

[Julien - ping for review, 2014/05/06 23:29:30]

I think we should just remove this artificial case and stop pretending that we
always have a text-decoration. If our callers can't ensure that, then it's a
perfectly fine answer to return nullptr here.

[andersr, 2014/05/07 11:17:53]

We are not pretending that we have *a* text-decoration, we choose to represent
the case of *no* text decorations as an empty list, for the convenience of
calling code.

In my opinion, this is an improvement over just returning nullptr, without any
real drawbacks. Calling code won't have to null-check/wonder if it should be
null-checked/wonder if it has been null-checked.

[Julien - ping for review, 2014/05/09 15:45:50]

OK, I still think it's artificial and that we shouldn't be called when we have
no text decoration but I can sympathize with this vision.

+    if (inherited_flags.m_textUnderline) {
+        DEFINE_STATIC_LOCAL(Vector<AppliedTextDecoration>, underline, (1, AppliedTextDecoration(TextDecorationUnderline)));
+        return underline;
+    }
+
+    return rareInheritedData->appliedTextDecorations->vector();
+}
+
 float RenderStyle::wordSpacing() const { return fontDescription().wordSpacing(); }
 float RenderStyle::letterSpacing() const { return fontDescription().letterSpacing(); }
 
@@ -1299,6 +1327,60 @@ void RenderStyle::setFontWeight(FontWeight weight)
     font().update(currentFontSelector);
 }
 
+void RenderStyle::addAppliedTextDecoration(const AppliedTextDecoration& decoration)
+{
+    bool isSimpleUnderline = decoration.isSimple() && decoration.line() == TextDecorationUnderline;
+
+    if (!rareInheritedData->appliedTextDecorations && isSimpleUnderline) {
+        // To save memory, we don't use AppliedTextDecoration objects in the
+        // common case of a single solid underline with the current color. Instead,
+        // we set a bit to indicate that a solid underline must be drawn with the
+        // current color.
+        inherited_flags.m_textUnderline = true;
+        return;
+    }

[Julien - ping for review, 2014/05/06 23:29:30]

I still think this should be moved into the caller (applyTextDecorations). My
counter-point to your answer
(https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...)
is that this is the low-level function that *shouldn't* have to know about how
the packing / unpacking done.

Not only will this be cleaner but it will also be less work as we don't need to
repeatedly check this case.

[andersr, 2014/05/07 11:17:53]

OK. In that case RenderStyle::addAppliedTextDecoration must be made private.

+
+    RefPtr<AppliedTextDecorationList>& list = rareInheritedData.access()->appliedTextDecorations;
+
+    if (!list)
+        list = AppliedTextDecorationList::create();
+    else if (!list->hasOneRef())
+        list = list->copy();

[Julien - ping for review, 2014/05/06 23:29:30]

I don't understand how you are saying that we can have more than 3 values (sorry
I mentioned 4 earlier but was off by one). The specification also seems to match
that (<text-decoration-line> := none | [underline || overline || line-through ||
blink ] and we don't support the value 'blink').

While I understand your worst case thinking about having an applied text
decoration per line, I don't buy that this complex code is giving us much. If
the Vector is shared, we would end up copying it here, which for me defeats the
purpose of this optimization. Unless you have data backing your claim for memory
saving, I would rather go with a more simple solution.

[andersr, 2014/05/07 11:17:53]

The worst case is not *an* applied text decoration per line, it's *five* applied
decorations per line.

Example: 15 decorations applied to a word in Firefox 29:
http://jsfiddle.net/a525B/
No, I don't think so. The alternative is always copying, and surely it's better
to share when we can (when no new decorations are applied/cleared), and copy
when we have to (when decorations *are* applied/cleared).
I'm all for that, but what do you suggest? I can't think of any other solutions
that are simpler.
Memory saved compared to which alternative approach?

[Julien - ping for review, 2014/05/09 15:45:50]

For people following at home, it turns out that we have an interesting cascade
for 'text-decoration' that allows multiple one with the same name. That's the
reason we can have a text-decoration explosion.
The simple solution was to not have the RefVector and just use a Vector, which
means copying the data more.

Andersr pointed that leaf RenderText will never override the RenderStyle from
their parent so it's a good optimization just for them, to which I agree. I
think this should be mentioned in the change description.

+
+    if (inherited_flags.m_textUnderline) {
+        inherited_flags.m_textUnderline = false;
+        list->append(AppliedTextDecoration(TextDecorationUnderline, TextDecorationStyleSolid, StyleColor::currentColor()));
+    }
+
+    list->append(decoration);
+}
+
+void RenderStyle::applyTextDecorations()
+{
+    if (textDecoration() == TextDecorationNone)
+        return;
+
+    TextDecorationStyle style = textDecorationStyle();
+    StyleColor styleColor = visitedDependentDecorationStyleColor();
+
+    int decorations = textDecoration();
+
+    if (decorations & TextDecorationUnderline)
+        addAppliedTextDecoration(AppliedTextDecoration(TextDecorationUnderline, style, styleColor));
+    if (decorations & TextDecorationOverline)
+        addAppliedTextDecoration(AppliedTextDecoration(TextDecorationOverline, style, styleColor));
+    if (decorations & TextDecorationLineThrough)
+        addAppliedTextDecoration(AppliedTextDecoration(TextDecorationLineThrough, style, styleColor));
+}
+
+void RenderStyle::clearAppliedTextDecorations()
+{
+    inherited_flags.m_textUnderline = false;
+
+    if (rareInheritedData->appliedTextDecorations)
+        rareInheritedData.access()->appliedTextDecorations = nullptr;
+}
+
 void RenderStyle::getShadowExtent(const ShadowList* shadowList, LayoutUnit &top, LayoutUnit &right, LayoutUnit &bottom, LayoutUnit &left) const
 {
     top = 0;
@@ -1376,10 +1458,29 @@ void RenderStyle::getShadowVerticalExtent(const ShadowList* shadowList, LayoutUn
     }
 }
 
-StyleColor RenderStyle::visitedDependentDecorationColor() const
+StyleColor RenderStyle::visitedDependentDecorationStyleColor() const
+{
+    bool visitedLink = insideLink() == InsideVisitedLink;

[Julien - ping for review, 2014/05/06 23:29:30]

Coding style: Name-5. Precede boolean values with words like "is" and "did".

[andersr, 2014/05/07 11:17:53]

OK.

+
+    StyleColor styleColor = visitedLink ? visitedLinkTextDecorationColor() : textDecorationColor();
+
+    if (!styleColor.isCurrentColor())
+        return styleColor;
+
+    if (textStrokeWidth()) {
+        // Prefer stroke color if possible but not if it's fully transparent.
+        StyleColor textStrokeStyleColor = visitedLink ? visitedLinkTextStrokeColor() : textStrokeColor();
+        if (!textStrokeStyleColor.isCurrentColor() && textStrokeStyleColor.color().alpha())
+            return textStrokeStyleColor;
+    }
+
+    return visitedLink ? visitedLinkTextFillColor() : textFillColor();
+}
+
+Color RenderStyle::visitedDependentDecorationColor() const
 {
-    // Text decoration color fallback is handled in RenderObject::decorationColor.

[Julien - ping for review, 2014/05/06 23:29:30]

Based on this comment, I think this part of the change should go in a follow-up
change.

[andersr, 2014/05/07 11:17:53]

This is necessary for resolving the style recalculation bug.

It could be moved, but then the whole fix should be moved to another patch. (It
would mean that AppliedTextDecoration would contain "line" only, for now).

I suppose I could do that, since you already suggested to submit the refactoring
separately.

-    return insideLink() == InsideVisitedLink ? visitedLinkTextDecorationColor() : textDecorationColor();
+    bool visitedLink = insideLink() == InsideVisitedLink;
+    return visitedDependentDecorationStyleColor().resolve(visitedLink ? visitedLinkColor() : color());
 }
 
 Color RenderStyle::colorIncludingFallback(int colorProperty, bool visitedLink) const