[cros, Aura] Refresh status area widget bounds on StatusAreaView layout.

[cros, Aura] Refresh status area widget bounds on StatusAreaView layout.
Make ShelfLayoutController a ShelfLayoutManager for launcher, status windows.

BUG=105661
TEST=Manual. Status area is represented with all icons on Aura.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113185

[Nikita (slow), 2011-11-30 15:52:37]

I'm not sure whether there's alternate and more correct way to fix this.

What I've found is that StatusAreaView::GetPreferredSize() was called only once
in StatusAreaHostAura::CreateStatusArea()
...
views::Widget::InitParams params(views::Widget::InitParams::TYPE_CONTROL);
gfx::Size ps = status_area_view_->GetPreferredSize();
params.bounds = gfx::Rect(0, 0, ps.width(), ps.height());
...

[Nikita (slow), 2011-11-30 15:53:32]

+Scott for review.

[stevenjb, 2011-11-30 17:39:28]

Thanks Nikita for looking into this. This approach seems fine to me, but I will
let sky take a look and give it a lgtm.

[sky, 2011-11-30 21:41:42]

I think it would be better to use PreferredSizeChanged, have StatusAreaView
override ChildPreferredSizeChanged and reset the widgets bounds. You'll likely
need to then change DesktopLayoutManager::SetChildBounds to trigger a layout if
it's the status area that changed.

[Nikita (slow), 2011-12-01 11:12:54]

On 2011/11/30 21:41:42, sky wrote:
> I think it would be better to use PreferredSizeChanged, have StatusAreaView
> override ChildPreferredSizeChanged and reset the widgets bounds. You'll likely
> need to then change DesktopLayoutManager::SetChildBounds to trigger a layout
if
> it's the status area that changed.

I've moved call to resize widget and trigger layout to
StatusAreaView::ChildPreferredSizeChanged().

DesktopLayoutManager::SetChildBounds() will be called on resize of one of the
Desktop children which are default containers so it's not used in case of
resizing status area widget.

[stevenjb, 2011-12-01 16:59:20]

I took a look through the code, and believe that calling GetWidget()->SetSize()
should trigger the Layout() call without having to access the global shell
instance. However, I'm not sure how straightforward this will be with the
current structure.  I will talk to sky and ben about this today.

[sky, 2011-12-01 17:24:31]

On Thu, Dec 1, 2011 at 3:12 AM,  <nkostylev@chromium.org> wrote:
> On 2011/11/30 21:41:42, sky wrote:
>>
>> I think it would be better to use PreferredSizeChanged, have
>> StatusAreaView
>> override ChildPreferredSizeChanged and reset the widgets bounds. You'll
>> likely
>> need to then change DesktopLayoutManager::SetChildBounds to trigger a
>> layout
>
> if
>>
>> it's the status area that changed.
>
>
> I've moved call to resize widget and trigger layout to
> StatusAreaView::ChildPreferredSizeChanged().
>
> DesktopLayoutManager::SetChildBounds() will be called on resize of one of
> the
> Desktop children which are default containers so it's not used in case of
> resizing status area widget.
>
> http://codereview.chromium.org/8743014/

Good point. You'll need to set a LayoutManager on the window with id
kShellWindowId_StatusContainer. It's created in shell.cc's
CreateSpecialContainer.

  -Scott

[Nikita (slow), 2011-12-02 17:05:58]

On 2011/12/01 17:24:31, sky wrote:
> On Thu, Dec 1, 2011 at 3:12 AM,  <mailto:nkostylev@chromium.org> wrote:
> > On 2011/11/30 21:41:42, sky wrote:
> >>
> >> I think it would be better to use PreferredSizeChanged, have
> >> StatusAreaView
> >> override ChildPreferredSizeChanged and reset the widgets bounds. You'll
> >> likely
> >> need to then change DesktopLayoutManager::SetChildBounds to trigger a
> >> layout
> >
> > if
> >>
> >> it's the status area that changed.
> >
> >
> > I've moved call to resize widget and trigger layout to
> > StatusAreaView::ChildPreferredSizeChanged().
> >
> > DesktopLayoutManager::SetChildBounds() will be called on resize of one of
> > the
> > Desktop children which are default containers so it's not used in case of
> > resizing status area widget.
> >
> > http://codereview.chromium.org/8743014/
> 
> Good point. You'll need to set a LayoutManager on the window with id
> kShellWindowId_StatusContainer. It's created in shell.cc's
> CreateSpecialContainer.

I've just realized that we could not call shelf_->LayoutShelf() from
DesktopLayoutManager::SetChildBounds() as

void ShelfLayoutController::LayoutShelf() {
  ...
  launcher_->SetBounds(target_bounds.launcher_bounds);
  status_->SetBounds(target_bounds.status_bounds);
  ...
}

Unless there're any other suggestions, I'm leaning towards my initial appoach,
patchset #2.

