Factor out [min|max]_size into ShellWindow::SizeConstraints

apps/shell_window.cc

 // Copyright 2013 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 "apps/shell_window.h"
 
 #include "apps/native_app_window.h"
 #include "apps/shell_window_geometry_cache.h"
 #include "apps/shell_window_registry.h"
 #include "base/strings/string_util.h"
@@ skipping 38 matching lines @@
 using web_modal::WebContentsModalDialogManager;
 
 namespace {
 const int kDefaultWidth = 512;
 const int kDefaultHeight = 384;
 
 }  // namespace
 
 namespace apps {
 
+ShellWindow::SizeConstraints::SizeConstraints()
+    : max_size_(kUnboundedSize, kUnboundedSize) {
+}
+
+ShellWindow::SizeConstraints::SizeConstraints(const gfx::Size& min_size,
+                                              const gfx::Size& max_size)
+    : min_size_(min_size),
+      max_size_(max_size) {
+}
+
+ShellWindow::SizeConstraints::~SizeConstraints() {}
+
+gfx::Rect ShellWindow::SizeConstraints::ClampBounds(
+    const gfx::Rect& bounds) const {
+  gfx::Rect result(bounds);
+  if (maximum_size().width() != kUnboundedSize)
+    result.set_width(std::min(result.width(), maximum_size().width()));
+  if (maximum_size().height() != kUnboundedSize)
+    result.set_height(std::min(result.height(), maximum_size().height()));
+  result.set_width(std::max(result.width(), minimum_size().width()));

[tapted, 2013/10/14 03:17:10]

I think this and the next line is just `result.SetToMax(minimum_size)`

[jackhou1, 2013/10/14 04:13:06]

SetToMax is in gfx::Size but not gfx::Rect.

[tapted, 2013/10/14 05:13:43]

Ah. Drat. But... what about changing the function signature to accept and return
gfx::Size? Since, it's not changing the Rect origin.. and the class is a
`SizeConstraints` not a `BoundsConstraints` ;).

[jackhou1, 2013/10/14 23:45:32]

Done.

+  result.set_height(std::max(result.height(), minimum_size().height()));
+  return result;
+}
+
+bool ShellWindow::SizeConstraints::HasMinimumSize() const {
+  return minimum_size().width() != kUnboundedSize ||
+         minimum_size().height() != kUnboundedSize;

[tapted, 2013/10/14 03:17:10]

nit(I think): only 4 spaces of indent for these

[jackhou1, 2013/10/14 04:13:06]

Done.

+}
+
+bool ShellWindow::SizeConstraints::HasMaximumSize() const {
+  return maximum_size().width() != kUnboundedSize ||
+         maximum_size().height() != kUnboundedSize;
+}
+
+bool ShellWindow::SizeConstraints::MinAndMaxEqual() const {
+  return !minimum_size().IsEmpty() && minimum_size() == maximum_size();
+}
+
+gfx::Size ShellWindow::SizeConstraints::minimum_size() const {
+  return min_size_;
+}
+
+gfx::Size ShellWindow::SizeConstraints::maximum_size() const {
+  return gfx::Size(
+      max_size_.width() == kUnboundedSize ?
+          kUnboundedSize : std::max(max_size_.width(), min_size_.width()),
+      max_size_.height() == kUnboundedSize ?
+          kUnboundedSize : std::max(max_size_.height(), min_size_.height()));
+}
+
 ShellWindow::CreateParams::CreateParams()
   : window_type(ShellWindow::WINDOW_TYPE_DEFAULT),
     frame(ShellWindow::FRAME_CHROME),
     transparent_background(false),
     bounds(INT_MIN, INT_MIN, 0, 0),
     creator_process_id(0),
     state(ui::SHOW_STATE_DEFAULT),
     hidden(false),
     resizable(true),
     focused(true) {}
@@ skipping 27 matching lines @@
   WebContentsModalDialogManager::CreateForWebContents(web_contents);
 
   web_contents->SetDelegate(this);
   WebContentsModalDialogManager::FromWebContents(web_contents)->
       SetDelegate(this);
   extensions::SetViewType(web_contents, extensions::VIEW_TYPE_APP_SHELL);
 
   // Initialize the window
   window_type_ = params.window_type;
 
   gfx::Rect bounds = params.bounds;

