[webkit-reviews] review denied: [Bug 50928] RTL: Select elements with a size attribute are always left aligned : [Attachment 78801] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Ofri Wolfus
<ofri at google.com>'s request for review:
Bug 50928: RTL: Select elements with a size attribute are always left aligned
https://bugs.webkit.org/show_bug.cgi?id=50928

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78801&action=review

Thanks for the patch, Ofri! I took a first pass at the quality.
You need to reach out to mitz or hyatt to check the meat.

> Source/WebCore/ChangeLog:8
> +	   Added support for alignment in RenderListBox.

I am surprised there's no "Test: fast/forms/listbox-bidi-align.html" in here.
Did you prepare this ChangeLog before writing the test?

> Source/WebCore/ChangeLog:11
> +	   * rendering/RenderListBox.cpp:
> +	   (WebCore::RenderListBox::paintItemForeground):

Please use these to provide description of changes in each method and file. It
both helps reviewers and archeologists to understand mechanics of the change.

> Source/WebCore/rendering/RenderListBox.cpp:355
> +    ETextAlign actualAlignment = element->renderStyle() ?
element->renderStyle()->textAlign() : style()->textAlign();
> +    // FIXME: Firefox doesn't respect JUSTIFY. Should we?
> +    if (actualAlignment == TAAUTO || actualAlignment == JUSTIFY) {
> +	 if (isLTR)
> +	   actualAlignment = LEFT;
> +	 else
> +	   actualAlignment = RIGHT;
> +    }
> +
> +    // Determine where the item text should be placed
> +    IntRect r = itemBoundingBoxRect(tx, ty, listIndex);
> +    if (actualAlignment == RIGHT || actualAlignment == WEBKIT_RIGHT) {
> +	 float textWidth = itemFont.floatWidth(textRun);
> +	 r.move(r.width() - textWidth - optionsSpacingHorizontal,
itemFont.ascent());
> +	 r.setWidth(textWidth);
> +    } else if (actualAlignment == CENTER) {
> +	 float textWidth = itemFont.floatWidth(textRun);
> +	 r.move((r.width() - textWidth) / 2, itemFont.ascent());
> +	 r.setWidth(textWidth);
> +    } else {
> +	 r.move(optionsSpacingHorizontal, itemFont.ascent());
> +    }

This is something the RTL folks should check.