New issue
Advanced search Search tips

Issue 707328 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

chrome.system.display.onDisplayChanged event not firing

Project Member Reported by steve...@chromium.org, Mar 31 2017

Issue description

This is likely a symptom of a larger problem.

chrome.system.display.onDisplayChanged is triggered in:

SystemInfoEventRouter::OnDisplayChanged

Which is triggered by display::DisplayObserver methods OnDisplayAdded, OnDisplayRemoved.

These are called from DisplayManager::UpdateDisplaysWith here:
https://cs.chromium.org/chromium/src/ui/display/manager/display_manager.cc?type=cs&l=800

When I add some logging there, it is getting called when a display is attached / detached, but removed_displays.size() and added_display_indices.size() are both always 0.

I will start a bisect next, but if anyone has any thoughts on what might have changed recently, that would be much appreciated.

 
I just realized that this only fails when mirroring is on, which makes sense because the number of "displays" doesn't actually change. It is possible that this never worked correctly, or that it worekd a year ago when it was first added to MD Settings but broke who-knows-when since.

The old options UI works by observing ash::WindowTreeHostManager::Observer. I think I will change SystemInfoEventRouter to do that instead.

Comment 2 by osh...@chromium.org, Mar 31 2017

Exactly when it is not called? When you're switching to/from mirroring, the number of displays must change. 

Comment 3 by osh...@chromium.org, Mar 31 2017

Ah, I guess unplugging an external display while mirroring?
This is not triggering:
https://cs.chromium.org/chromium/src/ui/display/manager/display_manager.cc?type=cs&q=NotifyDisplayAdded+package:%5Echromium$&l=804

I think it's because it is based on actual displays, not layout displays.

I think that the best solution is to add a flag for mirroring to DisplayObserver::DisplayMetric and detect when that changes.

Ah, yes, re #3, that wasn't clear. The symptom is that we do not receive an event when unplugging (or plugging in) an external display while mirroring.

Comment 6 by osh...@chromium.org, Mar 31 2017

#4 agreed
I remember I encountered a similar problem when it came to detecting mirror mode/docked mode and display additions and removals.

Please take a look at how I handled this here: https://cs.chromium.org/chromium/src/ash/system/chromeos/screen_layout_observer.cc?type=cs&q=ScreenLayoutObserver::OnDisplayConfigurationChanged&l=364-421
So, WmDisplayObserver::OnDisplayConfigurationChanged is more agressive I think about posting change notifications. This is what the old options code relies on.

Anything that relies on DisplayObserver currently misses changes to mirror mode when a display is added or removed. I have a fix here:

https://codereview.chromium.org/2792733002/


Project Member

Comment 9 by bugdroid1@chromium.org, Apr 4 2017

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

commit 0c150c2f05a1bc1e46f8bbace6228d365d4d50d2
Author: stevenjb <stevenjb@chromium.org>
Date: Tue Apr 04 20:13:49 2017

Display: dectect changes to mirror mode and send a notification.

Currently if an external display is removed exiting mirror mode,
not DisplayObserver notification is sent.

This CL adds a DisplayMetric bit and sets it whenever the mirror
mode changes.

BUG= 707328 

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

[modify] https://crrev.com/0c150c2f05a1bc1e46f8bbace6228d365d4d50d2/ui/display/display_observer.h
[modify] https://crrev.com/0c150c2f05a1bc1e46f8bbace6228d365d4d50d2/ui/display/manager/display_manager.cc
[modify] https://crrev.com/0c150c2f05a1bc1e46f8bbace6228d365d4d50d2/ui/display/manager/display_manager.h

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Chrome OS 9460.5.0, 59.0.3071.15

Sign in to add a comment