* Call LayoutManager::OnChildWindowVisibilityChanged first

because LayoutManager is responsible for updating the show state.
* Show the gained_window only if it wasn't visible.

While investigating DCHECK failure in BaseLayoutManager::OnWindowActivated (168383), I found the following:

a) The window's show status is managed and updated by WorkspaceLayoutManager.
b) OnWindowTargetVisibilityChanged sets the focus to the content area, which
   eventually activate the browser window.
c) b) invokes BaseLayoutManager::OnWindowActivated. At this point, the
  window's state is still minimized, but it's already shown. so showing it again
  doesn't change the state, thus hit DCHECK.

Strictly speaking, only base_layout_manager.cc change is necessary to fix
the DCHECK failure. I changed the OnChildWindowVisiblityChanged because
I think the delegate should see the latest state updated by the ash WM.

BUG=168383
TEST=covered by test.

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

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

2 files changed, 53 insertions, 1 deletions

diff --git a/ash/wm/base_layout_manager.cc b/ash/wm/base_layout_manager.cc
index 39beb97..8f4da18 100644
--- a/ash/wm/base_layout_manager.cc
+++ b/ash/wm/base_layout_manager.cc
@@ -156,7 +156,8 @@ void BaseLayoutManager::OnWindowDestroying(aura::Window* window) {
 void BaseLayoutManager::OnWindowActivated(aura::Window* gained_active,
                                           aura::Window* lost_active) {
   if (views::corewm::UseFocusController()) {
-    if (gained_active && wm::IsWindowMinimized(gained_active)) {
+    if (gained_active && wm::IsWindowMinimized(gained_active) &&
+        !gained_active->IsVisible()) {
       gained_active->Show();
       DCHECK(!wm::IsWindowMinimized(gained_active));
     }
diff --git a/ash/wm/base_layout_manager_unittest.cc b/ash/wm/base_layout_manager_unittest.cc
index 55fe957c..3093de9 100644
--- a/ash/wm/base_layout_manager_unittest.cc
+++ b/ash/wm/base_layout_manager_unittest.cc
@@ -76,6 +76,57 @@ TEST_F(BaseLayoutManagerTest, Minimize) {
   EXPECT_EQ(bounds.ToString(), window->bounds().ToString());
 }
 
+// A WindowDelegate which sets the focus when the window
+// becomes visible.
+class FocusDelegate : public aura::test::TestWindowDelegate {
+ public:
+  FocusDelegate()
+      : window_(NULL),
+        show_state_(ui::SHOW_STATE_END) {
+  }
+  virtual ~FocusDelegate() {}
+
+  void set_window(aura::Window* window) { window_ = window; }
+
+  // aura::test::TestWindowDelegate overrides:
+  virtual void OnWindowTargetVisibilityChanged(bool visible) {
+    if (window_) {
+      if (visible)
+        window_->Focus();
+      show_state_ = window_->GetProperty(aura::client::kShowStateKey);
+    }
+  }
+
+  ui::WindowShowState GetShowStateAndReset() {
+    ui::WindowShowState ret = show_state_;
+    show_state_ = ui::SHOW_STATE_END;
+    return ret;
+  }
+
+ private:
+  aura::Window* window_;
+  ui::WindowShowState show_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(FocusDelegate);
+};
+
+// Make sure that the window's show state is correct in
+// |WindowDelegate::OnWindowTargetVisibilityChanged|, and setting
+// focus in this callback doesn't cause DCHECK error.  See
+// crbug.com/168383.
+TEST_F(BaseLayoutManagerTest, FocusDuringUnminimize) {
+  FocusDelegate delegate;
+  scoped_ptr<aura::Window> window(CreateTestWindowInShellWithDelegate(
+      &delegate, 0, gfx::Rect(100, 100, 100, 100)));
+  delegate.set_window(window.get());
+  window->SetProperty(aura::client::kShowStateKey, ui::SHOW_STATE_MINIMIZED);
+  EXPECT_FALSE(window->IsVisible());
+  EXPECT_EQ(ui::SHOW_STATE_MINIMIZED, delegate.GetShowStateAndReset());
+  window->Show();
+  EXPECT_TRUE(window->IsVisible());
+  EXPECT_EQ(ui::SHOW_STATE_DEFAULT, delegate.GetShowStateAndReset());
+}
+
 // Tests maximized window size during root window resize.
 TEST_F(BaseLayoutManagerTest, MaximizeRootWindowResize) {
   gfx::Rect bounds(100, 100, 200, 200);