Make sure that 30%of restored window is always visible.

ash/wm/workspace/workspace_layout_manager.cc

Index: ash/wm/workspace/workspace_layout_manager.cc
diff --git a/ash/wm/workspace/workspace_layout_manager.cc b/ash/wm/workspace/workspace_layout_manager.cc
index 21aa08c604db7848611f2dbdebfab779e1613ee6..0e943c96f9b80538bf92f181ed3176b9306c489f 100644
--- a/ash/wm/workspace/workspace_layout_manager.cc
+++ b/ash/wm/workspace/workspace_layout_manager.cc
@@ -32,9 +32,9 @@ namespace internal {
 
 namespace {
 
-// This specifies how much percent (2/3=66%) of a window must be visible when
-// the window is added to the workspace.
-const float kMinimumPercentOnScreenArea = 0.66f;
+// This specifies how much percent 1/3 of a window rect (width / height)
+// must be visible when the window is added to the workspace.

[Mr4D (OOO till 08-26), 2013/08/06 20:22:51]

you might want to say 30% - seems better then 1/3.

[oshima, 2013/08/06 23:13:20]

Done.

+const float kMinimumPercentOnScreenArea = 0.33f;
 
 bool IsMaximizedState(ui::WindowShowState state) {
   return state == ui::SHOW_STATE_MAXIMIZED ||
@@ -60,7 +60,9 @@ void WorkspaceLayoutManager::SetShelf(internal::ShelfLayoutManager* shelf) {
 
 void WorkspaceLayoutManager::OnWindowAddedToLayout(Window* child) {
   // Adjust window bounds in case that the new child is out of the workspace.

[Mr4D (OOO till 08-26), 2013/08/06 20:22:51]

You should at least change the comment to explain what this really does (as
discussed) since this will likely haunt us later and it is not clear (looking at
the function names) what it really does.

[oshima, 2013/08/06 23:13:20]

Done.

-  AdjustWindowSizeForScreenChange(child, ADJUST_WINDOW_WINDOW_ADDED);
+  if (!child->bounds().IsEmpty() &&
+      !wm::HasUserChangedWindowPositionOrSize(child))
+    AdjustWindowBounds(child, ADJUST_WINDOW_WINDOW_ADDED);
   BaseLayoutManager::OnWindowAddedToLayout(child);
   UpdateDesktopVisibility();
   RearrangeVisibleWindowOnShow(child);
@@ -111,7 +113,7 @@ void WorkspaceLayoutManager::OnDisplayWorkAreaInsetsChanged() {
   const gfx::Rect work_area(ScreenAsh::GetDisplayWorkAreaBoundsInParent(
       window_->parent()));
   if (work_area != work_area_)
-    AdjustWindowSizesForScreenChange(ADJUST_WINDOW_DISPLAY_INSETS_CHANGED);
+    AdjustWindowsBounds(ADJUST_WINDOW_WORK_AREA_INSETS_CHANGED);
 }
 
 void WorkspaceLayoutManager::OnWindowPropertyChanged(Window* window,
@@ -170,13 +172,13 @@ void WorkspaceLayoutManager::ShowStateChanged(
   UpdateDesktopVisibility();
 }
 
-void WorkspaceLayoutManager::AdjustWindowSizesForScreenChange(
+void WorkspaceLayoutManager::AdjustWindowsBounds(
     AdjustWindowReason reason) {
   work_area_ = ScreenAsh::GetDisplayWorkAreaBoundsInParent(window_->parent());
-  BaseLayoutManager::AdjustWindowSizesForScreenChange(reason);
+  BaseLayoutManager::AdjustWindowsBounds(reason);
 }
 
-void WorkspaceLayoutManager::AdjustWindowSizeForScreenChange(
+void WorkspaceLayoutManager::AdjustWindowBounds(
     Window* window,
     AdjustWindowReason reason) {
   if (!GetTrackedByWorkspace(window))
@@ -190,7 +192,7 @@ void WorkspaceLayoutManager::AdjustWindowSizeForScreenChange(
   // cross fade. I think this is better, but should reconsider if someone
   // raises voice for this.
   if (wm::IsWindowMaximized(window) &&
-      reason == ADJUST_WINDOW_DISPLAY_INSETS_CHANGED) {
+      reason == ADJUST_WINDOW_WORK_AREA_INSETS_CHANGED) {
     CrossFadeToBounds(window, ScreenAsh::GetMaximizedWindowBoundsInParent(
         window->parent()->parent()));
     return;
@@ -200,17 +202,22 @@ void WorkspaceLayoutManager::AdjustWindowSizeForScreenChange(
     return;
 
   gfx::Rect bounds = window->bounds();
-  if (reason == ADJUST_WINDOW_SCREEN_SIZE_CHANGED) {
-    // The work area may be smaller than the full screen.  Put as much of the
-    // window as possible within the display area.
-    bounds.AdjustToFit(work_area_);
-  } else if (reason == ADJUST_WINDOW_DISPLAY_INSETS_CHANGED) {
-    ash::wm::AdjustBoundsToEnsureMinimumWindowVisibility(work_area_, &bounds);
-  } else if (reason == ADJUST_WINDOW_WINDOW_ADDED) {
-    int min_width = bounds.width() * kMinimumPercentOnScreenArea;
-    int min_height = bounds.height() * kMinimumPercentOnScreenArea;
-    ash::wm::AdjustBoundsToEnsureWindowVisibility(
-        work_area_, min_width, min_height, &bounds);
+  switch (reason) {
+    case ADJUST_WINDOW_DISPLAY_SIZE_CHANGED:
+      // The work area may be smaller than the full screen.  Put as much of the
+      // window as possible within the display area.
+      bounds.AdjustToFit(work_area_);
+      break;
+    case ADJUST_WINDOW_WORK_AREA_INSETS_CHANGED:
+      ash::wm::AdjustBoundsToEnsureMinimumWindowVisibility(work_area_, &bounds);
+      break;
+    case ADJUST_WINDOW_WINDOW_ADDED: {

[Mr4D (OOO till 08-26), 2013/08/06 20:22:51]

I am weary about this case that it might adjust windows which get added to the
desktop for a different reason (DnD, ..) => and suddenly the users windows get
repositioned in a way they should not be. With the old name this made more
sense, but with the new function name this seems to ask for trouble.

Side note: There is some code in the window positioner which does not allow the
user to drag the window further out then a few pixels. 

[oshima, 2013/08/06 23:13:20]

These conditions are excluded by if statement in OnWindowAddedToLayout, and
test fails if you remove these if.

This is also better fallback if there is any other case we're missing because
it's better to force a window inside work area, rather than leaving outside.

I agree that the current situation isn't ideal. I will revisit/refactor/cleanup
positioning code for m31 to address your concern.

+      int min_width = bounds.width() * kMinimumPercentOnScreenArea;
+      int min_height = bounds.height() * kMinimumPercentOnScreenArea;
+      ash::wm::AdjustBoundsToEnsureWindowVisibility(
+          work_area_, min_width, min_height, &bounds);
+      break;
+    }
   }
   if (window->bounds() != bounds)
     window->SetBounds(bounds);
@@ -229,10 +236,27 @@ void WorkspaceLayoutManager::UpdateBoundsFromShowState(Window* window) {
     case ui::SHOW_STATE_DEFAULT:
     case ui::SHOW_STATE_NORMAL: {
       const gfx::Rect* restore = GetRestoreBoundsInScreen(window);
+      // Make sure that the part of the window is always visible
+      // when restored.
+      gfx::Rect bounds_in_parent;
       if (restore) {
-        gfx::Rect bounds_in_parent =
+        bounds_in_parent =
             ScreenAsh::ConvertRectFromScreen(window->parent()->parent(),
                                              *restore);
+
+        ash::wm::AdjustBoundsToEnsureMinimumWindowVisibility(
+            work_area_, &bounds_in_parent);
+      } else {
+        // Minimized windows have no restore bounds.
+        // Use the current bounds instead.
+        bounds_in_parent = window->bounds();
+        ash::wm::AdjustBoundsToEnsureMinimumWindowVisibility(
+            work_area_, &bounds_in_parent);
+        // Don't start animation if the bounds didn't change.
+        if (bounds_in_parent == window->bounds())
+          bounds_in_parent.SetRect(0, 0, 0, 0);
+      }
+      if (!bounds_in_parent.IsEmpty()) {
         CrossFadeToBounds(
             window,
             BaseLayoutManager::BoundsWithScreenEdgeVisible(