ash: Make sure the uber-tray popup is positioned correctly.

ash: Make sure the uber-tray popup is positioned correctly.

BUG=120698
TEST=none

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

[sadrul, 2012-04-09 00:08:37]

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

http://codereview.chromium.org/10027007/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:226:
!shell->tray()->GetWidget()->GetWindowScreenBounds().IsEmpty()) {
Should 'IsVisible' check whether it has non-empty bounds?

[sky, 2012-04-09 15:11:21]

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

http://codereview.chromium.org/10027007/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:226:
!shell->tray()->GetWidget()->GetWindowScreenBounds().IsEmpty()) {
On 2012/04/09 00:08:37, sadrul wrote:
> Should 'IsVisible' check whether it has non-empty bounds?

Yes, I think that's a good idea.

[sadrul, 2012-04-09 20:56:28]

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

http://codereview.chromium.org/10027007/diff/1/ash/system/tray/system_tray.cc...
ash/system/tray/system_tray.cc:226:
!shell->tray()->GetWidget()->GetWindowScreenBounds().IsEmpty()) {
On 2012/04/09 15:11:21, sky wrote:
> On 2012/04/09 00:08:37, sadrul wrote:
> > Should 'IsVisible' check whether it has non-empty bounds?
> 
> Yes, I think that's a good idea.

Done.

[sky, 2012-04-09 21:01:48]

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

http://codereview.chromium.org/10027007/diff/6005/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.cc:132: bool empty =
status_->GetWindowScreenBounds().IsEmpty() &&
Can we use status_->IsVisible() instead? Also, I think checking one should be
enough.

[sadrul, 2012-04-09 23:59:34]

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

http://codereview.chromium.org/10027007/diff/6005/ash/wm/shelf_layout_manager...
ash/wm/shelf_layout_manager.cc:132: bool empty =
status_->GetWindowScreenBounds().IsEmpty() &&
On 2012/04/09 21:01:48, sky wrote:
> Can we use status_->IsVisible() instead? Also, I think checking one should be
> enough.

Changed to use status_->IsVisible. I have removed the check for launcher-widget,
but I believe the call to GetStatusWidth is still necessary?

[sky, 2012-04-10 00:23:39]

On 2012/04/09 23:59:34, sadrul wrote:
>
http://codereview.chromium.org/10027007/diff/6005/ash/wm/shelf_layout_manager.cc
> File ash/wm/shelf_layout_manager.cc (right):
> 
>
http://codereview.chromium.org/10027007/diff/6005/ash/wm/shelf_layout_manager...
> ash/wm/shelf_layout_manager.cc:132: bool empty =
> status_->GetWindowScreenBounds().IsEmpty() &&
> On 2012/04/09 21:01:48, sky wrote:
> > Can we use status_->IsVisible() instead? Also, I think checking one should
be
> > enough.
> 
> Changed to use status_->IsVisible. I have removed the check for
launcher-widget,
> but I believe the call to GetStatusWidth is still necessary?

Why do you want the check for statuswidth? Even if it were 0 the launcher would
still be visible. But the only for the status width to equal 0 is if that's it's
preferred width. Maybe there's some other bug here?

  -Scott

[sadrul, 2012-04-10 03:51:31]

On 2012/04/10 00:23:39, sky wrote:
> On 2012/04/09 23:59:34, sadrul wrote:
> >
>
http://codereview.chromium.org/10027007/diff/6005/ash/wm/shelf_layout_manager.cc
> > File ash/wm/shelf_layout_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/10027007/diff/6005/ash/wm/shelf_layout_manager...
> > ash/wm/shelf_layout_manager.cc:132: bool empty =
> > status_->GetWindowScreenBounds().IsEmpty() &&
> > On 2012/04/09 21:01:48, sky wrote:
> > > Can we use status_->IsVisible() instead? Also, I think checking one should
> be
> > > enough.
> > 
> > Changed to use status_->IsVisible. I have removed the check for
> launcher-widget,
> > but I believe the call to GetStatusWidth is still necessary?
> 
> Why do you want the check for statuswidth? Even if it were 0 the launcher
would
> still be visible. But the only for the status width to equal 0 is if that's
it's
> preferred width. Maybe there's some other bug here?

You are right. I don't need to check for non-zero status-width. I have removed
that check.

It turns out, changing the visibility of the tray-view leads to confusing
behaviour (because it does not contribute to the visibility of the widget, but
does affect the size it gets). So I have changed the code to 
change the visibility of the widget instead. PTAL.

[sky, 2012-04-10 16:30:46]

LGTM