Adding experimental maximize mode (behind a flag)

Adding experimental maximize mode (behind a flag)

This experimental feature is behind a flag and allows our user interface designers to experiment with an always maximized window mode. It also removes certain context menu options which make no sense in that mode anymore.

No goals for this CL are:
  - The minimal / maximal window size properties of V2 apps will get ignored, and the apps will always get maximized.
  - Rather then not creating buttons in the first place, they only get hidden to keep the changes to the code to a minimum.

Note: Since this feature is entirely experimental + behind a flag + subject to change + does only contain minor behavioral changes, there are no unit tests for this at this time.

BUG=230510
TEST=visual (browser, hosted apps, V1 apps)

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

[Mr4D (OOO till 08-26), 2013-04-22 20:01:28]

Please have a look!

[James Cook, 2013-04-22 22:15:46]

https://codereview.chromium.org/13934007/diff/2001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/13934007/diff/2001/ash/ash_switches.cc#newcod...
ash/ash_switches.cc:131: const char kForceMaximizeMode[] =
"force_maximize_mode";
force_maximize_mode -> force-maximize-mode

https://codereview.chromium.org/13934007/diff/2001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/13934007/diff/2001/ash/shell.h#newcode204
ash/shell.h:204: static bool IsForcedMaximizeMode();
Either this should be IsForceMaximizeMode() or the flag should be
forced-maximize-mode

https://codereview.chromium.org/13934007/diff/2001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6896: +        Force a "work always maximized
mode
Maybe call this "Always maximized mode"?  Regardless, there's an extra " here.

https://codereview.chromium.org/13934007/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6899: +        Forces the WM to use maximize
mode wherever possible.
WM -> window manager

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:997: "force_maximize_mode",
hyphens

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.cc:223: else
This feels awkward. Is there some other way to write it?  Maybe like the
ShouldForceFullscreenApp() thing below?

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:1043: #endif
#ifdefs that mess with indentation I find hard to read. How about:
#if defined(USE_ASH)
  if (ash::Shell:IsForceMaximizeMode())
    show_state = ui::SHOW_STATE_MAXIMIZED;
#endif

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:90:
!ash::Shell::IsForcedMaximizeMode()) {
Don't you want to do this below, not here?

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:49:
original_maximize_button_width_(0)
If we're going to do this mode, I think you should make this code (and other
frames) support having a NULL restore_button_.

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:163: // it can be maximized
and we want that feature.
Should you be testing if the window can be maximized, or is the comment wrong?

[Mr4D (OOO till 08-26), 2013-04-23 04:41:28]

Please have another look!

https://codereview.chromium.org/13934007/diff/2001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/13934007/diff/2001/ash/ash_switches.cc#newcod...
ash/ash_switches.cc:131: const char kForceMaximizeMode[] =
"force_maximize_mode";
On 2013/04/22 22:15:46, James Cook (Chromium) wrote:
> force_maximize_mode -> force-maximize-mode

Done.

https://codereview.chromium.org/13934007/diff/2001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/13934007/diff/2001/ash/shell.h#newcode204
ash/shell.h:204: static bool IsForcedMaximizeMode();
On 2013/04/22 22:15:46, James Cook (Chromium) wrote:
> Either this should be IsForceMaximizeMode() or the flag should be
> forced-maximize-mode

Done.

https://codereview.chromium.org/13934007/diff/2001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6896: +        Force a "work always maximized
mode
On 2013/04/22 22:15:46, James Cook (Chromium) wrote:
> Maybe call this "Always maximized mode"?  Regardless, there's an extra " here.

Done.

https://codereview.chromium.org/13934007/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6899: +        Forces the WM to use maximize
mode wherever possible.
On 2013/04/22 22:15:46, James Cook (Chromium) wrote:
> WM -> window manager

Done.

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:997: "force_maximize_mode",
On 2013/04/22 22:15:46, James Cook (Chromium) wrote:
> hyphens

Done.

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_api.cc:223: else
Well - I "improved" it, but now it is distributed over the function and harder
to read. We cannot do it as ShouldForce since otherwise we would need to carry
the information about the panel down there (and it is always better to create it
in the state we want then patching it afterwards).

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/sessions/se...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/sessions/se...
chrome/browser/sessions/session_restore.cc:1043: #endif
I tried to avoid that. However since you want it: Done.

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:90:
!ash::Shell::IsForcedMaximizeMode()) {
Good catch! There was a fore and back on this one. Done.

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:49:
original_maximize_button_width_(0)
Right. As stated in my CL comment: This was a no goal for this time. Somehow I
have the feeling that we do not want this mode ;). But if you think that this is
essential I will of course add the logic (but note that this might have much
spread consequences).

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:163: // it can be maximized
and we want that feature.
Actually: No. We maximize all windows, unless the owner specified that he does
not want it. Note also that this will only be called for browser windows/v1 apps
and they all get maximized.
Comment changed anyways a bit.

