Do not create a workspace for a maximized window.

Do not create a workspace for a maximized window.

This CL keeps the existing behavior 'as-is' as far as possible. Thus
this doesn't clean-up several messed code like maximized window vs
apps window. It'll be done in further CLs if possible.

BUG=245345
R=jamescook@chromium.org, sky@chromium.org
TEST=covered by ash_unittests

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

[James Cook, 2013-06-06 15:38:00]

LGTM assuming comment addressed

Not for this CL, but I think that eventually I want fullscreen windows to *not*
create a workspace, and if another window is created or brought on top of the
fullscreen window, it restores.  Do you think that would let us eliminate some
of the workspace creation code?

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
File ash/wm/workspace/workspace_manager.cc (right):

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager.cc:725: if (wm::IsWindowMaximized(child)) {
Should fullscreen windows hide the desktop background?

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:152: return
(workspace->is_fullscreen() ? "M" : "") +
It's a little strange that fullscreen is "M". I would expect "F". :-)

[Jun Mukai, 2013-06-06 18:31:05]

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
File ash/wm/workspace/workspace_manager.cc (right):

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager.cc:725: if (wm::IsWindowMaximized(child)) {
On 2013/06/06 15:38:00, James Cook (Chromium) wrote:
> Should fullscreen windows hide the desktop background?

that will happen on SetActiveWorkspace()

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

https://codereview.chromium.org/16093036/diff/1/ash/wm/workspace/workspace_ma...
ash/wm/workspace/workspace_manager_unittest.cc:152: return
(workspace->is_fullscreen() ? "M" : "") +
On 2013/06/06 15:38:00, James Cook (Chromium) wrote:
> It's a little strange that fullscreen is "M". I would expect "F". :-)

Done.

[Jun Mukai, 2013-06-06 18:38:42]

On 2013/06/06 15:38:00, James Cook (Chromium) wrote:
> LGTM assuming comment addressed
> 
> Not for this CL, but I think that eventually I want fullscreen windows to
*not*
> create a workspace, and if another window is created or brought on top of the
> fullscreen window, it restores.  Do you think that would let us eliminate some
> of the workspace creation code?
> 

It could be, but I'm afraid that it still needs some code something similar to
WorkspaceManager (especially for animations), and I'm not sure how it'll be
cleaned up.

[James Cook, 2013-06-06 18:40:37]

On 2013/06/06 18:38:42, Jun Mukai wrote:
> On 2013/06/06 15:38:00, James Cook (Chromium) wrote:
> > LGTM assuming comment addressed
> > 
> > Not for this CL, but I think that eventually I want fullscreen windows to
> *not*
> > create a workspace, and if another window is created or brought on top of
the
> > fullscreen window, it restores.  Do you think that would let us eliminate
some
> > of the workspace creation code?
> > 
> 
> It could be, but I'm afraid that it still needs some code something similar to
> WorkspaceManager (especially for animations), and I'm not sure how it'll be
> cleaned up.

OK. Still LGTM.

[commit-bot: I haz the power, 2013-06-06 21:40:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/16093036/6001

[commit-bot: I haz the power, 2013-06-06 22:57:28]

Retried try job too often on linux_chromeos for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Jun Mukai, 2013-06-07 02:16:36]

The test failures were the real issues.
Fixed (and removed some unnecessary ones).

added sky as R=. could you take time? At least I need your help for the change
on c/b/ui/views.

[sky, 2013-06-07 16:50:45]

If we're going to do away with multiple workspaces than I think it would be
better to disable Workspace* entirely. Is there a reason you didn't want to do
that?

[Jun Mukai, 2013-06-07 17:51:09]

On 2013/06/07 16:50:45, sky wrote:
> If we're going to do away with multiple workspaces than I think it would be
> better to disable Workspace* entirely. Is there a reason you didn't want to do
> that?

Right now fullscreen windows are still using the workspace code. That code can
be removed (probably by creating FullscreenContainer), but I think it's better
to be done in another CL.

[sky, 2013-06-07 18:14:01]

What is it about fullscreen windows that we want them to have a
workspace rather than making them behave as though maximized?

On Fri, Jun 7, 2013 at 10:51 AM,  <mukai@chromium.org> wrote:
> On 2013/06/07 16:50:45, sky wrote:
>>
>> If we're going to do away with multiple workspaces than I think it would
>> be
>> better to disable Workspace* entirely. Is there a reason you didn't want
>> to do
>> that?
>
>
> Right now fullscreen windows are still using the workspace code. That code
> can
> be removed (probably by creating FullscreenContainer), but I think it's
> better
> to be done in another CL.
>
> https://chromiumcodereview.appspot.com/16093036/

[Jun Mukai, 2013-06-07 18:26:48]

On 2013/06/07 18:14:01, sky wrote:
> What is it about fullscreen windows that we want them to have a
> workspace rather than making them behave as though maximized?
> 

For example, when another window gets focused while a window is
fullscreen, the fullscreen one has to be restored and the desktop
workspace (which contains all other normal windows) will get focused.

This CL just removes that logic from maximized windows, but still needs
for fullscreen windows.

> On Fri, Jun 7, 2013 at 10:51 AM,  <mailto:mukai@chromium.org> wrote:
> > On 2013/06/07 16:50:45, sky wrote:
> >>
> >> If we're going to do away with multiple workspaces than I think it would
> >> be
> >> better to disable Workspace* entirely. Is there a reason you didn't want
> >> to do
> >> that?
> >
> >
> > Right now fullscreen windows are still using the workspace code. That code
> > can
> > be removed (probably by creating FullscreenContainer), but I think it's
> > better
> > to be done in another CL.
> >
> > https://chromiumcodereview.appspot.com/16093036/

[sky, 2013-06-07 19:44:35]

https://chromiumcodereview.appspot.com/16093036/diff/29001/ash/wm/workspace/w...
File ash/wm/workspace/workspace_manager_unittest.cc (left):

https://chromiumcodereview.appspot.com/16093036/diff/29001/ash/wm/workspace/w...
ash/wm/workspace/workspace_manager_unittest.cc:365:
w1->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MAXIMIZED);
I tend to think that for all these tests you need to replace MAXIMIZED with
FULL_SCREEN. Otherwise they aren't really testing what they were intended to be
testing.

[Jun Mukai, 2013-06-08 00:00:39]

https://codereview.chromium.org/16093036/diff/29001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager_unittest.cc (left):

https://codereview.chromium.org/16093036/diff/29001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager_unittest.cc:365:
w1->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MAXIMIZED);
On 2013/06/07 19:44:35, sky wrote:
> I tend to think that for all these tests you need to replace MAXIMIZED with
> FULL_SCREEN. Otherwise they aren't really testing what they were intended to
be
> testing.

Done.

[sky, 2013-06-10 15:28:53]

LGTM

https://codereview.chromium.org/16093036/diff/37001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

https://codereview.chromium.org/16093036/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager_unittest.cc:243: 
nit: remove this.

[Jun Mukai, 2013-06-10 20:10:49]

https://codereview.chromium.org/16093036/diff/37001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager_unittest.cc (right):

https://codereview.chromium.org/16093036/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager_unittest.cc:243: 
On 2013/06/10 15:28:53, sky wrote:
> nit: remove this.

Done.

[commit-bot: I haz the power, 2013-06-10 20:12:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/16093036/43001

[commit-bot: I haz the power, 2013-06-11 00:04:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-06-11 00:39:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/16093036/63001

[commit-bot: I haz the power, 2013-06-11 11:08:05]

Change committed as 205493