[Webkit-unassigned] [Bug 50928] RTL: Select elements with a size attribute are always left aligned

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50928


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81972|review?                     |review-
               Flag|                            |




--- Comment #15 from Eric Seidel <eric at webkit.org>  2011-02-28 10:34:35 PST ---
(From update of attachment 81972)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.