Add left/right layout support for uber tray.

Add left/right launcher layout support for uber tray. The status area and uber tray bubble will adjust for left/right launcher layout.


BUG=127577
TEST=Status area and uber tray bubble UI looks right for left/right launcher layout.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137980

[jennyz, 2012-05-17 01:44:01]

I still need to change notification bubble to align with left/right launcher
layout. I would like to run this cl through with you before I proceed to the
notification bubble part. It seems a little bit hacky to adjust align the uber
tray inside SystemTrayBubble::GetAnchorRect(), maybe we should override the
SystemTrayBubbleBorder::GetInsets(0 instead? I am a little concerned that will
break some other UI adjustment if I do so. Would like to hear your opinion.

[sadrul, 2012-05-17 19:57:05]

Looks really good. There are some nits, and issue with arrow position offset.
But I think this is really close.

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:483: void SystemTray::SetAlignment(ShelfAlignment
alignment) {
SetShelfAlignment

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:500: return shelf_alignment_;
This should be unix_hacker style and inlined, i.e. in the header file, it should
be:
  ShelfAlignment shelf_alignment() const { return shelf_alignment_; }

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
File ash/system/tray/system_tray_bubble.cc (left):

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:131: class SystemTrayBubbleBackground :
public views::Background {
Oooh. It was on my TODO to remove SystemTrayBubbleBackground. Thanks!

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:238: const int arrow_offset_;
I think the removal of arrow_offset_ was accidental? We will still need this.
(see crbugs 124636, 124637)

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:1: 
- this line

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:148: virtual void
PaintChildBorder(gfx::Canvas* canvas) const {
This doesn't need to be virtual, right?

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:262: SystemTrayBubble* host, bool
can_activate)
bool can_activate should be in its own line

http://codereview.chromium.org/10384217/diff/1/ash/wm/status_area_layout_mana...
File ash/wm/status_area_layout_manager.cc (right):

http://codereview.chromium.org/10384217/diff/1/ash/wm/status_area_layout_mana...
ash/wm/status_area_layout_manager.cc:59: if (requested_bounds ==
child->bounds())
Is this change because the size doesn't change when switching between left/right
alignment but we still need to relayout?

[jennyz, 2012-05-17 22:13:30]

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:483: void SystemTray::SetAlignment(ShelfAlignment
alignment) {
On 2012/05/17 19:57:05, sadrul wrote:
> SetShelfAlignment

Done.

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:500: return shelf_alignment_;
On 2012/05/17 19:57:05, sadrul wrote:
> This should be unix_hacker style and inlined, i.e. in the header file, it
should
> be:
>   ShelfAlignment shelf_alignment() const { return shelf_alignment_; }

Done.

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
File ash/system/tray/system_tray_bubble.cc (left):

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:238: const int arrow_offset_;
On 2012/05/17 19:57:05, sadrul wrote:
> I think the removal of arrow_offset_ was accidental? We will still need this.
> (see crbugs 124636, 124637)
Yes, that was removed by accident during rebasing and merging, I added it back.
But I noticed the UI looks a little weird to me when the arrow kept moving with
the mouse clicking, but the arrow can looks funny and even off the bubble when
the mouse is pointing to the border of the bubble . Also this will further
create UI problems in left/right layout, if the arrow moves to the upper part of
the bubble, we will need to change the fill color of the arrow. The worst case
will be for the arrow to be positioned across the border between the dark and
light section of the bubble. I wonder if we will further refine this UI
behavior. For now I only added a kMinArrowOffset to limit the arrow not to move
off and too close to the edge of the bubble.

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:1: 
On 2012/05/17 19:57:05, sadrul wrote:
> - this line

Done.

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:148: virtual void
PaintChildBorder(gfx::Canvas* canvas) const {
On 2012/05/17 19:57:05, sadrul wrote:
> This doesn't need to be virtual, right?

Done.

http://codereview.chromium.org/10384217/diff/1/ash/system/tray/system_tray_bu...
ash/system/tray/system_tray_bubble.cc:262: SystemTrayBubble* host, bool
can_activate)
On 2012/05/17 19:57:05, sadrul wrote:
> bool can_activate should be in its own line

Done.

http://codereview.chromium.org/10384217/diff/1/ash/wm/status_area_layout_mana...
File ash/wm/status_area_layout_manager.cc (right):

http://codereview.chromium.org/10384217/diff/1/ash/wm/status_area_layout_mana...
ash/wm/status_area_layout_manager.cc:59: if (requested_bounds ==
child->bounds())
On 2012/05/17 19:57:05, sadrul wrote:
> Is this change because the size doesn't change when switching between
left/right
> alignment but we still need to relayout?
Yes.

[sadrul, 2012-05-17 23:30:28]

Address the nits, and LGTM

You raise very good points about the arrow position and color for vertical
trays. Let's look into fixing that after this one lands (we need to fix the
arrow color for volume etc. bubbles anyway).

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:490: }  else if (alignment ==
SHELF_ALIGNMENT_LEFT ||
two many spaces here

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:495: }
I think this could just be:

 tray_container_->SetLayoutManager(new views::BoxLayout(
    alignment == SHELF_ALIGNMENT_BOTTOM ? kHorizontal : kVertical,
    0, 0, 0));

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:210: path.incReserve(4);
Move this line after line 203 and remove from other blocks

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:226: int tip_y = y - arrow_offset_;
spacing in this block seems off

[jennyz, 2012-05-18 00:00:42]

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray.cc
File ash/system/tray/system_tray.cc (right):

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:490: }  else if (alignment ==
SHELF_ALIGNMENT_LEFT ||
On 2012/05/17 23:30:29, sadrul wrote:
> two many spaces here

Done.

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray.cc:495: }
On 2012/05/17 23:30:29, sadrul wrote:
> I think this could just be:
> 
>  tray_container_->SetLayoutManager(new views::BoxLayout(
>     alignment == SHELF_ALIGNMENT_BOTTOM ? kHorizontal : kVertical,
>     0, 0, 0));
Done. I was trying to save a little for switching between left/right, since it
does not need to change layout manager. But this probably is not a big deal and
the code looks cleaner regardless, changed.

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:210: path.incReserve(4);
On 2012/05/17 23:30:29, sadrul wrote:
> Move this line after line 203 and remove from other blocks

Done.

http://codereview.chromium.org/10384217/diff/6001/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:226: int tip_y = y - arrow_offset_;
On 2012/05/17 23:30:29, sadrul wrote:
> spacing in this block seems off

Done.

[commit-bot: I haz the power, 2012-05-18 00:03:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/10384217/6010

[commit-bot: I haz the power, 2012-05-18 00:04:02]

Failed to apply patch for ash/system/tray/system_tray_bubble.cc:
While running patch -p1 --forward --force;
patching file ash/system/tray/system_tray_bubble.cc
Hunk #1 succeeded at 35 (offset 1 line).
Hunk #2 succeeded at 132 (offset 1 line).
Hunk #3 succeeded at 177 (offset 1 line).
Hunk #4 FAILED at 187.
Hunk #5 succeeded at 241 (offset 5 lines).
Hunk #6 succeeded at 270 (offset 5 lines).
Hunk #7 succeeded at 385 (offset 12 lines).
Hunk #8 succeeded at 410 (offset 12 lines).
Hunk #9 succeeded at 436 (offset 12 lines).
1 out of 9 hunks FAILED -- saving rejects to file
ash/system/tray/system_tray_bubble.cc.rej

[commit-bot: I haz the power, 2012-05-18 00:13:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/10384217/3004

[commit-bot: I haz the power, 2012-05-18 00:13:48]

Presubmit check for 10384217-3004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.4s to calculate.

[sky, 2012-05-18 15:01:48]

Also, how about some tests.

http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager.cc
File ash/wm/shelf_layout_manager.cc (right):

http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.cc:404: *width = std::max(launcher_size.width(),
status_size.width());
Does the tray now support left/right alignment that its safe to do this?

http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.cc:427: gfx::Size status_size =
Shell::GetInstance()->tray() ?
Why does this and GetShelfSize need to query for the preferred size? Does the
shelf reset its widget bounds when its preferred size changes?

[sky, 2012-05-18 15:02:39]

On 2012/05/18 15:01:48, sky wrote:
> Also, how about some tests.
> 
>
http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager.cc
> File ash/wm/shelf_layout_manager.cc (right):
> 
>
http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager...
> ash/wm/shelf_layout_manager.cc:404: *width = std::max(launcher_size.width(),
> status_size.width());
> Does the tray now support left/right alignment that its safe to do this?

Ignore this. I was confused by your description. I see now that you're adding
this support. NICE!

  -Scott

>
http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager...
> ash/wm/shelf_layout_manager.cc:427: gfx::Size status_size =
> Shell::GetInstance()->tray() ?
> Why does this and GetShelfSize need to query for the preferred size? Does the
> shelf reset its widget bounds when its preferred size changes?

[jennyz, 2012-05-18 22:05:10]

http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager.cc
File ash/wm/shelf_layout_manager.cc (right):

http://codereview.chromium.org/10384217/diff/3004/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.cc:427: gfx::Size status_size =
Shell::GetInstance()->tray() ?
On 2012/05/18 15:01:48, sky wrote:
> Why does this and GetShelfSize need to query for the preferred size? Does the
> shelf reset its widget bounds when its preferred size changes?
Done. I updated the widget size when changing layout manager in
SystemTrayContainer, we can keep code as it was here.

[sky, 2012-05-18 22:26:41]

LGTM

[commit-bot: I haz the power, 2012-05-18 22:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/10384217/15004