Make select drop down popup RTL aware

Make select drop down popup RTL aware

This CL puts a vertical scrollbar on the left if dir="RTL"
for select drop-down list popups. Select menu list popups use
PopupListBox which is derived from ScrollView. ScrollView
places the vertical scrollbar always on right. Changes in
ScrollView.cpp places the vertical scrollbar on left if
shouldPlaceVerticalScrollbarOnLeft() is true.

Scrollbars on iframe (crbug.com/250514) and main frame
(viewport) outermost scrollbars (crbug.com/249860) are also
placed by ScrollView but shouldPlaceVerticalScrollbarOnLeft()
returns false so this CL does not alter the scrollbar
placement for iframe and main frame.

PopupListBox::shouldPlaceVerticalScrollbarOnLeft() returns
true if dir="RTL", so vertical scrollbar appears on left
for select menu list box. And the changes in
FramelessScrollView.cpp and PopupListBox.cpp is required
to move the windowClipRect and contents after the
scrollbar respectively.

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

[pals, 2013-11-05 12:42:37]

Please review. Could you please run trybot for this CL?

[ojan, 2013-11-05 18:39:00]

Is it possible to write a test for this?

[pals, 2013-11-08 11:26:41]

On 2013/11/05 18:39:00, ojan wrote:
> Is it possible to write a test for this?

Yes. Its possible on WIN with PopupMenuTest.cpp
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
Need to port PopupMenuTest.cpp for Linux and Mac.
But I work on Linux, I don't have WIN setup

[ojan, 2013-11-08 21:02:23]

Levi or Eric, mind doing this review? You know more about RTL than I do.

On 2013/11/08 11:26:41, sanjoyp wrote:
> On 2013/11/05 18:39:00, ojan wrote:
> > Is it possible to write a test for this?
> 
> Yes. Its possible on WIN with PopupMenuTest.cpp
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
> Need to port PopupMenuTest.cpp for Linux and Mac.
> But I work on Linux, I don't have WIN setup

Did you try just running it on Linux? I don't see anything in the test that
looks particularly platform-specific. It might be easy to port.

[pals, 2013-11-11 12:47:01]

I tried building this test in Linux. But its crashing with below strace. I am
not familiar with this part of code.
 [0x7fbbfd8749ee] base::debug::StackTrace::StackTrace()
 [0x7fbbfd874f18] base::debug::(anonymous namespace)::StackDumpSignalHandler()
 [0x7fbbf933dcb0] <unknown>
 [0x7fbbfda712f0] <unknown>
 [0x7fbbfc30c0d6] WebCore::SimpleFontData::platformInit()
 [0x7fbbfc2c14d4] WebCore::SimpleFontData::SimpleFontData()
 [0x7fbbfc292821] WebCore::FontCache::getFontResourceData()
 [0x7fbbfc29503d] WebCore::FontFallbackList::setPlatformFont()
 [0x7fbbfb88a367] (anonymous namespace)::TestPopupMenuClient::itemStyle()
 [0x7fbbfb50d1eb] WebCore::PopupListBox::updateFromElement()
 [0x7fbbfb50be3c] WebCore::PopupContainer::showInRect()
 [0x7fbbfb513d68] WebCore::PopupMenuChromium::show()
 [0x7fbbfb885884] (anonymous namespace)::SelectPopupMenuTest::showPopup()
 [0x7fbbfb8872f6] (anonymous
namespace)::SelectPopupMenuTest_SelectItemRemoveSelectOnClick_Test::TestBody()
 [0x7fbbfbe720ea] testing::Test::Run()
 [0x7fbbfbe721aa] testing::TestInfo::Run()
 [0x7fbbfbe722ef] testing::TestCase::Run()
 [0x7fbbfbe7257f] testing::internal::UnitTestImpl::RunAllTests()
 [0x7fbbfbe6d373] testing::UnitTest::Run()
 [0x7fbbfbe4f2be] base::TestSuite::Run()
 [0x00000040cd50] main
 [0x7fbbf8f8f76d] __libc_start_main
 [0x00000040cc79] <unknown>

[eseidel, 2013-11-11 18:53:36]

Could you attach the test for consideration?

Also, can you add more text to your description of this change?

[pals, 2013-11-13 12:09:19]

On 2013/11/11 18:53:36, eseidel wrote:
> Could you attach the test for consideration?
> 
> Also, can you add more text to your description of this change?

I have not written a test. To write a test in Linux, first we need to port
PopupMenuTest.cpp on Linux. So, I tried building PopupMenuTest.cpp on Linux and
running existing test cases which was crashing on Linux as few things are
missing at the bottom. A test can be written on windows for this CL though. But,
I don't have win setup. :) 

I have updated the patch and the commit message as well. Please review.

[eseidel, 2013-11-13 18:00:53]

https://codereview.chromium.org/59333002/diff/110001/Source/core/platform/Scr...
File Source/core/platform/ScrollView.cpp (right):

https://codereview.chromium.org/59333002/diff/110001/Source/core/platform/Scr...
Source/core/platform/ScrollView.cpp:454: IntRect
vBarRect(shouldPlaceVerticalScrollbarOnLeft() ? 0 : (width() -
m_verticalScrollbar->width()),
ScrollView is also a parent of FrameView.  I'm curious how this change affects
crbug.com/249860 or crbug.com/250514.

[eseidel, 2013-11-13 18:41:28]

lgtm

[commit-bot: I haz the power, 2013-11-13 18:42:26]

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

[commit-bot: I haz the power, 2013-11-13 20:37:17]

Change committed as 161944