Move maximize/fullscreen/restore to shell

Move maximize/fullscreen/restore to shell
 With this CL, window is now correctly maximized/fullscreen'ed on aura desktop.

* Added OnPropertyChanged to WindowObserver
* Added Get/Set IntProperty to simplify handling int value properties.
* Remove IsOrContainsFullscreen. Added Workspace::ContainsFullscreen instead. We need this to autohide launcher.

BUG=97257, 97259
TEST=new test for property change notification. existing tests are updated, except maximized/fullscreen drag test. I'll add it to new set of tests that verifies window dragging within workspace.

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

[oshima, 2011-10-29 01:36:51]

Scott, this is not ready for review yet, but want to check with you regarding my
change and use of WindowObserver. maximize/fullscreen tests are broken right now
and will fix if this looks ok to you.

[sky, 2011-10-31 14:09:46]

This approaches is fine by me.

[oshima, 2011-10-31 22:10:21]

Ready for review. PTL.

http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:25: virtual void OnPropertyChanged(Window* window,
const char* key, void* old) {}
Since this may get called even if the property hasn't changed, the name maybe a
big misleading. Let me know if
you prefer something like OnPropertySet or have better alternative.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_layout_manager_unittest.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager_unittest.cc:75: return
workspace_controller_->layout_manager()->workspace_manager();
I'd like to change the ownership so that
DCLM owns workspace_manager, workspace_controller and show_state_controller
because they should belong to one viewport. Let me know what you think.

[sky, 2011-10-31 23:17:34]

http://codereview.chromium.org/8400063/diff/9044/ui/aura/aura_constants.h
File ui/aura/aura_constants.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/aura_constants.h#new...
ui/aura/aura_constants.h:15: AURA_EXPORT extern const char* kShowStateKey;
nit: sort these, and add a description of where they are used.

http://codereview.chromium.org/8400063/diff/9044/ui/aura/layout_manager.h
File ui/aura/layout_manager.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/layout_manager.h#new...
ui/aura/layout_manager.h:43: // before apply, or ignore the request when
appropriate.
apply -> applying
remove 'when appropriate'

