Tweaks to show state:

. NativeWidgetAura now sets the true show state before added to a parent. This
  seems ideal so that the LayoutManager doesn't end up resizing the window.
. I removed NativeWidgetAura's call to OnNativeWidgetSizeChanged. This shouldn't
  be necessary as when the Window's bounds change OnNativeWidgetSizeChanged ends
  up being invoked.
. Browser::GetSavedWindowShowState can now return ui::SHOW_STATE_DEFAULT.
  I need this to tell the difference explicitly wanting NORMAL, and
  this is a new window so do whatever you want. I checked all callers and as
  far as I can tell they all deal with DEFAULT fine.

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

Review URL: https://chromiumcodereview.appspot.com/9121036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@119460 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 124 insertions, 14 deletions

diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 83be1e9..aaedd81 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -951,10 +951,8 @@ gfx::Rect Browser::GetSavedWindowBounds() const {
 
 ui::WindowShowState Browser::GetSavedWindowShowState() const {
   // Only tabbed browsers use the command line or preference state.
-  if (!is_type_tabbed()) {
-    return (show_state_ == ui::SHOW_STATE_DEFAULT)
-        ? ui::SHOW_STATE_NORMAL : show_state_;
-  }
+  if (!is_type_tabbed())
+    return show_state_;
 
   if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kStartMaximized))
     return ui::SHOW_STATE_MAXIMIZED;
@@ -967,7 +965,7 @@ ui::WindowShowState Browser::GetSavedWindowShowState() const {
   bool maximized = false;
   window_pref->GetBoolean("maximized", &maximized);
 
-  return maximized ? ui::SHOW_STATE_MAXIMIZED : ui::SHOW_STATE_NORMAL;
+  return maximized ? ui::SHOW_STATE_MAXIMIZED : ui::SHOW_STATE_DEFAULT;
 }
 
 SkBitmap Browser::GetCurrentPageIcon() const {
diff --git a/chrome/browser/ui/browser_unittest.cc b/chrome/browser/ui/browser_unittest.cc
new file mode 100644
index 0000000..2995582
--- /dev/null
+++ b/chrome/browser/ui/browser_unittest.cc
@@ -0,0 +1,25 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/browser.h"
+
+#include "chrome/test/base/browser_with_test_window_test.h"
+
+typedef BrowserWithTestWindowTest BrowserTest;
+
+// Various assertions around setting show state.
+TEST_F(BrowserTest, GetSavedWindowShowState) {
+  // Default show state is SHOW_STATE_DEFAULT.
+  EXPECT_EQ(ui::SHOW_STATE_DEFAULT, browser()->GetSavedWindowShowState());
+
+  // Explicitly specifying a state should stick though.
+  browser()->set_show_state(ui::SHOW_STATE_MAXIMIZED);
+  EXPECT_EQ(ui::SHOW_STATE_MAXIMIZED, browser()->GetSavedWindowShowState());
+  browser()->set_show_state(ui::SHOW_STATE_NORMAL);
+  EXPECT_EQ(ui::SHOW_STATE_NORMAL, browser()->GetSavedWindowShowState());
+  browser()->set_show_state(ui::SHOW_STATE_MINIMIZED);
+  EXPECT_EQ(ui::SHOW_STATE_MINIMIZED, browser()->GetSavedWindowShowState());
+  browser()->set_show_state(ui::SHOW_STATE_FULLSCREEN);
+  EXPECT_EQ(ui::SHOW_STATE_FULLSCREEN, browser()->GetSavedWindowShowState());
+}
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 5c4328a..c7cb4e3 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1763,6 +1763,7 @@
         'browser/themes/browser_theme_pack_unittest.cc',
         'browser/themes/theme_service_unittest.cc',
         'browser/ui/browser_list_unittest.cc',
+        'browser/ui/browser_unittest.cc',
         # It is safe to list */cocoa/* files in the "common" file list
         # without an explicit exclusion since gyp is smart enough to
         # exclude them from non-Mac builds.
diff --git a/ui/views/widget/native_widget_aura.cc b/ui/views/widget/native_widget_aura.cc
index 57026ea..a0220ad 100644
--- a/ui/views/widget/native_widget_aura.cc
+++ b/ui/views/widget/native_widget_aura.cc
@@ -89,6 +89,16 @@ void CloseAllSecondaryWidgetsCallback(Widget* widget) {
     widget->Close();
 }
 
+const gfx::Rect* GetRestoreBounds(aura::Window* window) {
+  return reinterpret_cast<gfx::Rect*>(
+      window->GetProperty(aura::client::kRestoreBoundsKey));
+}
+
+void SetRestoreBounds(aura::Window* window, const gfx::Rect& bounds) {
+  delete GetRestoreBounds(window);
+  window->SetProperty(aura::client::kRestoreBoundsKey, new gfx::Rect(bounds));
+}
+
 }  // namespace
 
 // Used when SetInactiveRenderingDisabled() is invoked to track when active
