Proper support for multiple text decorations.

Proper support for multiple text decorations.

This patch fixes two issues:

 * Incorrect style recalc for text-decoration-[style, color] for
   immediate child text nodes. (Issue 350840).
 * The ability of having multiple applied text decorations on the same line,
   for example a dashed underline over a wavy underline. (If that's your thing).

Applied decorations (any number) are now stored as a vector of
AppliedTextDecoration objects in StyleRareInheritedData. In the common case of
a solid-underline-currentColor decoration, we don't store AppliedTextDecoration
objects, but set a bit in inherited_flags instead. Some notes related to this:

 * If there are multiple solid-underline-currentColor decorations applied (i.e.
   if more than one parent generates such a decoration), it will not trigger
   the rare behavior.
 * The single-bit optimization will only be used if there are no other "rare"
   decorations applied already.

Other changes:

 * Computing the color for the decoration has been moved to RenderStyle
   (visitedDependentDecoration[Style]Color).
 * I attempted to beautify the painting code in InlineTextBox.cpp a little bit.
 * The isRubyText check in the former RenderObject::getTextDecorations has been
   moved to StyleAdjuster:doesNotInheritTextDecoration.
 * The 'firstlineStyle'-case of RenderObject::getTextDecorations is now an
   implementation detail of RenderObject::resolvedDecorations, rather than
   something the caller must remember to do.

Problems:

 * -webkit-text-decorations-in-effect (in its current form) is not compatible
    with CSS3 text decorations (in its current form). Behavior after this patch:
    if you set this property in a stylesheet, it will clear all decorations,
    and apply up to three solid-currentColor decorations as specified by the
    property. One solution to this would be to change the property to a comma-
    separated list of decorations (not recommended), or if at all possible
    *remove* the whole property (recommended). I attempted to remove the
    property, but it's used by core/editing in horrible ways I have yet to fully
    understand.

    Of course there are no issues if experimental features are not enabled. In
    this case the behavior should be indistinguishable from the current
    behavior.

 * text-decoration-line seems to accept multiple values (up to three), even
   though the spec does not allow it. (CSS2.1 does describe this for
   text-decoration, though, so it's possibly a problem with the spec rather
   than our implementation).

BUG=350840, 315271
TEST=text-decoration-color-recalc.html
     text-decoration-style-recalc.html
     text-decoration-line-recalc.html
     text-decoration-style-multiple-overlap.html

[andersr, 2014-03-31 17:05:17]

Note: this is a new attempt at fixing the style recalc for
text-decoration-[style,color]. Previous attempt:
https://codereview.chromium.org/189603010/

[esprehn, 2014-03-31 17:41:24]

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-color-recalc.html
(right):

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-color-recalc.html:9:
window.addEventListener("load", function() {
onload = function() {

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-line-recalc.html
(right):

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-line-recalc.html:9:
window.addEventListener("load", function() {
ditto.

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-line-recalc.html:10:
target.style["textDecorationLine"] = "overline";
.textDecorationLine, you don't need the quotes or brackets.

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style-multiple-overlap-expected.html
(right):

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style-multiple-overlap-expected.html:26:

Same. use ids and classes.

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style-multiple-overlap.html
(right):

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style-multiple-overlap.html:25:
#t3 > span:last-child > span  > span > span { text-decoration: underline dotted
green; color: #aaa; }
Lets use ids instead of these selectors, it's too hard to tell what you're
styling here.

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
File
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style-recalc.html
(right):

https://codereview.chromium.org/219633002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style-recalc.html:10:
target.style["textDecorationStyle"] = "dashed";
ditto

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Inline...
File Source/core/rendering/InlineTextBox.cpp (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Inline...
Source/core/rendering/InlineTextBox.cpp:1072: static void
getDecorationMetaData(const AppliedTextDecorationList* decorations, bool&
linesAreOpaque, bool& setDecorationAntialias)
I might use a struct instead of all these out params.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Inline...
Source/core/rendering/InlineTextBox.cpp:1074: for (size_t i = 0; i <
decorations->size(); ++i) {
Pass decorations by reference.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Inline...
Source/core/rendering/InlineTextBox.cpp:1089: int baseline;
Do we need a constructor that zero inits this thing?

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Inline...
Source/core/rendering/InlineTextBox.cpp:1092: static void
paintAppliedDecorationStyle(GraphicsContext* context, const
AppliedTextDecoration& decoration, const PaintAppliedDecorationData& d, int
wavyOffsetFactor)
Don't use single letter variable names.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Inline...
Source/core/rendering/InlineTextBox.cpp:1158: Color color =
renderer().getUnderlineColor(true, isFirstLineStyle());
this boolean argument is confusing, can we use an enum or something?

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:2983: && style->textDecorationStyle() ==
TextDecorationStyleSolid;
This should be a method on RenderStyle.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.h:792: Color getUnderlineColor(bool
quirksMode, bool firstlineStyle);
We should use enums, this is confusing. Also always passing true is weird for
quirksMode.

This should also not be prefixed with "get". Methods prefixed with get use out
params like getChildNodes(Vector&).

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/AppliedTextDecoration.cpp (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/AppliedTextDecoration.cpp:20: , m_color(o.m_color)
Does the default copy constructor not work?

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/AppliedTextDecoration.h (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/AppliedTextDecoration.h:16: AppliedTextDecoration()
{ }
This leaves values uninitialized. I don't think you ever want this.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/AppliedTextDecorationList.h (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/AppliedTextDecorationList.h:14: class
AppliedTextDecorationList : public RefCounted<AppliedTextDecorationList> {
This is so silly, we should add a template for the RefPtr'ed Vectors in
RenderStyle.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1202: decorations |=
applied->at(i).line();
This needs braces, we also prefer early return.

if (!applied)
  return 0;

for (...)
  ...

return...

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1341: const bool isSimple = style ==
TextDecorationStyleSolid && styleColor.isCurrentColor();
remove const

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1357: const int decorations =
textDecoration();
no const

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1454: const bool visitedLink =
insideLink() == InsideVisitedLink;
ditto

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1461: if (textStrokeWidth() > 0) {
Remove > 0, can this be negative?

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:585: TextDecoration
appliedTextUnderline() const { return
static_cast<TextDecoration>(inherited_flags.m_textUnderline); }
Instead of this you can encapsulate the optimization by returning a static const
Vector<AppliedTextDecoration> that just contains an underline. You can share
that Vector between all RenderStyles.


const AppliedTextDecorationList* appliedTextDecorations()
{
  DECLARE_STATIC_LOCAL(Vector<AppliedTextDecoration>, empty, ());
  DECLARE_STATIC_LOCAL(Vector<AppliedTextDecoration>, underline, (...));
  if (!inherited_flags.m_textUnderline &&
!rareInheritedData->appliedTextDecorations.size())
      return empty;
  if (inherited_flags.m_textUnderline &&
!rareInheritedData->appliedTextDecorations.size())
      return underline;
  return *rareInheritedData->appliedTextDecorations;
}

I would materialize the single underline inside the Vector too as the first
element.

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:586: const AppliedTextDecorationList*
appliedTextDecorations() const { return
rareInheritedData->appliedTextDecorations.get(); }
const Vector<AppliedTextDecoration>&, reference not pointer.

[andersr, 2014-04-01 17:07:54]

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/219633002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:585: TextDecoration
appliedTextUnderline() const { return
static_cast<TextDecoration>(inherited_flags.m_textUnderline); }
On 2014/03/31 17:41:24, esprehn wrote:
> Instead of this you can encapsulate the optimization by returning a static
const
> Vector<AppliedTextDecoration> that just contains an underline. You can share
> that Vector between all RenderStyles.

Note that I have not fully addressed this issue in Patch Set 3. (I've addressed
all other issues, though).

AppliedTextDecoration objects contain the resolved Color for the decoration,
whereas the color for the simple underline is resolved during painting. If we
are to "migrate" the simple underline into the rare list once rare decorations
appear, then we need to resolve the Color during style adjusting instead. In
other words, we need to implement RenderObject::underlineColor during style
adjustment. Is that possible?

[esprehn, 2014-04-12 06:14:57]

Any luck based on our discussion?

[andersr, 2014-04-14 09:24:09]

On 2014/04/12 06:14:57, esprehn wrote:
> Any luck based on our discussion?

I've let this lie for a while, to be honest.

The least inconsistent solution I have is to store a StyleColor in
AppliedTextDecoration rather than a Color. Then, if a
StyleColor[m_currentColor==true] is encountered during painting, it must be
resolved dynamically.

However, this is slightly misleading, because StyleColor[m_currentColor==true]
normally means "resolve with the current color of the current RenderStyle", but
that's not the case here. It's "resolve with the current color of some parent
matching some criteria". So maybe we want to typedef StyleColor into
DynamicColor or something, to indicate what might need to take place.

[andersr, 2014-04-30 10:55:20]

Patch Set 5:

The colors for *all* decorations are now resolved dynamically, rather than just
for the simple case. However the StyleColor of the decoration is also privately
stored in the AppliedTextDecoration object to ensure that RenderStyle always can
compute style diffs correctly.

[esprehn, 2014-05-01 17:17:54]

lgtm, this seems reasonable. fix nits before landing

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/AppliedTextDecoration.h (right):

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/AppliedTextDecoration.h:17:
AppliedTextDecoration(TextDecoration);
missing explicit

[Peter Beverloo, 2014-05-01 17:24:51]

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/AppliedTextDecoration.cpp (right):

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/AppliedTextDecoration.cpp:33: return m_color ==
o.m_color && m_line == o.m_line && m_style && o.m_style;
"m_style == o.m_style"?

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RefVector.h (right):

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RefVector.h:15: class RefVector : public
RefCounted<RefVector<T> > {
should this be in wtf/?

[andersr, 2014-05-02 11:10:34]

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/AppliedTextDecoration.cpp (right):

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/AppliedTextDecoration.cpp:33: return m_color ==
o.m_color && m_line == o.m_line && m_style && o.m_style;
On 2014/05/01 17:24:51, Peter Beverloo wrote:
> "m_style == o.m_style"?

Wow.

Thanks. Fixed.

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RefVector.h (right):

https://codereview.chromium.org/219633002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RefVector.h:15: class RefVector : public
RefCounted<RefVector<T> > {
On 2014/05/01 17:24:51, Peter Beverloo wrote:
> should this be in wtf/?

Probably. Moved.

I guess I need another LGTM, in that case. (From who? jchaffraix, maybe?)

[Julien - ping for review, 2014-05-05 17:45:07]

I don't think I have a good understanding of this change as it is changing
*everything* at once. You should really split this change into smaller more
focused patches. In the meantime, I don't think we should land this so not lgtm.

https://codereview.chromium.org/219633002/diff/120001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/219633002/diff/120001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:248:
style->setTextDecorationsInEffect(style->textDecoration());
AFAICT this was the only caller of setTextDecorationsInEffect and you don't add
any so the function is now dead code :(

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1224:
DEFINE_STATIC_LOCAL(Vector<AppliedTextDecoration>, underline, (1,
AppliedTextDecoration(TextDecorationUnderline)));
I would prefer the vectors to be defined inside their own branches. This matches
our coding style to define local variable the closest / deepest to where they
are needed.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1226: if
(!inherited_flags.m_textUnderline && !rareInheritedData->appliedTextDecorations)
This line makes me think that when inherited_flags.m_textUnderline is false,
rareInheritedData->appliedTextDecorations shouldn't be NULL.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1228: if
(inherited_flags.m_textUnderline && !rareInheritedData->appliedTextDecorations)
Wouldn't rareInheritedData->appliedTextDecorations be NULL if
inherited_flags.m_textUnderline is true based on the addAppliedTextDecoration
logic below?

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1343: bool isSimpleUnderline =
decoration.isSimple() && decoration.line() == TextDecorationUnderline;
It's weird that we check for this simple case repeatedly here when the caller
knows all the text-decorations that applies to an element so could easily set
the flag and insert the extra AppliedTextDecoration as needed.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1359: list = list->copy();
It's probably me but I have a hard time understanding:
A) Why we need this copy?
B) Why we need such a complex sharing code for a rare property and a Vector that
holds at most 4 elements?

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/StyleRareInheritedData.cpp (right):

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/StyleRareInheritedData.cpp:225: &&
appliedTextDecorationsEquivalent(appliedTextDecorations.get(),
o.appliedTextDecorations.get());
I think dataEquivalent(appliedTextDecorations, o.appliedTextDecorations) would
work here. If not, it would be better to implement a template specialization of
dataEquivalent for this case.

https://codereview.chromium.org/219633002/diff/120001/Source/wtf/RefVector.h
File Source/wtf/RefVector.h (right):

https://codereview.chromium.org/219633002/diff/120001/Source/wtf/RefVector.h#...
Source/wtf/RefVector.h:23: T& at(size_t i) { return m_vector.at(i); }
The at() functions are not used in the patch, couldn't they be removed?

[andersr, 2014-05-06 13:15:12]

On 2014/05/05 17:45:07, Julien Chaffraix - PST wrote:
> I don't think I have a good understanding of this change as it is changing
> *everything* at once. You should really split this change into smaller more
> focused patches. In the meantime, I don't think we should land this so not
lgtm.

Hardly "everything", but it *is* possible to split this into two patches (at
least), I agree.

Patch 1: Fix just the style recalculation issue. This patch would contain the
AppliedTextDecoration objects + the m_textUnderline optimization, but the
painting code would not be changed.

Patch 2: Fix multiple decoration issue. The painting code would be changed to
paint based on the AppliedTextDecoration vector rather than
textDecorationsInEffect.

[andersr, 2014-05-06 13:15:28]

https://codereview.chromium.org/219633002/diff/120001/Source/core/css/resolve...
File Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/219633002/diff/120001/Source/core/css/resolve...
Source/core/css/resolver/StyleAdjuster.cpp:248:
style->setTextDecorationsInEffect(style->textDecoration());
On 2014/05/05 17:45:08, Julien Chaffraix - PST wrote:
> AFAICT this was the only caller of setTextDecorationsInEffect and you don't
add
> any so the function is now dead code :(

Ah, yes. That is no reason to be sad. ;)

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1224:
DEFINE_STATIC_LOCAL(Vector<AppliedTextDecoration>, underline, (1,
AppliedTextDecoration(TextDecorationUnderline)));
Done.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1226: if
(!inherited_flags.m_textUnderline && !rareInheritedData->appliedTextDecorations)
Do you mean that it does not read well, or that it's logically wrong? (It looks
correct to me). What would you prefer?

!(m_textUnderline || appliedTextDecoration) // This?

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1228: if
(inherited_flags.m_textUnderline && !rareInheritedData->appliedTextDecorations)
That's true. Fixed.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1343: bool isSimpleUnderline =
decoration.isSimple() && decoration.line() == TextDecorationUnderline;
I don't know ... is that really weird?

Currently, addAppliedTextDecoration "packs" the simple decoration into a single
bit, and the appliedTextDecorations "unpacks" the simple decoration, as
appropriate. It seems natural to me that this optimization should be an
implementation detail of the setter/getter, and not something utility functions
(applyTextDecorations) and/or callers need to even know about.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.cpp:1359: list = list->copy();
A) The same reason we copy DataRef objects? When the applied decorations of a
child changes, it should not apply to the parent. I realize this behavior is
almost identical to DataRef (the difference is that DataRefs must always be
initialized, which we don't want), however, there is already a precedent in
RenderStyle to do it this way in such situations. (CursorList).

I could submit a follow up patch with optional lazy initialization for DataRef.
(Or something like that).

B) How did you arrive at the number '4'? I believe it can hold up to [number of
lines] * [number of styles] effective elements, which currently computes to
'15'.

Example: For each [underline, line-through, overline], you can draw (in order):
[wavy, double, solid, dashed, dotted], and portions of all those decorations
will be visible. Beyond that, I believe some decorations *could* be dropped,
yes, but it makes no sense to optimize such cases.

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/StyleRareInheritedData.cpp (right):

https://codereview.chromium.org/219633002/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/StyleRareInheritedData.cpp:225: &&
appliedTextDecorationsEquivalent(appliedTextDecorations.get(),
o.appliedTextDecorations.get());
Done.

https://codereview.chromium.org/219633002/diff/120001/Source/wtf/RefVector.h
File Source/wtf/RefVector.h (right):

https://codereview.chromium.org/219633002/diff/120001/Source/wtf/RefVector.h#...
Source/wtf/RefVector.h:23: T& at(size_t i) { return m_vector.at(i); }
Done.

[andersr, 2014-05-06 16:40:26]

I've split out the RenderStyle changes in a separate patch here:
https://codereview.chromium.org/272443002/

[Julien - ping for review, 2014-05-06 17:26:52]

On 2014/05/06 13:15:12, andersr wrote:
> On 2014/05/05 17:45:07, Julien Chaffraix - PST wrote:
> > I don't think I have a good understanding of this change as it is changing
> > *everything* at once. You should really split this change into smaller more
> > focused patches. In the meantime, I don't think we should land this so not
> lgtm.
> 
> Hardly "everything", but it *is* possible to split this into two patches (at
> least), I agree.
> 
> Patch 1: Fix just the style recalculation issue. This patch would contain the
> AppliedTextDecoration objects + the m_textUnderline optimization, but the
> painting code would not be changed.
> 
> Patch 2: Fix multiple decoration issue. The painting code would be changed to
> paint based on the AppliedTextDecoration vector rather than
> textDecorationsInEffect.

That's a first step to splitting the change. I usually advise people to always
split CSS and rendering into different patches as it's not the same subset of
people that can review them.

Now, Patch 1 is actually 2 changes:
- One refactoring: changing the internal representation of decoration to use
AppliedTextDecoration objects (depending on the granularity you want, this could
be several patches).
- Fixing the style recalculation issue.

I will continue my review on the new change you put up for review.