Reworks the workspace code. Here's the new heuristics:

Reworks the workspace code. Here's the new heuristics:
. All normal windows end up in a single workspace.
. Each maximized window ends up in its own workspace.
. There is an additional workspace for split windows, but the
  heuristics of this aren't implemented yet.

I've left some bits of the old code, in particular overview but there
isn't really a way to get into it yet and it'll need some reworking if
we decide to keep it.

BUG=111285
TEST=none
R=ben@chromium.org,oshima@chromium.org

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

[sky, 2012-01-25 01:03:57]

There's a lot of bits TODO here, but this is a good starting point.

[sky, 2012-01-25 15:58:57]

Since no one has looked at this yet I changed it around rather substantially. In
particular after talking with Nicholas he wants to view the workspaces as a
stack. Now all workspaces have the same bounds and instead the visibility of the
layer associated with each window is changed.

[Ben Goodger (Google), 2012-01-25 18:34:58]

I am going to let oshima have first swing at this since he knows the
existing code better than I do.

-Ben

On Wed, Jan 25, 2012 at 7:58 AM, <sky@chromium.org> wrote:

> Since no one has looked at this yet I changed it around rather
> substantially. In
> particular after talking with Nicholas he wants to view the workspaces as a
> stack. Now all workspaces have the same bounds and instead the visibility
> of the
> layer associated with each window is changed.
>
>
http://codereview.chromium.**org/9113045/<http://codereview.chromium.org/9113...
>

[oshima, 2012-01-25 23:21:12]

first round. need to go over a few more times.

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace.cc
File ash/wm/workspace/workspace.cc (right):

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace.c...
ash/wm/workspace/workspace.cc:43: type_ = type;
Since a workspace can morph to another type, it's probably better to check of
type transition is possible.

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_l...
File ash/wm/workspace/workspace_layout_manager.cc (right):

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:108: SetChildBoundsDirect(child,
requested_bounds);
What is the path to set a window's initial size is set? (assuming that in normal
workspace, we honor requested size)

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:81: return;  // Already know about this
window.
When this can happen?

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:90: if
(window_util::IsWindowMaximized(window) && !GetRestoreBounds(window))
don't we have to check full screen?

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:98: // current workspace isn't
split/maximized, then we maximize.
then create a maximized workspace.

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:128:
workspace->SetType(Workspace::TypeForWindow(window));
I believe this needs to be type_for_window?

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:274: Workspace* workspace =
FindBy(window);
const (if possible)

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:278: SetWindowBounds(window, *restore);
This is necessary?

[sky, 2012-01-26 00:34:09]

Incorporated your feedback and fixed a couple of bugs around setting maximized
state and removing window.

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_l...
File ash/wm/workspace/workspace_layout_manager.cc (right):

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_l...
ash/wm/workspace/workspace_layout_manager.cc:108: SetChildBoundsDirect(child,
requested_bounds);
On 2012/01/25 23:21:12, oshima wrote:
> What is the path to set a window's initial size is set? (assuming that in
normal
> workspace, we honor requested size)

Typically the initial size is set before the window is visible, in which case it
is allowed through as the window isn't visible.

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:81: return;  // Already know about this
window.
On 2012/01/25 23:21:12, oshima wrote:
> When this can happen?

Because all windows not on the active workspace get the layer hidden, it means
Show ends up triggering WorkspaceLayoutManager::OnChildWindowVisibilityChanged
and we end up here.

http://codereview.chromium.org/9113045/diff/4001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:90: if
(window_util::IsWindowMaximized(window) && !GetRestoreBounds(window))
On 2012/01/25 23:21:12, oshima wrote:
> don't we have to check full screen?

I added a TODO. I haven't dealt well with fullscreen in this patch.

[sky, 2012-01-26 00:34:19]

And I added tests for the cases I missed.

[oshima, 2012-01-26 16:37:25]

http://codereview.chromium.org/9113045/diff/5024/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

http://codereview.chromium.org/9113045/diff/5024/ash/wm/window_util.cc#newcode17
ash/wm/window_util.cc:17: static const char kOpenWindowSplitKey[] =
"OpenWindowSplit";
move this to anonymous namespace.

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:35: // Sets the visibility of the layer of
each window in |windows| to |value|.
I guess we set visibility on layer so that we treat visibility hasn't changed
(thus, no visibility notification etc), but can you add explanation why here?

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:41: }
Don't we have to change the visibility of transient children?

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:85: if
(!window->GetProperty(aura::client::kShowStateKey)) {
window->HasProperty

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:229: return workspace;
return new Workspace(this);

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:276: // is non-NULL.
It's possible, but it shouldn't happen, should it? DCHECK(!ignored_window_) ?

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:333: SetActiveWorkspace(FindBy(window));
Can this be
SetActiveWorksace(current_workspace)
?

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager_unittest.cc:87: 
EXPECT_FALSE(manager_->FindBy(w1.get()) , correct?

[sky, 2012-01-26 16:51:22]

I've incorporated all your feedback and uploaded new patch.

Thanks,

  -Scott

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:229: return workspace;
On 2012/01/26 16:37:25, oshima wrote:
> return new Workspace(this);

Probably not worth a method now. I removed it entirely.

http://codereview.chromium.org/9113045/diff/5024/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:276: // is non-NULL.
On 2012/01/26 16:37:25, oshima wrote:
> It's possible, but it shouldn't happen, should it? DCHECK(!ignored_window_) ?

It's possible with the current WorkspaceLayoutManager code. I need to straighten
it out before I add the DCHECK, hence the TODO. I'll fix it in a later patch.

[oshima_google, 2012-01-26 16:58:04]

LGTM

[oshima, 2012-01-26 16:59:44]

LGTM,  from right account.

[Ben Goodger (Google), 2012-01-26 17:03:53]

lgtm

http://codereview.chromium.org/9113045/diff/4008/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.h (right):

http://codereview.chromium.org/9113045/diff/4008/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.h:32: class ASH_EXPORT WorkspaceManager :
public aura::WindowObserver{
nit: space between r and {