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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 231559: Patch
https://bugs.webkit.org/attachment.cgi?id=231559&action=review

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