http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:1061: std::string PropertyChangeInfoAndClear() {
Add a description of what this does.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:48:
DefaultContainerLayoutManager::~DefaultContainerLayoutManager() {}
Should this iterate over all top level windows destroying kRestoreBoundsKey?

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:170:
views::Widget::InitParams::TYPE_WINDOW ||
Make sure you ignore windows with a transient parent too.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
File ui/aura_shell/show_state_controller.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
ui/aura_shell/show_state_controller.cc:33:
static_cast<gfx::Rect*>(window->GetProperty(aura::kRestoreBoundsKey));
This is accessed in a number of places. Could you move it to a common place. I'm
not saying in Window, but some where in the shell.

I wonder if instead of two properties we should have a single property that maps
to a struct, and we add to the struct as necessary. I don't feel strongly, so if
you prefer what you have that is fine too.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
ui/aura_shell/show_state_controller.cc:35: if (restore)
window->SetBounds(*restore);
split into two lines.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
File ui/aura_shell/show_state_controller.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
ui/aura_shell/show_state_controller.h:22: class ShowStateController : public
aura::WindowObserver {
Add a description of this.

[oshima, 2011-11-01 00:40:33]

Uploaded new patch. Added property_utuil.h|cc to add utility functions to
set/get/clear restore bounds property.

http://codereview.chromium.org/8400063/diff/9044/ui/aura/aura_constants.h
File ui/aura/aura_constants.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/aura_constants.h#new...
ui/aura/aura_constants.h:15: AURA_EXPORT extern const char* kShowStateKey;
On 2011/10/31 23:17:34, sky wrote:
> nit: sort these, and add a description of where they are used.

Done.

http://codereview.chromium.org/8400063/diff/9044/ui/aura/layout_manager.h
File ui/aura/layout_manager.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/layout_manager.h#new...
ui/aura/layout_manager.h:43: // before apply, or ignore the request when
appropriate.
On 2011/10/31 23:17:34, sky wrote:
> apply -> applying
> remove 'when appropriate'

Done.

http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:1061: std::string PropertyChangeInfoAndClear() {
On 2011/10/31 23:17:34, sky wrote:
> Add a description of what this does.

Done.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:48:
DefaultContainerLayoutManager::~DefaultContainerLayoutManager() {}
On 2011/10/31 23:17:34, sky wrote:
> Should this iterate over all top level windows destroying kRestoreBoundsKey?

They're deleted in OnWillRemoveWindow, which is called when child delete
themselves.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:170:
views::Widget::InitParams::TYPE_WINDOW ||
On 2011/10/31 23:17:34, sky wrote:
> Make sure you ignore windows with a transient parent too.

I need your change. Is it checked in?

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
File ui/aura_shell/show_state_controller.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
ui/aura_shell/show_state_controller.cc:33:
static_cast<gfx::Rect*>(window->GetProperty(aura::kRestoreBoundsKey));
On 2011/10/31 23:17:34, sky wrote:
> This is accessed in a number of places. Could you move it to a common place.
I'm
> not saying in Window, but some where in the shell.
> 
> I wonder if instead of two properties we should have a single property that
maps
> to a struct, and we add to the struct as necessary. I don't feel strongly, so
if
> you prefer what you have that is fine too.

I thought about it but didn't do it (for now) for following reasons.

1) Such struct has to be shared between shell and chrome/views, which violates
the rule.
  aura_constants.h is exception and i wanted to keep exception small, for now.
2) Changing the value becomes verbose, like
   SetProperty(new Struct(show_state));

We may revisit this if we've got more shell specific state.

Added util function.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
ui/aura_shell/show_state_controller.cc:35: if (restore)
window->SetBounds(*restore);
On 2011/10/31 23:17:34, sky wrote:
> split into two lines.

Done.

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
File ui/aura_shell/show_state_controller.h (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
ui/aura_shell/show_state_controller.h:22: class ShowStateController : public
aura::WindowObserver {
On 2011/10/31 23:17:34, sky wrote:
> Add a description of this.

Done.

[oshima, 2011-11-01 00:52:30]

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:170:
views::Widget::InitParams::TYPE_WINDOW ||
On 2011/11/01 00:40:33, oshima wrote:
> On 2011/10/31 23:17:34, sky wrote:
> > Make sure you ignore windows with a transient parent too.
> 

Sorry this is old comment. Done.
> I need your change. Is it checked in?

[sky, 2011-11-01 03:46:40]

On Mon, Oct 31, 2011 at 5:40 PM,  <oshima@chromium.org> wrote:
> Uploaded new patch. Added property_utuil.h|cc to add utility functions to
> set/get/clear restore bounds property.
>
>
> http://codereview.chromium.org/8400063/diff/9044/ui/aura/aura_constants.h
> File ui/aura/aura_constants.h (right):
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura/aura_constants.h#new...
> ui/aura/aura_constants.h:15: AURA_EXPORT extern const char*
> kShowStateKey;
> On 2011/10/31 23:17:34, sky wrote:
>>
>> nit: sort these, and add a description of where they are used.
>
> Done.
>
> http://codereview.chromium.org/8400063/diff/9044/ui/aura/layout_manager.h
> File ui/aura/layout_manager.h (right):
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura/layout_manager.h#new...
> ui/aura/layout_manager.h:43: // before apply, or ignore the request when
> appropriate.
> On 2011/10/31 23:17:34, sky wrote:
>>
>> apply -> applying
>> remove 'when appropriate'
>
> Done.
>
> http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_unittest.cc
> File ui/aura/window_unittest.cc (right):
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura/window_unittest.cc#n...
> ui/aura/window_unittest.cc:1061: std::string
> PropertyChangeInfoAndClear() {
> On 2011/10/31 23:17:34, sky wrote:
>>
>> Add a description of what this does.
>
> Done.
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
> File ui/aura_shell/default_container_layout_manager.cc (right):
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
> ui/aura_shell/default_container_layout_manager.cc:48:
> DefaultContainerLayoutManager::~DefaultContainerLayoutManager() {}
> On 2011/10/31 23:17:34, sky wrote:
>>
>> Should this iterate over all top level windows destroying
>
> kRestoreBoundsKey?
>
> They're deleted in OnWillRemoveWindow, which is called when child delete
> themselves.

What if the layout manager is destroyed before that?

>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/default_contai...
> ui/aura_shell/default_container_layout_manager.cc:170:
> views::Widget::InitParams::TYPE_WINDOW ||
> On 2011/10/31 23:17:34, sky wrote:
>>
>> Make sure you ignore windows with a transient parent too.
>
> I need your change. Is it checked in?

Yes.

>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
> File ui/aura_shell/show_state_controller.cc (right):
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
> ui/aura_shell/show_state_controller.cc:33:
> static_cast<gfx::Rect*>(window->GetProperty(aura::kRestoreBoundsKey));
> On 2011/10/31 23:17:34, sky wrote:
>>
>> This is accessed in a number of places. Could you move it to a common
>
> place. I'm
>>
>> not saying in Window, but some where in the shell.
>
>> I wonder if instead of two properties we should have a single property
>
> that maps
>>
>> to a struct, and we add to the struct as necessary. I don't feel
>
> strongly, so if
>>
>> you prefer what you have that is fine too.
>
> I thought about it but didn't do it (for now) for following reasons.
>
> 1) Such struct has to be shared between shell and chrome/views, which
> violates the rule.
>  aura_constants.h is exception and i wanted to keep exception small,
> for now.
> 2) Changing the value becomes verbose, like
>   SetProperty(new Struct(show_state));
>
> We may revisit this if we've got more shell specific state.
>
> Added util function.
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
> ui/aura_shell/show_state_controller.cc:35: if (restore)
> window->SetBounds(*restore);
> On 2011/10/31 23:17:34, sky wrote:
>>
>> split into two lines.
>
> Done.
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
> File ui/aura_shell/show_state_controller.h (right):
>
>
http://codereview.chromium.org/8400063/diff/9044/ui/aura_shell/show_state_con...
> ui/aura_shell/show_state_controller.h:22: class ShowStateController :
> public aura::WindowObserver {
> On 2011/10/31 23:17:34, sky wrote:
>>
>> Add a description of this.
>
> Done.
>
> http://codereview.chromium.org/8400063/
>

[sky, 2011-11-01 03:48:25]

http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util.h
File ui/aura_shell/property_util.h (right):

http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
ui/aura_shell/property_util.h:19: // Sets the restore bounds property on
|window|. It deletes
'It deletes' -> Deletes

http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
ui/aura_shell/property_util.h:23: // Returns the restore bounds property on
|window|.
Returning a pointer makes ownership a bit dicey. Could this return a gfx::Rect
by value instead?

[oshima, 2011-11-01 15:44:53]

http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util.h
File ui/aura_shell/property_util.h (right):

http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
ui/aura_shell/property_util.h:19: // Sets the restore bounds property on
|window|. It deletes
On 2011/11/01 03:48:25, sky wrote:
> 'It deletes' -> Deletes

Done.

http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
ui/aura_shell/property_util.h:23: // Returns the restore bounds property on
|window|.
On 2011/11/01 03:48:25, sky wrote:
> Returning a pointer makes ownership a bit dicey. Could this return a gfx::Rect
> by value instead?
It's pointer because the caller needs to know if the property exists.
It's possible to use gfx::Rect(), but I don't think that's good idea either.

I added comments to make it more clear. Let me know what you think.

[sky, 2011-11-01 21:31:55]

On 2011/11/01 15:44:53, oshima wrote:
> http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util.h
> File ui/aura_shell/property_util.h (right):
> 
>
http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
> ui/aura_shell/property_util.h:19: // Sets the restore bounds property on
> |window|. It deletes
> On 2011/11/01 03:48:25, sky wrote:
> > 'It deletes' -> Deletes
> 
> Done.
> 
>
http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
> ui/aura_shell/property_util.h:23: // Returns the restore bounds property on
> |window|.
> On 2011/11/01 03:48:25, sky wrote:
> > Returning a pointer makes ownership a bit dicey. Could this return a
gfx::Rect
> > by value instead?
> It's pointer because the caller needs to know if the property exists.
> It's possible to use gfx::Rect(), but I don't think that's good idea either.
> 
> I added comments to make it more clear. Let me know what you think.

LGTM just document that it may be NULL.

[oshima, 2011-11-01 21:57:39]

On 2011/11/01 21:31:55, sky wrote:
> On 2011/11/01 15:44:53, oshima wrote:
> >
http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util.h
> > File ui/aura_shell/property_util.h (right):
> > 
> >
>
http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
> > ui/aura_shell/property_util.h:19: // Sets the restore bounds property on
> > |window|. It deletes
> > On 2011/11/01 03:48:25, sky wrote:
> > > 'It deletes' -> Deletes
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8400063/diff/9052/ui/aura_shell/property_util....
> > ui/aura_shell/property_util.h:23: // Returns the restore bounds property on
> > |window|.
> > On 2011/11/01 03:48:25, sky wrote:
> > > Returning a pointer makes ownership a bit dicey. Could this return a
> gfx::Rect
> > > by value instead?
> > It's pointer because the caller needs to know if the property exists.
> > It's possible to use gfx::Rect(), but I don't think that's good idea either.
> > 
> > I added comments to make it more clear. Let me know what you think.
> 
> LGTM just document that it may be NULL.
done