Combined window positioning and show state determiniation in the WindowSizer

chrome/browser/ui/window_sizer/window_sizer.h

Index: chrome/browser/ui/window_sizer/window_sizer.h
diff --git a/chrome/browser/ui/window_sizer/window_sizer.h b/chrome/browser/ui/window_sizer/window_sizer.h
index 865f99b691ccc0d9a805fc2337a6482dacdb388c..d88371c10f1c2112431409895ed6bce95e3adf3a 100644
--- a/chrome/browser/ui/window_sizer/window_sizer.h
+++ b/chrome/browser/ui/window_sizer/window_sizer.h
@@ -8,6 +8,7 @@
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/ui/host_desktop.h"
+#include "ui/base/ui_base_types.h"
 #include "ui/gfx/rect.h"
 
 class Browser;
@@ -65,29 +66,45 @@ class WindowSizer {
 
     // Retrieve the persisted bounds of the window. Returns true if there was
     // persisted data to retrieve state information, false otherwise.
+    // The |show_state| variable will only be touched if there was persisted

[sky, 2012/10/05 22:54:03]

Why constrain based on default? Shouldn't it always be set if true is returned?
If the caller wants something else they should ignore the value.

[Mr4D (OOO till 08-26), 2012/10/06 01:10:10]

This is part of the process. The default can enforce a state - in which case the
show state should be used.

[sky, 2012/10/08 15:25:17]

Again, that should be up to the call site, right? If it doesn't care about the
return value it can do:

ui::WindowShowState default_state;
GetPersistentState(.., &default_state);
if (dont_care_about_persist_state)
  default_state = specific_state;
?
Doesn't that result in a lot less weird checks in the code? For example,
GetBrowserWindowBoundsAndShowStat does just this.

[sky, 2012/10/08 15:30:09]

One more comment on this. I'm fine with internal methods having the if/else
check, but I think all the public methods of WindowSizer should be assumed to
modify the ui::WindowStateState* passed in.

[Mr4D (OOO till 08-26), 2012/10/08 17:39:18]

As discussed - there are no 'public' functions which have this behavior. This is
all internal behavior. So I leave it as it is.

+    // data and the |show_state| variable is SHOW_STATE_DEFAULT.
     virtual bool GetPersistentState(gfx::Rect* bounds,
-                                    gfx::Rect* work_area) const = 0;
+                                    gfx::Rect* work_area,
+                                    ui::WindowShowState& show_state) const = 0;

[sky, 2012/10/05 22:54:03]

Style guide says no references like this, you want a pointer. This applies to
every where you're passing a ref.

[Mr4D (OOO till 08-26), 2012/10/06 01:10:10]

Okay. I thought I have seen it being used somewhere (or you mentioned I should
use '&' to avoid checking pointers - but you've meant most certainly const), and
thought I might use it here. Done.

[sky, 2012/10/08 15:25:17]

Yes. A function can take references, but only if it doesn't modify them in which
case they have to be const. Since you need to modify the values they have to be
pointers.

[Mr4D (OOO till 08-26), 2012/10/08 17:39:18]

I figured that after your first comment (somehow I must have forgotten). :)

 
     // Retrieve the bounds of the most recent window of the matching type.
     // Returns true if there was a last active window to retrieve state
     // information from, false otherwise.
-    virtual bool GetLastActiveWindowState(gfx::Rect* bounds) const = 0;
+    // The |show_state| variable will only be touched if we have found a
+    // suitable window and the |show_state| variable is SHOW_STATE_DEFAULT.
+    virtual bool GetLastActiveWindowState(gfx::Rect* bounds,

[sky, 2012/10/05 22:54:03]

wrap gfx::Rect* bounds to the next line so things line up nicely (like you did
on 93).

[Mr4D (OOO till 08-26), 2012/10/06 01:10:10]

Done.

+        ui::WindowShowState& show_state) const = 0;
   };
 
