Add workspace to desktop

Add workspace to desktop
* Windows are now managed by workspace manager and laid out as such.
* Window activation triggers active workspace switch.
* Clicking backround toggles overview mode
* Replaces TileWindow layout with overview mode.

You can drag window, but it doesn't actually move the window (yet).

BUG=none
TEST=new tests are added to workspace_manager_unittests,default_container_layout_manager_unittests

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

[oshima, 2011-10-25 20:08:07]

I'm still working on tests and will let you know when tests are done, but you
may start reviewing the rest.

[sky, 2011-10-25 20:58:05]

http://codereview.chromium.org/8381015/diff/12001/ui/aura/toplevel_window_eve...
File ui/aura/toplevel_window_event_filter.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura/toplevel_window_eve...
ui/aura/toplevel_window_event_filter.h:30: int window_component() const { return
window_component_; }
Add comment.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_event_filter.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_event_filter.cc:31:
layout_manager->PrepareForMoveOrResize(target, event);
Couldn't we end up invoking Prepare when we know you're not over a portion of
the window that could trigger a resize?
Would it be better to make HandleDrag protected and move some of this code
there?

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_event_filter.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_event_filter.h:32: bool MaybeDragStart();
How about naming this UpdateDragState with a comment something like:
If the mouse is currently over a portion of the window that should trigger a
drag or resize, drag_state_ is set appropriately and true is returned. If the
mouse is not over a portion of the window that should trigger a move or resize
drag_state_ is not updated and false is returned.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:25: aura::Window* owner,
WorkspaceManager* workspace_manager)
each param on its own line.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:80: aura::Window* child,
gfx::Rect* requested_bounds) {
each param on its own line.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:104: aura::Window* drag,
aura::MouseEvent* event) {
each param on its own line. This comment applies to all places in this file.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:110: drag_window_ = NULL;
Does this need to revert to original position?

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_layout_manager.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.h:33: aura::Window* owner,
WorkspaceManager* workspace_manager);
each param on its own line.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.h:45: // Invoked when a window
receives drag event.
nit: keep non-virtual methods before virtual methods.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.h:69: bool
layout_manager_has_control_;
How about naming this ignore_calculate_bounds_?

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:35: gfx::Rect
Workspace::GetWorkAreaBounds() const {
order doesn't match header.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:73: gfx::Rect work_area =
workspace_manager_->GetWorkAreaBounds(bounds_);
How will popups be handled? For example, the omnibox popup or the status bubble.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:98:
MoveWindowTo(windows_[0],work_area.origin(), new_window != windows_[0]);
nit: '[0],' -> '[0] ,'

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:122: aura::Window* window, const
gfx::Point& origin, bool animate) {
each param on its own line.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:24: // is visible to a user at a given time.
The size of workspace is
'to a user' -> 'to the user'
'size of workspace' -> 'size of the workspace'

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:25: // generally same as the size of the
monitor, and the desktiop can
'generally same' -> 'generally the same'
desktiop -> desktop

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:27: //  One workspace can contains limited
number of windows and the
Don't indent here.
'A worskpace contains a limited number of ..

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:28: // workspace manager may create new
workspace if there is no room for
'may create new' -> 'may create a new'
'no room' -> not enough room for a window.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:63: void Layout(aura::Window* new_window);
Document what new_window is.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:70: Workspaces::const_iterator i =
workspaces_.begin();
remove this.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:73: i++) {
++i

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:82: gfx::Rect
bounds(viewport_->bounds().size());
Can this be return GetWorkAreaBounds(gfx::Rect(viewport_->bounds().size());

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:104: int dx = overview_width <
workspace_size_.width() ?
If line 95 yields a scale < .3f and then you up it at 99 then part of the
overview isn't going to be visible. Maybe a TODO for it?

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:105: (workspace_size_.width() -
overview_width) /2 : 0;
'/2' -> '/ 2'

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:132:
//////////////////////////h/////////////////////////////////////////////////////
'/h/' -> ///

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:190: ui::Transform transform;
Both SetActiveWorkspace and this share code. Refactor so they can both share the
common code.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace_manager.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:31: class AURA_SHELL_EXPORT
WorkspaceManager : aura::DesktopObserver {
public aura::DesktopObserver

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:37: work_area_insets_ = insets;
Caching the insets here and in screen makes it easy to get them out of sync.
Could this get the values from Screen?

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:54: // Returns
Document this.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:75: // the |work_area_insets_|.
Returns the bounds of the work area given the workspace bounds.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:89: gfx::Size workspace_size_;
Document this.

[oshima, 2011-10-25 23:37:23]

Ben seems to be working on event filter, so I'll wait for his reply before
making changes we discussed. Still working on tests.

http://codereview.chromium.org/8381015/diff/12001/ui/aura/toplevel_window_eve...
File ui/aura/toplevel_window_event_filter.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura/toplevel_window_eve...
ui/aura/toplevel_window_event_filter.h:30: int window_component() const { return
window_component_; }
On 2011/10/25 20:58:05, sky wrote:
> Add comment.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_event_filter.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_event_filter.cc:31:
layout_manager->PrepareForMoveOrResize(target, event);
On 2011/10/25 20:58:05, sky wrote:
> Couldn't we end up invoking Prepare when we know you're not over a portion of
> the window that could trigger a resize?
> Would it be better to make HandleDrag protected and move some of this code
> there?

I don't like the current code either and was wondering what would be better
approach. Maybe we should move toplevel window_event_filter to shell and merge
this?

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_event_filter.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_event_filter.h:32: bool MaybeDragStart();
On 2011/10/25 20:58:05, sky wrote:
> How about naming this UpdateDragState with a comment something like:
> If the mouse is currently over a portion of the window that should trigger a
> drag or resize, drag_state_ is set appropriately and true is returned. If the
> mouse is not over a portion of the window that should trigger a move or resize
> drag_state_ is not updated and false is returned.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:25: aura::Window* owner,
WorkspaceManager* workspace_manager)
On 2011/10/25 20:58:05, sky wrote:
> each param on its own line.

Done. Seems like this is banned sometime this summer and I didn't notice it.
Sorry about that.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:80: aura::Window* child,
gfx::Rect* requested_bounds) {
On 2011/10/25 20:58:05, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:104: aura::Window* drag,
aura::MouseEvent* event) {
On 2011/10/25 20:58:05, sky wrote:
> each param on its own line. This comment applies to all places in this file.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.cc:110: drag_window_ = NULL;
On 2011/10/25 20:58:05, sky wrote:
> Does this need to revert to original position?
We shouldn't have to do anything here
because window wasn't moved or resized. (or a bug if it did happen)

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
File ui/aura_shell/default_container_layout_manager.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.h:33: aura::Window* owner,
WorkspaceManager* workspace_manager);
On 2011/10/25 20:58:05, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.h:45: // Invoked when a window
receives drag event.
On 2011/10/25 20:58:05, sky wrote:
> nit: keep non-virtual methods before virtual methods.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/default_conta...
ui/aura_shell/default_container_layout_manager.h:69: bool
layout_manager_has_control_;
On 2011/10/25 20:58:05, sky wrote:
> How about naming this ignore_calculate_bounds_?

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:35: gfx::Rect
Workspace::GetWorkAreaBounds() const {
On 2011/10/25 20:58:05, sky wrote:
> order doesn't match header.

fixed

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:73: gfx::Rect work_area =
workspace_manager_->GetWorkAreaBounds(bounds_);
On 2011/10/25 20:58:05, sky wrote:
> How will popups be handled? For example, the omnibox popup or the status
bubble.

popups/bubbles are excluded in DefaultContainerLayoutManager

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:98:
MoveWindowTo(windows_[0],work_area.origin(), new_window != windows_[0]);
On 2011/10/25 20:58:05, sky wrote:
> nit: '[0],' -> '[0] ,'

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.cc:122: aura::Window* window, const
gfx::Point& origin, bool animate) {
On 2011/10/25 20:58:05, sky wrote:
> each param on its own line.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:25: // generally same as the size of the
monitor, and the desktiop can
On 2011/10/25 20:58:05, sky wrote:
> 'generally same' -> 'generally the same'
> desktiop -> desktop

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:27: //  One workspace can contains limited
number of windows and the
On 2011/10/25 20:58:05, sky wrote:
> Don't indent here.
> 'A worskpace contains a limited number of ..

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:28: // workspace manager may create new
workspace if there is no room for
On 2011/10/25 20:58:05, sky wrote:
> 'may create new' -> 'may create a new'
> 'no room' -> not enough room for a window.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace.h:63: void Layout(aura::Window* new_window);
On 2011/10/25 20:58:05, sky wrote:
> Document what new_window is.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:70: Workspaces::const_iterator i =
workspaces_.begin();
On 2011/10/25 20:58:05, sky wrote:
> remove this.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:73: i++) {
On 2011/10/25 20:58:05, sky wrote:
> ++i

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:82: gfx::Rect
bounds(viewport_->bounds().size());
On 2011/10/25 20:58:05, sky wrote:
> Can this be return GetWorkAreaBounds(gfx::Rect(viewport_->bounds().size());

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:104: int dx = overview_width <
workspace_size_.width() ?
On 2011/10/25 20:58:05, sky wrote:
> If line 95 yields a scale < .3f and then you up it at 99 then part of the
> overview isn't going to be visible. Maybe a TODO for it?

Yes, that's one of things I'd like you to look into :) Added TODO. Made 0.3/0.9f
constant.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:105: (workspace_size_.width() -
overview_width) /2 : 0;
On 2011/10/25 20:58:05, sky wrote:
> '/2' -> '/ 2'

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:132:
//////////////////////////h/////////////////////////////////////////////////////
On 2011/10/25 20:58:05, sky wrote:
> '/h/' -> ///

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.cc:190: ui::Transform transform;
On 2011/10/25 20:58:05, sky wrote:
> Both SetActiveWorkspace and this share code. Refactor so they can both share
the
> common code.

The code in SetActiveWorkspace isn't necessary, my mistake. Removed.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
File ui/aura_shell/workspace/workspace_manager.h (right):

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:31: class AURA_SHELL_EXPORT
WorkspaceManager : aura::DesktopObserver {
On 2011/10/25 20:58:05, sky wrote:
> public aura::DesktopObserver

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:37: work_area_insets_ = insets;
On 2011/10/25 20:58:05, sky wrote:
> Caching the insets here and in screen makes it easy to get them out of sync.
> Could this get the values from Screen?

Good point. Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:54: // Returns
On 2011/10/25 20:58:05, sky wrote:
> Document this.

Done.

http://codereview.chromium.org/8381015/diff/12001/ui/aura_shell/workspace/wor...
ui/aura_shell/workspace/workspace_manager.h:75: // the |work_area_insets_|.
On 2011/10/25 20:58:05, sky wrote:
> Returns the bounds of the work area given the workspace bounds.

Done.

[sky, 2011-10-25 23:57:49]

LGTM