Store and propagate a list of applied text decorations.

Source/core/rendering/style/RenderStyle.cpp

 /*
  * Copyright (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2011 Adobe Systems Incorporated. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 10 matching lines @@
  */
 
 #include "config.h"
 #include "core/rendering/style/RenderStyle.h"
 
 #include <algorithm>
 #include "RuntimeEnabledFeatures.h"
 #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"
 #include "core/rendering/style/ShadowList.h"
 #include "core/rendering/style/StyleImage.h"
 #include "core/rendering/style/StyleInheritedData.h"
 #include "platform/LengthFunctions.h"
 #include "platform/fonts/Font.h"
 #include "platform/fonts/FontSelector.h"
 #include "platform/geometry/FloatRoundedRect.h"
@@ skipping 640 matching lines @@
 
         if (rareNonInheritedData->opacity != other.rareNonInheritedData->opacity)
             changedContextSensitiveProperties |= ContextSensitivePropertyOpacity;
 
         if (rareNonInheritedData->m_filter != other.rareNonInheritedData->m_filter)
             changedContextSensitiveProperties |= ContextSensitivePropertyFilter;
     }
 
     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()) {
             if (rareNonInheritedData->m_textDecorationStyle != other.rareNonInheritedData->m_textDecorationStyle
                 || rareNonInheritedData->m_textDecorationColor != other.rareNonInheritedData->m_textDecorationColor)
                 changedContextSensitiveProperties |= ContextSensitivePropertyTextOrColor;
         } else if (rareInheritedData.get() != other.rareInheritedData.get()) {
             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;
         }
     }
 
     return changedContextSensitiveProperties;
 }
 
 void RenderStyle::setClip(Length top, Length right, Length bottom, Length left)
 {
     StyleVisualData* data = visual.access();
@@ skipping 484 matching lines @@
 }
 
 const Font& RenderStyle::font() const { return inherited->font; }
 const FontMetrics& RenderStyle::fontMetrics() const { return inherited->font.fontMetrics(); }
 const FontDescription& RenderStyle::fontDescription() const { return inherited->font.fontDescription(); }
 float RenderStyle::specifiedFontSize() const { return fontDescription().specifiedSize(); }
 float RenderStyle::computedFontSize() const { return fontDescription().computedSize(); }
 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(); }
 
 bool RenderStyle::setFontDescription(const FontDescription& v)
 {
     if (inherited->font.fontDescription() != v) {
         inherited.access()->font = Font(v);
         return true;
     }
     return false;
@@ skipping 75 matching lines @@
 
 void RenderStyle::setFontWeight(FontWeight weight)
 {
     FontSelector* currentFontSelector = font().fontSelector();
     FontDescription desc(fontDescription());
     desc.setWeight(weight);
     setFontDescription(desc);
     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;
     right = 0;
     bottom = 0;
     left = 0;
 
     size_t shadowCount = shadowList ? shadowList->shadows().size() : 0;
     for (size_t i = 0; i < shadowCount; ++i) {
         const ShadowData& shadow = shadowList->shadows()[i];
@@ skipping 57 matching lines @@
         const ShadowData& shadow = shadowList->shadows()[i];
         if (shadow.style() == Inset)
             continue;
         float blurAndSpread = shadow.blur() + shadow.spread();
 
         top = min<LayoutUnit>(top, shadow.y() - blurAndSpread);
         bottom = max<LayoutUnit>(bottom, shadow.y() + blurAndSpread);
     }
 }
 
-StyleColor RenderStyle::visitedDependentDecorationColor() const
+StyleColor RenderStyle::visitedDependentDecorationStyleColor() 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;

[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
+{
+    bool visitedLink = insideLink() == InsideVisitedLink;
+    return visitedDependentDecorationStyleColor().resolve(visitedLink ? visitedLinkColor() : color());
 }
 
 Color RenderStyle::colorIncludingFallback(int colorProperty, bool visitedLink) const
 {
     StyleColor result(StyleColor::currentColor());
     EBorderStyle borderStyle = BNONE;
     switch (colorProperty) {
     case CSSPropertyBackgroundColor:
         result = visitedLink ? visitedLinkBackgroundColor() : backgroundColor();
         break;
@@ skipping 279 matching lines @@
     // right
     radiiSum = radii.topRight().height() + radii.bottomRight().height();
     if (radiiSum > rect.height())
         factor = std::min(rect.height() / radiiSum, factor);
 
     ASSERT(factor <= 1);
     return factor;
 }
 
 } // namespace WebCore