Improving window auto management between workspaces

Re-vamped the entire window auto positioning logic

As long as the user did not move / resize the window:

A single visible window (browser / app) on the screen will always be centered.
When a second visible (browser / app) window joins in, both will be pushed into opposite corners.
When more windows get piled up, they go every time into the opposite side of the currently active window.

When the user has resized / moved a window, it will be treated as a visible (browser / app) window, but it will not get moved - the others will however.


Addressing bug: Moving last remaining tabbed window back into screen center


BUG=153431, 150879, 153302
TEST=unit-tests & visual

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

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

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

[Mr4D (OOO till 08-26), 2012-10-10 21:14:29]

Please have a look!

[Mr4D (OOO till 08-26), 2012-10-11 19:20:43]

Scott - as discussed I have re-written the entire handling. Using only the
activation state (as suggtested) did not do the trick since state changes like
min -> norm, max -> min and what not did not rearrange the windows.

I will add more unit tests, but since there might come some other changes due to
the review I will do that in parallel. A few basic tests are already in.

Please have a look!

[sky, 2012-10-11 19:50:54]

http://codereview.chromium.org/11085053/diff/3001/ash/wm/window_util.h
File ash/wm/window_util.h (right):

http://codereview.chromium.org/11085053/diff/3001/ash/wm/window_util.h#newcode84
ash/wm/window_util.h:84: ASH_EXPORT bool IsWindowPositionManageable(const
aura::Window* window);
Managed (same with 87).

