Bug 132773 – Text decorations do not contribute to visual overflow

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Text decorations do not contribute to visual overflow

[Myles C. Maxfield]

<rdar://problem/16364632>

[Myles C. Maxfield]

Created attachment 231263 [details]
Patch

[Darin Adler]

Comment on attachment 231263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231263&action=review

I am saying review+ on this patch, but I think it‘s only partly right. I’ve mentioned many problems with it in the comments below.

> Source/WebCore/ChangeLog:8
> +        Tests: fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect.html

The test seems to cover only one case of many in the code. We really want tests that cover all those different code paths if we can.

> Source/WebCore/rendering/InlineTextBox.cpp:987
> +void InlineTextBox::extendVerticalVisualOverflowForDecorations(int& bottom, int& top) const

Is it really OK to do all this with integers?

> Source/WebCore/rendering/InlineTextBox.cpp:999
> +    float strokeThickness = textDecorationStrokeThickness(renderer().style().fontSize());
> +    float controlPointDistance;
> +    float step;
> +    getWavyStrokeParameters(strokeThickness, controlPointDistance, step);
> +
> +    float wavyOffset = wavyOffsetFromDecoration();
> +
> +    const RenderStyle& lineStyle = this->lineStyle();
> +    int baseline = lineStyle.fontMetrics().ascent();
> +    TextDecoration decoration = lineStyle.textDecorationsInEffect();
> +    TextDecorationStyle decorationStyle = lineStyle.textDecorationStyle();

I’m concerned that this function does too much extra work in cases that don’t need it. For example, computing the wavy stroke parameters even if the style is not wavy. And fetching the ascent even though it’s only used for line-through.

It would be better to structure it so that didn’t happen, unless it’s all efficient enough that it’s not a concern. Maybe it’s just too hard to reorganize to compute only what we need, and yet reuse things. Some items are recomputed below, such as logicalHeight(), which we can call up to three times.

> Source/WebCore/rendering/InlineTextBox.cpp:1003
> +        TextUnderlinePosition underlinePosition = lineStyle.textUnderlinePosition();

I don’t think it’s useful to put this into a local variable.

> Source/WebCore/rendering/InlineTextBox.cpp:1004
> +        const int underlineOffset = computeUnderlineOffset(underlinePosition, lineStyle.fontMetrics(), this, strokeThickness);

Seems strange to put const here when there are lots of other locals above, also constant, that don't say const. Better to be consistent I think.

> Source/WebCore/rendering/InlineTextBox.cpp:1007
> +            bottom = std::max(bottom, static_cast<int>(ceil(underlineOffset + wavyOffset + controlPointDistance + strokeThickness - logicalHeight())));

These functions are doing ceil on floats. That’s going to convert the float to a double just to do the ceil operation. Instead, please use ceilf, which takes a float, or std::ceil, which is overloaded for float.

> Source/WebCore/rendering/InlineTextBox.cpp:1008
> +            top = std::max(top, static_cast<int>(ceil(-(underlineOffset + wavyOffset - controlPointDistance - strokeThickness))));

Is max and ceil really right for top? I would have though that we’d want min and floor for top. Not enough test cases to see if it’s right or wrong unfortunately.

> Source/WebCore/rendering/InlineTextBox.h:162
> +    void extendVerticalVisualOverflowForDecorations(int& bottom, int& top) const;

Seems unusual to pass bottom first and then top. It’s usually the other way around.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:527
> +    // Treat text decoration visual overflow as glyph overflow

We put periods on our sentences in sentence style comments like this one.

I also think this sentence is a bit too enigmatic. I would write this instead:

    // Include text decoration visual overflow as part of the glyph overflow.

I also think the comment would be easier to read if it was a separate paragraph, so a blank line before the comment and another blank line after the call to extendVerticalVisualOverflowForDecorations.

> Source/WebCore/rendering/style/RenderStyle.cpp:385
> +bool RenderStyle::visualOverflowChanged(const RenderStyle* other, unsigned&) const

New functions should take references rather than pointers to things that can never be null, thus it should be a const RenderStyle&, not a const RenderStyle*.

