Clarify display::Screen::GetPrimaryDisplay() behavior |
||||
Issue descriptionAfter https://crrev.com/581987 ScreenBase::GetPrimaryDisplay() returns a Display instance with id=255 when there are no displays connected. This is not right thing to do because screen implementations may not know that this is a special display_id value and return it for connected displays. A better solution would be to return a null display instance (i.e. id = kInvalidDisplayId). It would also make it easy for callers to detect when there are no displays connected.
,
Aug 10
,
Aug 10
Can you be more specific about the issue? You'll need a display (real or non real) to be able to open a display, and a client should update itself if it depends on a display.
,
Aug 10
Copying my comments regarding this discussion from the CL here for posterity: "A valid display means one with ID not equal to kInvalidDisplayId, but rather is equal to the newly added kDefaultDisplayId. We need to distinguish between two scenarios. If we get a kInvalidDisplayId that means something terrible has happened ---> It means a bug in our code. But if we get kDefaultDisplayId, it means we're in an expected state such as when displays are being replaced during the switch to Unified Mode, or when a Chromebox boots up without a display connected." "Display IDs are not random. They have a known format as extracted from the display's EDID [1]: | manufacturer_id | product_code_hash | connector_index | The least significant 8 bits are occupied with the connector_index. The connector_index itself is [2] | device_index | display_index | I don't think a display ID of 0xFF (that is 0x00000000000000FF) is ever possible. You have to have a display with 0 manufacturer_id, 0 product_code_hash, and connected to 16th device, and the 16th connector. [1]: https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/drm_util.cc?type=cs&q=CreateDisplaySnapshot&g=0&l=461 [2]: https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/drm_util.cc?dr=C&q=%22ConnectorIndex%22&g=0&l=193"
,
Aug 10
That argument only applies to Chrome OS (windows and other platforms have to do something else). Sergey is working on the fuchsia port.
,
Aug 10
We can change the value of kDefaultDisplayId to a more general value that works for all platforms. But I still think we need to make a clear difference between an invalid state (a fatal bug in our code) and a valid state where there is no "physical" display is connected.
,
Aug 10
No physical display does not mean no display for apps. Applications should still be able to run correctly and to do that you need a display. For example, if you turns of your display, it may be seen as it is disconnected. Even in that case, it shouldn't be removed until new display configuration is seen, because when you turn it on back, it should stay the same as before. That's being said, I agree that the current value is cros specific and we may need more flexible way to specify the default display.
,
Aug 10
It seems like we already have a flexible way, by overriding Screen::GetPrimaryDisplay(). X11 uses "0" for its default display id. Windows hashes a string for its default display (and all other displays for that matter). I suppose it doesn't matter what the id is if it's only used when there are no other displays, but I'd still lean toward letting the subclasses deal with this since they all have different ways of generating ids.
,
Aug 13
afakhry@, I don't think kInvalidDisplayId should be treated as fatal error. If it was a fatal error then we would have NOTREACHED in the default Display constructor. Just searching for kInvalidDisplayId shows that there already plenty of code that handles Display with id==kInvalidDisplayId as non-fatal. There are many cases when this a null Display instances are useful. This is also Display::is_valid() that makes it easier to handle them. Currently there is a CHECK in WindowTreeHostManager::GetPrimaryDisplayId() that asserts that there is always a "primary" display. That assertion is invalid because there are cases when there is no primary display. IMO the right fix for crbug.com/866714 would be to remove that CHECK. oshima@, This bug is mainly about Screen::GetPrimaryDisplay() being required to return a valid display, even when system has no displays. It could be that the behavior you are suggesting makes sense on some platforms, but I don't think it should be required everywhere. Screen::GetPrimaryDisplay() should be allowed to return invalid Display instance when there is no "primary" display. When there is no primary display it means that of the displays that exist none can be considered primary. It doesn't mean that all windows should be destroyed. If there are cases when faking a display in no-display case makes sense then it should be platform-specific detail, not a requirement for all Screen implementations. Also it would be better to call it more something more specific instead of "default display" (e.g. kFakeTempDisplayId) and put it somewhere where it's clear it's ash/chrome-os specific.
,
Aug 14
sergeyu@, there should always be an display while OS is running because window / UI has to be on some display. It doesn't have to be *physical* display. You should be able to create a window, maximize it on Windows even if you don't have a physical display connected to the machine. All (at least major) OSses always has "main/primary" display even no display is connected, including Windows/Mac/Linux and Android. kInvalidDisplayId should not be visible to client code, hence it should be fatal if it becomes visible to a client. On the other hand, framework code does check because this can happen while configugring displays, but gfx::Screen shouldn't return an invalid display.
,
Aug 14
There are cases when we need chrome process running when it doesn't have any visible window UI and there may be no displays. For example on Fuchsia we'd like to have webrunner service that allows other processes to run web content. Caller may choose not to display that content on display. Background pages in extensions and service workers also can run without any UI. Even if some platforms simulate primary/main displays when no displays exist it doesn't mean that we need to do that in chrome on all platforms. I'm not sure how Windows and Mac handle this case, but on Linux there is no concept of a default display (though currently chrome can only run in context of an X session, which must be connected to a display, physical or not. but that's chrome's limitation not platform's). > kInvalidDisplayId should not be visible to client code, hence it > should be fatal if it becomes visible to a client. See my previous reply - there is plenty of code that uses kInvalidDisplayId and doesn't handle it as fatal error. There is also Display::is_invalid(). display.h doesn't say anything about kInvalidDisplayId being fatal. > On the other > hand, framework code does check because this can happen while > configugring displays, but gfx::Screen shouldn't return an invalid display. Why? What if the code above display layer wants to detect when there is no display?
,
Aug 16
> There are cases when we need chrome process running when it doesn't have any visible window UI That's different issues. chrome can run wihtouth UI using --headless. And if a program doesn't really need a display, there is no need to call GetPrimaryDisplay. > there is plenty of code that uses kInvalidDisplayId and doesn't handle it as fatal error Yes, because OS layer (ash/display management code) has to handle this case, but not chrome itself. It is also used as a "not specified", "or not matched", but not used for "no physical display" situation. > Why? What if the code above display layer wants to detect when there is no display? Low level code that provides Screen implementation can use that way. (ui/display/manager ash/ for example) Note that "display" is a logical concept. It may or may not be physical, and that's pretty common among major OSes. As I mentioned, otherOS provides a primary displya/main display and there is no need to check if exists. https://cs.chromium.org/chromium/src/ui/display/win/screen_win.cc?rcl=82f038dd29328069c2b89d674b707c42e90e6bd5&l=710 https://cs.chromium.org/chromium/src/ui/display/mac/screen_mac.mm?rcl=82f038dd29328069c2b89d674b707c42e90e6bd5&l=247 If there is a need for an app to detect the type of display (physical, remote, offscreen etc), we can provide that api.
,
Aug 16
Forgot to mention about Linux. > but on Linux there is no concept of a default display *Linux* doesn't provide a window service as a part of OS, and multi display support on linux has been very poor. That's being said, X + xrandr does provide a display with disconnected state. (Standard) Wayland protcol doesn't expose display information at all, but that's different issue. (We extended this for ARC++) For application, it's really up to the toolkit they uses on linux, but you shold be able to get some display information as long as the window service is up and running.
,
Aug 18
It looks like we are conflicting two separate questions: 1. Can the list of displays be empty? (i.e. can GetAllDisplay() return an empty list) 2. Can GetPrimaryDisplay() return an invalid Display instance? (when there is no primary display, regardless of how many displays are connected). You seem to argue that then answer to (1) is No, but this bug is about GetPrimaryDisplay(). My point is that GetPrimaryDisplay() should be allowed to return invalid display when there is no "primary" display, even when the list of displays is not empty. Anyway, see my comment on CR for why the current approach doesn't work well on Fuchsia: https://chromium-review.googlesource.com/c/chromium/src/+/1171586#message-888083d4afa7e9f3762a8153b5ddfceec10ba524
,
Aug 18
Primary is UX/WM concept, so it's up to each platfrom to determine which onein display list should be primary. For example, Android simply returns display 0. I think we should have VC because I'm also confused because the bug says "when no displa is connected", which seems to be unrelated to your problem?
,
Aug 20
> Primary is UX/WM concept, so it's up to each platfrom to determine which onein > display list should be primary. For example, Android simply returns display 0. What if the system doesn't have a concept of "primary" display, i.e. all displays are equal? If the answer is "return the first one", then the question is: what if there are no displays? > I think we should have VC because I'm also confused because the bug says "when > no displa is connected", which seems to be unrelated to your problem? Sorry, I see how this can be confusing. Removed "when no display is connected" from the bug title. The comments in screen.h say the following: // Returns the primary display. It is guaranteed that this will return a // display with a valid display ID even if there is no display connected. // A real display will be reported via DisplayObserver when it is connected. virtual Display GetPrimaryDisplay() const = 0; The problem here is that GetPrimaryDisplay() is not allowed to return invalid Display. That restrictions doesn't make sense on all platforms.
,
Aug 23
Could you please setup VC? I think it's much easier to discuss solution, and resolve misunderstanding (if any). Like I said before, the display at gfx::Screen level is logical, and there is no situation where no displays exist. |
||||
►
Sign in to add a comment |
||||
Comment 1 by sergeyu@chromium.org
, Aug 10