Project: chromium ▼ Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.
Issue 693081 mus: Fix ws::Display vs display::Display assumptions for external window mode
Starred by 3 users Project Member Reported by kylec...@chromium.org, Feb 16 Back to list
Status: Assigned
Owner:
toniki...@chromium.org
Cc:
█ fwang@chromium.org
rjkroege@chromium.org
sadrul@chromium.org
msw@chromium.org
riajiang@chromium.org
sky@chromium.org
toniki...@chromium.org
mustash-bugs@google.com
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
 
In mus-ws there are currently a number of assumptions where each ws::Display (top-level window) corresponds to a display::Display (display device). These assumptions make sense for internal window mode (Chrome OS) where there is one top level window for each display device. For external window mode (Desktop Chrome) top level windows are Chrome windows. There can be 0-N top level Chrome windows on each display device and our current code won't quite work.

External contributors are starting to work on external window mode and will be blocked by these assumptions soon. Fixing this will require changes in ws::DisplayManager, ws::UserDisplayManager and display::ScreenManager at least.
Project Member Comment 1 by bugdroid1@chromium.org, Feb 17 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/625c0cc94b7ee0441ac0debc5f295727f526eafd

commit 625c0cc94b7ee0441ac0debc5f295727f526eafd
Author: kylechar <kylechar@chromium.org>
Date: Fri Feb 17 02:36:54 2017

Split cursor location from UserDisplayManager.

The code to manage shared cursor location isn't closely related to the
other functionality in UserDisplayManager. The biggest similarity is
that cursor location is on a per user basis. Move code into
CursorLocationManager to make UserDisplayManager easier to understand.

BUG=693081

Review-Url: https://codereview.chromium.org/2696963003
Cr-Commit-Position: refs/heads/master@{#451193}

[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/BUILD.gn
[add] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/cursor_location_manager.cc
[add] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/cursor_location_manager.h
[add] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/cursor_location_manager_unittest.cc
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/display_manager.cc
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/display_manager.h
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/user_display_manager.cc
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/user_display_manager.h
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/user_display_manager_unittest.cc
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/625c0cc94b7ee0441ac0debc5f295727f526eafd/services/ui/ws/window_tree.cc
Project Member Comment 2 by bugdroid1@chromium.org, Feb 21 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/310a283e2982013c42652daa4af5c2ad621a5013

commit 310a283e2982013c42652daa4af5c2ad621a5013
Author: kylechar <kylechar@chromium.org>
Date: Tue Feb 21 21:51:56 2017

Remove usage of ws::Display in ws::UserDisplayManager.

For external mode we do not have one ws::Display per display::Display.
Have ws::UserDisplayManager take in a display::Display directly instead
of getting them from ws::Display. Also get list of current displays from
display::Screen instead of ws::DisplayManager.

BUG=693081

Review-Url: https://codereview.chromium.org/2692863009
Cr-Commit-Position: refs/heads/master@{#451841}

[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager.h
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager_ozone_external.cc
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager_ozone_external.h
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager_ozone_internal.cc
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager_ozone_internal.h
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager_stub_internal.cc
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/display/screen_manager_stub_internal.h
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/ws/display_manager.cc
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/ws/test_utils.cc
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/ws/test_utils.h
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/ws/user_display_manager.cc
[modify] https://crrev.com/310a283e2982013c42652daa4af5c2ad621a5013/services/ui/ws/user_display_manager.h
Comment 3 by kylec...@chromium.org, Feb 24
Owner: toniki...@chromium.org
Sws::UserDisplayManager now takes a display::Display instead ws::Display. I'm going to hand this one off to tonikitoo now. A few things still need to be handled to make it this work in external mode.

1. What calls into ws::UserDisplayMananger will need to change so you can modify display state and top level windows separately.
2. FrameDecorationValues don't make sense in external mode, so ws::UserDisplayMananger has to be changed to not care about that in external mode.
Comment 4 by riajiang@chromium.org, Mar 13
Cc: riajiang@chromium.org
Project Member Comment 5 by bugdroid1@chromium.org, Mar 14 
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4955b5d118973d7f99d3e5aa10e97b2446758d79

commit 4955b5d118973d7f99d3e5aa10e97b2446758d79
Author: kylechar <kylechar@chromium.org>
Date: Tue Mar 14 01:55:34 2017

Start to decouple display::Display from ws::PlatformDisplay.

External window mode will be saner if ws::PlatformDisplay is just a
pixel area and wasn't linked to the concept of display::Display. This is
because there won't be a ws::PlatformDisplay per display::Display in
external window mode.

This CL aims to simplify ws::PlatformDisplay so it is closer to just
understanding it's pixel size. Leave TODOs where further changes are
necessary.

1. Remove non-viewport information from ViewportMetrics. This way
   ws::PlatformDisplay only knows about the pixel size. It has to use
   it's delegate to get the display::Display (which might eventually
   become unnecessary).
2. Pass display::Display along with ViewportMetrics to
   ScreenManagerDelegate. This contains all the information removed from
   ViewportMetrics.
3. Delete PlatformDisplayInitParams. It's just the root ServerWindow +
   ViewportMetrics after changes so that can be passed in directly.
4. Update TestScreenManager to take a Display instead of a
   ViewportMetrics and update tests.
5. Remove a few unnecessary bits of the PlatformDisplay interface.

BUG=693081

Review-Url: https://codereview.chromium.org/2736943003
Cr-Commit-Position: refs/heads/master@{#456565}

[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/screen_manager_delegate.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/screen_manager_ozone_internal.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/screen_manager_ozone_internal_unittests.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/screen_manager_stub_internal.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/screen_manager_stub_internal.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/viewport_metrics.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/display/viewport_metrics.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/service.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/BUILD.gn
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/display.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/display.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/display_manager.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/display_manager.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/display_unittest.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/platform_display.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/platform_display.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/platform_display_default.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/platform_display_delegate.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/platform_display_factory.h
[delete] https://crrev.com/761ef91ba5d44f137daac6504c88490a207c7765/services/ui/ws/platform_display_init_params.cc
[delete] https://crrev.com/761ef91ba5d44f137daac6504c88490a207c7765/services/ui/ws/platform_display_init_params.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/test_utils.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/test_utils.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/window_manager_state_unittest.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/window_tree.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/window_tree_host_factory.cc
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/window_tree_host_factory.h
[modify] https://crrev.com/4955b5d118973d7f99d3e5aa10e97b2446758d79/services/ui/ws/window_tree_unittest.cc
Comment 6 by sadrul@chromium.org, May 25
Cc: sadrul@chromium.org
► Sign in to add a comment