Store and propagate a list of applied text decorations.

Store and propagate a list of applied text decorations.

This is preparatory work to fix an issue with style recalculation of text
decorations (350840), and to implement multiple text decorations per line.

In a future patch, the color and style of each decoration will be added to
the AppliedTextDecoration objects in order to fix 350840.

BUG=350840, 315271
TEST=Covered by existing tests

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173761

[andersr, 2014-05-06 16:29:43]

Note: This patch is an alternative to https://codereview.chromium.org/219633002/
, where we split the patch in two smaller patches.

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

https://codereview.chromium.org/272443002/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/272443002/diff/1/LayoutTests/fast/css3-text/c...
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-color-recalc.html:13:
<div id="target">
I like tests to be state:
- What they test (aka description of the test).
- What are the conditions for passing / failing.

This comment applies to all tests.

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:140: || style->isFloating() ||
style->hasOutOfFlowPosition() || isHTMLRTElement(e);
Is this tested anywhere? Where does the specification says that we can't allow
this to ruby?

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1381: // Text decoration color
fallback is handled in RenderObject::decorationColor.
Based on this comment, I think this part of the change should go in a follow-up
change.

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1226: }
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.

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1341: }
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.

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1348: list = list->copy();
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.

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1463: bool visitedLink =
insideLink() == InsideVisitedLink;
Coding style: Name-5. Precede boolean values with words like "is" and "did".

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:202: // 40 bits
I think we should just remove this one as it doesn't represent any meaningful
boundary.

[andersr, 2014-05-07 11:17:52]

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:140: || style->isFloating() ||
style->hasOutOfFlowPosition() || isHTMLRTElement(e);
This was done to replace the behavior in the rendering code, which also avoids
decoration if it encounters a ruby element. I should probably move this change
to a later patch.

We *do* have a test for it, and it *is* in the spec.

"When specified on or propagated to a ruby box, the decorations are propagated
only to the ruby base."

http://www.w3.org/TR/css-text-decor-3/#line-decoration

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1381: // Text decoration color
fallback is handled in RenderObject::decorationColor.
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.

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1226: }
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.

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1341: }
OK. In that case RenderStyle::addAppliedTextDecoration must be made private.

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1348: list = list->copy();
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/

> defeats the purpose of this optimization

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).

> go with a more simple solution

I'm all for that, but what do you suggest? I can't think of any other solutions
that are simpler.

> data backing your claim for memory saving

Memory saved compared to which alternative approach?

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1463: bool visitedLink =
insideLink() == InsideVisitedLink;
OK.

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.h:202: // 40 bits
That's true.

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

lgtm

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:140: || style->isFloating() ||
style->hasOutOfFlowPosition() || isHTMLRTElement(e);
On 2014/05/07 11:17:53, andersr wrote:
> This was done to replace the behavior in the rendering code, which also avoids
> decoration if it encounters a ruby element. I should probably move this change
> to a later patch.

Maybe it's related to this patch but the fact that you are mentioning rendering
on a css patch makes me somehow think this belongs to a follow-up change.

> We *do* have a test for it, and it *is* in the spec.

Sir, I wish I could read minds but I am limited to the code you write and the
words you put in the description. None of which leads me to believe there is a
test for this part of the change.

> "When specified on or propagated to a ruby box, the decorations are propagated
> only to the ruby base."
> 
> http://www.w3.org/TR/css-text-decor-3/#line-decoration

Fair enough, I missed that on quick read of the specification.

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

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1226: }
On 2014/05/07 11:17:53, andersr wrote:
> 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.

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.

https://codereview.chromium.org/272443002/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/RenderStyle.cpp:1348: list = list->copy();
On 2014/05/07 11:17:53, andersr wrote:
> 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/

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.

> > defeats the purpose of this optimization
> 
> 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).
> 
> > go with a more simple solution
> 
> I'm all for that, but what do you suggest? I can't think of any other
solutions
> that are simpler.
> 
> > data backing your claim for memory saving
> 
> Memory saved compared to which alternative approach?

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.

[commit-bot: I haz the power, 2014-05-09 15:46:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/andersr@opera.com/272443002/40001

[commit-bot: I haz the power, 2014-05-09 17:08:13]

Change committed as 173761

[andersr, 2014-05-12 11:12:48]

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/272443002/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:140: || style->isFloating() ||
style->hasOutOfFlowPosition() || isHTMLRTElement(e);
> Maybe it's related to this patch but the fact that you are mentioning
rendering
> on a css patch makes me somehow think this belongs to a follow-up change.

Yes, I agree. Note that I did move this change to the next patch, if that was
unclear.