Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042503002:
ozone/platform/wayland: Add support for wl_output_interface (Closed)
|
Created:
4 years, 6 months ago by joone Modified:
4 years, 2 months ago CC:
chromium-reviews, kalyank, ozone-reviews_chromium.org, vignatti (out of this project), dshwang, tonikitoo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone/platform/wayland: Add support for wl_output_interface
WaylandScreen is used to keep track of the current outputs
(display) that are available to Wayland clients.
This is a part of upstream work for the ozone-wayland project:
https://github.com/01org/ozone-wayland
BUG=578890
TEST=WaylandDisplayTest.Output
Committed: https://crrev.com/de57ce2d6a43b6593c770f583ccf06b8779e3ccb
Cr-Commit-Position: refs/heads/master@{#421157}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Applied review comments #
Total comments: 8
Patch Set 3 : remove WaylandDisplay::instance #
Total comments: 11
Patch Set 4 : hide the use of the base::Closure #
Total comments: 18
Patch Set 5 : WaylandDisplay => WaylandConnection, WaylandScreen => WaylandOutput #
Total comments: 8
Patch Set 6 : rebase on master #
Total comments: 10
Patch Set 7 : add OutputObserver #Patch Set 8 : remove unused headers #Patch Set 9 : Rebase on master #Patch Set 10 : add comments and use std::unique_ptr #
Total comments: 4
Patch Set 11 : [For landding] update comments #Messages
Total messages: 56 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
joone.hur@intel.com changed reviewers: + forney@google.com
Please take a look at this CL.
Description was changed from ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (screens/monitors) that are available to Wayland clients. This is upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ========== to ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (screens/monitors) that are available to Wayland clients. This is upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ==========
joone.hur@intel.com changed reviewers: + kalyan.kondapally@intel.com
tonikitoo@igalia.com changed reviewers: + tonikitoo@igalia.com
So WaylandScreen is a wrapper around wl_output. Its main function is inform WaylandDisplay that the screen dimension is available. WaylandDisplay, has hooks to let upper layer know how it (screen delegate class). Is my understanding correct? https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:73: instance_ = this; analogously, should not you set "instance_" to nullptr on ::~WaylandDisplay? https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.h (right): https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.h:44: WaylandScreen* PrimaryScreen() const { return primary_screen_; } Minor: For the purposes of a simple API, I believe we can have the PrimaryScreen getter, but maybe we should get rid of "primary_screen_" class variable, and just return screen_list.Front() here. Maybe something like: if (!screen_list_.Size()) return nullptr; return screen_list_.Front();
Yes, WaylandScreen is a wrapper around wl_output. https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:73: instance_ = this; On 2016/06/06 21:35:15, tonikitoo1 wrote: > analogously, should not you set "instance_" to nullptr on ::~WaylandDisplay? Done. https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.h (right): https://codereview.chromium.org/2042503002/diff/40001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.h:44: WaylandScreen* PrimaryScreen() const { return primary_screen_; } On 2016/06/06 21:35:15, tonikitoo1 wrote: > Minor: For the purposes of a simple API, I believe we can have the > PrimaryScreen getter, but maybe we should get rid of "primary_screen_" class > variable, and just return screen_list.Front() here. > > Maybe something like: > > if (!screen_list_.Size()) > return nullptr; > > return screen_list_.Front(); Done.
On 2016/06/06 21:35:15, tonikitoo1 wrote: > So WaylandScreen is a wrapper around wl_output. > > Its main function is inform WaylandDisplay that the screen dimension is > available. > > WaylandDisplay, has hooks to let upper layer know how it (screen delegate > class). > > Is my understanding correct? > Yes, right.
forney@ hi, could you review my CL?
You should add one of the ozone people as a reviewer. https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:26: WaylandDisplay* WaylandDisplay::instance_ = nullptr; Why do we need to make this a singleton? https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:146: display::Screen::GetScreen()->OnOutputGeometryChanged(name, rect); Can't WaylandScreen call this? Also, maybe I'm misunderstanding something, but where is the display::Screen::OnOutputGeometryChanged method declared? I don't see it in ui/display/screen.h https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:152: output_complete_closure_ = closure; I think this should be a method on WaylandScreen. https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:19: output_ = wl::Bind<wl_output>(registry, name, 1); I think you should bind the object in wayland_display.cc, and then pass the wl_output* to the constructor, like wayland_pointer.cc does. Otherwise, if wl_registry_bind returns NULL, this objects is only partially constructed, and the caller has no way to know.
I've updated the CL. Thanks for the review. https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:26: WaylandDisplay* WaylandDisplay::instance_ = nullptr; On 2016/06/10 18:12:21, Michael Forney wrote: > Why do we need to make this a singleton? We don't need WaylandDisplay::instance any more since WaylandDisplay::SetOutputCompleteClosure was moved to WaylandScreen. https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:146: display::Screen::GetScreen()->OnOutputGeometryChanged(name, rect); On 2016/06/10 18:12:21, Michael Forney wrote: > Can't WaylandScreen call this? > > Also, maybe I'm misunderstanding something, but where is the > display::Screen::OnOutputGeometryChanged method declared? > > I don't see it in ui/display/screen.h I missed to include the header file. https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:152: output_complete_closure_ = closure; On 2016/06/10 18:12:21, Michael Forney wrote: > I think this should be a method on WaylandScreen. Done. https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/60001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:19: output_ = wl::Bind<wl_output>(registry, name, 1); On 2016/06/10 18:12:21, Michael Forney wrote: > I think you should bind the object in wayland_display.cc, and then pass the > wl_output* to the constructor, like wayland_pointer.cc does. > > Otherwise, if wl_registry_bind returns NULL, this objects is only partially > constructed, and the caller has no way to know. Done.
joone.hur@intel.com changed reviewers: + marcheu@chromium.org, rjkroege@chromium.org
https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/fake_server.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/fake_server.cc:292: } Maybe instead you could add a virtual method to Global, OnBind or something, and just override that for MockOutput. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:189: LOG(ERROR) << "Failed to bind to wl_output global"; Looks like I have a typo double-space, which has propogated to here. Please fix here and above. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:26: void WaylandScreen::SetOutputCompleteClosure(const base::Closure& closure) { Where do you expect this to be used? Perhaps some sort of *Delegate class might be more appropriate?
tonikitoo@chromium.org changed reviewers: + tonikitoo@chromium.org
https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:26: void WaylandScreen::SetOutputCompleteClosure(const base::Closure& closure) { On 2016/06/14 00:55:18, Michael Forney wrote: > Where do you expect this to be used? > > Perhaps some sort of *Delegate class might be more appropriate? +1 https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:57: screen->refresh_ = refresh; In order to keep it simple, can we not have refresh_ as a class member at this time, and add it if needed in the future? Or do you have immediate need for it planed in the follow up patches? https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:60: display::Screen::GetScreen()->OnOutputGeometryChanged(screen->name_, I like this. We eliminate the need for some screen_delegate classes that ozone/wayland has, right?
forney@ tonikitoo1@ thanks for the review. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/fake_server.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/fake_server.cc:292: } On 2016/06/14 00:55:18, Michael Forney wrote: > Maybe instead you could add a virtual method to Global, OnBind or something, and > just override that for MockOutput. Done. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_display.cc:189: LOG(ERROR) << "Failed to bind to wl_output global"; On 2016/06/14 00:55:18, Michael Forney wrote: > Looks like I have a typo double-space, which has propogated to here. Please fix > here and above. Done. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:26: void WaylandScreen::SetOutputCompleteClosure(const base::Closure& closure) { On 2016/06/14 15:53:33, tonikito wrote: > On 2016/06/14 00:55:18, Michael Forney wrote: > > Where do you expect this to be used? > > > > Perhaps some sort of *Delegate class might be more appropriate? > > +1 I removed this method by hiding the use of base::Closure. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:57: screen->refresh_ = refresh; On 2016/06/14 15:53:33, tonikito wrote: > In order to keep it simple, can we not have refresh_ as a class member at this > time, and add it if needed in the future? > > Or do you have immediate need for it planed in the follow up patches? Done. https://codereview.chromium.org/2042503002/diff/80001/ui/ozone/platform/wayla... ui/ozone/platform/wayland/wayland_screen.cc:60: display::Screen::GetScreen()->OnOutputGeometryChanged(screen->name_, On 2016/06/14 15:53:33, tonikito wrote: > I like this. We eliminate the need for some screen_delegate classes that > ozone/wayland has, right? Yes!
https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.cc:65: run_loop.Run(); I think base::WaitableEvent may be more appropriate for this use case.
Conceivably, I am confused as the intent of this CL. Is this for a desktop build or single-window build? If the former, how does this wire up to https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_scr... https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_display.cc (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_display.cc:28: WaylandDisplay::~WaylandDisplay() { I am concerned (per comment in WaylandScreen) that this code inverts the Chrome naming convention of Screen and Display. Conceivably Wayland is the opposite of Chrome but if so, please explain what Screen and Display mean here and (preferably) follow the Chrome conventions. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_display.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_display.h:42: const std::vector<WaylandScreen*>& GetScreenList() const; you need method comments https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.cc:62: void WaylandScreen::WaitforOutputAvailable() { Do you need to use a RunLoop? It is better to avoid them. Can you make this an asynchronous interface? https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:15: struct wl_output; It would be nice if we didn't have to expose Wayland concepts at the top level in an include file. Is there a reasonably easy way to do this? Perhaps add a TODO to address this concern eventually. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:20: // WaylandScreen objects keep track of the current outputs (screens/monitors) In chrome terminology, a Display corresponds to a single physical output CRT / LCD etc while a Screen is the aggregate of all the attached Display(s). Despite the extent to which this naming convention has confused me, please clarify the comments here to make it clear if this is a Screen or a Display. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:27: // Returns the active allocation of the screen. These are are in DIP coordinates or DDP? Please indicate.
https://codereview.chromium.org/2042503002/diff/100001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/display/screen.h#ne... ui/display/screen.h:92: virtual void OnOutputGeometryChanged(int32_t name, const gfx::Rect& rect) { } What does this name field mean? https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.cc:15: WaylandScreen::WaylandScreen(wl_registry* registry, registry is unused. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:20: // WaylandScreen objects keep track of the current outputs (screens/monitors) On 2016/06/14 23:29:49, rjkroege wrote: > In chrome terminology, a Display corresponds to a single physical output CRT / > LCD etc while a Screen is the aggregate of all the attached Display(s). Despite > the extent to which this naming convention has confused me, please clarify the > comments here to make it clear if this is a Screen or a Display. I think this class corresponds to a wl_output which is a single physical output, so Display is probably the proper term to use here. Perhaps joone used Screen instead because WaylandDisplay was taken by the object that refers to the wl_display object (which is the base object in a wayland connection, like Display in Xlib or xcb_connection_t in xcb). Here are some suggestions to resolve this: - Rename WaylandDisplay -> WaylandConnection (or something like that), and this class -> WaylandDisplay. - Rename this class to WaylandOutput to match the wayland object it wraps.
https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.cc:65: run_loop.Run(); On 2016/06/14 23:25:33, Michael Forney wrote: > I think base::WaitableEvent may be more appropriate for this use case. I tried to use base::WaitableEvent instead of base::RunLoop, but it blocks the message loop so there is no way to get any callbacks from Wayland server.
Patchset #5 (id:120001) has been deleted
forney@ rjkroege@ thanks for the review. Could you take a look at the updated CL? https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.cc (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.cc:15: WaylandScreen::WaylandScreen(wl_registry* registry, On 2016/06/14 23:44:54, Michael Forney wrote: > registry is unused. Removed. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.cc:62: void WaylandScreen::WaitforOutputAvailable() { On 2016/06/14 23:29:48, rjkroege wrote: > Do you need to use a RunLoop? It is better to avoid them. Can you make this an > asynchronous interface? RunLoop can be used in the test case so I will remove this method. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:15: struct wl_output; On 2016/06/14 23:29:49, rjkroege wrote: > It would be nice if we didn't have to expose Wayland concepts at the top level > in an include file. Is there a reasonably easy way to do this? Perhaps add a > TODO to address this concern eventually. They are already forward-declared in wayland_object.h so I will remove them here. https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:20: // WaylandScreen objects keep track of the current outputs (screens/monitors) On 2016/06/14 23:44:54, Michael Forney wrote: > On 2016/06/14 23:29:49, rjkroege wrote: > > In chrome terminology, a Display corresponds to a single physical output CRT / > > LCD etc while a Screen is the aggregate of all the attached Display(s). > Despite > > the extent to which this naming convention has confused me, please clarify the > > comments here to make it clear if this is a Screen or a Display. > > I think this class corresponds to a wl_output which is a single physical output, > so Display is probably the proper term to use here. > > Perhaps joone used Screen instead because WaylandDisplay was taken by the object > that refers to the wl_display object (which is the base object in a wayland > connection, like Display in Xlib or xcb_connection_t in xcb). > > Here are some suggestions to resolve this: > > - Rename WaylandDisplay -> WaylandConnection (or something like that), and this > class -> WaylandDisplay. > - Rename this class to WaylandOutput to match the wayland object it wraps. Thanks for suggestions. The name of the classes will be changed as follows: Rename WaylandDisplay -> WaylandConnection Rename WaylandScreen -> WaylandOutput https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:27: // Returns the active allocation of the screen. On 2016/06/14 23:29:49, rjkroege wrote: > These are are in DIP coordinates or DDP? Please indicate. This is the resolution of the screen.
Description was changed from ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (screens/monitors) that are available to Wayland clients. This is upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ========== to ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ==========
Description was changed from ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ========== to ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is a part of upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ==========
forney@ rjkroege@ ping?
https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:20: // WaylandScreen objects keep track of the current outputs (screens/monitors) On 2016/06/20 22:22:42, joone wrote: > On 2016/06/14 23:44:54, Michael Forney wrote: > > On 2016/06/14 23:29:49, rjkroege wrote: > > > In chrome terminology, a Display corresponds to a single physical output CRT > / > > > LCD etc while a Screen is the aggregate of all the attached Display(s). > > Despite > > > the extent to which this naming convention has confused me, please clarify > the > > > comments here to make it clear if this is a Screen or a Display. > > > > I think this class corresponds to a wl_output which is a single physical > output, > > so Display is probably the proper term to use here. > > > > Perhaps joone used Screen instead because WaylandDisplay was taken by the > object > > that refers to the wl_display object (which is the base object in a wayland > > connection, like Display in Xlib or xcb_connection_t in xcb). > > > > Here are some suggestions to resolve this: > > > > - Rename WaylandDisplay -> WaylandConnection (or something like that), and > this > > class -> WaylandDisplay. > > - Rename this class to WaylandOutput to match the wayland object it wraps. > > Thanks for suggestions. The name of the classes will be changed as follows: > Rename WaylandDisplay -> WaylandConnection > Rename WaylandScreen -> WaylandOutput Would you consider moving the rename of WaylandDisplay -> WaylandConnection to its own CL?
Can someone point me at a design doc for the wayland platform implementation please? Or add one as a markdown file in this directory? https://codereview.chromium.org/2042503002/diff/140001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2042503002/diff/140001/ui/display/screen.h#ne... ui/display/screen.h:92: virtual void OnOutputGeometryChanged(int32_t name, const gfx::Rect& rect) { } There should be no reason to add this here. https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/DEPS File ui/ozone/DEPS (right): https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/DEPS#newcode5 ui/ozone/DEPS:5: "+ui/display", this is a layering violation. No. https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/fake_server.h (right): https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/fake_server.h:15: #include "ui/display/display.h" What is this file for? Can you add a class comment? https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_connection.cc (right): https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:7: #include <xdg-shell-unstable-v5-client-protocol.h> As someone who may have to maintain this code, should I be concerned that it uses an unstable API?
https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_connection.cc (right): https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:7: #include <xdg-shell-unstable-v5-client-protocol.h> On 2016/07/08 21:59:19, rjkroege wrote: > As someone who may have to maintain this code, should I be concerned that it > uses an unstable API? I don't think you should be too concerned. xdg-shell is pretty much the standard desktop window interface currently in wayland, and the official wl_shell protocol has numerous problems. GTK+ only implements the xdg-shell client. It looks like v5 has been in use for around 1.5 years now. It's also implemented by https://cs.chromium.org/chromium/src/components/exo/wayland/server.cc. When v6 gets finalized and they update exosphere to use it, this ozone platform will need to be updated too. I suspect the changes required will be fairly minimal though since our usage of it is very basic.
On 2016/07/08 21:59:19, rjkroege wrote: > Can someone point me at a design doc for the wayland platform implementation > please? Or add one as a markdown file in this directory? > Here is the design doc: https://docs.google.com/document/d/1WPdUbaJ6_UVxsJ6hLnDpGR-eMvS6j-0tF_TZ62DMt...
On 2016/07/11 20:08:18, joone wrote: > On 2016/07/08 21:59:19, rjkroege wrote: > > Can someone point me at a design doc for the wayland platform implementation > > please? Or add one as a markdown file in this directory? > > > > Here is the design doc: > https://docs.google.com/document/d/1WPdUbaJ6_UVxsJ6hLnDpGR-eMvS6j-0tF_TZ62DMt... Thanks for the link. Sadly, this isn't really the info that I was looking for. I want to understand how Chrome uses the ozone/wayland platform to sit on top of an existing wayland server. In particular, what goes in each process, how multiple display support is organized, routing of events -- that sort of thing.
https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_screen.h (right): https://codereview.chromium.org/2042503002/diff/100001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_screen.h:20: // WaylandScreen objects keep track of the current outputs (screens/monitors) On 2016/07/07 21:18:01, Michael Forney wrote: > > Would you consider moving the rename of WaylandDisplay -> WaylandConnection to > its own CL? I've created a separate CL: https://codereview.chromium.org/2147523003/
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2042503002/diff/140001/ui/display/screen.h File ui/display/screen.h (right): https://codereview.chromium.org/2042503002/diff/140001/ui/display/screen.h#ne... ui/display/screen.h:92: virtual void OnOutputGeometryChanged(int32_t name, const gfx::Rect& rect) { } On 2016/07/08 21:59:19, rjkroege wrote: > There should be no reason to add this here. Removed. https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/DEPS File ui/ozone/DEPS (right): https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/DEPS#newcode5 ui/ozone/DEPS:5: "+ui/display", On 2016/07/08 21:59:19, rjkroege wrote: > this is a layering violation. No. Removed. https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/fake_server.h (right): https://codereview.chromium.org/2042503002/diff/140001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/fake_server.h:15: #include "ui/display/display.h" On 2016/07/08 21:59:19, rjkroege wrote: > What is this file for? Can you add a class comment? I removed display::Display dependency.
On 2016/07/11 21:46:21, rjkroege wrote: > On 2016/07/11 20:08:18, joone wrote: > > On 2016/07/08 21:59:19, rjkroege wrote: > > > Can someone point me at a design doc for the wayland platform implementation > > > please? Or add one as a markdown file in this directory? > > > > > > > Here is the design doc: > > > https://docs.google.com/document/d/1WPdUbaJ6_UVxsJ6hLnDpGR-eMvS6j-0tF_TZ62DMt... > > Thanks for the link. Sadly, this isn't really the info that I was looking for. I > want to understand how Chrome uses the ozone/wayland platform to sit on top of > an existing wayland server. In particular, what goes in each process, how > multiple display support is organized, routing of events -- that sort of thing. The browser process directly communicates with Wayland compositor. The current implementation only uses the software rendering so we need to route Wayland messages to the GPU process to create accelerated surfaces.
https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_output.h (right): https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_output.h:30: const base::Closure closure_; Why the closure? Why not just make OnOutputReadyForUse pure virtual? What will use this interface?
https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_output.h (right): https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_output.h:30: const base::Closure closure_; On 2016/07/20 21:49:00, Michael Forney wrote: > Why the closure? Why not just make OnOutputReadyForUse pure virtual? > > What will use this interface? Observer is used in the test case in order to wait for the output messages from Wayland Server.
On 2016/07/20 23:23:59, joone wrote: > https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... > File ui/ozone/platform/wayland/wayland_output.h (right): > > https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... > ui/ozone/platform/wayland/wayland_output.h:30: const base::Closure closure_; > On 2016/07/20 21:49:00, Michael Forney wrote: > > Why the closure? Why not just make OnOutputReadyForUse pure virtual? > > > > What will use this interface? > > Observer is used in the test case in order to wait for the output messages from > Wayland Server. Observer is used to exit the base::RunLoop using the closure.
The CQ bit was checked by joone.hur@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi forney@, I added a subclass of Observer to wayland_connection_unittest.cc. Please take a look at the CL.
I forget to send these from an older version of the CL. If they make no sense, please ignore. I will provide updated comments. https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/fake_server.h (right): https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/fake_server.h:96: virtual void OnBind() {} Method docs? My dream is to have docs approaching base/callback.h for all of our code. So At least a sentence or two to start? Yes, the rest of the code isn't documented but no need to add yet more undocumented code? Sorry if this seems tedious. :-) https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/fake_server.h:132: class MockOutput : public Global { Say what Global is please. https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_connection.cc (right): https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:30: delete output; Keep WaylandOutput in a self-deleting type like std::unique_ptr? https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:31: output_list_.clear(); redundant I would think.
Hi rjkroege@ I've updated the CL. Please take a look it. Thanks! https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/fake_server.h (right): https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/fake_server.h:96: virtual void OnBind() {} On 2016/07/25 23:45:46, rjkroege wrote: > Method docs? My dream is to have docs approaching base/callback.h for all of > our code. So At least a sentence or two to start? Yes, the rest of the code > isn't documented but no need to add yet more undocumented code? Sorry if this > seems tedious. :-) Done. https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/fake_server.h:132: class MockOutput : public Global { On 2016/07/25 23:45:46, rjkroege wrote: > Say what Global is please. Done. https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_connection.cc (right): https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:30: delete output; On 2016/07/25 23:45:46, rjkroege wrote: > Keep WaylandOutput in a self-deleting type like std::unique_ptr? Done. https://codereview.chromium.org/2042503002/diff/160001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:31: output_list_.clear(); On 2016/07/25 23:45:46, rjkroege wrote: > redundant I would think. Done.
lgtm with two nits. Please fix before CQ-ing. https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_connection.cc (right): https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:184: } else if (strcmp(interface, "wl_output") == 0) { nit: Chrome style is probably to use something from base. https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_output.h (right): https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_output.h:34: // Callback functions that allows the display to initialize the screen's in Chrome naming, Display == 1 physical monitor, Screen == aggregate of attached Display. Are the screen/display used that way here? It would seem not. A screen cannot have a unique resolution: imagine a high-dpi laptop with a low-dpi attached display. Could you make sure that this isn't ambiguous?
Thanks for review! https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_connection.cc (right): https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_connection.cc:184: } else if (strcmp(interface, "wl_output") == 0) { On 2016/09/25 14:44:31, rjkroege wrote: > nit: Chrome style is probably to use something from base. Done. https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... File ui/ozone/platform/wayland/wayland_output.h (right): https://codereview.chromium.org/2042503002/diff/240001/ui/ozone/platform/wayl... ui/ozone/platform/wayland/wayland_output.h:34: // Callback functions that allows the display to initialize the screen's On 2016/09/25 14:44:31, rjkroege wrote: > in Chrome naming, Display == 1 physical monitor, Screen == aggregate of attached > Display. Are the screen/display used that way here? It would seem not. A screen > cannot have a unique resolution: imagine a high-dpi laptop with a low-dpi > attached display. > > Could you make sure that this isn't ambiguous? Done.
The CQ bit was checked by joone.hur@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org Link to the patchset: https://codereview.chromium.org/2042503002/#ps260001 (title: "[For landding] update comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is a part of upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ========== to ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is a part of upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is a part of upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output ========== to ========== ozone/platform/wayland: Add support for wl_output_interface WaylandScreen is used to keep track of the current outputs (display) that are available to Wayland clients. This is a part of upstream work for the ozone-wayland project: https://github.com/01org/ozone-wayland BUG=578890 TEST=WaylandDisplayTest.Output Committed: https://crrev.com/de57ce2d6a43b6593c770f583ccf06b8779e3ccb Cr-Commit-Position: refs/heads/master@{#421157} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/de57ce2d6a43b6593c770f583ccf06b8779e3ccb Cr-Commit-Position: refs/heads/master@{#421157} |