-  // Determines the position and size for a window as it is created. This
-  // function uses several strategies to figure out optimal size and placement,
-  // first looking for an existing active window, then falling back to persisted
-  // data from a previous session, finally utilizing a default
-  // algorithm. If |specified_bounds| are non-empty, this value is returned
-  // instead. For use only in testing.
-  void DetermineWindowBounds(const gfx::Rect& specified_bounds,
-                             gfx::Rect* bounds) const;
+  // Determines the position and size for a window as it is created as well
+  // as the initial state. This function uses several strategies to figure out
+  // optimal size and placement, first looking for an existing active window,
+  // then falling back to persisted data from a previous session, finally
+  // utilizing a default algorithm. If |specified_bounds| are non-empty, this
+  // value is returned instead. For use only in testing.
+  // |show_state| will be overwritten and return the initial visual state of
+  // the window to use.
+  void DetermineWindowBoundsAndShowState(
+      const gfx::Rect& specified_bounds,
+      gfx::Rect* bounds,
+      ui::WindowShowState& show_state) const;
 
   // Determines the size, position and maximized state for the browser window.
   // See documentation for DetermineWindowBounds above. Normally,
   // |window_bounds| is calculated by calling GetLastActiveWindowState(). To
   // explicitly specify a particular window to base the bounds on, pass in a
   // non-NULL value for |browser|.
+  static void GetBrowserWindowBoundsAndShowState(
+      const std::string& app_name,
+      const gfx::Rect& specified_bounds,
+      const Browser* browser,
+      gfx::Rect* window_bounds,
+      ui::WindowShowState& show_state);
   static void GetBrowserWindowBounds(const std::string& app_name,

[sky, 2012/10/05 22:54:03]

Can't we nuke this?

[Mr4D (OOO till 08-26), 2012/10/06 01:10:10]

Well - I thought that it is nicer to not have to pass this where we not need it.
But... Done.

                                      const gfx::Rect& specified_bounds,
                                      const Browser* browser,
@@ -115,13 +132,17 @@ class WindowSizer {
   // Gets the size and placement of the last window. Returns true if this data
   // is valid, false if there is no last window and the application should
   // restore saved state from preferences using RestoreWindowPosition.
-  bool GetLastWindowBounds(gfx::Rect* bounds) const;
+  // |show_state| will only be changed if it was set to SHOW_STATE_DEFAULT.
+  bool GetLastWindowBounds(gfx::Rect* bounds,
+                           ui::WindowShowState& show_state) const;
 
   // Gets the size and placement of the last window in the last session, saved
   // in local state preferences. Returns true if local state exists containing
   // this information, false if this information does not exist and a default
   // size should be used.
-  bool GetSavedWindowBounds(gfx::Rect* bounds) const;
+  // |show_state| will only be changed if it was set to SHOW_STATE_DEFAULT.
+  bool GetSavedWindowBounds(gfx::Rect* bounds,
+                            ui::WindowShowState& show_state) const;
 
   // Gets the default window position and size if there is no last window and
   // no saved window placement in prefs. This function determines the default
@@ -148,13 +169,20 @@ class WindowSizer {
   // will be called before DetermineWindowBounds. It will return true when the
   // function was setting the bounds structure to the desired size. Otherwise
   // another algorithm should get used to determine the correct bounds.
+  // |show_state| will only be changed if it was set to SHOW_STATE_DEFAULT.
   bool GetBoundsOverride(const gfx::Rect& specified_bounds,
-                         gfx::Rect* bounds) const;
+                         gfx::Rect* bounds,
+                         ui::WindowShowState& show_state) const;
 #if defined(USE_ASH)
   bool GetBoundsOverrideAsh(const gfx::Rect& specified_bounds,
-                            gfx::Rect* bounds_in_screen) const;
+                            gfx::Rect* bounds_in_screen,
+                            ui::WindowShowState& show_state) const;
 #endif
 
+  // Determine the default show state for the window - not looking at other
+  // windows or at persistent information.
+  ui::WindowShowState GetWindowDefaultShowState() const;
+
   // Providers for persistent storage and monitor metrics.
   scoped_ptr<StateProvider> state_provider_;
   scoped_ptr<MonitorInfoProvider> monitor_info_provider_;