@@ -162,8 +172,7 @@ void NativeWidgetAura::InitNativeWidget(const Widget::InitParams& params) {
   ownership_ = params.ownership;
   window_->set_user_data(this);
   window_->SetType(GetAuraWindowTypeForWidgetType(params.type));
-  // TODO(jamescook): Should this use params.show_state instead?
-  window_->SetIntProperty(aura::client::kShowStateKey, ui::SHOW_STATE_NORMAL);
+  window_->SetIntProperty(aura::client::kShowStateKey, params.show_state);
   window_->SetTransparent(params.transparent);
   window_->Init(params.create_texture_for_layer ?
                     ui::Layer::LAYER_HAS_TEXTURE :
@@ -172,7 +181,6 @@ void NativeWidgetAura::InitNativeWidget(const Widget::InitParams& params) {
     window_->Show();
 
   delegate_->OnNativeWidgetCreated();
-  window_->SetBounds(params.bounds);
   if (params.child) {
     window_->SetParent(params.GetParent());
   } else {
@@ -187,9 +195,14 @@ void NativeWidgetAura::InitNativeWidget(const Widget::InitParams& params) {
     SetAlwaysOnTop(params.keep_on_top);
     window_->SetParent(parent);
   }
+  // Wait to set the bounds until we have a parent. That way we can know our
+  // true state/bounds (the LayoutManager may enforce a particular
+  // state/bounds).
+  if (IsMaximized())
+    SetRestoreBounds(window_, params.bounds);
+  else
+    window_->SetBounds(params.bounds);
   window_->set_ignore_events(!params.accept_events);
-  // TODO(beng): do this some other way.
-  delegate_->OnNativeWidgetSizeChanged(params.bounds.size());
   can_activate_ = params.can_activate;
   DCHECK(GetWidget()->GetRootView());
 #if !defined(OS_MACOSX)
@@ -436,7 +449,7 @@ void NativeWidgetAura::Hide() {
 
 void NativeWidgetAura::ShowMaximizedWithBounds(
     const gfx::Rect& restored_bounds) {
-  window_->SetBounds(restored_bounds);
+  SetRestoreBounds(window_, restored_bounds);
   ShowWithWindowState(ui::SHOW_STATE_MAXIMIZED);
 }
 
diff --git a/ui/views/widget/native_widget_aura_unittest.cc b/ui/views/widget/native_widget_aura_unittest.cc
index 3f1427b..91e02a3 100644
--- a/ui/views/widget/native_widget_aura_unittest.cc
+++ b/ui/views/widget/native_widget_aura_unittest.cc
@@ -1,16 +1,18 @@
-
 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/widget/native_widget_aura.h"
 
-#include "ui/aura/root_window.h"
-#include "ui/aura/window.h"
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/layout_manager.h"
+#include "ui/aura/root_window.h"
+#include "ui/aura/window.h"
+#include "ui/views/widget/root_view.h"
+#include "ui/views/widget/widget_delegate.h"
 
 namespace views {
 namespace {
@@ -75,6 +77,77 @@ TEST_F(NativeWidgetAuraTest, CenterWindowSmallParent) {
   widget->CloseNow();
 }
 
+// Used by ShowMaximizedDoesntBounceAround. See it for details.
+class TestLayoutManager : public aura::LayoutManager {
+ public:
+  TestLayoutManager() {}
+
+  virtual void OnWindowResized() OVERRIDE {
+  }
+  virtual void OnWindowAddedToLayout(aura::Window* child) OVERRIDE {
+    // This simulates what happens when adding a maximized window.
+    SetChildBoundsDirect(child, gfx::Rect(0, 0, 300, 300));
+  }
+  virtual void OnWillRemoveWindowFromLayout(aura::Window* child) OVERRIDE {
+  }
+  virtual void OnChildWindowVisibilityChanged(aura::Window* child,
+                                              bool visible) OVERRIDE {
+  }
+  virtual void SetChildBounds(aura::Window* child,
+                              const gfx::Rect& requested_bounds) OVERRIDE {
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestLayoutManager);
+};
+
+// This simulates BrowserView, which creates a custom RootView so that
+// OnNativeWidgetSizeChanged that is invoked during Init matters.
+class TestWidget : public views::Widget {
+ public:
+  TestWidget() : did_size_change_more_than_once_(false) {
+  }
+
+  // Returns true if the size changes to a non-empty size, and then to another
+  // size.
+  bool did_size_change_more_than_once() const {
+    return did_size_change_more_than_once_;
+  }
+
+  virtual void OnNativeWidgetSizeChanged(const gfx::Size& new_size) OVERRIDE {
+    if (last_size_.IsEmpty())
+      last_size_ = new_size;
+    else if (!did_size_change_more_than_once_ && new_size != last_size_)
+      did_size_change_more_than_once_ = true;
+    Widget::OnNativeWidgetSizeChanged(new_size);
+  }
+
+ private:
+  bool did_size_change_more_than_once_;
+  gfx::Size last_size_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWidget);
+};
+
+// Verifies the size of the widget doesn't change more than once during Init if
+// the window ends up maximized. This is important as otherwise
+// RenderWidgetHostViewAura ends up getting resized during construction, which
+// leads to noticable flashes.
+TEST_F(NativeWidgetAuraTest, ShowMaximizedDoesntBounceAround) {
+  aura::RootWindow::GetInstance()->SetBounds(gfx::Rect(0, 0, 640, 480));
+  aura::RootWindow::GetInstance()->SetLayoutManager(new TestLayoutManager);
+  scoped_ptr<TestWidget> widget(new TestWidget());
+  Widget::InitParams params(Widget::InitParams::TYPE_WINDOW);
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.parent = NULL;
+  params.show_state = ui::SHOW_STATE_MAXIMIZED;
+  params.bounds = gfx::Rect(10, 10, 100, 200);
+  widget->Init(params);
+  aura::RootWindow::DeleteInstance();
+  EXPECT_FALSE(widget->did_size_change_more_than_once());
+  widget->CloseNow();
+}
+
 TEST_F(NativeWidgetAuraTest, GetClientAreaScreenBounds) {
   // Create a widget.
   Widget::InitParams params(Widget::InitParams::TYPE_WINDOW);