[sky, 2011-12-02 17:34:08]

On 2011/12/02 17:05:58, Nikita Kostylev wrote:
> On 2011/12/01 17:24:31, sky wrote:
> > On Thu, Dec 1, 2011 at 3:12 AM,  <mailto:nkostylev@chromium.org> wrote:
> > > On 2011/11/30 21:41:42, sky wrote:
> > >>
> > >> I think it would be better to use PreferredSizeChanged, have
> > >> StatusAreaView
> > >> override ChildPreferredSizeChanged and reset the widgets bounds. You'll
> > >> likely
> > >> need to then change DesktopLayoutManager::SetChildBounds to trigger a
> > >> layout
> > >
> > > if
> > >>
> > >> it's the status area that changed.
> > >
> > >
> > > I've moved call to resize widget and trigger layout to
> > > StatusAreaView::ChildPreferredSizeChanged().
> > >
> > > DesktopLayoutManager::SetChildBounds() will be called on resize of one of
> > > the
> > > Desktop children which are default containers so it's not used in case of
> > > resizing status area widget.
> > >
> > > http://codereview.chromium.org/8743014/
> > 
> > Good point. You'll need to set a LayoutManager on the window with id
> > kShellWindowId_StatusContainer. It's created in shell.cc's
> > CreateSpecialContainer.
> 
> I've just realized that we could not call shelf_->LayoutShelf() from
> DesktopLayoutManager::SetChildBounds() as
> 
> void ShelfLayoutController::LayoutShelf() {
>   ...
>   launcher_->SetBounds(target_bounds.launcher_bounds);
>   status_->SetBounds(target_bounds.status_bounds);
>   ...
> }
> 
> Unless there're any other suggestions, I'm leaning towards my initial appoach,
> patchset #2.

Nikita and myself IM'd about this. The resolution is to rename
ShelfLayoutController to ShelfLayoutManager, make it the LayoutManager for the
parent of the shelf/launcher. With this change DesktopLayoutManager won't need
to know about the shelf anymore.

[Nikita (slow), 2011-12-02 18:26:20]

So the fix is in status_area_view.cc, I've tested with and without it.

