Ensure that the first browser window which is created in Windows 8 ASH is maximized.

Ensure that the first browser window which is created in Windows 8 ASH is maximized.

This is done via the Ash WindowPositioner class. The change is in the WindowPositioner::GetBoundsAndShowStateForNewWindow
function which for windows returns SHOW_STATE_MAXIMIZED if this is the first window being opened.

Based on a discussion with oshima, I enabled the WindowSizerAshTest ash unit tests for Windows. There were a number
of places in the WindowSizer class where we were allowing browsers to be in ASH only for Chrome OS. Checking for
the host desktop type should be enough. This was needed to get the window to be maximized for ASH.

Fixes bug http://crbug.com/231603

BUG=231603
R=cpu@chromium.org, oshima@chromium.org, sky@chromium.org, cpu, oshima, sky

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

[cpu_(ooo_6.6-7.5), 2013-11-10 01:57:25]

this only maximizes on ash/metro right?

[sky, 2013-11-11 15:32:16]

I think this is the wrong place for this code. I think this is better done in
WindowSizer.

[sky, 2013-11-11 16:00:42]

Oshima has been centralizing some of the window sizing code ash. He might have a
suggestion for a better place for this code other than WindowSizer.

+oshima

[oshima, 2013-11-11 17:23:23]

On 2013/11/11 16:00:42, sky wrote:
> Oshima has been centralizing some of the window sizing code ash. He might have
a
> suggestion for a better place for this code other than WindowSizer.
> 
> +oshima

