Combined window positioning and show state determiniation in the WindowSizer

Combined window positioning and show state determiniation in the WindowSizer


BUG=153550
TEST=unit test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160775

[Mr4D (OOO till 08-26), 2012-10-05 21:53:37]

Sky, as discussed - the combined function and unit tests.

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

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/browser_wind...
File chrome/browser/ui/browser_window_state.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/browser_wind...
chrome/browser/ui/browser_window_state.h:31: gfx::Rect
GetSavedWindowBoundsAndShowState(const Browser* browser,
Be consistant. This should either take a gfx::Rect* so its consistant with
show_state, or return a struct that has the bounds and show_state.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.cc:19: #include
"ui/views/widget/widget.h"
This is cross platform code, don't use views

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.cc:120: bool maximized =
views::Widget::GetWidgetForNativeWindow(
You want window->IsMaximized(), which works on all platforms.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:69: // The |show_state| variable
will only be touched if there was persisted
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.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:73: ui::WindowShowState&
show_state) const = 0;
Style guide says no references like this, you want a pointer. This applies to
every where you're passing a ref.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:80: virtual bool
GetLastActiveWindowState(gfx::Rect* bounds,
wrap gfx::Rect* bounds to the next line so things line up nicely (like you did
on 93).

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:108: static void
GetBrowserWindowBounds(const std::string& app_name,
Can't we nuke this?

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

Addressed. Please have another look!

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/browser_wind...
File chrome/browser/ui/browser_window_state.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/browser_wind...
chrome/browser/ui/browser_window_state.h:31: gfx::Rect
GetSavedWindowBoundsAndShowState(const Browser* browser,
On 2012/10/05 22:54:03, sky wrote:
> Be consistant. This should either take a gfx::Rect* so its consistant with
> show_state, or return a struct that has the bounds and show_state.

Done.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.cc (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.cc:19: #include
"ui/views/widget/widget.h"
When I checked for minimize that was the only way I was able to come up with
(after a looong search). Really odd.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.cc:120: bool maximized =
views::Widget::GetWidgetForNativeWindow(
This is now funny. I was looking all over the system for a way to do this [..]
and have found then this - awkward - way. Why did I miss this? I suppose this is
not only on one location. Done.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:69: // The |show_state| variable
will only be touched if there was persisted
This is part of the process. The default can enforce a state - in which case the
show state should be used.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:73: ui::WindowShowState&
show_state) const = 0;
On 2012/10/05 22:54:03, sky wrote:
> Style guide says no references like this, you want a pointer. This applies to
> every where you're passing a ref.

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.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:80: virtual bool
GetLastActiveWindowState(gfx::Rect* bounds,
On 2012/10/05 22:54:03, sky wrote:
> wrap gfx::Rect* bounds to the next line so things line up nicely (like you did
> on 93).

Done.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:108: static void
GetBrowserWindowBounds(const std::string& app_name,
Well - I thought that it is nicer to not have to pass this where we not need it.
But... Done.

[sky, 2012-10-08 15:25:16]

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:69: // The |show_state| variable
will only be touched if there was persisted
On 2012/10/06 01:10:10, Mr4D wrote:
> This is part of the process. The default can enforce a state - in which case
the
> show state should be used.

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.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:73: ui::WindowShowState&
show_state) const = 0;
On 2012/10/06 01:10:10, Mr4D wrote:
> On 2012/10/05 22:54:03, sky wrote:
> > Style guide says no references like this, you want a pointer. This applies
to
> > every where you're passing a ref.
> 
> 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.

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.

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_browsertest.cc:148: #if defined(USE_AURA)
Why the ifdefs in this file?

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right):

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:132: class
TestBrowserWindowAura : public TestBrowserWindow {
This is common test code. You shouldn't have aura code here.

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

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:69: // The |show_state| variable
will only be touched if there was persisted
On 2012/10/08 15:25:17, sky wrote:
> On 2012/10/06 01:10:10, Mr4D wrote:
> > This is part of the process. The default can enforce a state - in which case
> the
> > show state should be used.
> 
> 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.

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:17]

Addressed. Please have another look!

(Still looking through the unit test / build failures in all cases, so early
comments are welcome, but you can also wait until everything is green.)

Thanks!

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:69: // The |show_state| variable
will only be touched if there was persisted
As discussed - there are no 'public' functions which have this behavior. This is
all internal behavior. So I leave it as it is.

http://codereview.chromium.org/11072002/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer.h:73: ui::WindowShowState&
show_state) const = 0;
I figured that after your first comment (somehow I must have forgotten). :)

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_browsertest.cc:148: #if defined(USE_AURA)
It was because of the "gtk" reference errors I've got when using
"GetNativeWindow". But thanks for the BaseWindow reference. Removed. Done.

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right):

http://codereview.chromium.org/11072002/diff/14001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:132: class
TestBrowserWindowAura : public TestBrowserWindow {
I have changed this already earlier this morning. But thanks anyways :). Done.

[sky, 2012-10-08 21:01:25]

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:491:
ui::WindowShowState show_state;
Explicitly initialize show_state here. I realize you're not using, but less
error prone in the future.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:534: ui::WindowShowState show_state;
Same comment about initializing here.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_browsertest.cc:26: #if defined(USE_AURA)
This shouldn't be needed.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_unittest.cc (left):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_unittest.cc:16: EXPECT_EQ(ui::SHOW_STATE_DEFAULT,
chrome::GetSavedWindowShowState(browser()));
I realize you said there is coverage of this now, so it can be removed. Where is
that?

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_window_state.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_window_state.cc:86: DCHECK(bounds);
If you're going to add DCHECKs, you should DCHECK browser too.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:95: &initial_show_state_);
Same comment about initializing initial_show_state_.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_controller.mm:271: ui::WindowShowState
show_state;
same comment and initializing. Also, I have a feling this should be showState
here.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1672: &show_state_after_show_);
initialize.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer.h:91: // |show_state| will be
overwritten and return the initial visual state of
I though you said this restriction on show_state wasn't part of the public api,
only the internal api?

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:141: if (browser_ &&
browser_->window() &&
browser_ check is handled on 134.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:21: #include
"ui/aura/client/aura_constants.h"
sort

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:39: aura::Window*
native_window_;
Document ownership.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:8: #include
"ui/gfx/rect.h"
after <vector>

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:13: #include
"chrome/browser/ui/window_sizer/window_sizer.h"
sort

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:126: Source
source,
Isn't not clear what source means here. Please document better.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:128:
ui::WindowShowState* out_show_state,
make out params last.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:140: gfx::Rect*
out_bounds,
same comment about source and out params.

[Mr4D (OOO till 08-26), 2012-10-08 23:04:27]

Addressed. Please have another look!

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:491:
ui::WindowShowState show_state;
Since it *always* get overridden I thought this is not necessary, but okay -
Done (in 4 different files)!

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
File chrome/browser/extensions/api/tabs/tabs.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/extensions/...
chrome/browser/extensions/api/tabs/tabs.cc:534: ui::WindowShowState show_state;
Yeah, done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_browsertest.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_browsertest.cc:26: #if defined(USE_AURA)
Right - not anymore! Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_unittest.cc (left):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_unittest.cc:16: EXPECT_EQ(ui::SHOW_STATE_DEFAULT,
chrome::GetSavedWindowShowState(browser()));
WindowSizerTestWithBrowser.TestShowStateDefaults (window_sizer_ash_unittest.cc).

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
File chrome/browser/ui/browser_window_state.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/browser_...
chrome/browser/ui/browser_window_state.cc:86: DCHECK(bounds);
On 2012/10/08 21:01:25, sky wrote:
> If you're going to add DCHECKs, you should DCHECK browser too.

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:95: &initial_show_state_);
Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_controller.mm:271: ui::WindowShowState
show_state;
Done

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:1672: &show_state_after_show_);
It does already get initialized. Unless you want to overwrite it always before
entering the function?

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer.h (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer.h:91: // |show_state| will be
overwritten and return the initial visual state of
Right. Read the comment: It says "It will be overwritten.."

So there is no restriction, it only states that the value will get changed no
matter what.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:141: if (browser_ &&
browser_->window() &&
On 2012/10/08 21:01:25, sky wrote:
> browser_ check is handled on 134.

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:21: #include
"ui/aura/client/aura_constants.h"
On 2012/10/08 21:01:25, sky wrote:
> sort

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:39: aura::Window*
native_window_;
On 2012/10/08 21:01:25, sky wrote:
> Document ownership.

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
File chrome/browser/ui/window_sizer/window_sizer_common_unittest.h (right):

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:8: #include
"ui/gfx/rect.h"
On 2012/10/08 21:01:25, sky wrote:
> after <vector>

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:13: #include
"chrome/browser/ui/window_sizer/window_sizer.h"
On 2012/10/08 21:01:25, sky wrote:
> sort

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:126: Source
source,
On 2012/10/08 21:01:25, sky wrote:
> Isn't not clear what source means here. Please document better.

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:128:
ui::WindowShowState* out_show_state,
On 2012/10/08 21:01:25, sky wrote:
> make out params last.

Done.

http://codereview.chromium.org/11072002/diff/29001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_common_unittest.h:140: gfx::Rect*
out_bounds,
There are already over 160 calls to this function existing. I Will do that
separately.

[sky, 2012-10-09 00:11:52]

LGTM

[commit-bot: I haz the power, 2012-10-09 00:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/11072002/24008

[commit-bot: I haz the power, 2012-10-09 02:43:37]

Change committed as 160775