What’s the point in passing in the never-used reference to changedContextSensitiveProperties. We can always add it later if we need it. I suggest omitting it.

Should this be marked inline?

> Source/WebCore/rendering/style/RenderStyle.cpp:387
> +    if (!rareNonInheritedData->shadowDataEquivalent(*other->rareNonInheritedData.get()))

Do we really need the call to get here? I guess we do because it was like that before, but I’m surprised.

> Source/WebCore/rendering/style/RenderStyle.cpp:424
> +    // FIXME: We should add an optimized form of layout that just recomputes visual overflow.
> +    if (visualOverflowChanged(other, changedContextSensitiveProperties))
> +        return true;

Does doing this check before the pointer equality check on rareNonInheritedData cost us some performance? It seems it probably does.

> Source/WebCore/rendering/style/RenderStyle.h:1939
> +    bool visualOverflowChanged(const RenderStyle*, unsigned& changedContextSensitiveProperties) const;

A function named "visual overflow changed" could be something that notifies code that “visual overflow has changed”; might want to find a better name for the predicate. Maybe changeAffectsVisualOverflow?

[Myles C. Maxfield]

Created attachment 231363 [details]
Patch

[Darin Adler]

Comment on attachment 231363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231363&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:1009
> +            top = std::max(top, -(underlineOffset + wavyOffset - controlPointDistance - strokeThickness));

I’m still a little puzzled by the use of std::max for top. I would have expected std::min.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:533
> +        glyphOverflow.top = static_cast<int>(ceilf(top));

I’m still a little puzzled by the use of ceilf for top. I would have expected floorf.

[Simon Fraser (smfr)]

Comment on attachment 231363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231363&action=review