[tapted, 2013/10/14 03:17:10]

While we're refactoring.... I think this is the CL where it makes sense to
decompose the crazy & massive constructor logic from this point down. The
relationship between bounds, params.bounds, new_params.bounds, and cached_bounds
is really unclear. Your change is an improvement, but the changes will be easier
to follow if this is tackled too.

In particular, here we break `bounds` off params.bounds, change it, then copy
the original params into new_params (which now has a stale bounds). Then we
change the broken off bounds (line 188) AND the unrelated new_params (line 189).
But, then, in the code you've changed so far, we add the original (and altered)
`bounds` back onto `new_params`, and couple them together again.


What I'm thinking would be good is:

window_key_ = params.window_key;
CreateParams new_params = LoadDefaultsAndConstrain(params);
native_app_window_.reset(delegate_->CreateNativeAppWindow(this, new_params));
if (!new_params.hidden) {
 /* etc */


which is a const member function:

ShellWindow::CreateParams ShellWindow::LoadDefaultsAndConstrain(CreateParams
params) const {
  /* mess with params */
  return params;
}


WDYT? It might be OK in a follow-up too, but I think it needs to be done at some
point.

[jackhou1, 2013/10/14 04:13:06]

Done.

 
   if (bounds.width() == 0)
     bounds.set_width(kDefaultWidth);
   if (bounds.height() == 0)
     bounds.set_height(kDefaultHeight);
 
   // If left and top are left undefined, the native shell window will center
   // the window on the main screen in a platform-defined manner.
 
   CreateParams new_params = params;
@@ skipping 16 matching lines @@
       gfx::Rect current_screen_bounds = display.work_area();
       AdjustBoundsToBeVisibleOnScreen(cached_bounds,
                                       cached_screen_bounds,
                                       current_screen_bounds,
                                       params.minimum_size,
                                       &bounds);
       new_params.state = cached_state;
     }
   }
 
-  gfx::Size& minimum_size = new_params.minimum_size;
-  gfx::Size& maximum_size = new_params.maximum_size;
-
-  // In the case that minimum size > maximum size, we consider the minimum
-  // size to be more important.
-  if (maximum_size.width() && maximum_size.width() < minimum_size.width())
-    maximum_size.set_width(minimum_size.width());
-  if (maximum_size.height() && maximum_size.height() < minimum_size.height())
-    maximum_size.set_height(minimum_size.height());
-
-  if (maximum_size.width() && bounds.width() > maximum_size.width())
-    bounds.set_width(maximum_size.width());
-  if (bounds.width() != INT_MIN && bounds.width() < minimum_size.width())
-    bounds.set_width(minimum_size.width());
-
-  if (maximum_size.height() && bounds.height() > maximum_size.height())
-    bounds.set_height(maximum_size.height());
-  if (bounds.height() != INT_MIN && bounds.height() < minimum_size.height())
-    bounds.set_height(minimum_size.height());
-
-  new_params.bounds = bounds;
+  size_constraints_ = SizeConstraints(params.minimum_size, params.maximum_size);
+  new_params.bounds = size_constraints_.ClampBounds(bounds);
+  new_params.minimum_size = size_constraints_.minimum_size();
+  new_params.minimum_size = size_constraints_.maximum_size();
 
   native_app_window_.reset(delegate_->CreateNativeAppWindow(this, new_params));
 
   if (!new_params.hidden) {
     if (window_type_is_panel())
       GetBaseWindow()->ShowInactive();  // Panels are not activated by default.
     else
       GetBaseWindow()->Show();
   }
 
@@ skipping 481 matching lines @@
         region.bounds.x(),
         region.bounds.y(),
         region.bounds.right(),
         region.bounds.bottom(),
         region.draggable ? SkRegion::kUnion_Op : SkRegion::kDifference_Op);
   }
   return sk_region;
 }
 
 }  // namespace apps