Reland Linux Window Control Alignment

Reland Linux Window Control Alignment

Modifying CustomFrameView to layout window controls based on user configurations. For linux begin observing the configs and changes. For other systems fall back on a default ordering.

Update tests that were originally missed.

Original review: https://codereview.chromium.org/240163006/
Revert review: https://codereview.chromium.org/276173002/
Revert "Revert of Linux Aura Task Manager Frame Buttons Misaligned (https://codereview.chromium.org/240163006/)"

This reverts commit 9031ef57ba679037fc675bf2f081f3880380de3a.

TEST=CustomFrameViewTest
BUG=351917
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272352

[jonross, 2014-05-15 19:23:02]

The unittest that failed was using hardcoded points for input. With changes to
the buttons based on OS those points were not always valid.

Additionally the layout of the buttons used to take two passes to hit the
correct positions, they now do so in one. Due to this the window sizes used in
the test are now too small. They have been increased.

[flackr, 2014-05-15 21:33:16]

lgtm with nit.

https://codereview.chromium.org/281353009/diff/20001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/20001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter_unittest.cc:142:
std::vector<views::FrameButton> leading;
nit: Comment why you need to set the window button order, and probably a TODO to
not be sensitive to these changes since it should be possible to be order
agnostic.

[jonross, 2014-05-15 22:41:00]

oshima@chromium.org, sadrul@chromium.org: Please review a change to reland
269597.

Some ash tests had failed as previous hardcoded window size and touch points
were no longer valid. They did not match the visual updates.

[sadrul, 2014-05-15 22:47:34]

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
File ui/views/window/window_button_order_provider.h (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
ui/views/window/window_button_order_provider.h:18: }  // namespace ash
You shouldn't add ash:: things in views.

You can either make SetWindowButtonOrder public, or add a test-api to get access
to it (see, for example, menu_runner_test_api)

[oshima, 2014-05-15 22:51:02]

ash lgtm

https://codereview.chromium.org/281353009/diff/40001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/40001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter_unittest.cc:148:
SetWindowButtonOrder(leading, trailing);
can you explain why we want to do this?

[varkha, 2014-05-15 23:33:18]

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
File ui/views/window/custom_frame_view.h (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view.h:113: // this beforehand.
|leading_buttons_| and |trailing_buttons_| are not in this class and not
introduced. Maybe this part of the comment belongs in the implementation.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
File ui/views/window/custom_frame_view_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view_unittest.cc:64:
std::vector<views::FrameButton> GetLeadingButtons() {
This may be just simple enough to call it leading_buttons().

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view_unittest.cc:72: ImageButton*
GetMinimizeButton() {
This may be just simple enough to call it minimize_button().

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view_unittest.cc:88: gfx::Rect GetTitleBounds() {
This may be just simple enough to call it title_bounds().

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
File ui/views/window/window_button_order_provider.h (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
ui/views/window/window_button_order_provider.h:33:
std::vector<views::FrameButton> const GetTrailingButtons() const;
Those could be leading_buttons() and trailing_buttons() since they don't do
much.
Did you consider returning a const vector<>& to avoid an extra copy?

[jonross, 2014-05-16 18:59:48]

https://codereview.chromium.org/281353009/diff/20001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/20001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter_unittest.cc:142:
std::vector<views::FrameButton> leading;
On 2014/05/15 21:33:17, flackr wrote:
> nit: Comment why you need to set the window button order, and probably a TODO
to
> not be sensitive to these changes since it should be possible to be order
> agnostic.

Done.

https://codereview.chromium.org/281353009/diff/40001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/40001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter_unittest.cc:148:
SetWindowButtonOrder(leading, trailing);
On 2014/05/15 22:51:02, oshima wrote:
> can you explain why we want to do this?

Done.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
File ui/views/window/custom_frame_view.h (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view.h:113: // this beforehand.
On 2014/05/15 23:33:18, varkha wrote:
> |leading_buttons_| and |trailing_buttons_| are not in this class and not
> introduced. Maybe this part of the comment belongs in the implementation.

Done.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
File ui/views/window/custom_frame_view_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view_unittest.cc:64:
std::vector<views::FrameButton> GetLeadingButtons() {
On 2014/05/15 23:33:18, varkha wrote:
> This may be just simple enough to call it leading_buttons().

Done.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view_unittest.cc:72: ImageButton*
GetMinimizeButton() {
On 2014/05/15 23:33:18, varkha wrote:
> This may be just simple enough to call it minimize_button().

Done.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/custom_f...
ui/views/window/custom_frame_view_unittest.cc:88: gfx::Rect GetTitleBounds() {
On 2014/05/15 23:33:18, varkha wrote:
> This may be just simple enough to call it title_bounds().

Done.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
File ui/views/window/window_button_order_provider.h (right):

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
ui/views/window/window_button_order_provider.h:18: }  // namespace ash
On 2014/05/15 22:47:34, sadrul wrote:
> You shouldn't add ash:: things in views.
> 
> You can either make SetWindowButtonOrder public, or add a test-api to get
access
> to it (see, for example, menu_runner_test_api)

Done.

https://codereview.chromium.org/281353009/diff/40001/ui/views/window/window_b...
ui/views/window/window_button_order_provider.h:33:
std::vector<views::FrameButton> const GetTrailingButtons() const;
On 2014/05/15 23:33:18, varkha wrote:
> Those could be leading_buttons() and trailing_buttons() since they don't do
> much.
> Did you consider returning a const vector<>& to avoid an extra copy?

Done.

[varkha, 2014-05-20 20:07:23]

LGTM with a nit.

https://codereview.chromium.org/281353009/diff/60001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/60001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter_unittest.cc:143: // both use hardcoded touch
points, that assume the be empty header space.
nit: Something with the grammar here?

[jonross, 2014-05-21 21:08:10]

Hi sadrul,

I just wanted to check that the adjustments to window_button_order_provider
addressed your concerns before I submit.

https://codereview.chromium.org/281353009/diff/60001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter_unittest.cc (right):

https://codereview.chromium.org/281353009/diff/60001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter_unittest.cc:143: // both use hardcoded touch
points, that assume the be empty header space.
On 2014/05/20 20:07:23, varkha wrote:
> nit: Something with the grammar here?

Done.

[sadrul, 2014-05-21 21:21:18]

+erg@

lgtm

[Elliot Glaysher, 2014-05-21 21:24:40]

lgtm

[jonross, 2014-05-21 21:30:39]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-05-21 21:34:04]

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

[jonross, 2014-05-21 22:15:27]

The CQ bit was unchecked by jonross@chromium.org

[jonross, 2014-05-22 14:13:00]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-05-22 14:14:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/281353009/100001

[commit-bot: I haz the power, 2014-05-22 18:42:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-22 22:50:48]

Change committed as 272352

[jonross, 2014-05-23 14:26:06]

On 2014/05/22 22:50:48, I haz the power (commit-bot) wrote:
> Change committed as 272352

This change was reverted by: r272356

Review: https://codereview.chromium.org/294473016