>> Source/WebCore/rendering/InlineTextBox.cpp:1009
>> +        if (decorationStyle == TextDecorationStyleWavy) {
>> +            getWavyStrokeParameters(strokeThickness, controlPointDistance, step);
>> +            wavyOffset = wavyOffsetFromDecoration();
>> +            gotWavyStrokeParameters = true;
>> +            bottom = std::max(bottom, underlineOffset + wavyOffset + controlPointDistance + strokeThickness - height);
>> +            top = std::max(top, -(underlineOffset + wavyOffset - controlPointDistance - strokeThickness));
> 
> I’m still a little puzzled by the use of std::max for top. I would have expected std::min.

I think it would be cleaner to hoist the if (decorationStyle == TextDecorationStyleWavy) block out and call it once, if any of the TextDecorationUnderline, TextDecorationOverline or TextDecorationLineThrough bits are set. This would be nicer if you added:

static inline bool decorationDrawsLine()... which does the bit testing for the 3 "line drawing" decorations.

[Myles C. Maxfield]

Comment on attachment 231363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231363&action=review

>>> Source/WebCore/rendering/InlineTextBox.cpp:1009
>>> +            top = std::max(top, -(underlineOffset + wavyOffset - controlPointDistance - strokeThickness));
>> 
>> I’m still a little puzzled by the use of std::max for top. I would have expected std::min.
> 
> I think it would be cleaner to hoist the if (decorationStyle == TextDecorationStyleWavy) block out and call it once, if any of the TextDecorationUnderline, TextDecorationOverline or TextDecorationLineThrough bits are set. This would be nicer if you added:
> 
> static inline bool decorationDrawsLine()... which does the bit testing for the 3 "line drawing" decorations.

Darin: The consumer of GlyphOverflow is InlineFlowBox::addTextBoxVisualOverflow(), and it treats each of the 4 values as an amount to extend that edge of the layout box by. (As a corollary: each of the values should be positive)

[Myles C. Maxfield]

http://trac.webkit.org/changeset/168750

[Darin Adler]

(In reply to comment #7)
> Darin: The consumer of GlyphOverflow is InlineFlowBox::addTextBoxVisualOverflow(), and it treats each of the 4 values as an amount to extend that edge of the layout box by. (As a corollary: each of the values should be positive)

I think then that the names “top” and “bottom” are not good names for these. Maybe topOverflow and bottomOverflow? Or maybe there are even better names. Typically a name like “top” implies the value is a coordinate, not a delta.

[Myles C. Maxfield]

(In reply to comment #9)
> (In reply to comment #7)
> > Darin: The consumer of GlyphOverflow is InlineFlowBox::addTextBoxVisualOverflow(), and it treats each of the 4 values as an amount to extend that edge of the layout box by. (As a corollary: each of the values should be positive)
> 
> I think then that the names “top” and “bottom” are not good names for these. Maybe topOverflow and bottomOverflow? Or maybe there are even better names. Typically a name like “top” implies the value is a coordinate, not a delta.

I filed <rdar://problem/16915242> for this.

[Sam Weinig]

This has caused repaint issues on on the bugzilla request queue page (and probably elsewhere as well) - https://bugs.webkit.org/request.cgi?action=queue&requester=&product=&type=review&requestee=&component=&group=requestee when you are using the Dusk theme.

[Sam Weinig]

I rolled out the 2nd patch in http://trac.webkit.org/changeset/168883 due to the repaint issue.

[Sam Weinig]

Created attachment 231482 [details]
Test Case showing repaint issue

[Myles C. Maxfield]

Thanks for the testcase, Sam.

I did some testing. If I only apply the changes to RenderStyle.cpp (marking TextDecoration changes as needing layout) the problem you brought up occurs. I'll be investigating why.

[Myles C. Maxfield]

Created attachment 231551 [details]
Patch

[Build Bot]

Comment on attachment 231551 [details]
Patch

Attachment 231551 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6656776204189696

New failing tests:
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-select.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-select-with-handler.html
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-mainframe.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-mainframe.html
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-div.html
fast/table/hittest-tablecell-with-borders-right-edge.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-mainframe-with-handler.html
fast/table/hittest-tablecell-right-edge.html
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-div-with-handler.html

[Build Bot]

Created attachment 231554 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231551 [details]
Patch

Attachment 231551 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5797463407460352

New failing tests:
fast/table/hittest-tablecell-right-edge.html
fast/table/hittest-tablecell-with-borders-right-edge.html

[Build Bot]

Created attachment 231555 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231551 [details]
Patch

Attachment 231551 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5722232760303616

New failing tests:
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-mainframe-with-handler.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-select.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-select-with-handler.html
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-mainframe.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-mainframe.html
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-div.html
fast/table/hittest-tablecell-with-borders-right-edge.html
platform/mac-wk2/tiled-drawing/fast-scroll-select-latched-mainframe-with-handler.html
fast/table/hittest-tablecell-right-edge.html
platform/mac-wk2/tiled-drawing/fast-scroll-div-latched-div-with-handler.html

[Build Bot]

Created attachment 231556 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

Created attachment 231559 [details]
Patch

[Myles C. Maxfield]

Regarding the platform/mac-wk2 test failures - these failures occur for me on a vanilla ToT checkout. They seem to be unrelated.

[Darin Adler]

Comment on attachment 231559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231559&action=review

review- because the logic in changeAffectsVisualOverflow seems obviously wrong, but I had a few other comments and concerns too

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:532
> +        float top = glyphOverflow.top;
> +        float bottom = glyphOverflow.bottom;

Should name these topOverflow and bottomOverflow.

> Source/WebCore/rendering/RenderTableSection.cpp:1054
> +    CellSpan coveredColumns = spannedColumns(damageRect, true);

In the WebKit project we try to avoid idioms like this. We use enums instead of booleans when the callers are going to pass constants, so the name is visible at the call site.

> Source/WebCore/rendering/RenderTableSection.h:307
> +    CellSpan spannedColumns(const LayoutRect& flippedRect, bool inclusive = false) const;

I have no idea what “inclusive” means in this context. If you used a longer variable name, I might understand. Or I guess you could write a comment.

> Source/WebCore/rendering/style/RenderStyle.cpp:398
> +        if (textUnderlinePosition() == TextUnderlinePositionUnder)
> +            return true;

This code is mysterious. Maybe a comment explaining why this rule is correct?

> Source/WebCore/rendering/style/RenderStyle.cpp:399
> +        float top = 0, bottom = 0;

Should name these topOverflow and bottomOverflow. Should define them on two different lines.

> Source/WebCore/rendering/style/RenderStyle.cpp:401
> +        extendVerticalVisualOverflowForDecorations(other, nullptr, top, bottom);
> +        return top || bottom;

Doesn’t seem right. This will return true if the old style extends visual overflow for decorations, but will return false if the old style does not and the new style does.

> Source/WebCore/style/InlineTextBoxStyle.cpp:38
> +    // Compute the gap between the font and the underline. Use at least one
> +    // pixel gap, if underline is thick then use a bigger gap.

Run-on sentence here, spliced with a comma. Please fix the grammar.

> Source/WebCore/style/InlineTextBoxStyle.cpp:39
> +    const int gap = std::max<int>(1, ceilf(textDecorationThickness / 2.0));

Not sure the const here is valuable.

> Source/WebCore/style/InlineTextBoxStyle.cpp:49
> +        // Position underline relative to the under edge of the lowest element's content box.

Is “under edge” a thing? I think “bottom edge” would be clearer.

> Source/WebCore/style/InlineTextBoxStyle.cpp:50
> +        const float offset = inlineTextBox->root().maxLogicalTop() - inlineTextBox->logicalTop();

Not sure the const here is valuable.

> Source/WebCore/style/InlineTextBoxStyle.cpp:65
> +    // as strockThickness increases to make the curve looks better.

Typo: strockThickness
Grammatical error: curve look better

> Source/WebCore/style/InlineTextBoxStyle.cpp:70
> +    // curve wider as strockThickness increases to make the curve looks better.

Same typo and grammatical error here.

> Source/WebCore/style/InlineTextBoxStyle.cpp:74
> +void extendVerticalVisualOverflowForDecorations(const RenderStyle& lineStyle, InlineTextBox* inlineTextBox, float& top, float& bottom)

Should name these arguments topOverflow and bottomOverflow.

> Source/WebCore/style/InlineTextBoxStyle.h:46
> +void extendVerticalVisualOverflowForDecorations(const RenderStyle& lineStyle, InlineTextBox*, float& top, float& bottom);

Arguments should be named topOverflow and bottomOverflow.

> Source/WebCore/style/InlineTextBoxStyle.h:48
> +int computeUnderlineOffset(const TextUnderlinePosition, const FontMetrics&, InlineTextBox*, const int textDecorationThickness);

Should omit the const in “const TextUnderlinePosition” and “const int”.

[Myles C. Maxfield]

Comment on attachment 231559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231559&action=review

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:532
>> +        float bottom = glyphOverflow.bottom;
> 
> Should name these topOverflow and bottomOverflow.

Done.

>> Source/WebCore/rendering/RenderTableSection.cpp:1054
>> +    CellSpan coveredColumns = spannedColumns(damageRect, true);
> 
> In the WebKit project we try to avoid idioms like this. We use enums instead of booleans when the callers are going to pass constants, so the name is visible at the call site.

Done.

>> Source/WebCore/rendering/RenderTableSection.h:307
>> +    CellSpan spannedColumns(const LayoutRect& flippedRect, bool inclusive = false) const;
> 
> I have no idea what “inclusive” means in this context. If you used a longer variable name, I might understand. Or I guess you could write a comment.

Done.

>> Source/WebCore/rendering/style/RenderStyle.cpp:398
>> +            return true;
> 
> This code is mysterious. Maybe a comment explaining why this rule is correct?

Done.

>> Source/WebCore/rendering/style/RenderStyle.cpp:399
>> +        float top = 0, bottom = 0;
> 
> Should name these topOverflow and bottomOverflow. Should define them on two different lines.

Done.

>> Source/WebCore/rendering/style/RenderStyle.cpp:401
>> +        return top || bottom;
> 
> Doesn’t seem right. This will return true if the old style extends visual overflow for decorations, but will return false if the old style does not and the new style does.

Yeah - I thought that too. I originally was testing on *this, but I was getting incorrect results. When I attached with a debugger, I found that the style changes I was looking for were occurring in other. I will verify this and do some digging to find out why.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:38
>> +    // pixel gap, if underline is thick then use a bigger gap.
> 
> Run-on sentence here, spliced with a comma. Please fix the grammar.

Done. These functions were copied and pasted, but this is a good opportunity to fix up these problems.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:39
>> +    const int gap = std::max<int>(1, ceilf(textDecorationThickness / 2.0));
> 
> Not sure the const here is valuable.

Done.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:49
>> +        // Position underline relative to the under edge of the lowest element's content box.
> 
> Is “under edge” a thing? I think “bottom edge” would be clearer.

Done.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:50
>> +        const float offset = inlineTextBox->root().maxLogicalTop() - inlineTextBox->logicalTop();
> 
> Not sure the const here is valuable.

Done.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:65
>> +    // as strockThickness increases to make the curve looks better.
> 
> Typo: strockThickness
> Grammatical error: curve look better

Done.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:70
>> +    // curve wider as strockThickness increases to make the curve looks better.
> 
> Same typo and grammatical error here.

Done.

>> Source/WebCore/style/InlineTextBoxStyle.cpp:74
>> +void extendVerticalVisualOverflowForDecorations(const RenderStyle& lineStyle, InlineTextBox* inlineTextBox, float& top, float& bottom)
> 
> Should name these arguments topOverflow and bottomOverflow.

Done.

>> Source/WebCore/style/InlineTextBoxStyle.h:46
>> +void extendVerticalVisualOverflowForDecorations(const RenderStyle& lineStyle, InlineTextBox*, float& top, float& bottom);
> 
> Arguments should be named topOverflow and bottomOverflow.

Done.

>> Source/WebCore/style/InlineTextBoxStyle.h:48
>> +int computeUnderlineOffset(const TextUnderlinePosition, const FontMetrics&, InlineTextBox*, const int textDecorationThickness);
> 
> Should omit the const in “const TextUnderlinePosition” and “const int”.

Done.

[Myles C. Maxfield]

Comment on attachment 231559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231559&action=review

>>> Source/WebCore/rendering/style/RenderStyle.cpp:401
>>> +        return top || bottom;
>> 
>> Doesn’t seem right. This will return true if the old style extends visual overflow for decorations, but will return false if the old style does not and the new style does.
> 
> Yeah - I thought that too. I originally was testing on *this, but I was getting incorrect results. When I attached with a debugger, I found that the style changes I was looking for were occurring in other. I will verify this and do some digging to find out why.

Verified. RenderElement::setStyle() calls m_style->diff(&style.get(), contextSensitiveProperties);

However, line 397 is wrong (I just fixed it)

[Myles C. Maxfield]

Created attachment 231606 [details]
Patch

[Darin Adler]

(In reply to comment #26)
> (From update of attachment 231559 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231559&action=review
> 
> >>> Source/WebCore/rendering/style/RenderStyle.cpp:401
> >>> +        return top || bottom;
> >> 
> >> Doesn’t seem right. This will return true if the old style extends visual overflow for decorations, but will return false if the old style does not and the new style does.
> > 
> > Yeah - I thought that too. I originally was testing on *this, but I was getting incorrect results. When I attached with a debugger, I found that the style changes I was looking for were occurring in other. I will verify this and do some digging to find out why.
> 
> Verified. RenderElement::setStyle() calls m_style->diff(&style.get(), contextSensitiveProperties);

I think you’re misunderstanding. Either one or the other could have overflow, so you need to check both. And also it’s not OK for this function to depend on the current state of the element in this way.

[zalan]

Myles, do you know why GlyphOverflow is all integer, while the rest of the related code is all float?

[Myles C. Maxfield]

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 231559 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=231559&action=review
> > 
> > >>> Source/WebCore/rendering/style/RenderStyle.cpp:401
> > >>> +        return top || bottom;
> > >> 
> > >> Doesn’t seem right. This will return true if the old style extends visual overflow for decorations, but will return false if the old style does not and the new style does.
> > > 
> > > Yeah - I thought that too. I originally was testing on *this, but I was getting incorrect results. When I attached with a debugger, I found that the style changes I was looking for were occurring in other. I will verify this and do some digging to find out why.
> > 
> > Verified. RenderElement::setStyle() calls m_style->diff(&style.get(), contextSensitiveProperties);
> 
> I think you’re misunderstanding. Either one or the other could have overflow, so you need to check both. And also it’s not OK for this function to depend on the current state of the element in this way.

I’m actually not sure that is the case. I was hoping to only have to call the function once (as an optimization) and I think we can get away with it here. There are three possible cases:

1. Decorations get added
2. All decorations get removed
3. Decorations get changed

In case 1, the visual overflow rect might grow and cause a layout. If the rect doesn't grow, the box simply gets redrawn.

In case 2, no layout will be triggered. Instead, RenderStyle::changeRequiresRepaintIfTextOrBorderOrOutline() will kick in, and the text box will be repainted with its old (larger) bounds. This is what happens without this patch.

In case 3, either a. the new style’s overflow is nonzero, or b. it isn’t. If the overflow is nonzero, a layout is triggered, and all will be drawn correctly. Otherwise, the same thing happens as in case 2.

However, I’m not quite sure what you mean when you say “it’s not OK for this function to depend on the current state of the element in this way.” If you mean that I shouldn’t distinguish between the old style and the new style, then, you’re right, I’ll have to call this function twice.

I also did some testing with each of the three cases, and I couldn’t trigger a case where the wrong output was produced. I’ll add these as new tests. (Though making these new tests uncovered a case where wavy decorations were contributing to horizontal overflow rather than vertical overflow. I did some refactoring to fix this).

Calling the function twice is not a big deal; I'm just not sure that it’s necessary.

@Zalan: No, I don’t know why GlyphOverflow uses integers. At some point I’d like to switch it over (probably to LayoutUnits). I started doing that for this patch but I think it’s out of scope.

[Myles C. Maxfield]

Created attachment 231640 [details]
Patch

[Antti Koivisto]

Comment on attachment 231640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231640&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:395
> +        || rareNonInheritedData->m_textDecorationColor != other.rareNonInheritedData->m_textDecorationColor

This is called from changeRequiresLayout(). Why test for color change? It can't affect layout.

> Source/WebCore/rendering/style/RenderStyle.cpp:400
> +        // is specified. We can take an early out here.
> +        if (other.textUnderlinePosition() == TextUnderlinePositionUnder)
> +            return true;

What if the current style has TextUnderlinePositionUnder and the new one doesn't? That may affect layout.

> Source/WebCore/rendering/style/RenderStyle.cpp:401
> +        return !visualOverflowForDecorations(other, nullptr).isEmpty();

Shouldn't this be testing if anything changed?

visualOverflowForDecorations(other, nullptr) != visualOverflowForDecorations(*this, nullptr) 

or something. You both invalidate unnecessarily if nothing changes and fail to invalidate if existing overflow goes away.

Passing nullptr here means that visualOverflowForDecorations is not taking the effect of TextUnderlinePositionUnder into account. You test it above but I think that subtlety deserves a comment at least.

[Darin Adler]

(In reply to comment #30)
> However, I’m not quite sure what you mean when you say “it’s not OK for this function to depend on the current state of the element in this way.”

I was confused. I thought this was a member of RenderElement or something rather than RenderStyle.

[Darin Adler]

I know I’ve been reviewing earlier versions of this patch, but I’d like someone else to review at this point; I’m perhaps a little out of my depth.

Antti made a few comments, maybe he can. Or some other expert on this code.

I hope my comments to this point were helpful.

[Myles C. Maxfield]

Created attachment 231679 [details]
Patch

[Antti Koivisto]

Comment on attachment 231679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231679&action=review

r=me

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:533
> +    if (glyphOverflow.top || glyphOverflow.bottom || glyphOverflow.left || glyphOverflow.right) {

!glyphOverflow.isEmpty() (since you added it)

[Myles C. Maxfield]

<rdar://problem/16967198>

[Myles C. Maxfield]

http://trac.webkit.org/changeset/169089