[James Cook, 2013-04-23 18:27:32]

LGTM but please file a bug for the restore/maximize buttons.

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:49:
original_maximize_button_width_(0)
On 2013/04/23 04:41:28, Mr4D wrote:
> Right. As stated in my CL comment: This was a no goal for this time. Somehow I
> have the feeling that we do not want this mode ;). But if you think that this
is
> essential I will of course add the logic (but note that this might have much
> spread consequences).

OK. Keep the button hiding the way you have it, but please file a bug for
converting this to skip the construction of the button if we decide to keep this
mode and reference it in a comment.

https://codereview.chromium.org/13934007/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/13934007/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:109: bool
force_maximize = ash::Shell::IsForcedMaximizeMode();
I like this better - less strange ifdef-induced indentation.

[Mr4D (OOO till 08-26), 2013-04-23 22:24:00]

Added comment and submitted. As discussed I added a TODO for this case.

Thanks for your review!

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/13934007/diff/2001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:49:
original_maximize_button_width_(0)
Added a TODO(skuhne) comment here to make sure it gets properly changed!

https://codereview.chromium.org/13934007/diff/15001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/13934007/diff/15001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:109: bool
force_maximize = ash::Shell::IsForcedMaximizeMode();
Right - but it is adding code which is only needed for USE_ASH and which cannot
be clearly identified as such. But - I agree that it looks better!

[commit-bot: I haz the power, 2013-04-23 22:25:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/13934007/34001

[commit-bot: I haz the power, 2013-04-23 22:25:41]

Presubmit check for 13934007-34001 failed and returned exit status 1.

INFO:root:Found 16 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/extensions/api/app_window/app_window_api.cc
    chrome/browser/extensions/extension_prefs.cc
    chrome/browser/sessions/session_restore.cc
    chrome/browser/ui/app_list/extension_app_item.cc
    chrome/browser/ui/window_sizer/window_sizer_ash.cc

Presubmit checks took 1.7s to calculate.

[Mr4D (OOO till 08-26), 2013-04-23 22:33:26]

Please do an owners review for 

chrome/browser/extensions/api/app_window/app_window_api.cc
chrome/browser/extensions/extension_prefs.cc
chrome/browser/sessions/session_restore.cc
chrome/browser/ui/app_list/extension_app_item.cc
chrome/browser/ui/window_sizer/window_sizer_ash.cc

Thanks in advance!

[sky, 2013-04-24 00:31:59]

https://codereview.chromium.org/13934007/diff/34001/chrome/browser/sessions/s...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/13934007/diff/34001/chrome/browser/sessions/s...
chrome/browser/sessions/session_restore.cc:1046: if
(ash::Shell::IsForcedMaximizeMode())
It seems like we shouldn't have to patch all the call sites, instead the window
manager should force all 'interesting' windows to be maximized. By interesting
windows I mean presumably there will be some windows that aren't maximized (say
dialogs).

[Mr4D (OOO till 08-26), 2013-04-29 17:49:04]

Please have another look!

[sky, 2013-04-29 20:36:53]

https://codereview.chromium.org/13934007/diff/41001/ash/wm/property_util.h
File ash/wm/property_util.h (right):

https://codereview.chromium.org/13934007/diff/41001/ash/wm/property_util.h#ne...
ash/wm/property_util.h:75: ASH_EXPORT void
SetCanBeMaximizedByWorkspace(aura::Window* window, bool value);
This is roughly what I was thinking. But CanBeMazimized is specific to the
current case. What I think you want is:

enum WorkspaceWindowType {
  // Windows of this type have their size and location managed in some way.
  WORKSPACE_WINDOW_TYPE_MANAGED,

  // Windows of this type float across all workspaces.
  WORKSPACE_WINDOW_TYPE_PERSISTS_ACROSS_ALL_WORKSPACES,

  // Windows of this type are completely unmanaged by the workspace code.
  WORKSPACE_WINDOW_TYPE_UNMANAGED,
};