http://codereview.chromium.org/11085053/diff/3001/ash/wm/window_util.h#newcode94
ash/wm/window_util.h:94: ASH_EXPORT void
UserHasChangedWindowPositionOrSize(aura::Window* window,
SetUserHas...

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:50: const base::TimeDelta
kWindowAutoMoveDuration =
Move all this logic out of this class. Maybe into the file window_management or
auto_window_management.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:54: // occupies screen space. Note: The
minimized state is not included in this
'since it occupies screen space'?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:57: bool
HasWindowImpactOnPositionManaging(const aura::Window* window) {
Name this WindowHasImpactOnPositioning

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:58: return
(!wm::IsWindowMaximized(window) &&
Don't you need to check visibility here? And why the maximized check?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:102: if (j && window != j &&
You should not need a check for validity of j.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:104:
work_area->Intersects(j->GetBoundsInScreen()) &&
Why the work_area check?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:115: // Move the given |bounds| on the
available |work_area_width| to the
work_area_width is a misleading name since it isn't the work area.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:118: bool MoveRect(int work_area_width,
gfx::Rect& bounds, bool move_right) {
MoveRect is an extremely generic name. How about MoveToSide.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:144: if (shown_window &&
IsWindowPositionManageable(shown_window)) {
Why the shown_window test? Doesn't GetOtherVisibleAndManageableWindow only
return true if shown_window is set?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:149: animator->set_preemption_strategy(
Why are you setting the preemption_strategy?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:161: void
RearrangeVisibleWindowOnJoining(aura::Window* added_window) {
How about RearrangeVisibleWindowOnShow and RearranbleVisibleWindowOnHide for the
names.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:169: if
(!IsWindowPositionManageable(added_window) || shown_window)
How could shown_window be non-null here?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:663: if (child->IsVisible())
Why make this depend upon whether the workspace is active?

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:691:
RearrangeVisibleWindowOnRemove(child);
Minimize also results in OnWorkspaceChildWindowVisibilityChanged being invoked.
Are you sure you need this?

http://codereview.chromium.org/11085053/diff/3001/ui/aura/client/aura_constan...
File ui/aura/client/aura_constants.h (right):

http://codereview.chromium.org/11085053/diff/3001/ui/aura/client/aura_constan...
ui/aura/client/aura_constants.h:50: AURA_EXPORT extern const
WindowProperty<bool>* const
These should be in ash.

[Mr4D (OOO till 08-26), 2012-10-12 00:29:17]

Addressed (mostly).

Unit tests are still to come.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/window_util.h
File ash/wm/window_util.h (right):

http://codereview.chromium.org/11085053/diff/3001/ash/wm/window_util.h#newcode84
ash/wm/window_util.h:84: ASH_EXPORT bool IsWindowPositionManageable(const
aura::Window* window);
On 2012/10/11 19:50:54, sky wrote:
> Managed (same with 87).

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/window_util.h#newcode94
ash/wm/window_util.h:94: ASH_EXPORT void
UserHasChangedWindowPositionOrSize(aura::Window* window,
On 2012/10/11 19:50:54, sky wrote:
> SetUserHas...

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:50: const base::TimeDelta
kWindowAutoMoveDuration =
On 2012/10/11 19:50:54, sky wrote:
> Move all this logic out of this class. Maybe into the file window_management
or
> auto_window_management.

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:54: // occupies screen space. Note: The
minimized state is not included in this
Hmm. Yeah, Somehow I have re-written this comment too often. Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:57: bool
HasWindowImpactOnPositionManaging(const aura::Window* window) {
On 2012/10/11 19:50:54, sky wrote:
> Name this WindowHasImpactOnPositioning

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:58: return
(!wm::IsWindowMaximized(window) &&
Visibility check: Made sense, so I have tried it and it did not work. I can only
assume that the windows are in a transitional state when I come here and have to
reposition.

Maximized check: A maximized window makes no sense in this scenario. More so:
Note that this test is also used for the initial check. If the passed window is
maximized there is no need for any further processing.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:102: if (j && window != j &&
On 2012/10/11 19:50:54, sky wrote:
> You should not need a check for validity of j.

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:104:
work_area->Intersects(j->GetBoundsInScreen()) &&
To make sure that the window is on this screen (and not another one) -
WindowCycleController will possibly return all top level windows, no matter on
which screen they are? Unless I misunderstood something here.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:115: // Move the given |bounds| on the
available |work_area_width| to the
On 2012/10/11 19:50:54, sky wrote:
> work_area_width is a misleading name since it isn't the work area.

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:118: bool MoveRect(int work_area_width,
gfx::Rect& bounds, bool move_right) {
Named it "MoveRectToOneSide". Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:144: if (shown_window &&
IsWindowPositionManageable(shown_window)) {
Sorry. My kind of defensive programming. Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:149: animator->set_preemption_strategy(
Because I have seen it being used in a comparable case. And - more importantly -
when I do not set it, no animation is shown.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:161: void
RearrangeVisibleWindowOnJoining(aura::Window* added_window) {
On 2012/10/11 19:50:54, sky wrote:
> How about RearrangeVisibleWindowOnShow and RearranbleVisibleWindowOnHide for
the
> names.

Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:169: if
(!IsWindowPositionManageable(added_window) || shown_window)
Read the comment for GetOtherVisibleAndManageableWindow: If there are more then
one suitable windows found, the function returns false to indidcate that the
window should not get managed. However... This one special case (the window
itself needs to be repositioned) has still to be performed.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:663: if (child->IsVisible())
There are two reasons:
1.) (the logical one) Why should you re-order windows on the visible desktop
when the state change is happening on a non visible workspace?
2.) (since I was already wondering the same before I was sending out this CL I
have tried it): The result was that in several occasions I have seen odd
behavior - like e.g. on start up some windows shifted around, etc.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:691:
RearrangeVisibleWindowOnRemove(child);
Yes I am. I was fixing several special cases I have seen with transitioning
between minimized and maximized and normal which caused problems. See comment
above.

http://codereview.chromium.org/11085053/diff/3001/ui/aura/client/aura_constan...
File ui/aura/client/aura_constants.h (right):

http://codereview.chromium.org/11085053/diff/3001/ui/aura/client/aura_constan...
ui/aura/client/aura_constants.h:50: AURA_EXPORT extern const
WindowProperty<bool>* const
On 2012/10/11 19:50:54, sky wrote:
> These should be in ash.

Done.

[sky, 2012-10-12 14:27:04]

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:58: return
(!wm::IsWindowMaximized(window) &&
On 2012/10/12 00:29:18, Mr4D wrote:
> Visibility check: Made sense, so I have tried it and it did not work. I can
only
> assume that the windows are in a transitional state when I come here and have
to
> reposition.

Can you point me at a test where what I have suggested fails so that I can
investigate?

> Maximized check: A maximized window makes no sense in this scenario. More so:
> Note that this test is also used for the initial check. If the passed window
is
> maximized there is no need for any further processing.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:104:
work_area->Intersects(j->GetBoundsInScreen()) &&
On 2012/10/12 00:29:18, Mr4D wrote:
> To make sure that the window is on this screen (and not another one) -
> WindowCycleController will possibly return all top level windows, no matter on
> which screen they are? Unless I misunderstood something here.

Actually, why are you using the WindowCycleController here? You should be
checking the children of the parent.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:149: animator->set_preemption_strategy(
On 2012/10/12 00:29:18, Mr4D wrote:
> Because I have seen it being used in a comparable case. And - more importantly
-
> when I do not set it, no animation is shown.

You need to investigate this. There should be no need for it and blindly setting
it isn't right. Additionally setting the property like you have done here
changes the animator for this window after the animation is done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:663: if (child->IsVisible())
On 2012/10/12 00:29:18, Mr4D wrote:
> There are two reasons:
> 1.) (the logical one) Why should you re-order windows on the visible desktop
> when the state change is happening on a non visible workspace?

You should rearrange in the desktop the window is in.

> 2.) (since I was already wondering the same before I was sending out this CL I
> have tried it): The result was that in several occasions I have seen odd
> behavior - like e.g. on start up some windows shifted around, etc.

Again, this likely indicates another problem that needs to be investigate.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:691:
RearrangeVisibleWindowOnRemove(child);
On 2012/10/12 00:29:18, Mr4D wrote:
> Yes I am. I was fixing several special cases I have seen with transitioning
> between minimized and maximized and normal which caused problems. See comment
> above. 

Can you point me at the tests that fail when this is moved?

[Mr4D (OOO till 08-26), 2012-10-12 23:21:30]

Please have another look!

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:58: return
(!wm::IsWindowMaximized(window) &&
Because when switching from maximize to minimize it will fail. If you still want
to investigate (and I hope my memory does not fail me)
WorkspaceLayoutManager2Test.MaxToMinRepositionsRemaining (and some more will
fail when you try to do it there).

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:104:
work_area->Intersects(j->GetBoundsInScreen()) &&
I thought it might be better to only include "noteworthy" windows to begin with.
But I would guess that the introduction of the flag makes this not really
necessary anymore. Done.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:149: animator->set_preemption_strategy(
Yes, I investigated it for 2.5 hours and it was exactly as it is meant to be. I
will send you a separate eMail.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:663: if (child->IsVisible())
Changed the algorithm from my "on access" method to the requested "upfront"
change method.

http://codereview.chromium.org/11085053/diff/3001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_manager2.cc:691:
RearrangeVisibleWindowOnRemove(child);
Removed.

[Mr4D (OOO till 08-26), 2012-10-16 15:58:20]

Please have another look

[sky, 2012-10-16 17:25:34]

In hopes of getting this landed could you remove the browser and session restore
changes from this patch?

http://codereview.chromium.org/11085053/diff/39001/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/11085053/diff/39001/ash/ash.gyp#newcode375
ash/ash.gyp:375: 'wm/workspace/auto_window_management.h',
.cc first, then .h

http://codereview.chromium.org/11085053/diff/39001/ash/ash_constants.h
File ash/ash_constants.h (right):

http://codereview.chromium.org/11085053/diff/39001/ash/ash_constants.h#newcode21
ash/ash_constants.h:21: ASH_EXPORT extern const aura::WindowProperty<bool>*
const
Move these constants to ash/wm/window_properties.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:215:
wm::SetUserHasChangedWindowPositionOrSize(details.window, true);
Putting this here means if the user reverts the move we still treat the window
as moved. I don't think we should for the revert case.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:22: const base::TimeDelta
kWindowAutoMoveDuration =
This triggers a static initializer, increasing startup time. We don't want
those. Use an int instead.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:25: // Check if the given |window| is
visible on the screen and has therefore
This doesn't check visibility.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:33: // Check if a given |window| is
considered to be an auto location manageable
Clarify how this and WindowHasImpactOnPositioning differ. It would be good if
you could name them better too. Right now when I look at the names they both
mean the same thing to me.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:44: // automated window location
management. If there are more then one windows,
'there are' -> 'there is'

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:45: // false get returned, but the
|other_window| will be set to the first one
get -> is

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:60: *work_area =
window->parent()->bounds();
*work_area = gfx::Rect(window->parent()->bounds().size());

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:65: // Find a single open browser
window.
browser -> managed

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:67: aura::Window* j = windows[i];
nit: use something more descriptive than 'j'. 'j' is typically used for
iterators (as you have with i above).

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:69:
work_area->Intersects(j->GetBoundsInScreen()) &&
You're comparing coordinates in different coordinate system. You know j is a
child and we force windows on screen, so I think you can remove this line
entirely.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:83: bool MoveRectToOneSide(int
parent_width, gfx::Rect& bounds, bool move_right) {
gfx::Rect& -> gfx::Rect* and it should be the last parameter.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:112: // Center the window (only in
x).
How come only in x?
Shouldn't this restore to the position the window was at before the
auto-arrange? Might that not be centered?

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:124: // Check if after insertion  of
the given |added_window| an automated desktop
Don't duplicate comments here.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:127: // Find a single open browser
window.
browser->managed

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:136: // If so we have to make sure it
is centered.
How come you don't vertically center?

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:149: ui::LayerAnimator* animator =
shown_window->layer()->GetAnimator();
Don't animate if either of the are set:

      window->GetProperty(aura::client::kAnimationsDisabledKey) ||
      CommandLine::ForCurrentProcess()->HasSwitch(
          ash::switches::kAshWindowAnimationsDisabled)) {

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:159:
added_window->SetBounds(added_bounds);
Don't you want to animate this too?

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:602: RearrangeVisibleWindowOnShow(child);
Don't you only want to do any of the calls to auto-management if the window is
TargetVisibilty()? Alternatively the check could be in the functions.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:620: 
Move this to 630ish.

[Mr4D (OOO till 08-26), 2012-10-16 19:00:28]

Addressed.

Please have another look

http://codereview.chromium.org/11085053/diff/39001/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/11085053/diff/39001/ash/ash.gyp#newcode375
ash/ash.gyp:375: 'wm/workspace/auto_window_management.h',
On 2012/10/16 17:25:34, sky wrote:
> .cc first, then .h

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/ash_constants.h
File ash/ash_constants.h (right):

http://codereview.chromium.org/11085053/diff/39001/ash/ash_constants.h#newcode21
ash/ash_constants.h:21: ASH_EXPORT extern const aura::WindowProperty<bool>*
const
On 2012/10/16 17:25:34, sky wrote:
> Move these constants to ash/wm/window_properties.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:215:
wm::SetUserHasChangedWindowPositionOrSize(details.window, true);
On 2012/10/16 17:25:34, sky wrote:
> Putting this here means if the user reverts the move we still treat the window
> as moved. I don't think we should for the revert case.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:22: const base::TimeDelta
kWindowAutoMoveDuration =
On 2012/10/16 17:25:34, sky wrote:
> This triggers a static initializer, increasing startup time. We don't want
> those. Use an int instead.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:25: // Check if the given |window| is
visible on the screen and has therefore
On 2012/10/16 17:25:34, sky wrote:
> This doesn't check visibility.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:33: // Check if a given |window| is
considered to be an auto location manageable
On 2012/10/16 17:25:34, sky wrote:
> Clarify how this and WindowHasImpactOnPositioning differ. It would be good if
> you could name them better too. Right now when I look at the names they both
> mean the same thing to me.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:44: // automated window location
management. If there are more then one windows,
On 2012/10/16 17:25:34, sky wrote:
> 'there are' -> 'there is'

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:45: // false get returned, but the
|other_window| will be set to the first one
On 2012/10/16 17:25:34, sky wrote:
> get -> is

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:60: *work_area =
window->parent()->bounds();
I have the feeling that this (including the removal below) might bite us later.
Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:65: // Find a single open browser
window.
On 2012/10/16 17:25:34, sky wrote:
> browser -> managed

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:67: aura::Window* j = windows[i];
On 2012/10/16 17:25:34, sky wrote:
> nit: use something more descriptive than 'j'. 'j' is typically used for
> iterators (as you have with i above).

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:69:
work_area->Intersects(j->GetBoundsInScreen()) &&
On 2012/10/16 17:25:34, sky wrote:
> You're comparing coordinates in different coordinate system. You know j is a
> child and we force windows on screen, so I think you can remove this line
> entirely.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:83: bool MoveRectToOneSide(int
parent_width, gfx::Rect& bounds, bool move_right) {
On 2012/10/16 17:25:34, sky wrote:
> gfx::Rect& -> gfx::Rect* and it should be the last parameter.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:112: // Center the window (only in
x).
Because we only center in X. There was no mentioning of centering in Y
explicitly in this algorithm. BUT... the window will remain automatically Y
centered if it was created the way.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:124: // Check if after insertion  of
the given |added_window| an automated desktop
On 2012/10/16 17:25:34, sky wrote:
> Don't duplicate comments here.

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:127: // Find a single open browser
window.
On 2012/10/16 17:25:34, sky wrote:
> browser->managed

Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:136: // If so we have to make sure it
is centered.
Again - see comment above.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:149: ui::LayerAnimator* animator =
shown_window->layer()->GetAnimator();
Sorry - isn't this something which belongs into the animation class? Somehow I
really expected that to be there. Done.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:159:
added_window->SetBounds(added_bounds);
No. As you can read in the comment above, this window gets dropped into the
desktop at this time and there is no reason to animate it from a 'fictive' point
where it might even never had been in the past. Animations should only be used
where they help the user understand what is going on - this animation would only
be confusing.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:602: RearrangeVisibleWindowOnShow(child);
That is handled by the RearrangeVisible... functions already. I don't see a
reason why we should add that here to the code as well.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:620: 
No, we want to do that BEFORE we move the window between workspaces.

[sky, 2012-10-16 22:01:14]

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:149: ui::LayerAnimator* animator =
shown_window->layer()->GetAnimator();
On 2012/10/16 19:00:28, Mr4D wrote:
> Sorry - isn't this something which belongs into the animation class? Somehow I
> really expected that to be there. Done.

Different packages.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:620: 
On 2012/10/16 19:00:28, Mr4D wrote:
> No, we want to do that BEFORE we move the window between workspaces.

This doesn't move the window between workspaces, but it may move others. Are you
sure you don't need to do it on both sides? At a minimum remove newline @ 620
and add better comments.

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:621: if (child->IsVisible())
Are you sure you don't want TargetVisibilty here?

[sky, 2012-10-17 15:59:38]

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:129: // Find a single open managed
window.
This code would be a lot easier to reason about if you added an early out if
!added_window->TargetVisibility(). And yes, I get that that eventually happens
now.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:134: &shown_window)) {
shown_window is particularly confusing name to use here since this method is is
named with Show yet show_window is not the paramater. Maybe other_window as that
corresponds with GetOtherVisibleAndManageableWindow? Use the same name in
RearrangeVisibleWindowOnHide similarly above.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:154: ui::LayerAnimator* animator =
shown_window->layer()->GetAnimator();
This is why nothing is animating. As soon as ScopedLayerAnimatorSettings drops
out of scope the transition duration goes back to the default and SetBounds is
effectively immediate. You need to move shown_window->SetBounds() into this
block.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_layout_manager2_unittest.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2_unittest.cc:138: // Test the basic
auto placement of one and or two window.
Since your patch doesn't touch WorkspaceLayoutManager2 these tests shouldn't be
here. Either put them in with the other workspacemanager2 tests, or create a new
file auto_window_management_unittest

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2_unittest.cc:149: // Note: we need to
perform an activation change to trigger the update.
Then why hide and show? Use Activate() if you need to explicitly activate. That
said, why does the activate matter here? This code only has the desktop
workspace so you shouldn't need to explicitly activate anything. Same comment
for the rest of the files.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2_unittest.cc:164: // Test 2: Set up
two managed windows and check their auto positioning.
Is there a compelling reason to combine so many assertions in a single test? It
makes for a test that is harder to reason about.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:621: if (child->IsVisible())
I don't get why this needs to be before the ShouldMoveToPending call. You
previous said this mattered, but I don't see that. Your tests work fine if this
looks like:

  if (workspace->ShouldMoveToPending()) {
    MoveWorkspaceToPendingOrDelete(workspace, NULL, SWITCH_VISIBILITY_CHANGED);
  } else {
    if (child->TargetVisibility())
      RearrangeVisibleWindowOnShow(child);
    else
      RearrangeVisibleWindowOnHide(child);
    if (workspace == active_workspace_)
      UpdateShelfVisibility();
  }

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:117: if
(browser_view->browser()->initial_user_has_changed_window_or_position())
I don't think we should persist this. That's too mysterious. Who can remember
whether they actually moved a window from the last session?

The only thing we really need is to make sure when restoring we create the
window with the specified bounds. This is also needed for other places that want
to create a browser at a specific location (such as tab dragging).

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

Addressed

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:620: 
Done.

The auto management code takes all associated workspace windows into account and
positions the others... (and or this one - dependent on the call). For the
question of: "Don't we need that on both sides" - I thought the same, but I was
not able to come up with a single test case where it went wrong. If we ever get
there I am gladly adding it. :)

http://codereview.chromium.org/11085053/diff/39001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:621: if (child->IsVisible())
Okay - I guess we should probably do something about the name "IsVisible" since
it is misleading.. Done.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:129: // Find a single open managed
window.
On 2012/10/17 15:59:38, sky wrote:
> This code would be a lot easier to reason about if you added an early out if
> !added_window->TargetVisibility(). And yes, I get that that eventually happens
> now.

Done.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:134: &shown_window)) {
On 2012/10/17 15:59:38, sky wrote:
> shown_window is particularly confusing name to use here since this method is
is
> named with Show yet show_window is not the paramater. Maybe other_window as
that
> corresponds with GetOtherVisibleAndManageableWindow? Use the same name in
> RearrangeVisibleWindowOnHide similarly above.

Done.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:154: ui::LayerAnimator* animator =
shown_window->layer()->GetAnimator();
Aha! So that was the problem... Well - as stated earlier: I don't really like
how the animation system works. Having the same function doing two different
things in the same closure seems bad. Done - and thanks for finding that!

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_layout_manager2_unittest.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2_unittest.cc:138: // Test the basic
auto placement of one and or two window.
Moved into WorkspaceManager2Test since the WorkspaceManager is also tested with
this (there are calls through it into this auto window manager after all).

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2_unittest.cc:149: // Note: we need to
perform an activation change to trigger the update.
Added comments since the previous comments were not precise enough and the pure
Activation will not work.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2_unittest.cc:164: // Test 2: Set up
two managed windows and check their auto positioning.
This is a "simulation: of a standard workflow" + it goes over several state
transitions which set up would need a lot of coding otherwise. If you feel
strongly I dissect it into 4 separate tests.

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:621: if (child->IsVisible())
I am tired arguing. You get it the way and I fix the bugs as they come in. Done.

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:117: if
(browser_view->browser()->initial_user_has_changed_window_or_position())
If we don't, the desktop will not be restored to the previous state upon
restart. This *is* unfortunately necessary. I talked with kuscher and he agrees
that the restoration of the previous desktop is required.

[sky, 2012-10-18 16:11:18]

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/45003/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:621: if (child->IsVisible())
On 2012/10/17 18:43:56, Mr4D wrote:
> I am tired arguing. You get it the way and I fix the bugs as they come in.
Done.

The reason it should be like this is that if ShouldMoveToPending() returns true
then there should be no visible windows in the workspace. Additionally the
workspace may be deleted shortly. In other words, doing anything if
ShouldMoveToPending() is true is unnecessary.

If you keep the code above ShouldMoveToPending() it begs the question why? Why
would we need to do something to a workspace that is no longer interesting and
may be deleted.

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:117: if
(browser_view->browser()->initial_user_has_changed_window_or_position())
On 2012/10/17 18:43:56, Mr4D wrote:
> If we don't, the desktop will not be restored to the previous state upon
> restart. This *is* unfortunately necessary. I talked with kuscher and he
agrees
> that the restoration of the previous desktop is required.

smaller patches=easier to review and land .

Can you push this to another patch? Otherwise this one is likely to drag on
longer.

[sky, 2012-10-18 16:36:10]

I'm focusing on just the ash workspace changes. I think you're nearly there for
that part of the code. One questions though. For tab dragging we don't want to
do this. In particular if you have a single window, drag a tab out of the window
we shouldn't attempt to move either window. How is that handled?

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:29: return
(!window->bounds().IsEmpty() && wm::IsWindowPositionManaged(window));
What made you add the !IsEmpty() check? Is there really code that is trying to
hide/show an empty window? Even if there is, does it effect this code in anyway?

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:44: // automated window location
management. If there is more then one windows,
windows -> window

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:114: if (bounds !=
other_shown_window->bounds()) {
Do you really need the bounds != bounds check here? If the bounds is the same
won't this effectively result in nothing? Yes, you might try to animate but it
shouldn't cause a problem, right?

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:115: if
(!other_shown_window->GetProperty(
Simplify this by creating a function like:

base::TimeDelta AdjustAnimationTime(int time_ms) {
  if (other_shown_window->GetProperty(aura::client::kAnimationsDisabledKey) ||
     
CommandLine::ForCurrentProcess()->HasSwitch(ash::switches::kAshWindowAnimationsDisabled)
    return base::TimeDelta();
  return base::TimeDelta::FromMilliseconds(time_ms);
}

In fact you can put this in window_animations if you want.

That way you can remove the conditional branch and always create the animator.
Same comment below.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.h (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:10: // Check if after removal of the
given |removed_window| an automated desktop
Your description and name for both of these don't match. Your description says
remove, the name says hide. RearrangeVisibleWindowOnHideOrRemove is clearer.
Either way, clean up the documentation for both to clarify when invoked.

[sky, 2012-10-18 23:47:14]

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:115: if
(!other_shown_window->GetProperty(
On 2012/10/18 16:36:10, sky wrote:
> Simplify this by creating a function like:
> 
> base::TimeDelta AdjustAnimationTime(int time_ms) {
>   if (other_shown_window->GetProperty(aura::client::kAnimationsDisabledKey) ||
>      
>
CommandLine::ForCurrentProcess()->HasSwitch(ash::switches::kAshWindowAnimationsDisabled)
>     return base::TimeDelta();
>   return base::TimeDelta::FromMilliseconds(time_ms);
> }
> 
> In fact you can put this in window_animations if you want.
> 
> That way you can remove the conditional branch and always create the animator.
> Same comment below.

The other approach that avoids unnecessary allocation is:

ui::ScopedLayerAnimationSettings*
CreateScopedAnimatorAnimationSettings(aura::Window* window) {
  if (animations disabled)
    return NULL;
  return new ...;
}

scoped_ptr<ui::ScopedLayerAnimationSettings> settings(CreateScoped...);
other_shown_window->SetBounds(bounds);

You could make the first one return a scoped_ptr to make ownership clearer, but
you get the idea.

[Mr4D (OOO till 08-26), 2012-10-19 20:46:03]

Not fully done yet. Your last comment needs still to be addressed, but you can
have a look if you have time.

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/11085053/diff/45003/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:117: if
(browser_view->browser()->initial_user_has_changed_window_or_position())
Since the requirement has changed this was removed.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:29: return
(!window->bounds().IsEmpty() && wm::IsWindowPositionManaged(window));
Think about this: Suddenly your window moves to the right but nothing comes on
the left. I have found this check for empty somewhere else and I think it is a
good check. Essentially this window is as invisible as a window with a
TargetVisibility of 0 and should therefore not have any impact on the placement.
Don't you agree? Since the test was somewhere else I am fairly certain that
there might be special cases where this can get a problem. And since this is
true for all windows (added/removed/remaining) this test is in here and not the
other function.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:44: // automated window location
management. If there is more then one windows,
On 2012/10/18 16:36:10, sky wrote:
> windows -> window

Done.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:114: if (bounds !=
other_shown_window->bounds()) {
You told me to add this because otherwise I might end up with a double SetBounds
and the animation would get aborted. Isn't that true anymore? Sure it could get
folded into the next if to suppress the animation and set the bounds instead
directly, but that would make no sense.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.h (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:10: // Check if after removal of the
given |removed_window| an automated desktop
On 2012/10/18 16:36:10, sky wrote:
> Your description and name for both of these don't match. Your description says
> remove, the name says hide. RearrangeVisibleWindowOnHideOrRemove is clearer.
> Either way, clean up the documentation for both to clarify when invoked.

Done.

[sky, 2012-10-19 22:00:32]

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:29: return
(!window->bounds().IsEmpty() && wm::IsWindowPositionManaged(window));
On 2012/10/19 20:46:05, Mr4D wrote:
> Think about this: Suddenly your window moves to the right but nothing comes on
> the left. I have found this check for empty somewhere else and I think it is a
> good check. Essentially this window is as invisible as a window with a
> TargetVisibility of 0 and should therefore not have any impact on the
placement.
> Don't you agree? Since the test was somewhere else I am fairly certain that
> there might be special cases where this can get a problem. And since this is
> true for all windows (added/removed/remaining) this test is in here and not
the
> other function.

I'm reminded of your comment earlier today about AOL forcing 29 paints just to
be sure. The problem with this line of thinking is where do you draw the line?
Why not a NULL check for window here too? There are tons of other bad states
things could get into and this is just one.

In general we don't code defensively. There are good arguments to be made in
both directions, but that happens to be style we have.

A 0x0 window is a bug some where and shouldn't happen. We shouldn't have to
resort to tons of errors checks for cases that 'could' come up.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:114: if (bounds !=
other_shown_window->bounds()) {
On 2012/10/19 20:46:05, Mr4D wrote:
> You told me to add this because otherwise I might end up with a double
SetBounds
> and the animation would get aborted. Isn't that true anymore? Sure it could
get
> folded into the next if to suppress the animation and set the bounds instead
> directly, but that would make no sense.

Remember bounds is the current bounds. If you really want to check this (see my
earlier comments about my thinking here) then you need to check the target
bounds, not the current ones.

[Mr4D (OOO till 08-26), 2012-10-19 22:54:41]

Addressed and done. Please have another look!

The remaining task (restore to previous user position instead of centering last
window) will done in a separate CL to get this thing finally out of the door
(before the next merge will get horrible).

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:29: return
(!window->bounds().IsEmpty() && wm::IsWindowPositionManaged(window));
I don't think that the example fits here. This code is preventing bad things
from happening not intentionally doing things multiple times to make sure that
the result might be now correct.

I was looking over the code and looked for IsEmpty() references and have only
found them in conjunction with the layout manager (which looks for setting the
bounds recursively if they are empty), the tray and a few other places. I think
therefore that it must be safe to remove this. Done.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:114: if (bounds !=
other_shown_window->bounds()) {
That answers that question I had in my back of my mind for a while. Done.

http://codereview.chromium.org/11085053/diff/47009/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:115: if
(!other_shown_window->GetProperty(
On 2012/10/18 23:47:14, sky wrote:
> On 2012/10/18 16:36:10, sky wrote:
> > Simplify this by creating a function like:
> > 
> > base::TimeDelta AdjustAnimationTime(int time_ms) {
> >   if (other_shown_window->GetProperty(aura::client::kAnimationsDisabledKey)
||
> >      
> >
>
CommandLine::ForCurrentProcess()->HasSwitch(ash::switches::kAshWindowAnimationsDisabled)
> >     return base::TimeDelta();
> >   return base::TimeDelta::FromMilliseconds(time_ms);
> > }
> > 
> > In fact you can put this in window_animations if you want.
> > 
> > That way you can remove the conditional branch and always create the
animator.
> > Same comment below.
> 
> The other approach that avoids unnecessary allocation is:
> 
> ui::ScopedLayerAnimationSettings*
> CreateScopedAnimatorAnimationSettings(aura::Window* window) {
>   if (animations disabled)
>     return NULL;
>   return new ...;
> }
> 
> scoped_ptr<ui::ScopedLayerAnimationSettings> settings(CreateScoped...);
> other_shown_window->SetBounds(bounds);
> 
> You could make the first one return a scoped_ptr to make ownership clearer,
but
> you get the idea.

Done.

[sky, 2012-10-22 16:40:50]

http://codereview.chromium.org/11085053/diff/61001/ash/wm/default_window_resi...
File ash/wm/default_window_resizer.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/default_window_resi...
ash/wm/default_window_resizer.cc:45:
wm::SetUserHasChangedWindowPositionOrSize(details_.window, true);
I don't think you need this since WorkspaceWindowResizer is the window resizer
used for workspace code.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:44: // automated windows location
management. If there is more then one windows,
windows -> window

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:48: // Note: |work_area| is undefined
if false is returned and |other_window| is
This code doesn't even use the work_area. Is there really a reason to have this
function set it? It's especially weird since it only some times uses it.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:123: return;
nit: Move !WindowPositionCanBeManaged here too so you early return on both.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:132: settings->SetTransitionDuration(
You need a null check here for settings. Or move SetTransitionDuration to
CreateAnimationSettings.

This leaks settings. Make CreateAnimationSettings return a
scoped_ptr<ScopedLayerAnimationSettings> so that ownership is clearer and less
error prone.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:140: // Early bail out if the window
isn't visible of the feature is unwanted.
of -> or
unwanted -> disabled

But this description just matches the code, so I would remove it or describe why
we do nothing when not visible, eg:
// If the window isn't visible it was just added. Invisible windows have no
effect on auto window management.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.h (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:10: // Check if after removal of the
given |removed_window| an automated desktop
removal or hide

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:11: // location management can be
performed and rearrange accordingly
nit: end with '.'.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:14: // Check if after insertion  of
the given |added_window| an automated desktop
insertion or showing.
Also, you have two spaces between insertion and of

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:619: if (workspace == active_workspace_)
Is there a reason you need to do UpdateShelfVisibility first? The reason I
wanted it later is that Rearrange changes the window bounds and
UpdatSHelfVisibility keys off window bounds.

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:116: // done after the
window got shown.
How about:
Turn on auto window management if we don't need an explicit bounds. This way the
requested bounds are honored.

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:172: // If the window is
able to get automatically managed, it can be marked now
How about:
Once the window has been shown we know the requested bounds (if provided) have
been honored and we can switch on window management.

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

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:138: int count =
GetNumberOfValidTopLevelBrowserWindows(work_area);
Can't you rip out a lot more of this code?

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

Have another look!

http://codereview.chromium.org/11085053/diff/61001/ash/wm/default_window_resi...
File ash/wm/default_window_resizer.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/default_window_resi...
ash/wm/default_window_resizer.cc:45:
wm::SetUserHasChangedWindowPositionOrSize(details_.window, true);
On 2012/10/22 16:40:50, sky wrote:
> I don't think you need this since WorkspaceWindowResizer is the window resizer
> used for workspace code.

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:44: // automated windows location
management. If there is more then one windows,
On 2012/10/22 16:40:50, sky wrote:
> windows -> window

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:48: // Note: |work_area| is undefined
if false is returned and |other_window| is
After the rest of the code got so drastically changed this isn't really
necessary here anymore. Separated out and moved all invocations into separate
call.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:123: return;
On 2012/10/22 16:40:50, sky wrote:
> nit: Move !WindowPositionCanBeManaged here too so you early return on both.

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:132: settings->SetTransitionDuration(
You know that this contradicts somehow your previous request/suggestion? Done
(differently).

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:140: // Early bail out if the window
isn't visible of the feature is unwanted.
On 2012/10/22 16:40:50, sky wrote:
> of -> or
> unwanted -> disabled
> 
> But this description just matches the code, so I would remove it or describe
why
> we do nothing when not visible, eg:
> // If the window isn't visible it was just added. Invisible windows have no
> effect on auto window management.

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.h (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:10: // Check if after removal of the
given |removed_window| an automated desktop
On 2012/10/22 16:40:50, sky wrote:
> removal or hide

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:11: // location management can be
performed and rearrange accordingly
On 2012/10/22 16:40:50, sky wrote:
> nit: end with '.'.

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.h:14: // Check if after insertion  of
the given |added_window| an automated desktop
On 2012/10/22 16:40:50, sky wrote:
> insertion or showing.
> Also, you have two spaces between insertion and of

Done.

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager2.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager2.cc:619: if (workspace == active_workspace_)
Done. We'll see later what does not work.

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:116: // done after the
window got shown.
On 2012/10/22 16:40:50, sky wrote:
> How about:
> Turn on auto window management if we don't need an explicit bounds. This way
the
> requested bounds are honored.

Done.

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_aura.cc:172: // If the window is
able to get automatically managed, it can be marked now
On 2012/10/22 16:40:50, sky wrote:
> How about:
> Once the window has been shown we know the requested bounds (if provided) have
> been honored and we can switch on window management.

Done.

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

http://codereview.chromium.org/11085053/diff/61001/chrome/browser/ui/window_s...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:138: int count =
GetNumberOfValidTopLevelBrowserWindows(work_area);
No. Not now. 
a) our new code only handles 2 windows, this also addresses more than that.
b) it would look quite ugly if every window comes up animated.
c) the code will get changed with the next iteration and then a better way can
be found.

This CL is for *THIS* iteration and of very short lived temporary nature
anyways.

[sky, 2012-10-22 21:26:21]

LGTM

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/61001/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:132: settings->SetTransitionDuration(
On 2012/10/22 20:27:23, Mr4D wrote:
> You know that this contradicts somehow your previous request/suggestion? Done
> (differently).

This suggestion:
ui::ScopedLayerAnimationSettings*
CreateScopedAnimatorAnimationSettings(aura::Window* window) {
  if (animations disabled)
    return NULL;
  return new ...;
}

? If so, the core of the idea is still there. My new suggestion was based on two
things you missed: NULL check and leaking. I'm still fine with original
suggestion, but your patch crashes (and leaks). It seemed good to make it less
error prone.

[Mr4D (OOO till 08-26), 2012-10-23 17:50:02]

I added now the tab dragging special (no auto movement for any window which is
involved in the tab dragging).

Please have another look!

[sky, 2012-10-23 19:35:39]

http://codereview.chromium.org/11085053/diff/60012/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/60012/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:150:
CommandLine::ForCurrentProcess()->HasSwitch(
Instead of sprinkling checks for the switch have a single method that returned
if enabled. That way it's easier to change the default.

http://codereview.chromium.org/11085053/diff/60012/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11085053/diff/60012/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:832: // To avoid the auto
window management to kick in we set the user changed
What we're saying is that when you drag a window as the result of tab dragging
it should disable auto-management. So, the code should do that. Have RunLoop
(and DetachIntoNewBrowserAndRunMoveLoop) disable window management on the
window. That way the auto-management code isn't looking at yet another property
(meaning you can remove the code that looks at GetTrackedByWorkspace).

[Mr4D (OOO till 08-26), 2012-10-24 16:41:37]

Addressed

http://codereview.chromium.org/11085053/diff/60012/ash/wm/workspace/auto_wind...
File ash/wm/workspace/auto_window_management.cc (right):

http://codereview.chromium.org/11085053/diff/60012/ash/wm/workspace/auto_wind...
ash/wm/workspace/auto_window_management.cc:150:
CommandLine::ForCurrentProcess()->HasSwitch(
I had exactly that at the beginning: GetOtherVisible... was taking care of such
cases initially and throughout this code review the initial idea was
degenerated. I removed my style and fitted it to yours. Done.

http://codereview.chromium.org/11085053/diff/60012/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right):

http://codereview.chromium.org/11085053/diff/60012/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller.cc:832: // To avoid the auto
window management to kick in we set the user changed
Done indirectly as requested.

[sky, 2012-10-24 22:03:12]

This misses a couple of cases:

. Create a window, call it A, then another window, call it B. Move B around then
close it, A should move over.
. Do the same, but drag the tab from B to A. At this point A should not move.

It would be good if you add test coverage of these. The patch I sent your way
covers these cases.

[Mr4D (OOO till 08-26), 2012-10-25 17:52:15]

I merged (most of the) changes from your other sent CL.
I also added the unit tests as requested. Note that I was adding the few test
conditions into the tab_drag_controller tests since they are effected by these
tests as much as the auto positioning code.

Please have another look.

[sky, 2012-10-25 19:40:36]

LGTM

http://codereview.chromium.org/11085053/diff/79005/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

http://codereview.chromium.org/11085053/diff/79005/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:447:
ASSERT_TRUE(ash::wm::IsWindowPositionManaged(
In general you only use ASSERT for things for that are fatal and you want to
stop test execution and use EXPECT otherwise. 430 is a good example of this. If
the browser list count is not 2 (say 1), then the next line is going to crash,
that's why it uses ASSERT. So, both of these should be EXPECT.

And create a IsWindowPositionManaged function like I did for
GetTrackedByWorkspace so that you don't need the ifdefs.

http://codereview.chromium.org/11085053/diff/79005/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:867: #if
defined(USE_ASH)
Same comment here about the ifdef, remove it and always do the test.

[Mr4D (OOO till 08-26), 2012-10-25 20:44:05]

Done. Submitting. Thanks!

http://codereview.chromium.org/11085053/diff/79005/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

http://codereview.chromium.org/11085053/diff/79005/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:447:
ASSERT_TRUE(ash::wm::IsWindowPositionManaged(
On 2012/10/25 19:40:36, sky wrote:
> In general you only use ASSERT for things for that are fatal and you want to
> stop test execution and use EXPECT otherwise. 430 is a good example of this.
If
> the browser list count is not 2 (say 1), then the next line is going to crash,
> that's why it uses ASSERT. So, both of these should be EXPECT.
> 
> And create a IsWindowPositionManaged function like I did for
> GetTrackedByWorkspace so that you don't need the ifdefs.

Done.

http://codereview.chromium.org/11085053/diff/79005/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:867: #if
defined(USE_ASH)
On 2012/10/25 19:40:36, sky wrote:
> Same comment here about the ifdef, remove it and always do the test.

Done.

[commit-bot: I haz the power, 2012-10-25 20:46:42]

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

[commit-bot: I haz the power, 2012-10-25 21:43:47]

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

[commit-bot: I haz the power, 2012-10-25 23:04:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-25 23:32:36]

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

[commit-bot: I haz the power, 2012-10-26 14:55:01]

Commit queue had an internal error.
Something went really wrong, probably a crash, a hickup or
simply the monkeys went out for dinner.
The relevant dude was pinged already.

[commit-bot: I haz the power, 2012-10-26 14:55:22]

Commit queue had an internal error.
Something went really wrong, probably a crash, a hickup or
simply the monkeys went out for dinner.
The relevant dude was pinged already.

[commit-bot: I haz the power, 2012-10-26 16:24:47]

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

[commit-bot: I haz the power, 2012-10-26 17:54:41]

Retried try job too often for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, interactive_ui_tests,
ipc_tests, jingle_unittests, media_unittests, nacl_integration, net_unittests,
printing_unittests, remoting_unittests, safe_browsing_tests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2012-10-26 18:22:50]

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

[commit-bot: I haz the power, 2012-10-26 18:29:34]

Change committed as 164352

[commit-bot: I haz the power, 2012-10-26 19:40:34]

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

[commit-bot: I haz the power, 2012-10-27 12:20:55]

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

[commit-bot: I haz the power, 2012-10-29 14:43:31]

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

[commit-bot: I haz the power, 2012-10-29 16:45:01]

Change committed as 164652

[commit-bot: I haz the power, 2012-10-30 18:30:32]

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

[commit-bot: I haz the power, 2012-10-30 20:31:43]

Change committed as 164993