[sky, 2011-12-02 19:25:53]

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
File ui/aura_shell/shelf_layout_manager.cc (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
ui/aura_shell/shelf_layout_manager.cc:79: SetChildBoundsDirect(child,
requested_bounds);
I think you want if/else, eg:

if (!in_layout_)
  LayoutShelf();
else
  SetChildBoundsDirect(child, requested_bounds);

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
ui/aura_shell/shelf_layout_manager.cc:88: in_layout_ = true;
use base/auto_reset

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
File ui/aura_shell/shelf_layout_manager.h (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
ui/aura_shell/shelf_layout_manager.h:29: views::Widget* status);
nit: update indentation

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:199:
GetContainer(aura_shell::internal::kShellWindowId_LauncherContainer)->
This won't work. Window owns the LayoutManager, meaning this code will give us a
double delete. I say rename StatusContainer to ShelfContainer and remove
LauncherContainer.

[Nikita (slow), 2011-12-02 21:34:27]

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:199:
GetContainer(aura_shell::internal::kShellWindowId_LauncherContainer)->
On 2011/12/02 19:25:53, sky wrote:
> This won't work. Window owns the LayoutManager, meaning this code will give us
a
> double delete. I say rename StatusContainer to ShelfContainer and remove
> LauncherContainer.

They have to stay in a separate containers as launcher has to stay below the
lock / login screen and status area above them.

Is there any other approach than placing launcher in status area container and
hiding it when any window is added to the lock container. That's not a flexible
approach.

[sky, 2011-12-02 23:00:04]

On Fri, Dec 2, 2011 at 1:34 PM,  <nkostylev@chromium.org> wrote:
>
> http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc
> File ui/aura_shell/shell.cc (right):
>
>
http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc#newco...
> ui/aura_shell/shell.cc:199:
> GetContainer(aura_shell::internal::kShellWindowId_LauncherContainer)->
> On 2011/12/02 19:25:53, sky wrote:
>>
>> This won't work. Window owns the LayoutManager, meaning this code will
>
> give us a
>>
>> double delete. I say rename StatusContainer to ShelfContainer and
>
> remove
>>
>> LauncherContainer.
>
>
> They have to stay in a separate containers as launcher has to stay below
> the lock / login screen and status area above them.
>
> Is there any other approach than placing launcher in status area
> container and hiding it when any window is added to the lock container.
> That's not a flexible approach.
>
> http://codereview.chromium.org/8743014/

Oy! Sorry, I hadn't realized that.
You'll need two LayoutManagers than that work closely together then.

  -Scott

[Nikita (slow), 2011-12-05 14:41:46]

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
File ui/aura_shell/shelf_layout_manager.cc (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
ui/aura_shell/shelf_layout_manager.cc:79: SetChildBoundsDirect(child,
requested_bounds);
On 2011/12/02 19:25:53, sky wrote:
> I think you want if/else, eg:
> 
> if (!in_layout_)
>   LayoutShelf();
> else
>   SetChildBoundsDirect(child, requested_bounds);

Not really. It works like:
GetWidget()->SetSize(GetPreferredSize());
SetChildBoundsDirect(child, requested_bounds);
and then re-layout.

Otherwise status area is not refreshed as I see it.

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
ui/aura_shell/shelf_layout_manager.cc:88: in_layout_ = true;
On 2011/12/02 19:25:53, sky wrote:
> use base/auto_reset

Done.

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
File ui/aura_shell/shelf_layout_manager.h (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shelf_layout_m...
ui/aura_shell/shelf_layout_manager.h:29: views::Widget* status);
On 2011/12/02 19:25:53, sky wrote:
> nit: update indentation

Done.

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8743014/diff/5003/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:199:
GetContainer(aura_shell::internal::kShellWindowId_LauncherContainer)->
On 2011/12/02 19:25:53, sky wrote:
> This won't work. Window owns the LayoutManager, meaning this code will give us
a
> double delete. I say rename StatusContainer to ShelfContainer and remove
> LauncherContainer.

Done.

[Nikita (slow), 2011-12-05 14:41:58]

Added separate layout manager for status area.

[sky, 2011-12-05 15:42:21]

Doesn't the ShelfLayoutManager need to tell the StatusAreaLayoutManager when
it's doing layout?

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
File ui/aura_shell/shelf_layout_manager.cc (right):

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:114: TargetBounds* target_bounds) {
nit: reformat

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:114: TargetBounds* target_bounds) {
nit: fix indentation.

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:132: const gfx::Rect& target_bounds,
nit: fix indentation

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:132: const gfx::Rect& target_bounds,
nit: fix indentation

[sky, 2011-12-05 15:42:57]

On 2011/12/05 15:42:21, sky wrote:
> Doesn't the ShelfLayoutManager need to tell the StatusAreaLayoutManager when
> it's doing layout?

Or maybe it could just ask to see if shelf is already in layout, and if so don't
try to layout again.

  -Scott

[Nikita (slow), 2011-12-05 17:43:47]

On 2011/12/05 15:42:57, sky wrote:
> On 2011/12/05 15:42:21, sky wrote:
> > Doesn't the ShelfLayoutManager need to tell the StatusAreaLayoutManager when
> > it's doing layout?
> 
> Or maybe it could just ask to see if shelf is already in layout, and if so
don't
> try to layout again.

Added check.

[Nikita (slow), 2011-12-05 17:44:05]

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
File ui/aura_shell/shelf_layout_manager.cc (right):

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:114: TargetBounds* target_bounds) {
On 2011/12/05 15:42:21, sky wrote:
> nit: fix indentation.

Done.

http://codereview.chromium.org/8743014/diff/14001/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:132: const gfx::Rect& target_bounds,
On 2011/12/05 15:42:21, sky wrote:
> nit: fix indentation

Done.

[sky, 2011-12-05 18:16:34]

LGTM

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
File ui/aura_shell/shelf_layout_manager.h (right):

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.h:33: bool in_layout() { return in_layout_; }
const

[stevenjb, 2011-12-05 20:08:59]

It seems like the status area layout code could be moved from ShelfLayoutManager
to StatusAreaLayoutManager, but that can wait for a followup CL.
LGTM with nits.

http://codereview.chromium.org/8743014/diff/16009/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/status_area_view.cc (right):

http://codereview.chromium.org/8743014/diff/16009/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/status_area_view.cc:17: #include
"ui/aura_shell/shell.h"
We don't need this include any more.

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
File ui/aura_shell/shelf_layout_manager.cc (right):

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:29: views::Widget* status)
nit: alignment

[Nikita (slow), 2011-12-06 08:26:19]

http://codereview.chromium.org/8743014/diff/16009/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/status_area_view.cc (right):

http://codereview.chromium.org/8743014/diff/16009/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/status_area_view.cc:17: #include
"ui/aura_shell/shell.h"
On 2011/12/05 20:08:59, Steven Bennetts wrote:
> We don't need this include any more.

Done.

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
File ui/aura_shell/shelf_layout_manager.cc (right):

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.cc:29: views::Widget* status)
On 2011/12/05 20:08:59, Steven Bennetts wrote:
> nit: alignment

Done.

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
File ui/aura_shell/shelf_layout_manager.h (right):

http://codereview.chromium.org/8743014/diff/16009/ui/aura_shell/shelf_layout_...
ui/aura_shell/shelf_layout_manager.h:33: bool in_layout() { return in_layout_; }
On 2011/12/05 18:16:34, sky wrote:
> const

Done.