chromeos: converts WindowState to aura::Window

chromeos: converts WindowState to aura::Window

BUG=722553
TEST=covered by tests
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2901663003
Cr-Commit-Position: refs/heads/master@{#473992}
Committed: https://chromium.googlesource.com/chromium/src/+/fd5eef96e6af8e2c6a5350524a1b782162b0b34d

[sky, 2017-05-23 00:12:12]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 00:12:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-05-23 00:13:27]

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:102: // WindowState is registered as an owned property of
|window_|, and window
This comment comes from before I created WmWindow. It's actually ok to remove
here, but I left the comment as is.

[commit-bot: I haz the power, 2017-05-23 01:01:35]

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

[commit-bot: I haz the power, 2017-05-23 01:01:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2017-05-23 03:42:21]

I had to deal with WmWindowTestApi in this patch.

[sky, 2017-05-23 03:42:30]

The CQ bit was checked by sky@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 03:42:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 05:07:34]

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

[commit-bot: I haz the power, 2017-05-23 05:07:35]

Dry run: This issue passed the CQ dry run.

[msw, 2017-05-23 05:46:55]

lgtm with nits and qs

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:591: SetChildBoundsDirect(panel, bounds);
q: The impls aren't obviously equivalent, should this also do
|wm::SnapWindowToPixelBoundary(window)| or use |WindowState::SetBoundsDirect|?
Ditto elsewhere.

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/window_state.cc
File ash/wm/window_state.cc (right):

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/window_state.cc#...
ash/wm/window_state.cc:102: // WindowState is registered as an owned property of
|window_|, and window
On 2017/05/23 00:13:27, sky wrote:
> This comment comes from before I created WmWindow. It's actually ok to remove
> here, but I left the comment as is.

Acknowledged.

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_event_handler.cc:105: target->delegate()
optional nit: since this block is contingent on a valid non-client hit
component, you can probably put "|| !target->delegate()" in the conditional
above and then assume it's valid here.

https://codereview.chromium.org/2901663003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
(right):

https://codereview.chromium.org/2901663003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:110:
window_state_ = NULL;
optional nit: nullptr

https://codereview.chromium.org/2901663003/diff/40001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2901663003/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:62: // NOTE: many of these
tests use CustomFrameViewAshSizeLock. This is needed as
q: I wonder if we could just increase the sizes for some of the windows and
avoid this test-specific workaround; maybe in a followup. wdyt?

https://codereview.chromium.org/2901663003/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:125: class
CustomFrameViewAshSizeLock {
nit: comment; maybe move the one from above down here?

[sky, 2017-05-23 17:14:39]

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:591: SetChildBoundsDirect(panel, bounds);
On 2017/05/23 05:46:55, msw wrote:
> q: The impls aren't obviously equivalent, should this also do
> |wm::SnapWindowToPixelBoundary(window)| or use |WindowState::SetBoundsDirect|?
> Ditto elsewhere.

You are an awesome reviewer!
I was worried about the problem you mention during this patch and looked back at
the history. Specifically here:
https://chromium.googlesource.com/chromium/src/+/a9454747427889f70d14df4646f7...
. AFAICT all the code there called SetChildBoundsDirect, so this patch really
just converts PanelLayoutManager back to what the code was before the porting
layer.

I tend to think this code should snap to pixel boundary, but I would rather
handle that separately. I've filed 725553.

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/2901663003/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_event_handler.cc:105: target->delegate()
On 2017/05/23 05:46:55, msw wrote:
> optional nit: since this block is contingent on a valid non-client hit
> component, you can probably put "|| !target->delegate()" in the conditional
> above and then assume it's valid here.

Done.

https://codereview.chromium.org/2901663003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc
(right):

https://codereview.chromium.org/2901663003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc:110:
window_state_ = NULL;
On 2017/05/23 05:46:55, msw wrote:
> optional nit: nullptr

Done.

https://codereview.chromium.org/2901663003/diff/40001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2901663003/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:62: // NOTE: many of these
tests use CustomFrameViewAshSizeLock. This is needed as
On 2017/05/23 05:46:55, msw wrote:
> q: I wonder if we could just increase the sizes for some of the windows and
> avoid this test-specific workaround; maybe in a followup. wdyt?

Yes, I think that would be good.

https://codereview.chromium.org/2901663003/diff/40001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:125: class
CustomFrameViewAshSizeLock {
On 2017/05/23 05:46:55, msw wrote:
> nit: comment; maybe move the one from above down here?

Done.

[sky, 2017-05-23 17:14:47]

The CQ bit was checked by sky@chromium.org

[sky, 2017-05-23 17:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2901663003/#ps60001
(title: "feedback")

[commit-bot: I haz the power, 2017-05-23 17:15:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 18:31:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495559687436910,
"parent_rev": "02950edae45e711363532457ec564092ca1666f6", "commit_rev":
"fd5eef96e6af8e2c6a5350524a1b782162b0b34d"}

[commit-bot: I haz the power, 2017-05-23 18:31:31]

Description was changed from

==========
chromeos: converts WindowState to aura::Window

BUG=722553
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
chromeos: converts WindowState to aura::Window

BUG=722553
TEST=covered by tests
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2901663003
Cr-Commit-Position: refs/heads/master@{#473992}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd5eef96e6af8e2c6a5350524a1b...
==========

[commit-bot: I haz the power, 2017-05-23 18:31:32]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fd5eef96e6af8e2c6a5350524a1b...