Assuming that the session restore should still restore windows on metro too,
WindowPositioner::GetBoundsAndSHowStateForNewWindow in
ash/wm/window_positioner.cc has
a logic to maximize the first window opened without saved bounds (which is
typically
the very first window opened in cros, and I assume it's same in metro mode).
You can adjust the force manimized width limit for metro mode so that it'll
always
open in maximized mode.

[cpu_(ooo_6.6-7.5), 2013-11-12 00:56:15]

so you are saying that we want to keep state in the WindowPositioner to keep
track if it is the first window? 

Or you mean that for every top level window which has no saved size show it
maximized every time?

[oshima, 2013-11-12 01:27:08]

On 2013/11/12 00:56:15, cpu wrote:
> so you are saying that we want to keep state in the WindowPositioner to keep
> track if it is the first window? 
> 
> Or you mean that for every top level window which has no saved size show it
> maximized every time?

Yes, this is how chromeos opens the very first window in the first login session
in
maximized state. All other new windows will have the saved state, AFAIK.

[ananta, 2013-11-12 01:40:46]

Updated the patch to use the WindowPositioner to return
SHOW_STATE_MAXIMIZED for the first window for Windows ASH

[oshima, 2013-11-12 01:43:55]

change lgtm, but this will probably break
WindowSizerTestWithBrowser::PlaceNewBrowserWindowOnEmptyDesktop on win8. Can you
test and update the test for win8?

[ananta, 2013-11-14 00:39:21]

On 2013/11/12 01:43:55, oshima wrote:
> change lgtm, but this will probably break
> WindowSizerTestWithBrowser::PlaceNewBrowserWindowOnEmptyDesktop on win8. Can
you
> test and update the test for win8?

Please take a look at the updated patch. It has the window sizing ash unittests
enabled for windows
and some changes to support this.

[oshima, 2013-11-14 00:41:52]

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer.cc:401: // TODO(beng): insufficient
but currently necessary. http://crbug.com/133312
we can remove this comment now?

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer.cc:408: // TODO(beng): insufficient
but currently necessary. http://crbug.com/133312
ditto

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:96:
CommandLine::ForCurrentProcess()->AppendSwitch(switches::kOpenAsh);
I wonder if we should do this in AshTestBase. WDYT?

[ananta, 2013-11-14 01:45:25]

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer.cc:401: // TODO(beng): insufficient
but currently necessary. http://crbug.com/133312
On 2013/11/14 00:41:53, oshima wrote:
> we can remove this comment now?

Done.

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer.cc:408: // TODO(beng): insufficient
but currently necessary. http://crbug.com/133312
On 2013/11/14 00:41:53, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:96:
CommandLine::ForCurrentProcess()->AppendSwitch(switches::kOpenAsh);
On 2013/11/14 00:41:53, oshima wrote:
> I wonder if we should do this in AshTestBase. WDYT?
Good point. Done

[ananta, 2013-11-14 01:53:50]

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/62333015/diff/740003/chrome/browser/ui/window...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:96:
CommandLine::ForCurrentProcess()->AppendSwitch(switches::kOpenAsh);
On 2013/11/14 01:45:25, ananta wrote:
> On 2013/11/14 00:41:53, oshima wrote:
> > I wonder if we should do this in AshTestBase. WDYT?
> Good point. Done
> 
Leaving this as is as the OpenAsh switch is defined in chrome\common

[oshima, 2013-11-14 19:53:38]

ok lgtm

[cpu_(ooo_6.6-7.5), 2013-11-14 20:20:25]

lgtm

https://codereview.chromium.org/62333015/diff/770007/ash/wm/window_positioner.cc
File ash/wm/window_positioner.cc (right):

https://codereview.chromium.org/62333015/diff/770007/ash/wm/window_positioner...
ash/wm/window_positioner.cc:241: #if !defined(OS_WIN)
can you add a brief comment about why we don't do this fir win?

[ananta, 2013-11-14 20:39:00]

https://codereview.chromium.org/62333015/diff/770007/ash/wm/window_positioner.cc
File ash/wm/window_positioner.cc (right):

https://codereview.chromium.org/62333015/diff/770007/ash/wm/window_positioner...
ash/wm/window_positioner.cc:241: #if !defined(OS_WIN)
On 2013/11/14 20:20:26, cpu wrote:
> can you add a brief comment about why we don't do this fir win?

Done.

[sky, 2013-11-15 16:53:44]

https://codereview.chromium.org/62333015/diff/1510001/ash/wm/window_positione...
File ash/wm/window_positioner.cc (right):

https://codereview.chromium.org/62333015/diff/1510001/ash/wm/window_positione...
ash/wm/window_positioner.cc:245: work_area.width() <=
GetForceMaximizedWidthLimit() &&
Can this be expressed as a property rather than a ifdef? Much easier to test and
read.

[ananta, 2013-11-15 19:55:16]

https://codereview.chromium.org/62333015/diff/1510001/ash/wm/window_positione...
File ash/wm/window_positioner.cc (right):

https://codereview.chromium.org/62333015/diff/1510001/ash/wm/window_positione...
ash/wm/window_positioner.cc:245: work_area.width() <=
GetForceMaximizedWidthLimit() &&
On 2013/11/15 16:53:44, sky wrote:
> Can this be expressed as a property rather than a ifdef? Much easier to test
and
> read.

Done. Added a property kMaximizeFirstWindow to ash/wm/window_properties.h. This
is set in the ChromeMetroViewerProcessHost::OnSetTargetSurface function.

[sky, 2013-11-16 00:25:05]

Sorry I wasn't clear. I wasn't thinking a windows property but just a static
bool some where. Maybe WindowPositioner or WindowSizer. Oshima likely has more
of an opinion as to where would be a good place.

[ananta, 2013-11-16 00:56:46]

On 2013/11/16 00:25:05, sky wrote:
> Sorry I wasn't clear. I wasn't thinking a windows property but just a static
> bool some where. Maybe WindowPositioner or WindowSizer. Oshima likely has more
> of an opinion as to where would be a good place.

Done. Added a static accessor SetMaximizeFirstWindow to the WindowPositioner
class and used it from
ChromeMetroViewerProcessHost::OnSetTargetSurface and in the WindowPositioner
unittest.

[sky, 2013-11-18 14:27:14]

LGTM

[oshima, 2013-11-19 17:58:26]

https://codereview.chromium.org/62333015/diff/2180001/ash/wm/window_positione...
File ash/wm/window_positioner.cc (right):

https://codereview.chromium.org/62333015/diff/2180001/ash/wm/window_positione...
ash/wm/window_positioner.cc:246: (!new_window ||
!wm::GetWindowState(new_window)->IsFullscreen())))
why not single if?

