New issue
Advanced search Search tips

Issue 770243 link

Starred by 1 user

Issue metadata

Status: Fixed
Merged: issue 764472
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 779845

Blocking:
issue 764472



Sign in to add a comment

Wire mirroring display and unified desktop in mus

Project Member Reported by afakhry@chromium.org, Sep 29 2017

Issue description

See:

void AshWindowTreeHostMus::RegisterMirroringHost(
    AshWindowTreeHost* mirroring_ash_host) {
  // This should not be called, but it is because mirroring isn't wired up for
  // mus. Once that is done, this should be converted to a NOTREACHED.
  NOTIMPLEMENTED();
}


Currently the following tests in ash_unittests --mus will be disabled.

UnifiedMouseWarpControllerTest.BoundaryTestGrid
UnifiedMouseWarpControllerTest.WarpMouse
UnifiedMouseWarpControllerTest.BoundaryAndWarpSimpleTest
 
Cc: xiy...@chromium.org jamescook@chromium.org osh...@chromium.org sky@chromium.org
The following tests are new. I added as part of the work to make unified desktop support a matrix layout: https://chromium-review.googlesource.com/c/chromium/src/+/683094

UnifiedMouseWarpControllerTest.BoundaryTestGrid
UnifiedMouseWarpControllerTest.BoundaryAndWarpSimpleTest

The test UnifiedMouseWarpControllerTest.WarpMouse is old, but its started failing because we changed how handle mouse warping in unified desktop mode.
Owner: kylec...@chromium.org
Status: Assigned (was: Untriaged)
Kyle, please triage. Thanks!

Comment 3 by sky@chromium.org, Oct 3 2017

Mergedinto: 764472
Status: Duplicate (was: Assigned)
Blocking: 764472
Status: Assigned (was: Duplicate)
I spent sometime debugging. The difference after my CL https://chromium-review.googlesource.com/c/chromium/src/+/683094 is that we're no longer using the default ash test base EventGenerator [https://cs.chromium.org/chromium/src/ash/test/ash_test_base.cc?q=GetEventGenera&sq=package:chromium&l=201]. This event generator has a delegate that will always return the host of the unified display rather than the target mirroring display host [https://cs.chromium.org/chromium/src/ash/test/ash_test_base.cc?type=cs&q=AshEventGeneratorDelegate&l=71]. That's not what we want. We want to dispatch the event to the mirroring display host, so instead we create our own event generator here [https://chromium-review.googlesource.com/c/chromium/src/+/683094/7/ash/display/unified_mouse_warp_controller_unittest.cc#68] using the window of the mirroring host.

