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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.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 81972: Patch
https://bugs.webkit.org/attachment.cgi?id=81972&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81972&action=review

Honestly this all looks fine.  I'm sorry I didn't review it sooner.  Please try
pulling out some of your new code into a helper function and re-post.  Ping me
right away if you don't see an immediate review and I'll be happy to look
again.

> Source/WebCore/rendering/RenderListBox.cpp:337
> +    ETextAlign actualAlignment = itemStyle->textAlign();
> +    // FIXME: Firefox doesn't respect JUSTIFY. Should we?
> +    if (actualAlignment == TAAUTO || actualAlignment == JUSTIFY) {
> +	 if (isLTR)
> +	   actualAlignment = LEFT;
> +	 else
> +	   actualAlignment = RIGHT;
> +    }

Since this is part of the later if-cascade, I would just include it in the
helper function as well.

> Source/WebCore/rendering/RenderListBox.cpp:341
> +    // Determine where the item text should be placed
> +    IntRect r = itemBoundingBoxRect(tx, ty, listIndex);
> +    if (actualAlignment == RIGHT || actualAlignment == WEBKIT_RIGHT) {

I would have made this whole if-cascade a helper function with a nice name
which returned an IntSize (since move() takes an intsize).

Something like this:
r.move(itemOffsetForAlignment(itemStyle, itemFont, textRun))

Depending on how you wanted to break it up, you could even push teh whole
bounds collection into a helper function.  Since it doesn't look like it needs
any member variables, it could just be a static inline.

> Source/WebCore/rendering/RenderListBox.cpp:349
> +    } else {
> +	 r.move(optionsSpacingHorizontal, itemFont.fontMetrics().ascent());
> +    }

Style.