[webkit-reviews] review denied: [Bug 98597] Text overflow ellipsis is affected by vertical-align : [Attachment 175212] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Rob Buis <rwlbuis at gmail.com>'s request
for review:
Bug 98597: Text overflow ellipsis is affected by vertical-align
https://bugs.webkit.org/show_bug.cgi?id=98597

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=175212&action=review


r-

Get me a vertical-rl test also please.

> Source/WebCore/rendering/InlineBox.h:273
> +    virtual float placeEllipsisBox(bool ltr, float visibleLeftEdge, float
visibleRightEdge, float ellipsisWidth, float &truncatedWidth, bool&, float&);

You should include a parameter name here, since it's not clear what the extra
float is otherwise.

> Source/WebCore/rendering/InlineFlowBox.h:197
> +    virtual float placeEllipsisBox(bool ltr, float blockLeftEdge, float
blockRightEdge, float ellipsisWidth, float &truncatedWidth, bool&, float&)
OVERRIDE;

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:249
> +	   boxY = y();

Shouldn't this be logicalTop()?

> Source/WebCore/rendering/InlineTextBox.cpp:265
> +	   boxY = y();

Ditto.

> Source/WebCore/rendering/InlineTextBox.cpp:273
> +	   boxY = y();

Ditto.

> Source/WebCore/rendering/RootInlineBox.cpp:153
>      ellipsisBox->setX(position);
> +    if (foundBox)
> +	   ellipsisBox->setY(boxY);

Not accounting for vertical writing here.