Add test case for updating the bounds of snapped window.

Add test case for updating the bounds of snapped window.
Cleanups:
  Explicitly handle snapped case in SetChildBounds
  Use SetMaximizedOrFullscreenBounds in OnWindowAddedToLayout

BUG=none

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

[pkotwicz, 2014-02-12 23:17:52]

Can you please reupload the CL? I can't see it :(

[oshima, 2014-02-12 23:52:54]

done. crrev.com has been in bad shape today.

[pkotwicz, 2014-02-13 00:32:38]

https://codereview.chromium.org/161293003/diff/280001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/161293003/diff/280001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:124:
window_state->AddObserver(this);
Isn't the logic below redundant and already performed in
AdjustWindowBoundsWhenAdded(window_state)

https://codereview.chromium.org/161293003/diff/280001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:170: } else if
(window_state->IsSnapped()) {
This allows a snapped window to be as wide as it wants, notably several x times
the width of the workarea?

[oshima, 2014-02-13 01:10:13]

https://codereview.chromium.org/161293003/diff/280001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/161293003/diff/280001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:124:
window_state->AddObserver(this);
On 2014/02/13 00:32:39, pkotwicz wrote:
> Isn't the logic below redundant and already performed in
> AdjustWindowBoundsWhenAdded(window_state)

Good point. Looks like we've been doing this twice. Removed.

https://codereview.chromium.org/161293003/diff/280001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:170: } else if
(window_state->IsSnapped()) {
On 2014/02/13 00:32:39, pkotwicz wrote:
> This allows a snapped window to be as wide as it wants, notably several x
times
> the width of the workarea?

Fixed. I think the same thing can happen during drag (JS may set bounds during
drag), but I'll address that later.

[pkotwicz, 2014-02-13 02:26:16]

LGTM with nits

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

https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:19: #include "ui/gfx/rect.h"
Nit: include ui/gfx/geometry/size.h here

https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.cc#n...
ash/wm/window_util.cc:80: // Adjust bounds so that the size does not exceeds
|max_size|.
Nit: You have a comment in the .h file. It is unnecessary here.

https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.h
File ash/wm/window_util.h (right):

https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.h#ne...
ash/wm/window_util.h:51: // Adjust |bounds| so that the size does not exceeds
|max_size|.
Nit: Adjust -> Adjusts
     exceeds -> exceed

https://codereview.chromium.org/161293003/diff/340001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/161293003/diff/340001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:119: wm::WindowState* window_state
= wm::GetWindowState(child);
Nit: You did not need to make |window_state| its own variable.

[oshima, 2014-02-13 02:29:13]

On 2014/02/13 02:26:16, pkotwicz wrote:
> LGTM with nits
> 
> https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.cc
> File ash/wm/window_util.cc (right):
> 
>
https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.cc#n...
> ash/wm/window_util.cc:19: #include "ui/gfx/rect.h"
> Nit: include ui/gfx/geometry/size.h here
> 
>
https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.cc#n...
> ash/wm/window_util.cc:80: // Adjust bounds so that the size does not exceeds
> |max_size|.
> Nit: You have a comment in the .h file. It is unnecessary here.
> 
> https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.h
> File ash/wm/window_util.h (right):
> 
>
https://codereview.chromium.org/161293003/diff/340001/ash/wm/window_util.h#ne...
> ash/wm/window_util.h:51: // Adjust |bounds| so that the size does not exceeds
> |max_size|.
> Nit: Adjust -> Adjusts
>      exceeds -> exceed
> 
>
https://codereview.chromium.org/161293003/diff/340001/ash/wm/workspace/worksp...
> File ash/wm/workspace/workspace_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/161293003/diff/340001/ash/wm/workspace/worksp...
> ash/wm/workspace/workspace_layout_manager.cc:119: wm::WindowState*
window_state
> = wm::GetWindowState(child);
> Nit: You did not need to make |window_state| its own variable.

All done.

[oshima, 2014-02-13 02:29:45]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:31:31]

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

[commit-bot: I haz the power, 2014-02-13 03:34:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 03:34:55]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[oshima, 2014-02-13 04:58:03]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-02-13 04:58:29]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-02-13 14:08:57]

https://codereview.chromium.org/161293003/diff/340001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/161293003/diff/340001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager.cc:119: wm::WindowState* window_state
= wm::GetWindowState(child);
On 2014/02/13 02:26:16, pkotwicz wrote:
> Nit: You did not need to make |window_state| its own variable.

It's actually used in two places, so I'll leave it this way.

[oshima, 2014-02-13 14:09:31]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-02-13 14:09:50]

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

[commit-bot: I haz the power, 2014-02-13 16:23:04]

Change committed as 251051