Now the reason why this works in classic ash, but not MUS is that in classic ash we have the AshWindowTreeHostUnified which sets the event targeter for the mirroring host when it registers it [https://cs.chromium.org/chromium/src/ash/host/ash_window_tree_host_unified.cc?type=cs&q=AshWindowTreeHostUnified::RegisterMirroringHost&l=72].

whereas with AshWindowTreeHostMus, it does nothing [https://cs.chromium.org/chromium/src/ash/mus/ash_window_tree_host_mus.cc?type=cs&q=AshWindowTreeHostMus::RegisterMirroringHost&l=66].

When I copied the exact same event targeter code to AshWindowTreeHostMus, all tests pass.

It used to work before just by coincidence, since we only supported mouse warping between 2 displays only, and we didn't depend on the knowledge of which mirroring host the event is dispatched to. But now, we do [https://chromium-review.googlesource.com/c/chromium/src/+/683094/7/ash/display/unified_mouse_warp_controller.cc#71]. Otherwise, with a complicated display matrix such as the one which is being tested by the test  UnifiedMouseWarpControllerTest.BoundaryTestGrid  (which looks like this https://screenshot.googleplex.com/SDB8wKqdz3E.png), trying to detect which host it is by point_in_unified_host [https://chromium-review.googlesource.com/c/chromium/src/+/683094/7/ash/display/unified_mouse_warp_controller.cc#b101] will often return the wrong host due to rounding errors when converting from native points to screen points and back.
Owner: msw@chromium.org
msw@ is going to be working on unified mode in mus/mash. Unified mode is unsupported in mus/mash right now, so I'm not surprised that AshWindowTreeHostMus doesn't work correctly. Any unified mode tests can just be disabled in ash_unittests-mus or ash_unittests-mash until it's implemented.

I'm not sure if this issue is substantially different than  issue 764472 , but there is some useful info in #4. I'll let msw@ deduplicate issues again.

Comment 6 by msw@chromium.org, Oct 30 2017

Blockedon: 779845
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 13 2017

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

commit e5c68a8694cce9a415f0ef658f432afbc8b1b957
Author: Mike Wasserman <msw@chromium.org>
Date: Wed Dec 13 00:17:53 2017

mus: Support display mirroring for mus without viz

Append mirrors to the list of displays in DisplaySynchronizer.
(creates normal ws::Displays for ash to handle mirror content)

Add & call aura::WindowTreeHostObserver::OnAcceleratedWidgetOverridden.
(lets MirrorWindowController handle mus's async initialization)

Support WindowTree::ProcessSetDisplayRoot use with existing displays.
Remove WindowManagerState::DeleteWindowManagerDisplayRoot NOTREACHED.
(now hit when re-using a display and switching mirror->extended)

Allow ash/window_manager.cc to include ash/host/ash_window_tree_host.h

Bug:  770243 
Change-Id: I21ada68097cff001bb98b9a8a22482d59d0ac1c8
Reviewed-on: https://chromium-review.googlesource.com/806238
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523618}
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ash/DEPS
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ash/display/display_synchronizer.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ash/display/mirror_window_controller.h
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ash/display/window_tree_host_manager.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ash/window_manager.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/services/ui/ws/display_manager.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/services/ui/ws/server_window.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/services/ui/ws/server_window.h
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/services/ui/ws/window_tree.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ui/aura/mus/window_tree_host_mus.cc
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ui/aura/window_tree_host.h
[modify] https://crrev.com/e5c68a8694cce9a415f0ef658f432afbc8b1b957/ui/aura/window_tree_host_observer.h

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 13 2017

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

commit fe22ec61947330dde6a40a315c573afb252e38a0
Author: Christos Froussios <cfroussios@chromium.org>
Date: Wed Dec 13 09:38:18 2017

Revert "mus: Support display mirroring for mus without viz"

This reverts commit e5c68a8694cce9a415f0ef658f432afbc8b1b957.

Reason for revert: Suspected for breaking several tests in ash_unittests-mus on  Linux Chromium OS ASan LSan
E.g. https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/25187/steps/ash_unittests-mus

Original change's description:
> mus: Support display mirroring for mus without viz
> 
> Append mirrors to the list of displays in DisplaySynchronizer.
> (creates normal ws::Displays for ash to handle mirror content)
> 
> Add & call aura::WindowTreeHostObserver::OnAcceleratedWidgetOverridden.
> (lets MirrorWindowController handle mus's async initialization)
> 
> Support WindowTree::ProcessSetDisplayRoot use with existing displays.
> Remove WindowManagerState::DeleteWindowManagerDisplayRoot NOTREACHED.
> (now hit when re-using a display and switching mirror->extended)
> 
> Allow ash/window_manager.cc to include ash/host/ash_window_tree_host.h
> 
> Bug:  770243 
> Change-Id: I21ada68097cff001bb98b9a8a22482d59d0ac1c8
> Reviewed-on: https://chromium-review.googlesource.com/806238
> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: kylechar <kylechar@chromium.org>
> Commit-Queue: Michael Wasserman <msw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523618}

TBR=sadrul@chromium.org,sky@chromium.org,msw@chromium.org,kylechar@chromium.org

Change-Id: I87b3b57193b38c955f3089f039a5c479c869c50c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  770243 
Reviewed-on: https://chromium-review.googlesource.com/824042
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523730}
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ash/DEPS
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ash/display/display_synchronizer.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ash/display/mirror_window_controller.h
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ash/display/window_tree_host_manager.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ash/window_manager.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/services/ui/ws/display_manager.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/services/ui/ws/server_window.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/services/ui/ws/server_window.h
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/services/ui/ws/window_tree.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ui/aura/mus/window_tree_host_mus.cc
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ui/aura/window_tree_host.h
[modify] https://crrev.com/fe22ec61947330dde6a40a315c573afb252e38a0/ui/aura/window_tree_host_observer.h

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 13 2017

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

commit 625b7388f6fbcaa419b51ccc4cd0670785b5f423
Author: Michael Wasserman <msw@chromium.org>
Date: Wed Dec 13 16:34:32 2017

Revert "Revert "mus: Support display mirroring for mus without viz""

This reverts commit fe22ec61947330dde6a40a315c573afb252e38a0.

Reason for revert: Revert didn't fix the issue; original CL was innocent.

Original change's description:
> Revert "mus: Support display mirroring for mus without viz"
> 
> This reverts commit e5c68a8694cce9a415f0ef658f432afbc8b1b957.
> 
> Reason for revert: Suspected for breaking several tests in ash_unittests-mus on  Linux Chromium OS ASan LSan
> E.g. https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/25187/steps/ash_unittests-mus
> 
> Original change's description:
> > mus: Support display mirroring for mus without viz
> > 
> > Append mirrors to the list of displays in DisplaySynchronizer.
> > (creates normal ws::Displays for ash to handle mirror content)
> > 
> > Add & call aura::WindowTreeHostObserver::OnAcceleratedWidgetOverridden.
> > (lets MirrorWindowController handle mus's async initialization)
> > 
> > Support WindowTree::ProcessSetDisplayRoot use with existing displays.
> > Remove WindowManagerState::DeleteWindowManagerDisplayRoot NOTREACHED.
> > (now hit when re-using a display and switching mirror->extended)
> > 
> > Allow ash/window_manager.cc to include ash/host/ash_window_tree_host.h
> > 
> > Bug:  770243 
> > Change-Id: I21ada68097cff001bb98b9a8a22482d59d0ac1c8
> > Reviewed-on: https://chromium-review.googlesource.com/806238
> > Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: kylechar <kylechar@chromium.org>
> > Commit-Queue: Michael Wasserman <msw@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#523618}
> 
> TBR=sadrul@chromium.org,sky@chromium.org,msw@chromium.org,kylechar@chromium.org
> 
> Change-Id: I87b3b57193b38c955f3089f039a5c479c869c50c
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  770243 
> Reviewed-on: https://chromium-review.googlesource.com/824042
> Reviewed-by: Christos Froussios <cfroussios@chromium.org>
> Commit-Queue: Christos Froussios <cfroussios@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#523730}

TBR=sadrul@chromium.org,sky@chromium.org,msw@chromium.org,kylechar@chromium.org,cfroussios@chromium.org

Change-Id: I66c2dbea1a2c5294f5210ad227cab87bf45f30ef
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  770243 
Reviewed-on: https://chromium-review.googlesource.com/824782
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523785}
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ash/DEPS
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ash/display/display_synchronizer.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ash/display/mirror_window_controller.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ash/display/mirror_window_controller.h
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ash/display/window_tree_host_manager.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ash/window_manager.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/services/ui/ws/display_manager.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/services/ui/ws/server_window.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/services/ui/ws/server_window.h
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/services/ui/ws/window_tree.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ui/aura/mus/window_tree_host_mus.cc
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ui/aura/window_tree_host.h
[modify] https://crrev.com/625b7388f6fbcaa419b51ccc4cd0670785b5f423/ui/aura/window_tree_host_observer.h

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 14 2017

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

commit 5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a
Author: Mike Wasserman <msw@chromium.org>
Date: Thu Dec 14 21:34:12 2017

mus: Support the unified display config for mus without viz

Add AshWindowTreeHostMusUnified and AshWindowTreeHostMusMirroringUnified.
(near copies of AshWindowTreeHostUnified and AshWindowTreeHostMirroringUnified)
Make ShellPortMus::CreateAshWindowTreeHost construct these like AshWindowTreeHost::Create.

Loosen DisplayManager::SetDisplayConfiguration restrictions for unified (w/o viz).
Do not show the PlatformDisplay's window for the virtual/offscreen unified display.

TODO: Fix targeting of events on the second display during capture.
TODO: Fix a crash on shutdown when unified mode is still active.

Bug:  764472 , 770243 
TBR: dpranke@chromium.org
Test: Unified mode works reasonably in Mus (needs event handling work)
Change-Id: I25a4f2f917275b4fd9f73cb62a5f22aaf01806ca
Reviewed-on: https://chromium-review.googlesource.com/817761
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524186}
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/BUILD.gn
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/DEPS
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/display/mirror_window_controller.cc
[add] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/host/ash_window_tree_host_mus_mirroring_unified.cc
[add] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/host/ash_window_tree_host_mus_mirroring_unified.h
[add] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/host/ash_window_tree_host_mus_unified.cc
[add] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/host/ash_window_tree_host_mus_unified.h
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/ash/shell_port_mus.cc
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/services/ui/ws/display_manager.cc
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/services/ui/ws/platform_display_default.cc
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/testing/buildbot/chromium.chromiumos.json
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/testing/buildbot/chromium.memory.json
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/testing/buildbot/filters/BUILD.gn
[delete] https://crrev.com/b992fb57cbd2064f7895a2af938a39e35c17a1a2/testing/buildbot/filters/ash_unittests_mus.filter
[modify] https://crrev.com/5cf0fa8b5e659d6ea48d2e0839dde4e2144a9d9a/testing/buildbot/test_suites.pyl

Comment 11 by msw@chromium.org, Jan 22 2018

Status: Fixed (was: Assigned)
I'm going to mark this Fixed, unified display management is generally working in --mus.
There are mouse/touch defects tracked in  Issue 804460 .
There is more work to be done for mus+viz later.
Components: -MUS Internals>Services>WindowService

Sign in to add a comment