[ananta, 2013-11-19 19:37:10]

https://codereview.chromium.org/62333015/diff/2180001/ash/wm/window_positione...
File ash/wm/window_positioner.cc (right):

https://codereview.chromium.org/62333015/diff/2180001/ash/wm/window_positione...
ash/wm/window_positioner.cc:246: (!new_window ||
!wm::GetWindowState(new_window)->IsFullscreen())))
On 2013/11/19 17:58:26, oshima wrote:
> why not single if?
> 

Done.

[oshima, 2013-11-19 19:41:12]

lgtm

[commit-bot: I haz the power, 2013-11-19 23:45:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ananta@chromium.org/62333015/30002

[commit-bot: I haz the power, 2013-11-19 23:45:07]

Failed to apply patch for ash/test/ash_test_base.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file ash/test/ash_test_base.cc
  Hunk #2 FAILED at 129.
  1 out of 2 hunks FAILED -- saving rejects to file
ash/test/ash_test_base.cc.rej

Patch:       ash/test/ash_test_base.cc
Index: ash/test/ash_test_base.cc
===================================================================
--- ash/test/ash_test_base.cc	(revision 235803)
+++ ash/test/ash_test_base.cc	(working copy)
@@ -17,6 +17,7 @@
 #include "ash/test/test_session_state_delegate.h"
 #include "ash/test/test_shell_delegate.h"
 #include "ash/wm/coordinate_conversion.h"
+#include "ash/wm/window_positioner.h"
 #include "base/command_line.h"
 #include "content/public/test/web_contents_tester.h"
 #include "ui/aura/client/aura_constants.h"
@@ -128,21 +129,23 @@
   ash::Shell::GetInstance()->cursor_manager()->EnableMouseEvents();
 
 #if defined(OS_WIN)
-  if (base::win::GetVersion() >= base::win::VERSION_WIN8 &&
-      !command_line->HasSwitch(ash::switches::kForceAshToDesktop)) {
-    ipc_thread_.reset(new base::Thread("test_metro_viewer_ipc_thread"));
-    base::Thread::Options options;
-    options.message_loop_type = base::MessageLoop::TYPE_IO;
-    ipc_thread_->StartWithOptions(options);
+  if (!command_line->HasSwitch(ash::switches::kForceAshToDesktop)) {
+    if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
+      ipc_thread_.reset(new base::Thread("test_metro_viewer_ipc_thread"));
+      base::Thread::Options options;
+      options.message_loop_type = base::MessageLoop::TYPE_IO;
+      ipc_thread_->StartWithOptions(options);
 
-    metro_viewer_host_.reset(
-        new TestMetroViewerProcessHost(ipc_thread_->message_loop_proxy()));
-    CHECK(metro_viewer_host_->LaunchViewerAndWaitForConnection(
-        win8::test::kDefaultTestAppUserModelId,
-        L"test_open"));
-    aura::RemoteRootWindowHostWin* root_window_host =
-        aura::RemoteRootWindowHostWin::Instance();
-    CHECK(root_window_host != NULL);
+      metro_viewer_host_.reset(
+          new TestMetroViewerProcessHost(ipc_thread_->message_loop_proxy()));
+      CHECK(metro_viewer_host_->LaunchViewerAndWaitForConnection(
+          win8::test::kDefaultTestAppUserModelId,
+          L"test_open"));
+      aura::RemoteRootWindowHostWin* root_window_host =
+          aura::RemoteRootWindowHostWin::Instance();
+      CHECK(root_window_host != NULL);
+    }
+    ash::WindowPositioner::SetMaximizeFirstWindow(true);
   }
 #endif
 }

[ananta, 2013-11-20 04:05:25]

Committed patchset #18 manually as r236134 (presubmit successful).