WORKSPACES_WINDOW_TYPE_PERSISTS_ACROSS_ALL_WORKSPACES replaces
SetPersistsAcrossAllWorkspaces.
WORKSPACE_WINDOW_TYPE_UNMANAGED replaces SetTrackedByWorkspace.

In addition to a setter/getter you'll need a SetDefaultWorkspaceWindowType which
replaces SetDefaultPersistsAcrossAllWorkspaces.

[Mr4D (OOO till 08-26), 2013-05-01 01:08:05]

As discussed - reworked.

I might have missed something (since the day was long) and will have another
pass through it tomorrow, but since I will be in later you can have a first stab
at it - if you want.

Thanks!

[sky, 2013-05-01 04:32:03]

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:314: if
(ash::Shell::IsForcedMaximizeMode())
Is there a reason you went this route rather than SetVisible(false) in
CustomFrameViewAsh? AFAICT that is the only place that changes the visibility.

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:174: if (key ==
ash::internal::kWindowPersistsAcrossAllWorkspacesKey &&
I don't think you intended to keep this.

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:264: if
(force_maximize)
And I don't think you wanted this either.

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.cc:2510:
ash::SetPersistsAcrossAllWorkspaces(view->GetNativeWindow(),
And nuke this too.

[Mr4D (OOO till 08-26), 2013-05-01 17:57:23]

Removed the flag usage again. Please have another look!

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:314: if
(ash::Shell::IsForcedMaximizeMode())
There are multiple frames which use this button (app_non_client_frame_view_ash,
browser_non_client_frame_view_ash, custom_frame_view_ash. Furthermore who knows
who else will add this. Rooting it out at this point ensures that it is gone for
all (for this experiment).

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:174: if (key ==
ash::internal::kWindowPersistsAcrossAllWorkspacesKey &&
Ahh. No problem. Removed and reverted back to what I originally had.

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:264: if
(force_maximize)
On 2013/05/01 04:32:04, sky wrote:
> And I don't think you wanted this either.

Done.

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/13934007/diff/58001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.cc:2510:
ash::SetPersistsAcrossAllWorkspaces(view->GetNativeWindow(),
On 2013/05/01 04:32:04, sky wrote:
> And nuke this too.

Done.

[sky, 2013-05-01 21:01:56]

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:314: if
(ash::Shell::IsForcedMaximizeMode())
On 2013/05/01 17:57:23, Mr4D wrote:
> There are multiple frames which use this button
(app_non_client_frame_view_ash,
> browser_non_client_frame_view_ash, custom_frame_view_ash. Furthermore who
knows
> who else will add this. Rooting it out at this point ensures that it is gone
for
> all (for this experiment).

Which code is easier to understand, what you have here, or at the call site:

if (!ShouldMaximize())
  button_->SetVisible(true);

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:110: return
gfx::Size(shadow_->width() - original_maximize_button_width_,
Can you use restore_button_->GetPreferredSize().width() here?

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:164: if ((*show_state ==
ui::SHOW_STATE_DEFAULT || browser_) &&
Just to be sure, popups don't hit this code path, right?

[Mr4D (OOO till 08-26), 2013-05-01 22:48:49]

Please have another look!

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/13934007/diff/58001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:314: if
(ash::Shell::IsForcedMaximizeMode())
This makes sure that no one ever will shows this button - purposely (e.g. when
switching from maximize button to "restore" button or what other cases may
exist). If it wouldn't be for that, this code could go.
In regards of line 95/96 - it is better to have this logic in one central place
instead of 3 places.
If you think that we should comb out any "reactivation" (now and - maybe in the
future) I will remove this.

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/app_non_client_frame_view_ash.cc:110: return
gfx::Size(shadow_->width() - original_maximize_button_width_,
Yepp - it works. Done.

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

https://codereview.chromium.org/13934007/diff/77001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:164: if ((*show_state ==
ui::SHOW_STATE_DEFAULT || browser_) &&
Tests said it would not, but I looked at the code and have seen that it could
come there if it does not specify a target size (unlikely but not impossible). I
added therefore another special case here.

[sky, 2013-05-02 00:34:44]

LGTM

[commit-bot: I haz the power, 2013-05-02 00:36:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/13934007/102001

[commit-bot: I haz the power, 2013-05-02 02:56:47]

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

[commit-bot: I haz the power, 2013-05-02 03:18:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/13934007/102001

[commit-bot: I haz the power, 2013-05-02 07:53:49]

Change committed as 197856