Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2313033002:
Refactor X11ForeignWindowManager (Reland) (Closed)
|
Created:
4 years, 3 months ago by Tom (Use chromium acct) Modified:
4 years, 3 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org, tfarina, grt+watch_chromium.org, dcheng, pennymac+watch_chromium.org, wfh+watch_chromium.org, Michael Moss Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL renames XForeignWindowManager to XWindowEventManager since it will be
used on local windows now. Get rid of OnWindowDestroyed since this will be
used by the XWindowCache for windows other than top-level windows as well.
Finally, refactor the old Request implementation to use a Mask instead.
This is needed by the XWindowCache which will select events on all windows, and
therefore needs to know which masks we've already selected on Chrome windows.
This will be the first in the line of 3 CLs.
1. Refactor X11ForeignWindowManager
2. Update Chrome code to use the XWindowManager instead of calling XSelectInput
manually
3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or
XCB_CW_EVENT_MASK is not added to new code.
BUG=634084
Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415
Committed: https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2
Cr-Original-Commit-Position: refs/heads/master@{#417793}
Cr-Commit-Position: refs/heads/master@{#418005}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rename to XWindowEventmanager #
Total comments: 21
Patch Set 3 : Remove nested class #Patch Set 4 : Refactor #Patch Set 5 : Fix bug #
Total comments: 6
Patch Set 6 : Make Select/Deselect private #
Total comments: 4
Patch Set 7 : Nits #Patch Set 8 : Update expected_build_deps #Patch Set 9 : Rebase #Dependent Patchsets: Messages
Total messages: 59 (39 generated)
Description was changed from ========== BUG= ========== to ========== This CL renames XForeignWindowManager to XWindowManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. ==========
thomasanderson@google.com changed reviewers: + derat@chromium.org, erg@chromium.org, thestig@chromium.org
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...
Description was changed from ========== This CL renames XForeignWindowManager to XWindowManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. ========== to ========== This CL renames XForeignWindowManager to XWindowManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. ==========
erg@ + derat@ for X11 stuff thestig@ for expected_build_deps
https://codereview.chromium.org/2313033002/diff/1/ui/base/x/x11_window_manager.h File ui/base/x/x11_window_manager.h (right): https://codereview.chromium.org/2313033002/diff/1/ui/base/x/x11_window_manage... ui/base/x/x11_window_manager.h:27: class UI_BASE_X_EXPORT XWindowManager { The name XWindowManager is a bit deceptive for anyone who knows about X. Something like XwindowSelectionManager? XWindowEventManager?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
Also added ScopedEventSelector because it was useful for the dependant patch set https://codereview.chromium.org/2313033002/diff/1/ui/base/x/x11_window_manager.h File ui/base/x/x11_window_manager.h (right): https://codereview.chromium.org/2313033002/diff/1/ui/base/x/x11_window_manage... ui/base/x/x11_window_manager.h:27: class UI_BASE_X_EXPORT XWindowManager { On 2016/09/06 20:11:56, Elliot Glaysher wrote: > The name XWindowManager is a bit deceptive for anyone who knows about X. > Something like XwindowSelectionManager? XWindowEventManager? Done. Renamed to XWindowEventManager
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
looks like a new patch set went up while i was writing comments, so some of these might be stale. please also update the CL description to match the new class name. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.cc (right): https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.cc:41: old_mask_ = mask_map_[xid].ToMask(); caching this in a single member feels a bit scary. why not just copy it to a local in SelectEvents and DeselectEvents and then pass the old mask to AfterMaskChanged? https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:26: // created by Chrome. This class allows several clients to select events nit: maybe "... allows multiple clients within Chrome to select ..."? i think that that helps a bit to make it clear that this isn't referring to X clients, i.e. connections to the X server. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:32: class ScopedEventSelector { nit: add a comment documenting what this does https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:42: // Guarantees that events on |event_mask| will be reported to Chrome. nit: s/on/in/ https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:43: void SelectEvents(XID xid, uint32_t event_mask); under what circumstances would a caller use this instead of creating a ScopedEventSelector? if callers should only use ScopedEventSelector, i think you can make these methods be private. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:52: struct MultiMask { this should be a class instead of a struct, since it isn't a simple wrapper around data. please add a brief comment describing what it does, and consider just forward-declaring it here and putting the full definition in the .cc file. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:62: unsigned int mask_bits[kMaskSize]; rename to mask_bits_; also just make this be an int https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:63: }; DISALLOW_COPY_AND_ASSIGN(MultiMask); https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:68: void BeforeMaskChanged(XID xid); nit: add comments documenting what these methods do https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:74: std::map<XID, MultiMask> mask_map_; std::unique_ptr<MultiMask>
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.cc (right): https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.cc:41: old_mask_ = mask_map_[xid].ToMask(); On 2016/09/07 00:37:33, Daniel Erat wrote: > caching this in a single member feels a bit scary. why not just copy it to a > local in SelectEvents and DeselectEvents and then pass the old mask to > AfterMaskChanged? Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:26: // created by Chrome. This class allows several clients to select events On 2016/09/07 00:37:33, Daniel Erat wrote: > nit: maybe "... allows multiple clients within Chrome to select ..."? i think > that that helps a bit to make it clear that this isn't referring to X clients, > i.e. connections to the X server. Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:32: class ScopedEventSelector { On 2016/09/07 00:37:33, Daniel Erat wrote: > nit: add a comment documenting what this does Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:42: // Guarantees that events on |event_mask| will be reported to Chrome. On 2016/09/07 00:37:33, Daniel Erat wrote: > nit: s/on/in/ Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:43: void SelectEvents(XID xid, uint32_t event_mask); On 2016/09/07 00:37:33, Daniel Erat wrote: > under what circumstances would a caller use this instead of creating a > ScopedEventSelector? if callers should only use ScopedEventSelector, i think you > can make these methods be private. There are some classes where an X window is not bound to any object's lifetime. For example in X11WholeScreenMoveLoop, we can have X11WholeScreenMoveLoop() RunMoveLoop() - creates a window EndMoveLoop() - destroys the window RunMoveLoop() - creates a window EndMoveLoop() - destroys the window ~X11WholeScreenMoveLoop() https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:52: struct MultiMask { On 2016/09/07 00:37:33, Daniel Erat wrote: > this should be a class instead of a struct, since it isn't a simple wrapper > around data. please add a brief comment describing what it does, and consider > just forward-declaring it here and putting the full definition in the .cc file. Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:62: unsigned int mask_bits[kMaskSize]; On 2016/09/07 00:37:33, Daniel Erat wrote: > rename to mask_bits_; also just make this be an int Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:63: }; On 2016/09/07 00:37:33, Daniel Erat wrote: > DISALLOW_COPY_AND_ASSIGN(MultiMask); Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:68: void BeforeMaskChanged(XID xid); On 2016/09/07 00:37:33, Daniel Erat wrote: > nit: add comments documenting what these methods do Done. https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:74: std::map<XID, MultiMask> mask_map_; On 2016/09/07 00:37:33, Daniel Erat wrote: > std::unique_ptr<MultiMask> Done.
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 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...
https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:43: void SelectEvents(XID xid, uint32_t event_mask); On 2016/09/07 18:02:23, Tom Anderson wrote: > On 2016/09/07 00:37:33, Daniel Erat wrote: > > under what circumstances would a caller use this instead of creating a > > ScopedEventSelector? if callers should only use ScopedEventSelector, i think > you > > can make these methods be private. > > There are some classes where an X window is not bound to any object's lifetime. > For example in X11WholeScreenMoveLoop, we can have > > X11WholeScreenMoveLoop() > RunMoveLoop() - creates a window > EndMoveLoop() - destroys the window > RunMoveLoop() - creates a window > EndMoveLoop() - destroys the window > ~X11WholeScreenMoveLoop() okay, but can't X11WholeScreenMoveLoop just have an std::unique_ptr<XScopedEventSelector> member and init and reset it as needed? i'm concerned about the case where one class has a bug in its calls to SelectEvents() and DeselectEvents(), which ends up introducing unpredictable and hard-to-track-down issues around other classes not receiving events. making XScopedEventSelector be the only interface for selecting events feels like a good way to prevent this. https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.cc (right): https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.cc:75: std::unique_ptr<MultiMask>& mask = mask_map_[xid]; nit: move this up one line and use it to get old_mask https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.cc:101: XScopedEventSelector::XScopedEventSelector(XID xid, uint32_t event_mask) nit: move these to the top of the file to match the order in the header https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2016?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/07 20:00:07, Daniel Erat wrote: > https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... > File ui/base/x/x11_window_event_manager.h (right): > > https://codereview.chromium.org/2313033002/diff/20001/ui/base/x/x11_window_ev... > ui/base/x/x11_window_event_manager.h:43: void SelectEvents(XID xid, uint32_t > event_mask); > On 2016/09/07 18:02:23, Tom Anderson wrote: > > On 2016/09/07 00:37:33, Daniel Erat wrote: > > > under what circumstances would a caller use this instead of creating a > > > ScopedEventSelector? if callers should only use ScopedEventSelector, i think > > you > > > can make these methods be private. > > > > There are some classes where an X window is not bound to any object's > lifetime. > > For example in X11WholeScreenMoveLoop, we can have > > > > X11WholeScreenMoveLoop() > > RunMoveLoop() - creates a window > > EndMoveLoop() - destroys the window > > RunMoveLoop() - creates a window > > EndMoveLoop() - destroys the window > > ~X11WholeScreenMoveLoop() > > okay, but can't X11WholeScreenMoveLoop just have an > std::unique_ptr<XScopedEventSelector> member and init and reset it as needed? > i'm concerned about the case where one class has a bug in its calls to > SelectEvents() and DeselectEvents(), which ends up introducing unpredictable and > hard-to-track-down issues around other classes not receiving events. making > XScopedEventSelector be the only interface for selecting events feels like a > good way to prevent this. > Fair enough, done. https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.cc (right): https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.cc:75: std::unique_ptr<MultiMask>& mask = mask_map_[xid]; On 2016/09/07 20:00:07, Daniel Erat wrote: > nit: move this up one line and use it to get old_mask Done. https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.cc:101: XScopedEventSelector::XScopedEventSelector(XID xid, uint32_t event_mask) On 2016/09/07 20:00:07, Daniel Erat wrote: > nit: move these to the top of the file to match the order in the header Done. https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/80001/ui/base/x/x11_window_ev... ui/base/x/x11_window_event_manager.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/09/07 20:00:07, Daniel Erat wrote: > nit: 2016? Done.
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...
Description was changed from ========== This CL renames XForeignWindowManager to XWindowManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. ==========
thanks, lgtm with a few comments https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... File ui/base/x/x11_window_event_manager.cc (right): https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... ui/base/x/x11_window_event_manager.cc:90: XWindowEventManager::~XWindowEventManager() {} nit: move these above SelectEvents to match the header order https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... ui/base/x/x11_window_event_manager.h:35: }; DISALLOW_COPY_AND_ASSIGN(XScopedEventSelector)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Pinging thestig@. Also, are there any trybots I can add for official builders? The changes in expected_deps might break on precise, and I don't want to wait until a revert to find out. https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... File ui/base/x/x11_window_event_manager.cc (right): https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... ui/base/x/x11_window_event_manager.cc:90: XWindowEventManager::~XWindowEventManager() {} On 2016/09/07 21:45:06, Daniel Erat wrote: > nit: move these above SelectEvents to match the header order Done. https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... File ui/base/x/x11_window_event_manager.h (right): https://codereview.chromium.org/2313033002/diff/100001/ui/base/x/x11_window_e... ui/base/x/x11_window_event_manager.h:35: }; On 2016/09/07 21:45:06, Daniel Erat wrote: > DISALLOW_COPY_AND_ASSIGN(XScopedEventSelector) Done.
Description was changed from ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 ==========
lgtm
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2313033002/#ps120001 (title: "Nits")
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 thomasanderson@google.com
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, erg@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2313033002/#ps130001 (title: "Update expected_build_deps")
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 ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Description was changed from ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:130001) has been created in https://codereview.chromium.org/2326223002/ by dmurph@chromium.org. The reason for reverting is: Broke the build! BUG=645713.
Message was sent while issue was closed.
Description was changed from ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793} ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793} ==========
The CQ bit was checked by thomasanderson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, erg@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2313033002/#ps150001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Going to try relanding because this linux-only change couldn't have broken the Windows build
Message was sent while issue was closed.
Description was changed from ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793} ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793} ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Cr-Commit-Position: refs/heads/master@{#417793} ========== to ========== This CL renames XForeignWindowManager to XWindowEventManager since it will be used on local windows now. Get rid of OnWindowDestroyed since this will be used by the XWindowCache for windows other than top-level windows as well. Finally, refactor the old Request implementation to use a Mask instead. This is needed by the XWindowCache which will select events on all windows, and therefore needs to know which masks we've already selected on Chrome windows. This will be the first in the line of 3 CLs. 1. Refactor X11ForeignWindowManager 2. Update Chrome code to use the XWindowManager instead of calling XSelectInput manually 3. Add a PRESUBMIT check to make sure CWEventMask, XSelectInput, or XCB_CW_EVENT_MASK is not added to new code. BUG=634084 Committed: https://crrev.com/5c04e8aa6732cca81e518dacf06c48398426b415 Committed: https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2 Cr-Original-Commit-Position: refs/heads/master@{#417793} Cr-Commit-Position: refs/heads/master@{#418005} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6c01662f504beee3970373de51a1bcf5c0b911d2 Cr-Commit-Position: refs/heads/master@{#418005} |
