Combined window positioning and show state determiniation in the WindowSizer

chrome/browser/ui/window_sizer/window_sizer.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_WINDOW_SIZER_WINDOW_SIZER_H_
 #define CHROME_BROWSER_UI_WINDOW_SIZER_WINDOW_SIZER_H_
 
 #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;
 
 // An interface implemented by an object that can retrieve information about
 // the monitors on the system.
 class MonitorInfoProvider {
  public:
   virtual ~MonitorInfoProvider() {}
 
@@ skipping 37 matching lines @@
   virtual ~WindowSizer();
 
   // An interface implemented by an object that can retrieve state from either a
   // persistent store or an existing window.
   class StateProvider {
    public:
     virtual ~StateProvider() {}
 
     // 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,
                                      gfx::Rect* window_bounds);
 
   // Returns the default origin for popups of the given size.
   static gfx::Point GetDefaultPopupOrigin(const gfx::Size& size,
                                           chrome::HostDesktopType type);
 
   // The number of pixels which are kept free top, left and right when a window
   // gets positioned to its default location.
   static const int kDesktopBorderSize;
 
   // Maximum width of a window even if there is more room on the desktop.
   static const int kMaximumWindowWidth;
 
   // How much horizontal and vertical offset there is between newly
   // opened windows.  This value may be different on each platform.
   static const int kWindowTilePixels;
 
  private:
   // The edge of the screen to check for out-of-bounds.
   enum Edge { TOP, LEFT, BOTTOM, RIGHT };
 
   // 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
   // size based on monitor size, etc.
   void GetDefaultWindowBounds(gfx::Rect* default_bounds) const;
 #if defined(USE_ASH)
   void GetDefaultWindowBoundsAsh(gfx::Rect* default_bounds) const;
 #endif
 
   // Adjusts |bounds| to be visible on-screen, biased toward the work area of
   // the monitor containing |other_bounds|.  Despite the name, this doesn't
   // guarantee the bounds are fully contained within this monitor's work rect;
   // it just tried to ensure the edges are visible on _some_ work rect.
   // If |saved_work_area| is non-empty, it is used to determine whether the
   // monitor configuration has changed. If it has, bounds are repositioned and
   // resized if necessary to make them completely contained in the current work
   // area.
   void AdjustBoundsToBeVisibleOnMonitorContaining(
       const gfx::Rect& other_bounds,
       const gfx::Rect& saved_work_area,
       gfx::Rect* bounds) const;
 
   // Determines the position and size for a window as it gets created. This
   // 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_;
 
   // Note that this browser handle might be NULL.
   const Browser* browser_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowSizer);
 };
 
 #endif  // CHROME_BROWSER_UI_WINDOW_SIZER_WINDOW_SIZER_H_