Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2177823002:
X11: Add window cache
|
Created:
4 years, 5 months ago by Tom (Use chromium acct) Modified:
4 years, 2 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org, grt+watch_chromium.org, wfh+watch_chromium.org, Michael Moss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionX11: Add window cache
Design doc:
https://docs.google.com/document/d/1RhfO2Z4dorbR6Ytlv6DGob9Nqpa7JlfsC0XtFi_am0U/edit#
BUG=634084
Patch Set 1 #Patch Set 2 : Refactor and add unit tests #Patch Set 3 : Remove changes from dependent patchset #
Total comments: 6
Patch Set 4 : Refactor #
Total comments: 13
Patch Set 5 : Move Request code to source file #Patch Set 6 : Reset when in a bad state & handle EINTR #Patch Set 7 : Don't open another xcb_connection #Patch Set 8 : Fix test compilation #
Total comments: 5
Patch Set 9 : Fix API #
Messages
Total messages: 43 (24 generated)
Description was changed from ========== X11: Add window cache ========== to ========== X11: Add window cache Depends on https://codereview.chromium.org/2163623003/ ==========
thomasanderson@google.com changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:220: void XWindowCache::PrintWindowTreeForTestingAux(Window* window, This was useful when developing, but it causes a presubmit error for using printf. Should this be changed to use logging instead, or just get rid of this? https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:256: void XWindowCache::OnConnectionReadable() { So we process events last as the comment points out. Would it be reliable to use the sequence number given in each reply/event to merge the events/replies back into a chronological stream? The X11 protocol spec says that events "contains the least significant 16 bits of the sequence number of the last request issued by the client that was (or is currently being) processed by the server." It's the "or is currently being" that worries me. https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.h (right): https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.h:41: // Clients should not modify any members of Window. Not sure what the best API design is here. XWindowCache::GetWindow returns const Window*, but Window::children is a list of Window*. The list is immutable but the elements are not. I still want users to be able to get the number of elements in the list, iterate over the list, etc. It would be a lot of code to add support for all of these use cases manually, so I took the lazy option and added documentation saying clients should treat const Window* as logically const. Similar issues for Window::properties, Window::parent, and Property::data.
Patchset #4 (id:60001) has been deleted
thomasanderson@google.com changed reviewers: + danakj@chromium.org, erg@chromium.org
Ping. sadrul@ for OWNERS approval + erg@ and danakj@ (I know you're currently OOO) for X11 stuff if interested https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:220: void XWindowCache::PrintWindowTreeForTestingAux(Window* window, On 2016/07/26 20:17:42, Tom Anderson wrote: > This was useful when developing, but it causes a presubmit error for using > printf. > > Should this be changed to use logging instead, or just get rid of this? No longer applicable. Removed. https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:256: void XWindowCache::OnConnectionReadable() { On 2016/07/26 20:17:42, Tom Anderson wrote: > So we process events last as the comment points out. > > Would it be reliable to use the sequence number given in each reply/event to > merge the events/replies back into a chronological stream? > > The X11 protocol spec says that events "contains the least significant 16 bits > of the sequence number of the last request issued by the client that was (or is > currently being) processed by the server." It's the "or is currently being" > that worries me. No longer applicable. Reordered events/replies https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.h (right): https://codereview.chromium.org/2177823002/diff/40001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.h:41: // Clients should not modify any members of Window. On 2016/07/26 20:17:43, Tom Anderson wrote: > Not sure what the best API design is here. > > XWindowCache::GetWindow returns const Window*, but Window::children is a list of > Window*. The list is immutable but the elements are not. I still want users to > be able to get the number of elements in the list, iterate over the list, etc. > It would be a lot of code to add support for all of these use cases manually, so > I took the lazy option and added documentation saying clients should treat const > Window* as logically const. > > Similar issues for Window::properties, Window::parent, and Property::data. Still need help on this
Description was changed from ========== X11: Add window cache Depends on https://codereview.chromium.org/2163623003/ ========== to ========== X11: Add window cache Depends on https://codereview.chromium.org/2163623003/ BUG=634084 ==========
derat@chromium.org changed reviewers: + derat@chromium.org
please write a design doc for this change, if there isn't one already. https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:209: connection_ = xcb_connect(DisplayString(display), nullptr); this is establishing a second connection to the X server. this makes me anxious; races are likely when doing this.
On 2016/08/06 00:49:22, Daniel Erat wrote: > please write a design doc for this change, if there isn't one already. > There was an email thread about it, I'll cc you. erg@ also suggested a design doc, so I'll have to get started on that > https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... > File ui/base/x/x11_window_cache.cc (right): > > https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... > ui/base/x/x11_window_cache.cc:209: connection_ = > xcb_connect(DisplayString(display), nullptr); > this is establishing a second connection to the X server. this makes me anxious; > races are likely when doing this. Yep, the second connection is by design. The event queue can only be owned by one of xlib or xcb, and since it would be a big change to switch over our main event loop to use xcb events, we're forced to use a separate connection. I'm aware of the possible races due to multiple connections. You can't do something like change the window size on the main XDisplay and expect the cache to immediately reflect that. This cache is more for windows not managed by Chrome. An example use case would be X11TopmostWindowFinder. But if you really need to synchronize the connections, I provide SynchronizeWith(XDisplay).
https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:244: // TODO(thomasanderson): should we handle EAGAIN? I suspect that you might. If you EAGAIN here, will you just do nothing and reenter the poll()? https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.h (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.h:104: struct Request { Forward declare Request and move it and all its subclasses to the cc file? https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.h:105: virtual void OnReply(void* reply, XWindowCache* cache) = 0; IIRC, you should be passing an xcb_generic_reply_t* instead of void*. https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache_unittest.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache_unittest.cc:16: class XWindowCacheTest : public testing::Test { Does this test need to be an interactive ui test? It looks like you're creating windows against a real server.
Description was changed from ========== X11: Add window cache Depends on https://codereview.chromium.org/2163623003/ BUG=634084 ========== to ========== X11: Add window cache Depends on https://codereview.chromium.org/2163623003/ Draft of design doc https://docs.google.com/document/d/1RhfO2Z4dorbR6Ytlv6DGob9Nqpa7JlfsC0XtFi_am... BUG=634084 ==========
On 2016/08/06 00:49:22, Daniel Erat wrote: > please write a design doc for this change, if there isn't one already. > Draft of design doc. PLMK which parts need clarification/changing https://docs.google.com/document/d/1RhfO2Z4dorbR6Ytlv6DGob9Nqpa7JlfsC0XtFi_am...
thanks! mind granting permission to leave comments? it looks like i currently can't leave comments with my google.com account and can't even view it with my chromium.org account.
On 2016/08/08 21:46:48, Daniel Erat wrote: > thanks! mind granting permission to leave comments? it looks like i currently > can't leave comments with my http://google.com account and can't even view it with my > http://chromium.org account. oops. You can comment now :)
https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:209: connection_ = xcb_connect(DisplayString(display), nullptr); On 2016/08/06 00:49:22, Daniel Erat wrote: > this is establishing a second connection to the X server. this makes me anxious; > races are likely when doing this. Acknowledged. https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:244: // TODO(thomasanderson): should we handle EAGAIN? On 2016/08/08 18:17:28, Elliot Glaysher wrote: > I suspect that you might. If you EAGAIN here, will you just do nothing and > reenter the poll()? yeah pretty much. So I guess the best thing to do is to keep polling while poll returns -1 (which could even be because we got a signal), with a timeout? poll() doesn't tell us how much time is left like select() does, so we'd have to manage the timeout ourselves https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.h (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.h:104: struct Request { On 2016/08/08 18:17:28, Elliot Glaysher wrote: > Forward declare Request and move it and all its subclasses to the cc file? Done. https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.h:105: virtual void OnReply(void* reply, XWindowCache* cache) = 0; On 2016/08/08 18:17:28, Elliot Glaysher wrote: > IIRC, you should be passing an xcb_generic_reply_t* instead of void*. Done. https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache_unittest.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache_unittest.cc:16: class XWindowCacheTest : public testing::Test { On 2016/08/08 18:17:28, Elliot Glaysher wrote: > Does this test need to be an interactive ui test? It looks like you're creating > windows against a real server. yeah, the tests do create (and map) windows. ui/base/x/selection_requestor_unittest.cc also makes windows, but I'll let you make the definitive decision about this.
https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache_unittest.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache_unittest.cc:16: class XWindowCacheTest : public testing::Test { On 2016/08/08 23:23:38, Tom Anderson wrote: > On 2016/08/08 18:17:28, Elliot Glaysher wrote: > > Does this test need to be an interactive ui test? It looks like you're > creating > > windows against a real server. > > yeah, the tests do create (and map) windows. > ui/base/x/selection_requestor_unittest.cc also makes windows, but I'll let you > make the definitive decision about this. So do compositor_unittests.
https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache.cc:244: // TODO(thomasanderson): should we handle EAGAIN? On 2016/08/08 23:23:38, Tom Anderson wrote: > On 2016/08/08 18:17:28, Elliot Glaysher wrote: > > I suspect that you might. If you EAGAIN here, will you just do nothing and > > reenter the poll()? > > yeah pretty much. So I guess the best thing to do is to keep polling while poll > returns -1 (which could even be because we got a signal), with a timeout? > poll() doesn't tell us how much time is left like select() does, so we'd have to > manage the timeout ourselves Fixed. Turns out we have a convention just to handle EINTR, and there's macros for this in //base. https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... File ui/base/x/x11_window_cache_unittest.cc (right): https://codereview.chromium.org/2177823002/diff/80001/ui/base/x/x11_window_ca... ui/base/x/x11_window_cache_unittest.cc:16: class XWindowCacheTest : public testing::Test { On 2016/08/09 22:18:44, danakj wrote: > On 2016/08/08 23:23:38, Tom Anderson wrote: > > On 2016/08/08 18:17:28, Elliot Glaysher wrote: > > > Does this test need to be an interactive ui test? It looks like you're > > creating > > > windows against a real server. > > > > yeah, the tests do create (and map) windows. > > ui/base/x/selection_requestor_unittest.cc also makes windows, but I'll let you > > make the definitive decision about this. > > So do compositor_unittests. Ok, just leaving as a unit test in that case.
I've decided that derat@ is right and it's better to use the main XDisplay for this cache as well. So this means a few things need to change: 1. Parts of XWindowCache need to be rewritten 2. The glib CL that I haven't sent out yet needs to be scrapped https://codereview.chromium.org/2199063003/ 3. There needs to be logic in X11EventSource to handle xcb replies, instead of doing this in XWindowCache 4. The out-of-order processing of events from BlockUntilWindowMapped needs to be removed (This is OK, we just don't dispatch events, but peek at them instead) 5. There needs to be a way for XWindowCache to select events on windows that we've already selected events on. 5 is the only thing that makes me worried, because we'll have to trust clients to use the X11ForeignWindowManager in //ui/base to manage the event masks and not use XSelectInput directly. danakj@: dpranke@ said a clang plugin could work to forbid calling XSelectInput (and to make sure the CWEventMask bit is unset during XCreateWindow) or the xcb equivalent, and he mentioned you've written some in the past. Do you think this would be a good option?
On Mon, Aug 29, 2016 at 3:46 PM, <thomasanderson@google.com> wrote: > I've decided that derat@ is right and it's better to use the main > XDisplay for > this cache as well. > > So this means a few things need to change: > 1. Parts of XWindowCache need to be rewritten > 2. The glib CL that I haven't sent out yet needs to be scrapped > https://codereview.chromium.org/2199063003/ > 3. There needs to be logic in X11EventSource to handle xcb replies, > instead of > doing this in XWindowCache > 4. The out-of-order processing of events from BlockUntilWindowMapped needs > to be > removed > (This is OK, we just don't dispatch events, but peek at them instead) > 5. There needs to be a way for XWindowCache to select events on windows > that > we've already selected events on. > > 5 is the only thing that makes me worried, because we'll have to trust > clients > to use the X11ForeignWindowManager in //ui/base to manage the event masks > and > not use XSelectInput directly. > > danakj@: dpranke@ said a clang plugin could work to forbid calling > XSelectInput > (and to make sure the CWEventMask bit is unset during XCreateWindow) or > the xcb > equivalent, and he mentioned you've written some in the past. Do you think > this > would be a good option? > Maybe a bit overkill, a PRESUBMIT that checks for CWEventMask or XSelectInput might be good enough? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/29 22:58:13, danakj wrote: > On Mon, Aug 29, 2016 at 3:46 PM, <mailto:thomasanderson@google.com> wrote: > > > I've decided that derat@ is right and it's better to use the main > > XDisplay for > > this cache as well. > > > > So this means a few things need to change: > > 1. Parts of XWindowCache need to be rewritten > > 2. The glib CL that I haven't sent out yet needs to be scrapped > > https://codereview.chromium.org/2199063003/ > > 3. There needs to be logic in X11EventSource to handle xcb replies, > > instead of > > doing this in XWindowCache > > 4. The out-of-order processing of events from BlockUntilWindowMapped needs > > to be > > removed > > (This is OK, we just don't dispatch events, but peek at them instead) > > 5. There needs to be a way for XWindowCache to select events on windows > > that > > we've already selected events on. > > > > 5 is the only thing that makes me worried, because we'll have to trust > > clients > > to use the X11ForeignWindowManager in //ui/base to manage the event masks > > and > > not use XSelectInput directly. > > > > danakj@: dpranke@ said a clang plugin could work to forbid calling > > XSelectInput > > (and to make sure the CWEventMask bit is unset during XCreateWindow) or > > the xcb > > equivalent, and he mentioned you've written some in the past. Do you think > > this > > would be a good option? > > > > Maybe a bit overkill, a PRESUBMIT that checks for CWEventMask or > XSelectInput might be good enough? > That should work. Don't know why I didn't think of that before :)
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by thomasanderson@google.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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by thomasanderson@google.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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Pinging reviewers Since the last update, I've refactored this CL so that we don't have to open a second connection. As a bonus, we now get to handle server replies asynchronously in Chrome's event loop. Just one thing needs to be done before this can be used: using the XForeignWindowManager to handle event mask selection. I'll be sending out a series of CLs today for this.
some high-level comments https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... File ui/base/x/x11_window_cache.cc (right): https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... ui/base/x/x11_window_cache.cc:582: windows_.erase(id); are you leaking Window objects here? please use std::unique_ptr (or just have Window be a simple copyable struct that can be stored directly in the map). https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... File ui/base/x/x11_window_cache.h (right): https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... ui/base/x/x11_window_cache.h:61: GetWindowAttributesRequest* attributes_request; storing information about requests inside of the Window and Property classes like this feels confusing to me -- ownership of these objects is unclear and there's a lot of reaching up and down in the class hierarchy and modifying other objects' members directly. i'd suggest exploring an approach where the Property and Window structs only hold the information that's needed by clients (id, parent, override_redirect, geometry data, etc.) and all of the implementation lives inside of XWindowCache or other private classes within it. https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... ui/base/x/x11_window_cache.h:81: const Property* GetProperty(XID atom) const; see the style guide's guidance on using structs vs. classes: https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes there also shouldn't be private-esque members like |cache_| inside of a struct. https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... ui/base/x/x11_window_cache.h:90: const Window* GetWindow(XID id) const; please document the lifetime of the returned object. can the caller hang onto it at all, or is it possible it'll be deleted as soon as events are pulled off of the X queue? https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... ui/base/x/x11_window_cache.h:128: xcb_atom_t net_wm_icon_; put DISALLOW_COPY_AND_ASSIGN at the end of classes like this one that shouldn't be copied
On 2016/09/06 20:35:59, Daniel Erat wrote: > some high-level comments > > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > File ui/base/x/x11_window_cache.cc (right): > > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > ui/base/x/x11_window_cache.cc:582: windows_.erase(id); > are you leaking Window objects here? please use std::unique_ptr (or just have > Window be a simple copyable struct that can be stored directly in the map). > Windows have a list of unique_ptr that own children windows. The window cache has a unique_ptr to the root, so no memory will be leaked. > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > File ui/base/x/x11_window_cache.h (right): > > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > ui/base/x/x11_window_cache.h:61: GetWindowAttributesRequest* attributes_request; > storing information about requests inside of the Window and Property classes > like this feels confusing to me -- ownership of these objects is unclear and > there's a lot of reaching up and down in the class hierarchy and modifying other > objects' members directly. > > i'd suggest exploring an approach where the Property and Window structs only > hold the information that's needed by clients (id, parent, override_redirect, > geometry data, etc.) and all of the implementation lives inside of XWindowCache > or other private classes within it. > > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > ui/base/x/x11_window_cache.h:81: const Property* GetProperty(XID atom) const; > see the style guide's guidance on using structs vs. classes: > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes > > there also shouldn't be private-esque members like |cache_| inside of a struct. > > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > ui/base/x/x11_window_cache.h:90: const Window* GetWindow(XID id) const; > please document the lifetime of the returned object. can the caller hang onto it > at all, or is it possible it'll be deleted as soon as events are pulled off of > the X queue? > > https://codereview.chromium.org/2177823002/diff/220001/ui/base/x/x11_window_c... > ui/base/x/x11_window_cache.h:128: xcb_atom_t net_wm_icon_; > put DISALLOW_COPY_AND_ASSIGN at the end of classes like this one that shouldn't > be copied Thanks for your comments on client windows vs implementation windows. I did think it was odd having this implementation detail being given to clients. I like your proposed solution to separate out these structures and I think it should work.
Patchset #9 (id:240001) has been deleted
Description was changed from ========== X11: Add window cache Depends on https://codereview.chromium.org/2163623003/ Draft of design doc https://docs.google.com/document/d/1RhfO2Z4dorbR6Ytlv6DGob9Nqpa7JlfsC0XtFi_am... BUG=634084 ========== to ========== X11: Add window cache Design doc: https://docs.google.com/document/d/1RhfO2Z4dorbR6Ytlv6DGob9Nqpa7JlfsC0XtFi_am... BUG=634084 ==========
The CQ bit was checked by thomasanderson@google.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...
derat@ I've fixed the API, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/20 02:01:18, Tom Anderson wrote:
> derat@ I've fixed the API, PTAL
it feels like the implementation is still spread across XWindowCache and its
nested classes in a way that feels a bit confusing to me. for example, Property
reaches into XWindowCache directly, while GetPropertyRequest reaches into
Property (and also XWindowCache) directly:
554 XWindowCache::Property::Property(xcb_atom_t name,
555 Window* window,
556 XWindowCache* cache)
557 : name_(name), property_request_(nullptr), data_(nullptr),
cache_(cache) {
558 cache->event_source_->EnqueueRequest(
559 new GetPropertyRequest(window, this, name, cache));
560 xcb_flush(cache->connection_);
561 }
562
563 XWindowCache::Property::~Property() {
564 if (property_request_)
565 cache_->event_source_->DiscardRequest(property_request_);
566
567 delete[] data_;
568 }
569
570 bool XWindowCache::Property::Validate() {
571 return (!property_request_ ||
572 cache_->event_source_->DispatchRequestNow(property_request_));
573 }
...
191 struct XWindowCache::GetPropertyRequest : public X11EventSource::Request {
...
211 void OnReply(xcb_generic_reply_t* r, xcb_generic_error_t* error) override
{
...
237 property_->property_request_ = nullptr;
238 property_->type_ = reply->type;
239 DCHECK(reply->format == 8 || reply->format == 16 || reply->format ==
32);
240 property_->format_ = reply->format;
241 property_->length_ = xcb_get_property_value_length(reply);
242 uint32_t data_bytes = property_->length_ * (property_->format_ / 8);
243 property_->data_ = new uint8_t[data_bytes];
244 std::memcpy(property_->data_, xcb_get_property_value(reply),
data_bytes);
245 }
i think i was trying to suggest earlier that all of the implementation live in
XWindowCache (and its hidden *Request classes), so that Property and Window are
just structs that contain public data members and no methods.
is there a reason why something like that wouldn't work? it seems like it'd be
much easier to follow:
- XWindowCache creates and enqueues requests itself
- XWindowCache owns Property and Window objects
- requests update Property and Window objects
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
