[webkit-reviews] review granted: [Bug 132773] Text decorations do not contribute to visual overflow : [Attachment 231263] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 132773: Text decorations do not contribute to visual overflow
https://bugs.webkit.org/show_bug.cgi?id=132773

Attachment 231263: Patch
https://bugs.webkit.org/attachment.cgi?id=231263&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.ht
ml

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?