Make sure that 30%of restored window is always visible.

Make sure that 30% of restored window is always visible.

WorkspaceLayoutManager::UpdateBoundsFromShowState change

 * Make sure the restore bounds fits to the workspace.
 * Minimized window doesn't have restore bounds, so
   use the window bounds to adjust the bounds.

AdjustBoundsToEnsureWindowVisibility
 * Do not check Intersects because we always should make
   min width/height visible.

define separate AdjustWindowsBoundsWhenAdded

I also make the minimum percentage to 30% of width/height
because 66% seems to be too much. This change is short term.
This will be changed so that we keep relative position to
the old display in new display.

BUG=249043, 264044, 248961, 263862
TEST=covered by test
R=jamescook@chromium.org, skuhne@chromium.org

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

[Mr4D (OOO till 08-26), 2013-08-06 20:22:51]

Several comments.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/base_layout_manager.h
File ash/wm/base_layout_manager.h (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:97: virtual void
AdjustWindowsBounds(AdjustWindowReason reason);
I actually liked the previous name much more since it stated what it does.
"AdjustWindowBounds" might get misused. What about
AdjustWindowSizesForWorkAreaChange?

https://codereview.chromium.org/21979005/diff/37001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:102: AdjustWindowReason reason);
Same here.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:174: bounds->set_width(std::min(bounds->width(),
work_area.width()));
I assume this is not really a problem - but what is if the new width is smaller
then min_width? Shouldn't this min include the std::max(min_width,
std::min(...))?

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:180: bounds->set_x(min_width - bounds->width());
Indenting incorrect.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:182: if (bounds->y() + min_height > work_area.height()) {
work_Area.height is probably not correct. Shouldn't it be ".bottom()" ? With the
shelf alignment this could be otherwise correct or wrong.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:36: // must be visible when the
window is added to the workspace.
you might want to say 30% - seems better then 1/3.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:62: // Adjust window bounds in case
that the new child is out of the workspace.
You should at least change the comment to explain what this really does (as
discussed) since this will likely haunt us later and it is not clear (looking at
the function names) what it really does.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:214: case
ADJUST_WINDOW_WINDOW_ADDED: {
I am weary about this case that it might adjust windows which get added to the
desktop for a different reason (DnD, ..) => and suddenly the users windows get
repositioned in a way they should not be. With the old name this made more
sense, but with the new function name this seems to ask for trouble.

Side note: There is some code in the window positioner which does not allow the
user to drag the window further out then a few pixels.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager_unittest.cc:193:
EXPECT_GT(bounds.width(), out_window->bounds().width() * 0.3);
1/3 != 0.3

[oshima, 2013-08-06 23:13:20]

https://codereview.chromium.org/21979005/diff/37001/ash/wm/base_layout_manager.h
File ash/wm/base_layout_manager.h (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:97: virtual void
AdjustWindowsBounds(AdjustWindowReason reason);
On 2013/08/06 20:22:51, Mr4D wrote:
> I actually liked the previous name much more since it stated what it does.
> "AdjustWindowBounds" might get misused. What about
> AdjustWindowSizesForWorkAreaChange?

It actually didn't state what it does, and that's why I wanted to rename it.
It does change bounds, not just size, and reason covers more scenarios
not just screen change. How about

AdjustWindowsBoundsForWorkAreaChange (with CHECK to reject ADDED because it
doesn't make sense in this scenario)

and

AdjustWindowBounds

?

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:174: bounds->set_width(std::min(bounds->width(),
work_area.width()));
On 2013/08/06 20:22:51, Mr4D wrote:
> I assume this is not really a problem - but what is if the new width is
smaller
> then min_width? Shouldn't this min include the std::max(min_width,
> std::min(...))?

min_width/min_height shouldn't be bigger than wrok_area so
I added that condition. (plus test)

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:180: bounds->set_x(min_width - bounds->width());
On 2013/08/06 20:22:51, Mr4D wrote:
> Indenting incorrect.

Done.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:182: if (bounds->y() + min_height > work_area.height()) {
On 2013/08/06 20:22:51, Mr4D wrote:
> work_Area.height is probably not correct. Shouldn't it be ".bottom()" ? With
the
> shelf alignment this could be otherwise correct or wrong.

You're right. fixed.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:36: // must be visible when the
window is added to the workspace.
On 2013/08/06 20:22:51, Mr4D wrote:
> you might want to say 30% - seems better then 1/3.

Done.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:62: // Adjust window bounds in case
that the new child is out of the workspace.
On 2013/08/06 20:22:51, Mr4D wrote:
> You should at least change the comment to explain what this really does (as
> discussed) since this will likely haunt us later and it is not clear (looking
at
> the function names) what it really does.

Done.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:214: case
ADJUST_WINDOW_WINDOW_ADDED: {
On 2013/08/06 20:22:51, Mr4D wrote:
> I am weary about this case that it might adjust windows which get added to the
> desktop for a different reason (DnD, ..) => and suddenly the users windows get
> repositioned in a way they should not be. With the old name this made more
> sense, but with the new function name this seems to ask for trouble.
> 
> Side note: There is some code in the window positioner which does not allow
the
> user to drag the window further out then a few pixels. 

These conditions are excluded by if statement in OnWindowAddedToLayout, and
test fails if you remove these if.

This is also better fallback if there is any other case we're missing because
it's better to force a window inside work area, rather than leaving outside.

I agree that the current situation isn't ideal. I will revisit/refactor/cleanup
positioning code for m31 to address your concern.

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/21979005/diff/37001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager_unittest.cc:193:
EXPECT_GT(bounds.width(), out_window->bounds().width() * 0.3);
On 2013/08/06 20:22:51, Mr4D wrote:
> 1/3 != 0.3

It's rounded, you know :). I updated the comment and
test as it's changed to 30%.

[Mr4D (OOO till 08-26), 2013-08-06 23:28:56]

Sorry two more nits.

https://codereview.chromium.org/21979005/diff/28003/ash/wm/base_layout_manager.h
File ash/wm/base_layout_manager.h (right):

https://codereview.chromium.org/21979005/diff/28003/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:102: AdjustWindowReason reason);
Sorry about this - but you might have missed that I wanted both functions to be
renamed?

https://codereview.chromium.org/21979005/diff/28003/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/21979005/diff/28003/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:183: bounds->set_x(min_width - bounds->width());
Indenting still looks wrong.

https://codereview.chromium.org/21979005/diff/28003/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/21979005/diff/28003/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager_unittest.cc:193:
EXPECT_GT(bounds.width(), out_window->bounds().width() * 0.29);
Ha! I see the curse of using floats here.

[oshima, 2013-08-06 23:40:36]

https://codereview.chromium.org/21979005/diff/28003/ash/wm/base_layout_manager.h
File ash/wm/base_layout_manager.h (right):

https://codereview.chromium.org/21979005/diff/28003/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:102: AdjustWindowReason reason);
On 2013/08/06 23:28:56, Mr4D wrote:
> Sorry about this - but you might have missed that I wanted both functions to
be
> renamed?

I didn't missed. I explained that this function does
change bounds not only when work area changes, but also
a window is added. Another option is to define separate function,
which may be better even though it'll dup a few logic. WDTY?

https://codereview.chromium.org/21979005/diff/28003/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/21979005/diff/28003/ash/wm/window_util.cc#new...
ash/wm/window_util.cc:183: bounds->set_x(min_width - bounds->width());
On 2013/08/06 23:28:56, Mr4D wrote:
> Indenting still looks wrong.
Doh! I guess I undone it. Sorry, fixed.

[oshima, 2013-08-06 23:48:15]

Uploaded new patch with separate method 
AjustWindowBoundsWhenAdded. Let me know if you prefer this way and I'll update
the description.

[Mr4D (OOO till 08-26), 2013-08-06 23:48:50]

Sorry one more nit, but then lgtm.

https://codereview.chromium.org/21979005/diff/53001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/21979005/diff/53001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager.cc:159: oom
I guess this does not belong here.

[oshima, 2013-08-06 23:51:41]

On Tue, Aug 6, 2013 at 4:48 PM, <skuhne@chromium.org> wrote:

> Sorry one more nit, but then lgtm.
>
>
> https://codereview.chromium.**org/21979005/diff/53001/ash/**
>
wm/workspace/workspace_layout_**manager.cc<https://codereview.chromium.org/21979005/diff/53001/ash/wm/workspace/workspace_layout_manager.cc>
> File ash/wm/workspace/workspace_**layout_manager.cc (right):
>
> https://codereview.chromium.**org/21979005/diff/53001/ash/**
>
wm/workspace/workspace_layout_**manager.cc#newcode159<https://codereview.chromium.org/21979005/diff/53001/ash/wm/workspace/workspace_layout_manager.cc#newcode159>
> ash/wm/workspace/workspace_**layout_manager.cc:159: oom
> I guess this does not belong here.
>

Yep, removed from latest patch.


>
https://codereview.chromium.**org/21979005/<https://codereview.chromium.org/2...
>

[oshima, 2013-08-06 23:52:25]

jamescook -> OWNERS

[James Cook, 2013-08-07 00:36:32]

LGTM with nits

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manager.h
File ash/wm/base_layout_manager.h (right):

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:90: // the display on which the window exists has
changd.
changd -> changed

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:92: // is ADJUST_WINDOW_SCREEN_SIZE_CHANGED), the
non-maximized/non-fullscreen
ADJUST_WINDOW_DISPLAY_SIZE_CHANGED ?

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:96: virtual void
AdjustWindowsBoundsForWorkAreaChange(AdjustWindowReason reason);
This might be clearer as AdjustAllWindowBoundsForWorkAreaChange

https://codereview.chromium.org/21979005/diff/60001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/21979005/diff/60001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager_unittest.cc:201:
EXPECT_LT(bounds.width(), out_window->bounds().width() * 0.29);
Should these be EXPECT_LT(..., width * 0.3) instead of 0.29 ?

[oshima, 2013-08-07 16:56:38]

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manager.h
File ash/wm/base_layout_manager.h (right):

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:90: // the display on which the window exists has
changd.
On 2013/08/07 00:36:32, James Cook wrote:
> changd -> changed

Done.

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:92: // is ADJUST_WINDOW_SCREEN_SIZE_CHANGED), the
non-maximized/non-fullscreen
On 2013/08/07 00:36:32, James Cook wrote:
> ADJUST_WINDOW_DISPLAY_SIZE_CHANGED ?

Done.

https://codereview.chromium.org/21979005/diff/60001/ash/wm/base_layout_manage...
ash/wm/base_layout_manager.h:96: virtual void
AdjustWindowsBoundsForWorkAreaChange(AdjustWindowReason reason);
On 2013/08/07 00:36:32, James Cook wrote:
> This might be clearer as AdjustAllWindowBoundsForWorkAreaChange

Agreed. Done.

https://codereview.chromium.org/21979005/diff/60001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/21979005/diff/60001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_layout_manager_unittest.cc:201:
EXPECT_LT(bounds.width(), out_window->bounds().width() * 0.29);
On 2013/08/07 00:36:32, James Cook wrote:
> Should these be EXPECT_LT(..., width * 0.3) instead of 0.29 ?

This is to make sure that the initial bounds is positioned
so that less than 30% is visible. I added comment and changed
to 0.26 to make it clear.

[commit-bot: I haz the power, 2013-08-07 17:00:23]

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

[oshima, 2013-08-07 18:25:54]

+cc:mukai

[commit-bot: I haz the power, 2013-08-07 19:04:14]

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

[oshima, 2013-08-07 20:07:06]

Committed patchset #8 manually as r216258 (presubmit successful).