|
|
DescriptionDecouple X11WindowEventFilter into WindowEventFilter parent class.
This CL decouples X11WindowEventFilter into two classes: WindowEventFilter
and X11WindowEventFilter. X11WindowEventFilter would inherit from WindowEventFilter
and continued instantiated/used by stock Chrome/Chromium Linux/X11 builds,
as it is today, so there would be no functionality change here.
However, on the other side, downstream forks of Chromium could benefit from
the window system agnostic parts of the implementation, in the newly added
WindowEventFilter class. For instance, the Chromium Ozone/Mus implementation in
[1] would greatly benefit from it in other to provide resize and window dragging
functionalities.
FWIW, Ozone backends like X11 and Wayland already work with it downstream.
BUG=666958
Review-Url: https://codereview.chromium.org/2948553002
Cr-Commit-Position: refs/heads/master@{#483200}
Committed: https://chromium.googlesource.com/chromium/src/+/e293efc74b2050a2eb73e5d17009a59a3d57dc51
Patch Set 1 #Patch Set 2 : remove unnecessary includes #Patch Set 3 : rebased, added erg to owners #
Total comments: 12
Patch Set 4 : fixing nits #Patch Set 5 : change function order #
Messages
Total messages: 39 (28 generated)
The CQ bit was checked by msisov@igalia.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 msisov@igalia.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.
Description was changed from ========== Extract X11WindowEventFilter into WindowEventFilter parent class. This CL decouples X11WindowEventFilter into a WindowEventFilter and X11WindowEventFilter. The WindowEventFilter is used both by Linux X11 and Ozone Linux X11/Wayland builds downstream. More or less the functionality is the same both for non-ozone and ozone builds, but there is a difference how, for example, X11 window manager is said to start interactive dragging or resizing the window. What is more, this is one of the preparations to upstream the adaptations for "external window mode" in the future. BUG=666958 ========== to ========== Decouple X11WindowEventFilter into WindowEventFilter parent class. This CL decouples X11WindowEventFilter into a WindowEventFilter and X11WindowEventFilter. The WindowEventFilter is used both by Linux X11 and Ozone Linux X11/Wayland builds downstream. More or less the functionality is the same both for non-ozone and ozone builds, but there is a difference how, for example, X11 window manager is said to start interactive dragging or resizing the window. What is more, this is one of the preparations to upstream the adaptations for "external window mode" in the future. BUG=666958 ==========
msisov@igalia.com changed reviewers: + egm@chromium.org
Please review
gentle reminder
msisov@igalia.com changed reviewers: + tapted@chromium.org
tapted@chromium.org: Please review changes in ui/views/BUILD.gn; egm: gentle reminder
tapted@chromium.org changed reviewers: + erg@chromium.org - egm@chromium.org
I think you meant for erg@ to review, rather than egm@ :) Also, I suggest including a change to OWNERS so that erg@ is still an owner of window_event_filter.cc (or whatever alternative name erg@ may suggest)
msisov@igalia.com changed reviewers: + erg@google.com - erg@chromium.org
msisov@igalia.com changed reviewers: + erg@chromium.org - erg@google.com
On 2017/06/24 04:50:32, tapted wrote: > I think you meant for erg@ to review, rather than egm@ :) > > Also, I suggest including a change to OWNERS so that erg@ is still an owner of > window_event_filter.cc (or whatever alternative name erg@ may suggest) Ouch, sure. Thanks! erg@, please review
The CQ bit was checked by msisov@igalia.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.
Description was changed from ========== Decouple X11WindowEventFilter into WindowEventFilter parent class. This CL decouples X11WindowEventFilter into a WindowEventFilter and X11WindowEventFilter. The WindowEventFilter is used both by Linux X11 and Ozone Linux X11/Wayland builds downstream. More or less the functionality is the same both for non-ozone and ozone builds, but there is a difference how, for example, X11 window manager is said to start interactive dragging or resizing the window. What is more, this is one of the preparations to upstream the adaptations for "external window mode" in the future. BUG=666958 ========== to ========== Decouple X11WindowEventFilter into WindowEventFilter parent class. This CL decouples X11WindowEventFilter into two classes: WindowEventFilter and X11WindowEventFilter. X11WindowEventFilter would inherit from WindowEventFilter and continued instantiated/used by stock Chrome/Chromium Linux/X11 builds, as it is today, so there would be no functionality change here. However, on the other side, downstream forks of Chromium could benefit from the window system agnostic parts of the implementation, in the newly added WindowEventFilter class. For instance, the Chromium Ozone/Mus implementation in [1] would greatly benefit from it in other to provide resize and window dragging functionalities. FWIW, Ozone backends like X11 and Wayland already work with it downstream. BUG=666958 ==========
tonikitoo@igalia.com changed reviewers: + sadrul@chromium.org
lgtm
stampy lgtm for BUILD.gn
Some nits. lgtm https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/window_event_filter.cc (right): https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/window_event_filter.h (right): https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:17: // An EventFilter that sets properties on native windows. Add a comment that downstream projects use this (maybe with links), because otherwise nothing is stopping someone from merging it back with X11WindowEventFilter. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:36: // to act as if a border or titlebar drag occurred. This documentation should be updated to reflect what this should do, and not mention x11 specific things (maybe as an example). https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:40: virtual void LowerWindow(); Document what this is expected to do. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_window_event_filter.cc (right): https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_window_event_filter.cc:131: void X11WindowEventFilter::LowerWindow() { function order in .cc should match the order in .h. So this should move above.
The CQ bit was checked by msisov@igalia.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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by msisov@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, erg@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2948553002/#ps80001 (title: "change function order")
https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/window_event_filter.cc (right): https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/28 04:41:58, sadrul wrote: > No (c) Done. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/window_event_filter.h (right): https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/28 04:41:58, sadrul wrote: > No (c) Done. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:17: // An EventFilter that sets properties on native windows. On 2017/06/28 04:41:58, sadrul wrote: > Add a comment that downstream projects use this (maybe with links), because > otherwise nothing is stopping someone from merging it back with > X11WindowEventFilter. Done. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:36: // to act as if a border or titlebar drag occurred. On 2017/06/28 04:41:58, sadrul wrote: > This documentation should be updated to reflect what this should do, and not > mention x11 specific things (maybe as an example). Done. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/window_event_filter.h:40: virtual void LowerWindow(); On 2017/06/28 04:41:58, sadrul wrote: > Document what this is expected to do. Done. https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/x11_window_event_filter.cc (right): https://codereview.chromium.org/2948553002/diff/40001/ui/views/widget/desktop... ui/views/widget/desktop_aura/x11_window_event_filter.cc:131: void X11WindowEventFilter::LowerWindow() { On 2017/06/28 04:41:58, sadrul wrote: > function order in .cc should match the order in .h. So this should move above. Done.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498683284544600, "parent_rev": "40b77f4471a386ccf0be08d1a6a333470f392a01", "commit_rev": "e293efc74b2050a2eb73e5d17009a59a3d57dc51"}
Message was sent while issue was closed.
Description was changed from ========== Decouple X11WindowEventFilter into WindowEventFilter parent class. This CL decouples X11WindowEventFilter into two classes: WindowEventFilter and X11WindowEventFilter. X11WindowEventFilter would inherit from WindowEventFilter and continued instantiated/used by stock Chrome/Chromium Linux/X11 builds, as it is today, so there would be no functionality change here. However, on the other side, downstream forks of Chromium could benefit from the window system agnostic parts of the implementation, in the newly added WindowEventFilter class. For instance, the Chromium Ozone/Mus implementation in [1] would greatly benefit from it in other to provide resize and window dragging functionalities. FWIW, Ozone backends like X11 and Wayland already work with it downstream. BUG=666958 ========== to ========== Decouple X11WindowEventFilter into WindowEventFilter parent class. This CL decouples X11WindowEventFilter into two classes: WindowEventFilter and X11WindowEventFilter. X11WindowEventFilter would inherit from WindowEventFilter and continued instantiated/used by stock Chrome/Chromium Linux/X11 builds, as it is today, so there would be no functionality change here. However, on the other side, downstream forks of Chromium could benefit from the window system agnostic parts of the implementation, in the newly added WindowEventFilter class. For instance, the Chromium Ozone/Mus implementation in [1] would greatly benefit from it in other to provide resize and window dragging functionalities. FWIW, Ozone backends like X11 and Wayland already work with it downstream. BUG=666958 Review-Url: https://codereview.chromium.org/2948553002 Cr-Commit-Position: refs/heads/master@{#483200} Committed: https://chromium.googlesource.com/chromium/src/+/e293efc74b2050a2eb73e5d17009... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/e293efc74b2050a2eb73e5d17009... |