Fix rtl text shifting in PopupListBox

Fix rtl text shifting in PopupListBox

What is happening is:
1. User opens popup
2. PopupListBox::layout measure's row width
  2-1. text width is calculated as ltr
  2-2. this width for ltr is registered to the WidthCache
3. PopupListBox::paintRow is called
  3-1. WidthCache has a interval mechanism so the cached ltr width is not used
  3-2. text width is calculated as rtl
  3-3. Text is  drawn as rtl
4. User hovers over items
5. PopupListBox::paintRow is called each time the selection changes
  5-1. WidthCache's cached ltr width is used
  5-2. Text is  drawn as rtl (rtl width is shorter than the rtl width so there is extra space on the right side)

There are two problems
- PopupListBox::layout is measuring row width using ltr always
- WidthCache is returning ltr width for rtl text

This just fixes PopupListBox::layout.
BUG=398929
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179906

[keishi, 2014-08-08 07:05:36]

What is happening is:
1. User opens popup
2. PopupListBox::layout measure's row width
  2-1. text width is calculated as ltr = 192px
  2-2. 192px is registered to WidthCache
3. PopupListBox::paintRow is called
  3-1. WidthCache has a interval mechanism so the cached 192px is not used
  3-2. text width is calculated as rtl = 173px (not cached)
  3-3. Text is  drawn as rtl
4. User hovers over items
5. PopupListBox::paintRow is called each time the selection changes
  5-1. WidthCache's cached value is used = 192px
  5-2. Text is  drawn as rtl (there is 192-173=19px space on the right side)

There are two problems
- PopupListBox::layout is measuring row width using ltr always
- WidthCache is returning ltr width for rtl text

This just fixes PopupListBox::layout.

[tkent, 2014-08-08 07:10:05]

On 2014/08/08 07:05:36, keishi wrote:
> What is happening is:
> 1. User opens popup
> 2. PopupListBox::layout measure's row width
>   2-1. text width is calculated as ltr = 192px
>   2-2. 192px is registered to WidthCache
> 3. PopupListBox::paintRow is called
>   3-1. WidthCache has a interval mechanism so the cached 192px is not used
>   3-2. text width is calculated as rtl = 173px (not cached)
>   3-3. Text is  drawn as rtl
> 4. User hovers over items
> 5. PopupListBox::paintRow is called each time the selection changes
>   5-1. WidthCache's cached value is used = 192px
>   5-2. Text is  drawn as rtl (there is 192-173=19px space on the right side)
> 
> There are two problems
> - PopupListBox::layout is measuring row width using ltr always
> - WidthCache is returning ltr width for rtl text
> 
> This just fixes PopupListBox::layout.

Please add this to the CL description.

Can you add C++ unit test?

[keishi, 2014-08-08 14:27:30]

On 2014/08/08 07:10:05, tkent wrote:
> On 2014/08/08 07:05:36, keishi wrote:
> > What is happening is:
> > 1. User opens popup
> > 2. PopupListBox::layout measure's row width
> >   2-1. text width is calculated as ltr = 192px
> >   2-2. 192px is registered to WidthCache
> > 3. PopupListBox::paintRow is called
> >   3-1. WidthCache has a interval mechanism so the cached 192px is not used
> >   3-2. text width is calculated as rtl = 173px (not cached)
> >   3-3. Text is  drawn as rtl
> > 4. User hovers over items
> > 5. PopupListBox::paintRow is called each time the selection changes
> >   5-1. WidthCache's cached value is used = 192px
> >   5-2. Text is  drawn as rtl (there is 192-173=19px space on the right side)
> > 
> > There are two problems
> > - PopupListBox::layout is measuring row width using ltr always
> > - WidthCache is returning ltr width for rtl text
> > 
> > This just fixes PopupListBox::layout.
> 
> Please add this to the CL description.
Done.
> 
> Can you add C++ unit test?
I added SelectPopupMenuStyleTest

[tkent, 2014-08-11 01:35:29]

https://codereview.chromium.org/451983004/diff/20001/Source/web/tests/SelectP...
File Source/web/tests/SelectPopupMenuStyleTest.cpp (right):

https://codereview.chromium.org/451983004/diff/20001/Source/web/tests/SelectP...
Source/web/tests/SelectPopupMenuStyleTest.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Please do not add new files to Source/web/tests/.
I think this should be Source/web/PopupListBoxTest.cpp.

https://codereview.chromium.org/451983004/diff/20001/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/451983004/diff/20001/Source/web/web.gypi#newc...
Source/web/web.gypi:292: 'tests/SelectPopupMenuStyleTest.cpp',
Should we exclude this on Mac?

[keishi, 2014-08-11 03:08:52]

https://codereview.chromium.org/451983004/diff/20001/Source/web/tests/SelectP...
File Source/web/tests/SelectPopupMenuStyleTest.cpp (right):

https://codereview.chromium.org/451983004/diff/20001/Source/web/tests/SelectP...
Source/web/tests/SelectPopupMenuStyleTest.cpp:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/08/11 01:35:29, tkent wrote:
> Please do not add new files to Source/web/tests/.
> I think this should be Source/web/PopupListBoxTest.cpp.

Done.

https://codereview.chromium.org/451983004/diff/20001/Source/web/web.gypi
File Source/web/web.gypi (right):

https://codereview.chromium.org/451983004/diff/20001/Source/web/web.gypi#newc...
Source/web/web.gypi:292: 'tests/SelectPopupMenuStyleTest.cpp',
On 2014/08/11 01:35:29, tkent wrote:
> Should we exclude this on Mac?

Done.

[tkent, 2014-08-11 03:16:08]

lgtm

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
File Source/web/tests/PopupMenuTest.cpp (right):

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
Source/web/tests/PopupMenuTest.cpp:560: virtual void SetUp()
OVERRIDE

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
Source/web/tests/PopupMenuTest.cpp:565: virtual void TearDown()
OVERRIDE

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
Source/web/tests/PopupMenuTest.cpp:595: #if OS(MACOSX) || OS(ANDROID)
We should exclude this file by a build rule.  However this is acceptable because
we already have such #if in this file.

[keishi, 2014-08-11 03:33:21]

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
File Source/web/tests/PopupMenuTest.cpp (right):

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
Source/web/tests/PopupMenuTest.cpp:560: virtual void SetUp()
On 2014/08/11 03:16:07, tkent wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/451983004/diff/60001/Source/web/tests/PopupMe...
Source/web/tests/PopupMenuTest.cpp:565: virtual void TearDown()
On 2014/08/11 03:16:07, tkent wrote:
> OVERRIDE

Done.

[keishi, 2014-08-11 03:33:25]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2014-08-11 03:33:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keishi@chromium.org/451983004/80001

[commit-bot: I haz the power, 2014-08-11 04:30:17]

Change committed as 179906