Correctly restore the auto managed window to the original bounds when becoming single window

Correctly restore the auto managed window to the original bounds when becoming single window.

* Consolidate the flush left/right logic for new window with one for visibility change.
* The logic to get initial tab should be same as one for tabbed browser window.
  This CL should address this issue.

Next step. move the initial bounds logic (incl. window positioner) to ash and
remove window_sizer_ash.cc

BUG=273825, 272460
TEST=covered by test. moved relevant tests to ash.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220428

[Mr4D (OOO till 08-26), 2013-08-28 21:58:15]

A few comments, but a great start to move the functionality to a central place!

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.cc (right):

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:153: if (found && found != window) {
Please add here a comment why if (single_window) isn't needed.

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:214: // Don't override pre audo
managed bounds as the current bounds
audo -> auto

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.h (right):

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.h:29: // management.  This is
temporarily made public and exported until we
Did you put in here two spaces on purpose?

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.h:30: // move the initial bounds logic
in window_sizer_ash.cc into ash.
It might be better to add here a TODO.

https://codereview.chromium.org/23205027/diff/40001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/23205027/diff/40001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:646:
gfx::Rect(secondary_bounds.CenterPoint().x() - 100, 10, 200, 200),
[this is one comment for multiple lines [615, 637, 681, 692, 707, ..]:

I was under the assumption that this CL's purpose is to move code around - not
to change the basic placement functionality. Shouldn't this be the same as
before? Why did you change the placement outcome?

Also - I like unit tests to validate a predictable outcome - means to check for
an explicit solution (as it was before in e.g, 692) and not a general vague
result (like 656).

[oshima, 2013-08-29 09:18:04]

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.cc (right):

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:153: if (found && found != window) {
On 2013/08/28 21:58:15, Mr4D wrote:
> Please add here a comment why if (single_window) isn't needed.

Done.

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:214: // Don't override pre audo
managed bounds as the current bounds
On 2013/08/28 21:58:15, Mr4D wrote:
> audo -> auto

Done.

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.h (right):

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.h:29: // management.  This is
temporarily made public and exported until we
On 2013/08/28 21:58:15, Mr4D wrote:
> Did you put in here two spaces on purpose?

fixed

https://codereview.chromium.org/23205027/diff/40001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.h:30: // move the initial bounds logic
in window_sizer_ash.cc into ash.
On 2013/08/28 21:58:15, Mr4D wrote:
> It might be better to add here a TODO.

Done.

https://codereview.chromium.org/23205027/diff/40001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/23205027/diff/40001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:646:
gfx::Rect(secondary_bounds.CenterPoint().x() - 100, 10, 200, 200),
On 2013/08/28 21:58:15, Mr4D wrote:
> [this is one comment for multiple lines [615, 637, 681, 692, 707, ..]:
> 
> I was under the assumption that this CL's purpose is to move code around - not
> to change the basic placement functionality. Shouldn't this be the same as
> before? Why did you change the placement outcome?

This is because I removed the code from GetBoundsOverrideAsh, which changed
expectation, and changed the condition to test.

> 
> Also - I like unit tests to validate a predictable outcome - means to check
for
> an explicit solution (as it was before in e.g, 692) and not a general vague
> result (like 656).

What I wanted to test is if it's opened on the same display as reference window,
instead of if it has some exact bounds because the exact location to be used
is unknown without RearrangeVisibleWindowOnShow (the bounds is updated in this
funtion).

In order to test the actual bounds that will be used, I need to combine
WindowSizerAsh and auto management code, which is next step. I added TODO to
update the test.

If you want to check the exact bounds now, it'll look like this:

    EXPECT_EQ(browser_window->GetNativeWindow()->GetBoundsInScreen().ToString(),
              window_bounds.ToString());

I can change to this if you want. Please let me know.

[Mr4D (OOO till 08-26), 2013-08-29 15:58:15]

lgtm

https://codereview.chromium.org/23205027/diff/40001/chrome/browser/ui/window_...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/23205027/diff/40001/chrome/browser/ui/window_...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:646:
gfx::Rect(secondary_bounds.CenterPoint().x() - 100, 10, 200, 200),
Okay - lets keep the TODO for the moment then.

[oshima, 2013-08-29 17:05:35]

sky -> OWNERS

[sky, 2013-08-29 19:33:31]

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.cc (right):

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:128: const std::vector<aura::Window*>
windows =
Are you sure we want all windows here? It was my understanding the auto
management didn't apply to all windows. For example, should the task manager be
included? Popups?

[oshima, 2013-08-29 19:49:52]

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.cc (right):

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:128: const std::vector<aura::Window*>
windows =
On 2013/08/29 19:33:31, sky wrote:
> Are you sure we want all windows here? It was my understanding the auto
> management didn't apply to all windows. For example, should the task manager
be
> included? Popups?

They're excluded by IsWindowPositionManaged check.

[sky, 2013-08-29 20:18:09]

LGTM

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.cc (right):

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:154: // no need to check
!signle_window because the function must have
single_window

[oshima, 2013-08-29 20:48:04]

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
File ash/wm/workspace/auto_window_management.cc (right):

https://codereview.chromium.org/23205027/diff/46001/ash/wm/workspace/auto_win...
ash/wm/workspace/auto_window_management.cc:154: // no need to check
!signle_window because the function must have
On 2013/08/29 20:18:09, sky wrote:
> single_window

Done.

[commit-bot: I haz the power, 2013-08-29 20:51:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/23205027/46001

[commit-bot: I haz the power, 2013-08-29 23:37:29]

Change committed as 220428