Making multiple select element RTL direction aware

Making multiple select element RTL direction aware

BUG=85833
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160202

[pals, 2013-10-21 11:52:26]

Please review.

[eseidel, 2013-10-21 18:51:20]

It appears we're repeating the same code 4 times?  Maybe a helper on
RenderBox::scrollbarLeft()?

[pals, 2013-10-22 05:05:42]

Fixed the review comment. PTAL. Thanks

[eseidel, 2013-10-22 05:59:21]

Looks pretty good!

https://codereview.chromium.org/31393004/diff/100001/Source/core/rendering/Re...
File Source/core/rendering/RenderListBox.cpp (right):

https://codereview.chromium.org/31393004/diff/100001/Source/core/rendering/Re...
Source/core/rendering/RenderListBox.cpp:277: return
LayoutRect(additionalOffset.x() + borderLeft() + paddingLeft() +
(style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ? m_vBar->width() :
0),
Same here.  Can we help explain to the reader what this is doing?

https://codereview.chromium.org/31393004/diff/100001/Source/core/rendering/Re...
Source/core/rendering/RenderListBox.cpp:495: if (offset.width() < borderLeft() +
paddingLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ?
scrollbarWidth : 0) || offset.width() > width() - borderRight() - paddingRight()
- (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ? 0 :
scrollbarWidth))
This is pretty ugly.  Can we use local variables to make this more
understandable?

[pals, 2013-10-22 08:45:18]

Fixed. Please review.

https://codereview.chromium.org/31393004/diff/100001/Source/core/rendering/Re...
File Source/core/rendering/RenderListBox.cpp (right):

https://codereview.chromium.org/31393004/diff/100001/Source/core/rendering/Re...
Source/core/rendering/RenderListBox.cpp:277: return
LayoutRect(additionalOffset.x() + borderLeft() + paddingLeft() +
(style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ? m_vBar->width() :
0),
On 2013/10/22 05:59:21, eseidel wrote:
> Same here.  Can we help explain to the reader what this is doing?

Done.

https://codereview.chromium.org/31393004/diff/100001/Source/core/rendering/Re...
Source/core/rendering/RenderListBox.cpp:495: if (offset.width() < borderLeft() +
paddingLeft() + (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ?
scrollbarWidth : 0) || offset.width() > width() - borderRight() - paddingRight()
- (style()->shouldPlaceBlockDirectionScrollbarOnLogicalLeft() ? 0 :
scrollbarWidth))
On 2013/10/22 05:59:21, eseidel wrote:
> This is pretty ugly.  Can we use local variables to make this more
> understandable?

Done.

[eseidel, 2013-10-22 08:47:43]

lgtm

Thanks!

[commit-bot: I haz the power, 2013-10-22 08:48:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/31393004/210001

[commit-bot: I haz the power, 2013-10-22 13:00:14]

Change committed as 160202