Fix app menu borders for RTL

Fix app menu borders for RTL

BUG=612485
TEST=Set to RTL language, open main browser (app) menu, observe that
button separators in Zoom and Edit rows are properly placed, e.g.

[ ]  |  +  100%  -  |   Zoom

Paste | Copy | Cut |   Edit

Committed: https://crrev.com/e7a1a61e269ba16a335eab35a6eab3e1ae1b30b6
Cr-Commit-Position: refs/heads/master@{#394850}

[Greg Levin, 2016-05-18 15:26:53]

glevin@chromium.org changed reviewers:
+ msw@chromium.org

[Greg Levin, 2016-05-18 15:26:54]

This CL is more code cleanup than bug fix, but the two are directly related, as
the uncleaned code was obfuscating what needed fixing.

Please have a look!

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (left):

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:151: SINGLE_BUTTON,
The LEFT_BUTTON, CENTER_BUTTON, and SINGLE_BUTTON were functionally identical,
and all drew a left-edge border.  The RIGHT_BUTTON was drawing no border.  I
consolidated and renamed these to clarify their functional difference.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:246: ButtonType TypeAdjustedForRTL()
const {
Function wasn't being called from anywhere.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:262: const CustomButton*
right_button_;
These were being written to, but not read, so I removed them and the functions
that set them.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:167: canvas->FillRect(rect,
BorderColor(view, views::Button::STATE_NORMAL));
These four lines are where the bug actually gets fixed.  The label buttons
weren't getting mirrored in RTL; only the ImageButton-derived FullscreenButton
was:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

[msw, 2016-05-18 19:10:03]

lgtm with minor nits. I wonder if using explicit separators would really be too
ugly, but this is definitely an improvement; thanks!

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (left):

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:151: SINGLE_BUTTON,
On 2016/05/18 15:26:54, Greg Levin wrote:
> The LEFT_BUTTON, CENTER_BUTTON, and SINGLE_BUTTON were functionally identical,
> and all drew a left-edge border.  The RIGHT_BUTTON was drawing no border.  I
> consolidated and renamed these to clarify their functional difference.

Acknowledged.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:246: ButtonType TypeAdjustedForRTL()
const {
On 2016/05/18 15:26:54, Greg Levin wrote:
> Function wasn't being called from anywhere.

Acknowledged.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:262: const CustomButton*
right_button_;
On 2016/05/18 15:26:54, Greg Levin wrote:
> These were being written to, but not read, so I removed them and the functions
> that set them.

Acknowledged.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:145: LEFT_BORDER,
optional nit: maybe change 'left' to 'leading' here and elsewhere?

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:165: ? gfx::Rect(0, 0, 1, h)
optional nit: leverage View::GetMirrored*

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:167: canvas->FillRect(rect,
BorderColor(view, views::Button::STATE_NORMAL));
On 2016/05/18 15:26:54, Greg Levin wrote:
> These four lines are where the bug actually gets fixed.  The label buttons
> weren't getting mirrored in RTL; only the ImageButton-derived FullscreenButton
> was:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...

Acknowledged.

[Greg Levin, 2016-05-19 17:15:39]

> lgtm with minor nits. I wonder if using explicit separators would really be
too
> ugly, but this is definitely an improvement; thanks!

I think I'll pass on trying out the separate separators.  I wanted to pick a
little low-hanging fruit while vacationing in the browser, but I need to get
back to work on Chrome OS :-)

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:145: LEFT_BORDER,
On 2016/05/18 19:10:02, msw wrote:
> optional nit: maybe change 'left' to 'leading' here and elsewhere?

Done.

https://codereview.chromium.org/1988983004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/app_menu.cc:165: ? gfx::Rect(0, 0, 1, h)
On 2016/05/18 19:10:02, msw wrote:
> optional nit: leverage View::GetMirrored*

Done.
(This was the best I could come up with, since we don't actually want the
mirrored x for the FullScreen image button.)

[msw, 2016-05-19 17:21:16]

still lgtm; thanks

[Greg Levin, 2016-05-19 18:16:42]

The CQ bit was checked by glevin@chromium.org

[commit-bot: I haz the power, 2016-05-19 18:17:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988983004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1988983004/20001

[commit-bot: I haz the power, 2016-05-19 20:44:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-19 20:46:31]

Description was changed from

==========
Fix app menu borders for RTL

BUG=612485
TEST=Set to RTL language, open main browser (app) menu, observe that
button separators in Zoom and Edit rows are properly placed, e.g.

[ ]  |  +  100%  -  |   Zoom

Paste | Copy | Cut |   Edit
==========

to

==========
Fix app menu borders for RTL

BUG=612485
TEST=Set to RTL language, open main browser (app) menu, observe that
button separators in Zoom and Edit rows are properly placed, e.g.

[ ]  |  +  100%  -  |   Zoom

Paste | Copy | Cut |   Edit

Committed: https://crrev.com/e7a1a61e269ba16a335eab35a6eab3e1ae1b30b6
Cr-Commit-Position: refs/heads/master@{#394850}
==========

[commit-bot: I haz the power, 2016-05-19 20:46:32]

Patchset 2 (id:??) landed as
https://crrev.com/e7a1a61e269ba16a335eab35a6eab3e1ae1b30b6
Cr-Commit-Position: refs/heads/master@{#394850}