New issue
Advanced search Search tips

Issue 912191 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 910249



Sign in to add a comment

limit creation of ash::wm::WindowState to certain windows

Project Member Reported by est...@chromium.org, Dec 5

Issue description

Only toplevel windows or windows that are children of workspace containers should get a WindowState created for them. Currently mash client content windows are also getting WindowState objects, which causes problems because it doesn't make sense to track or change window state (maximized, minimized, pinned, etc.) for content windows.
 
Status: Started (was: Assigned)
Hi Oshima-san, have you had time to take a look at this? I'm not sure how badly it affects arc++ apps in single process mash, but it seems like it might break some aspects of window management for them, and we'd like to turn on single process mash soon, so it would be good to either address this or at least understand the problem better soon.
Blocking: 910249
I'm working on it right now. I was cros gardener last week and had to hold it.
thanks!
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/173fb79b6757380f1e5b5426511b124371a8713a

commit 173fb79b6757380f1e5b5426511b124371a8713a
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Thu Dec 20 17:33:13 2018

Remove unnecessary window check change in InteriorResizeHandleTargeter

ShouldUseExtendedBounds returns true only for children, which dones't
have its own window state, so this shouldn't be necessary.

Bug:  912191 
Test: no functional change. all tests should pass.
Change-Id: Ia3ea35a59d860ad62a0517daa66f6f1a5b26c070
Reviewed-on: https://chromium-review.googlesource.com/c/1385122
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618260}
[modify] https://crrev.com/173fb79b6757380f1e5b5426511b124371a8713a/ash/wm/window_util.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jan 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5040590a8af1183f55060a60869591f2c4e93ddc

commit 5040590a8af1183f55060a60869591f2c4e93ddc
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Thu Jan 03 18:42:10 2019

Don't allow TYPE_CONTROL winow in default container

* Don't use root window as transient parent in test.
* Don't create WindowState before adding to parent intest.
Instead set the window show type to specify the maximized state.

These are preparations to limit WindowState to specific containers.

Bug:  912191 
Test: no functional change. all tests should pass.
Change-Id: Ic4587153395552eb140f06d440c5df7afb5b689a
Reviewed-on: https://chromium-review.googlesource.com/c/1387839
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619701}
[modify] https://crrev.com/5040590a8af1183f55060a60869591f2c4e93ddc/ash/shelf/shelf_window_watcher_unittest.cc
[modify] https://crrev.com/5040590a8af1183f55060a60869591f2c4e93ddc/ash/wm/tablet_mode/tablet_mode_window_manager_unittest.cc
[modify] https://crrev.com/5040590a8af1183f55060a60869591f2c4e93ddc/ash/wm/workspace/workspace_layout_manager.cc
[modify] https://crrev.com/5040590a8af1183f55060a60869591f2c4e93ddc/ash/wm/workspace/workspace_layout_manager_unittest.cc
[modify] https://crrev.com/5040590a8af1183f55060a60869591f2c4e93ddc/ash/wm/workspace_controller_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f569554d64f08d5b3bc5a0836c990af7d98eaa8

commit 2f569554d64f08d5b3bc5a0836c990af7d98eaa8
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Wed Jan 09 07:56:46 2019

Remove WORKSPACE_WINDOW_STATE_WINDOW_OVERLAPS_SHELF and related code

Shelf no longer becomes transparent when on overlapped windows,
so we can remove this.

Bug:  912191 
Test: updated existing tests to reflect the change.
Change-Id: I5bd31fb131703e4ba779b2444838e87d31358192
Reviewed-on: https://chromium-review.googlesource.com/c/1399047
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621070}
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/app_list/app_list_presenter_delegate_impl.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/shelf/shelf_layout_manager.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/shelf/shelf_window_watcher.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/shelf/shelf_window_watcher_unittest.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/overview/overview_utils.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/top_level_window_factory.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/window_state.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/window_state.h
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/workspace/workspace_types.h
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/workspace_controller.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/ash/wm/workspace_controller_unittest.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/chrome/browser/ui/views/status_bubble_views.cc
[modify] https://crrev.com/2f569554d64f08d5b3bc5a0836c990af7d98eaa8/services/ws/public/mojom/window_manager.mojom

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44972d384a132962c1bdddafccc5b6cdf073bf35

commit 44972d384a132962c1bdddafccc5b6cdf073bf35
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Mon Jan 14 18:09:22 2019

Don't create WindowState when unnecessary.

* GetWindowState will create a WindowState only if
  - it has parent
  - its parent is one of activatable, or arc virtual keyboard container.
  - not TYPE_CHILD (this shouldn't happen in normal situation but still possibele in tests)

* ShadowController will update the shadow state when the window is added to parent
  because ash need to acces WindowState to check the shadow state.

* Changed BrowserFrameAsh to initialize the WindowState after Widget is initialized.

Bug:  912191 
Test: shadow change is covered by unit test. Tested manully on slate.
Change-Id: Ia1886398d074acdee3e8d7c309cc75535b9413e6
Reviewed-on: https://chromium-review.googlesource.com/c/1382504
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622511}
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/frame/non_client_frame_view_ash.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/root_window_controller.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/test/ash_test_base.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/fullscreen_window_finder.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/window_positioning_utils.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/window_state.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/window_util.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/wm_shadow_controller_delegate.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/wm_toplevel_window_event_handler.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/workspace/workspace_event_handler.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ash/wm/workspace/workspace_layout_manager.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/chrome/browser/ui/views/frame/browser_frame_ash.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/chrome/browser/ui/views/frame/browser_frame_ash.h
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/components/exo/pointer_unittest.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ui/wm/core/shadow_controller.cc
[modify] https://crrev.com/44972d384a132962c1bdddafccc5b6cdf073bf35/ui/wm/core/shadow_controller_unittest.cc

Comment 10 by osh...@chromium.org, Jan 16 (6 days ago)

Status: Fixed (was: Started)

Sign in to add a comment