Issue 2165083002: Linux: Refactor X11DesktopHandler

Tom (Use chromium acct)

Description was changed from ========== Remove activation responsibility from X11DesktopHandler ========== to ========== * Remove ...

4 years, 5 months ago (2016-07-20 21:56:01 UTC) #1

Tom (Use chromium acct)

Description was changed from ========== Remove activation responsibility from X11DesktopHandler ========== to ========== * Remove ...

4 years, 5 months ago (2016-07-20 21:56:01 UTC) #2

Tom (Use chromium acct)

thomasanderson@google.com changed reviewers: + danakj@chromium.org, erg@google.com, pkasting@google.com, sadrul@chromium.org

4 years, 5 months ago (2016-07-20 21:56:01 UTC) #3

Tom (Use chromium acct)

thomasanderson@google.com changed reviewers: + danakj@chromium.org, erg@google.com, pkasting@google.com, sadrul@chromium.org

4 years, 5 months ago (2016-07-20 21:56:01 UTC) #4

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 5 months ago (2016-07-20 22:00:44 UTC) #5

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/1

4 years, 5 months ago (2016-07-20 22:01:15 UTC) #6

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-07-20 23:02:26 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/266153)

4 years, 5 months ago (2016-07-20 23:02:27 UTC) #8

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-04 03:49:00 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/20001

4 years, 4 months ago (2016-08-04 03:49:10 UTC) #10

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-04 03:51:23 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/230820)

4 years, 4 months ago (2016-08-04 03:51:24 UTC) #12

Tom (Use chromium acct)

Description was changed from ========== * Remove responsibility for handling _NET_ACTIVE_WINDOW * Remove timestamp management ...

4 years, 4 months ago (2016-08-04 03:52:26 UTC) #13

Tom (Use chromium acct)

thomasanderson@google.com changed reviewers: + erg@chromium.org, sky@chromium.org, thestig@chromium.org - erg@google.com, pkasting@google.com, sadrul@chromium.org

4 years, 4 months ago (2016-08-04 03:52:26 UTC) #14

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-04 05:34:54 UTC) #15

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/40001

4 years, 4 months ago (2016-08-04 05:35:12 UTC) #16

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-04 06:36:04 UTC) #17

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/203930)

4 years, 4 months ago (2016-08-04 06:36:05 UTC) #18

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-04 17:17:57 UTC) #19

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/60001

4 years, 4 months ago (2016-08-04 17:18:16 UTC) #20

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-04 18:55:07 UTC) #21

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/274407)

4 years, 4 months ago (2016-08-04 18:55:08 UTC) #22

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-04 21:36:54 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/80001

4 years, 4 months ago (2016-08-04 21:37:19 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-04 22:45:36 UTC) #25

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/274619)

4 years, 4 months ago (2016-08-04 22:45:37 UTC) #26

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-05 19:44:48 UTC) #29

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/100001

4 years, 4 months ago (2016-08-05 19:45:06 UTC) #30

Tom (Use chromium acct)

Description was changed from ========== * Remove responsibility for handling _NET_ACTIVE_WINDOW * Remove timestamp management ...

4 years, 4 months ago (2016-08-05 19:52:46 UTC) #31

Elliot Glaysher

I've looked through this and it all looks mechanically ok, though this is changing enough ...

4 years, 4 months ago (2016-08-05 20:38:52 UTC) #33

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-05 21:43:19 UTC) #34

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-05 21:43:20 UTC) #35

Tom (Use chromium acct)

Description was changed from ========== * Remove all tracking of _NET_ACTIVE_WINDOW * Remove timestamp tracking ...

4 years, 4 months ago (2016-08-05 23:07:30 UTC) #36

Lei Zhang

With multiple reviewers, please specify who is reviewing what.

4 years, 4 months ago (2016-08-06 00:03:26 UTC) #38

Tom (Use chromium acct)

On 2016/08/06 00:03:26, Lei Zhang wrote: > With multiple reviewers, please specify who is reviewing ...

4 years, 4 months ago (2016-08-06 00:07:57 UTC) #39

Tom (Use chromium acct)

thomasanderson@google.com changed reviewers: - thestig@chromium.org

4 years, 4 months ago (2016-08-06 00:08:22 UTC) #40

sky

On 2016/08/06 00:07:57, Tom Anderson wrote: > On 2016/08/06 00:03:26, Lei Zhang wrote: > > ...

4 years, 4 months ago (2016-08-08 15:03:01 UTC) #41

Elliot Glaysher

lgtm, but i'd like it if dana also got a chance to look over this ...

4 years, 4 months ago (2016-08-08 17:28:04 UTC) #42

danakj

hey sorry for the delay, am reviewing :) On Thu, Aug 11, 2016 at 1:02 ...

4 years, 4 months ago (2016-08-11 20:31:38 UTC) #44

danakj

Sorry for many comments x.x Maybe they're all wrong. :) https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (left): https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc#oldcode369 ...

4 years, 4 months ago (2016-08-11 21:48:20 UTC) #45

Sorry for many comments x.x Maybe they're all wrong. :)

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (left):

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:369: int time =
views::X11DesktopHandler::get()->wm_user_time_ms();
Using a time is better if we can. Not being familiar with our X11 event handlers
and all, would it be possible to do something like

int time = ui::X11EventSource::GetInstance()->SomeServerTimeFunction() which
does

if (we're reacting to an X event handler with a time stamp like keypress or
whatever)
  return the X event's timestamp;
else
  return UpdateLastSeenServerTime();


I see you added DesktopWindowTreeHostX11::GetTimestamp() which is basically
this. I think it'd make sense to put that function on X11EventSource and use it
for these presents. What do you think?

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:716:
ui::X11EventSource::GetInstance()->SetLastSeenServerTime(
I'm a bit unsure about why we're using the "window launch time" as the "last
seen server time". This would confuse things if we wanted to get a current time
from the server, cuz this is the time where the window was requested to be
created, which is different.

I'll continue reading the CL to learn more but I'm a bit skeptical this is what
we want here.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:256:
native_widget_delegate_->AsWidget()->GetRootView()->SchedulePaint();
This really only needs to be done when becoming active? But I wonder if the
order matters relative to the line above?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:649:
xclient.xclient.data.l[2] = None;
This is actually a lie if another chrome window is active. Maybe leave a TODO?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:660:
XSetInputFocus(xdisplay_, xwindow_, RevertToParent, CurrentTime);
Can you explain/document why CurrentTime and not GetTimestamp()?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:687: return
window_mapped_ &&
the has focus things can't be true while window_mapped_ is false right? so
checking window_mapped_ looks redundant or could be a DCHECK instead?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1546: if (time ==
CurrentTime) {
This feels like an approximation for what we want. But if we saw an event with a
time in it, and then leave the event handler, and later call this function,
we'll get an out-of-date timestamp.

I think this would do the right thing if last_seen_server_time became invalid
(CurrentTime) when we end any X event handler. Or maybe we do that?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1749: has_pointer_
= false;
I'd expect these to all be initialized to false, and set to false in UnmapNotify
perhaps, but they should already all be false when we go to Map the window if
the window isn't already mapped. Does that happen?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1754: unsigned long
wm_user_time_ms = ignore_input_ ? 0 : GetTimestamp();
So here we want to use the "window launch time" in preference to the most recent
event we've seen, right? It may be older and then the WM could know that the
user is using some other window in the meantime and avoid focusing the window
we're mapping here.

Or maybe only if this is happening from the startup_browser_creator. Are there
other cases where we'd map a window ourselves? In those cases we would want to
use the last event timestamp. Do you think there's a way to tell what we want
here?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1834:
if(!xev->xcrossing.focus && has_window_focus_) {
git cl format

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1835:
has_pointer_focus_ = has_pointer_;
In the header we claimed that has_window_focus_ and has_pointer_focus_ are
mutually exclusive but this code here appears to make it possible to disagree if
has_pointer_ is true.

I'm also a bit unclear what the if condition is going for.

!xcrossing.focus means X server does not thing our window or ancestor has focus.

has_window_focus_ means we had a FocusIn on our window but no FocusOut since.

How do we get into that position? XServer must have sent a FocusOut to us in
order to not think our window is focused which we should have seen before this
event?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1836: ignore_input_
= false;
I'm hesitant about the whole ignore_input_ thing. Can you tell me about the
Deactivate() thing and what the goals are there? That's not when we middle-click
the window titlebar area is it? Currently that sends to the bottom of the
stacking order but does not try to ignore input.

The implementation of ignore_input_ seems to be that if we Deactivate() then
move the mouse out and back into the window, we will begin responding to input
events, which seems inconsistent or hard to understand. What is the goal in
ignoring input events?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1899:
has_pointer_focus_ = !focus_in;
I think you only want to do this for FocusOut. Otherwise if focus moved to an
inferior window that we're tracking, say, and the pointer was in it, you'd set
has_pointer_focus_ = true, but also has_window_focus_ = true?

I'd suggest breaking up the FocusIn vs FocusOut cases to make this easier. The
documentation for this case says:

> We can separate out the transitions from !has_pointer_focus(W) to
> has_pointer_focus(W) into four cases:
> 
> T1:, we get a FocusOut with a mode of Ancestor or Virtual
>   We need to separately track has_pointer(W) to distinguish
>   this from the case where we get these events and !has_pointer(W)

It's explicitly for FocusOut only. Unless you're combining some other FocusIn
case, but then comparing the code to the docs is getting very hard at that
point.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1904:
has_window_focus_ = focus_in;
The focus_tracking.txt doc says has_window_focus should be changed when detail
!= NotifyInferior. However you're not changing it on NotifyPointer, how come?

There are 3 cases where NotifyPointer can happen:
https://tronche.com/gui/x/xlib/events/input-focus/normal-and-grabbed.html

They all depend on the pointer being in an inferior window of a chrome window,
so it's possible they can't happen, but I'm curious why they're being treated as
they are.

I think it'd be easier to read this if we split the code for setting
has_pointer_focus_ and has_window_focus_ apart. We can have 2 switches if
needed.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:372: // Does
|xwindow_| have the pointer grab?  This does not necissarily indicate
typo

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:380: //
|has_window_focus_| and |has_pointer_focus_| are mutually exclusive.
nit: don't line wrap for every sentence

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:381: // |xwindow_|
will receive in put iff:
input

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:382: //
|has_window_focus_| || |has_pointer_grab_| || |has_pointer_focus_|
nit: indent this a bit will read easier

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-15 18:33:00 UTC) #46

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/120001

4 years, 4 months ago (2016-08-15 18:33:25 UTC) #47

Tom (Use chromium acct)

On 2016/08/11 21:48:20, danakj wrote: > Sorry for many comments x.x Maybe they're all wrong. ...

4 years, 4 months ago (2016-08-15 18:42:46 UTC) #48

On 2016/08/11 21:48:20, danakj wrote:
> Sorry for many comments x.x Maybe they're all wrong. :)

I actually prefer reviewers tear my code apart haha

Also I realized sometimes we only get XI2 enter/leave/focus events instead of
regular ones (not sure exactly the conditions for this, but it happened on
Cinnamon), so handle those too.

I think it's possible for state to get messed up if there's multiple input
devices (one device may enter and another may leave),  but I took a look at the
GTK source code and it makes no effort to separate the events, so I think this
should probably be good enough.

Maybe the "right" thing to do would be to create a set of devices that have
focus and treat the window as focused if the set is nonempty?

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (left):

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:369: int time =
views::X11DesktopHandler::get()->wm_user_time_ms();
On 2016/08/11 21:48:19, danakj wrote:
> Using a time is better if we can. Not being familiar with our X11 event
handlers
> and all, would it be possible to do something like
> 
> int time = ui::X11EventSource::GetInstance()->SomeServerTimeFunction() which
> does
> 
> if (we're reacting to an X event handler with a time stamp like keypress or
> whatever)
>   return the X event's timestamp;
> else
>   return UpdateLastSeenServerTime();
> 
> 
> I see you added DesktopWindowTreeHostX11::GetTimestamp() which is basically
> this. I think it'd make sense to put that function on X11EventSource and use
it
> for these presents. What do you think?

:(

../../chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:370: error: undefined
reference to 'ui::X11EventSource::GetInstance()'
../../chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:370: error: undefined
reference to 'ui::X11EventSource::GetTimestamp()'
../../chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:173: error:
undefined reference to 'ui::X11EventSource::GetInstance()'
../../chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:173: error:
undefined reference to 'ui::X11EventSource::GetTimestamp()'

I agree this sounds like the right thing to do.  Maybe we could split off
x11_events_platform into it's own .so, or provide a proxy in views?

Here's a list of possible places we could put it

thomasanderson@thomasanderson:~/Developer/chromium_6/src$ ldd
out/Debug/libgtk2ui.so
	linux-vdso.so.1 =>  (0x00007ffe5f9d5000)
	libbase.so => not found
	libbase_i18n.so => not found
	libprinting.so => not found
	libskia.so => not found
	libaura.so => not found
	libui_base.so => not found
	libui_base_ime.so => not found
	libdisplay.so => not found
	libevents.so => not found
	libevents_base.so => not found
	libgfx.so => not found
	libgeometry.so => not found
	libgfx_x11.so => not found
	libnative_theme.so => not found
	libshell_dialogs.so => not found
	libviews.so => not found
	libicui18n.so => not found
	libicuuc.so => not found
	libcontent.so => not found
	libgpu.so => not found
	libgles2_utils.so => not found
	librange.so => not found
	libgl_wrapper.so => not found
	libplatform.so => not found
	libgl_init.so => not found
	libipc.so => not found
	libmojo_public_system.so => not found
	libkeycodes_x11.so => not found
	libui_base_x.so => not found
	liburl.so => not found
	libmojo_common_lib.so => not found
	libcc.so => not found
	libaccessibility.so => not found
	liburl_ipc.so => not found
	libcompositor.so => not found

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator.cc:716:
ui::X11EventSource::GetInstance()->SetLastSeenServerTime(
On 2016/08/11 21:48:19, danakj wrote:
> I'm a bit unsure about why we're using the "window launch time" as the "last
> seen server time". This would confuse things if we wanted to get a current
time
> from the server, cuz this is the time where the window was requested to be
> created, which is different.
> 
> I'll continue reading the CL to learn more but I'm a bit skeptical this is
what
> we want here.

It's because metacity/mutter/muffin all deny activation requests if a timestamp
is old.
https://github.com/GNOME/mutter/blob/5ee0f24ab90d457f3b53a32e96f89db816a98f0a...

Actually, they discard the request if it's older than the _NET_WM_USER_TIME of
ANY other window.
The EWMH spec says "The timestamp is Client's last user activity timestamp (see
_NET_WM_USER_TIME) at the time of the request" about the _NET_ACTIVE_WINDOW
client message.  So I guess these WMs have a bug that all date back to 2005 when
this commit was made in metacity
https://github.com/GNOME/mutter/commit/d11681e5050b9ddf37d211f09c07557bef374277 
As a side note, compiz actually gets this right and checks only against the
window's _NET_WM_USER_TIME
https://github.com/compiz-reloaded/compiz/blob/a7a23334283cbe4572061a86842c83...

You bring up an interesting point in another comment:  we should add
X11EventSource::GetTimestamp() to return the time of the event that's currently
being dispatched, and fall back on UpdateLastSeenServerTime if there's no event
being processed, or if the current event doesn't have a timestamp.  This should
make these misbehaving WMs happy, and we can remove the SetLastSeenServerTime
logic.  I think this would belong in a separate refactoring CL.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:256:
native_widget_delegate_->AsWidget()->GetRootView()->SchedulePaint();
On 2016/08/11 21:48:20, danakj wrote:
> This really only needs to be done when becoming active? But I wonder if the
> order matters relative to the line above?

Not only when it becomes active, when the activation changes.
Do you mean we should also repaint when the mouse grab or window focus changes?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:649:
xclient.xclient.data.l[2] = None;
On 2016/08/11 21:48:20, danakj wrote:
> This is actually a lie if another chrome window is active. Maybe leave a TODO?

Done.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:660:
XSetInputFocus(xdisplay_, xwindow_, RevertToParent, CurrentTime);
On 2016/08/11 21:48:20, danakj wrote:
> Can you explain/document why CurrentTime and not GetTimestamp()?

Copy/paste bug.  Fixed.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:687: return
window_mapped_ &&
On 2016/08/11 21:48:20, danakj wrote:
> the has focus things can't be true while window_mapped_ is false right? so
> checking window_mapped_ looks redundant or could be a DCHECK instead?

Done.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1546: if (time ==
CurrentTime) {
On 2016/08/11 21:48:20, danakj wrote:
> This feels like an approximation for what we want. But if we saw an event with
a
> time in it, and then leave the event handler, and later call this function,
> we'll get an out-of-date timestamp.
> 
> I think this would do the right thing if last_seen_server_time became invalid
> (CurrentTime) when we end any X event handler. Or maybe we do that?

We don't currently do that :(
But I think this is the right thing to do.  Maybe this would be better off in a
separate CL?

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1749: has_pointer_
= false;
On 2016/08/11 21:48:20, danakj wrote:
> I'd expect these to all be initialized to false, and set to false in
UnmapNotify
> perhaps, but they should already all be false when we go to Map the window if
> the window isn't already mapped. Does that happen?

shouldn't happen, done

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1754: unsigned long
wm_user_time_ms = ignore_input_ ? 0 : GetTimestamp();
On 2016/08/11 21:48:20, danakj wrote:
> So here we want to use the "window launch time" in preference to the most
recent
> event we've seen, right? 

We need the window launch time for _NET_ACTIVE_WINDOW.  New browser windows
always seem to get focus.  The problem is when we try to open a new tab in an
existing browser window.

> It may be older and then the WM could know that the
> user is using some other window in the meantime and avoid focusing the window
> we're mapping here.
> 
> Or maybe only if this is happening from the startup_browser_creator. Are there
> other cases where we'd map a window ourselves? In those cases we would want to
> use the last event timestamp. Do you think there's a way to tell what we want
> here?

I like your idea of rewriting GetTimestamp() to use the current event timestamp,
and I think we should always use that.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1834:
if(!xev->xcrossing.focus && has_window_focus_) {
On 2016/08/11 21:48:20, danakj wrote:
> git cl format

Done.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1835:
has_pointer_focus_ = has_pointer_;
On 2016/08/11 21:48:20, danakj wrote:
> In the header we claimed that has_window_focus_ and has_pointer_focus_ are
> mutually exclusive but this code here appears to make it possible to disagree
if
> has_pointer_ is true.
> 
> I'm also a bit unclear what the if condition is going for.
> 
> !xcrossing.focus means X server does not thing our window or ancestor has
focus.
> 
> has_window_focus_ means we had a FocusIn on our window but no FocusOut since.
> 
> How do we get into that position? XServer must have sent a FocusOut to us in
> order to not think our window is focused which we should have seen before this
> event?

Thanks for catching this terrible logic :)
The ! should have been on has_window_focus_.  Please double check again

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1836: ignore_input_
= false;
On 2016/08/11 21:48:20, danakj wrote:
> I'm hesitant about the whole ignore_input_ thing. Can you tell me about the
> Deactivate() thing and what the goals are there? That's not when we
middle-click
> the window titlebar area is it? Currently that sends to the bottom of the
> stacking order but does not try to ignore input.
> 

I don't know when we call Deactivate() in production code, but I know that we're
supposed to stop receiving input (or, at least that's what someone thought at
some point, based on some test cases).  We can't give up our focus, so we need
to just ignore events instead.

> The implementation of ignore_input_ seems to be that if we Deactivate() then
> move the mouse out and back into the window, we will begin responding to input
> events, which seems inconsistent or hard to understand. What is the goal in
> ignoring input events?

Even if we deactivate, users can still manually reactivate the window to make it
receive input again.  I wasn't sure whether we should do this for focus and
crossing events, or just focus events.  But now you have me convinced that it
should be just for focus events, so removing ignore_input_ = false here.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1899:
has_pointer_focus_ = !focus_in;
On 2016/08/11 21:48:20, danakj wrote:
> I think you only want to do this for FocusOut. Otherwise if focus moved to an
> inferior window that we're tracking, say, and the pointer was in it, you'd set
> has_pointer_focus_ = true, but also has_window_focus_ = true?
> 
> I'd suggest breaking up the FocusIn vs FocusOut cases to make this easier. The
> documentation for this case says:
> 
> > We can separate out the transitions from !has_pointer_focus(W) to
> > has_pointer_focus(W) into four cases:
> > 
> > T1:, we get a FocusOut with a mode of Ancestor or Virtual
> >   We need to separately track has_pointer(W) to distinguish
> >   this from the case where we get these events and !has_pointer(W)
> 
> It's explicitly for FocusOut only. Unless you're combining some other FocusIn
> case, but then comparing the code to the docs is getting very hard at that
> point.

Doesn't it say to do this for FocusIn as well?

> The transitions from has_pointer_focus(W) to !has_pointer_focus(W)
> are exactly the opposite

> F1: we get a FocusIn with a mode of Ancestor or Virtual
>   We need to separately track has_pointer(W) to distinguish
>   this from the case we get these events and !has_pointer(W)

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1904:
has_window_focus_ = focus_in;
On 2016/08/11 21:48:20, danakj wrote:
> The focus_tracking.txt doc says has_window_focus should be changed when detail
> != NotifyInferior. However you're not changing it on NotifyPointer, how come?
> 

Probably a mistake in the doc.  NotifyPointer shouldn't change the window focus,
right?

> There are 3 cases where NotifyPointer can happen:
> https://tronche.com/gui/x/xlib/events/input-focus/normal-and-grabbed.html
> 
> They all depend on the pointer being in an inferior window of a chrome window,
> so it's possible they can't happen, but I'm curious why they're being treated
as
> they are.
> 
> I think it'd be easier to read this if we split the code for setting
> has_pointer_focus_ and has_window_focus_ apart. We can have 2 switches if
> needed.

Done (split code for has_pointer_focus_ and has_window_focus_)

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:372: // Does
|xwindow_| have the pointer grab?  This does not necissarily indicate
On 2016/08/11 21:48:20, danakj wrote:
> typo

Done.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:380: //
|has_window_focus_| and |has_pointer_focus_| are mutually exclusive.
On 2016/08/11 21:48:20, danakj wrote:
> nit: don't line wrap for every sentence

Done.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:381: // |xwindow_|
will receive in put iff:
On 2016/08/11 21:48:20, danakj wrote:
> input

Done.

https://codereview.chromium.org/2165083002/diff/100001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:382: //
|has_window_focus_| || |has_pointer_grab_| || |has_pointer_focus_|
On 2016/08/11 21:48:20, danakj wrote:
> nit: indent this a bit will read easier

Done.

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-16 20:51:20 UTC) #49

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/140001

4 years, 4 months ago (2016-08-16 20:51:41 UTC) #50

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-16 22:36:22 UTC) #51

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/281012)

4 years, 4 months ago (2016-08-16 22:36:23 UTC) #52

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-17 01:10:08 UTC) #53

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/160001

4 years, 4 months ago (2016-08-17 01:10:25 UTC) #54

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-17 02:18:03 UTC) #55

commit-bot: I haz the power

Dry run: Exceeded global retry quota

4 years, 4 months ago (2016-08-17 02:18:04 UTC) #56

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-17 17:46:34 UTC) #57

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/180001

4 years, 4 months ago (2016-08-17 17:47:17 UTC) #58

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-17 18:28:39 UTC) #59

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-17 18:28:40 UTC) #60

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-19 00:24:58 UTC) #61

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/200001

4 years, 4 months ago (2016-08-19 00:25:56 UTC) #62

Tom (Use chromium acct)

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (left): https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc#oldcode369 chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:369: int time = views::X11DesktopHandler::get()->wm_user_time_ms(); On 2016/08/15 18:42:45, Tom Anderson ...

4 years, 4 months ago (2016-08-19 00:26:31 UTC) #63

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc (left):

https://codereview.chromium.org/2165083002/diff/100001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:369: int time =
views::X11DesktopHandler::get()->wm_user_time_ms();
On 2016/08/15 18:42:45, Tom Anderson wrote:
> On 2016/08/11 21:48:19, danakj wrote:
> > Using a time is better if we can. Not being familiar with our X11 event
> handlers
> > and all, would it be possible to do something like
> > 
> > int time = ui::X11EventSource::GetInstance()->SomeServerTimeFunction() which
> > does
> > 
> > if (we're reacting to an X event handler with a time stamp like keypress or
> > whatever)
> >   return the X event's timestamp;
> > else
> >   return UpdateLastSeenServerTime();
> > 
> > 
> > I see you added DesktopWindowTreeHostX11::GetTimestamp() which is basically
> > this. I think it'd make sense to put that function on X11EventSource and use
> it
> > for these presents. What do you think?
> 
> :(
> 
> ../../chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:370: error: undefined
> reference to 'ui::X11EventSource::GetInstance()'
> ../../chrome/browser/ui/libgtk2ui/print_dialog_gtk2.cc:370: error: undefined
> reference to 'ui::X11EventSource::GetTimestamp()'
> ../../chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:173: error:
> undefined reference to 'ui::X11EventSource::GetInstance()'
> ../../chrome/browser/ui/libgtk2ui/select_file_dialog_impl_gtk2.cc:173: error:
> undefined reference to 'ui::X11EventSource::GetTimestamp()'
> 
> I agree this sounds like the right thing to do.  Maybe we could split off
> x11_events_platform into it's own .so, or provide a proxy in views?
> 
> Here's a list of possible places we could put it
> 
> thomasanderson@thomasanderson:~/Developer/chromium_6/src$ ldd
> out/Debug/libgtk2ui.so
> 	linux-vdso.so.1 =>  (0x00007ffe5f9d5000)
> 	libbase.so => not found
> 	libbase_i18n.so => not found
> 	libprinting.so => not found
> 	libskia.so => not found
> 	libaura.so => not found
> 	libui_base.so => not found
> 	libui_base_ime.so => not found
> 	libdisplay.so => not found
> 	libevents.so => not found
> 	libevents_base.so => not found
> 	libgfx.so => not found
> 	libgeometry.so => not found
> 	libgfx_x11.so => not found
> 	libnative_theme.so => not found
> 	libshell_dialogs.so => not found
> 	libviews.so => not found
> 	libicui18n.so => not found
> 	libicuuc.so => not found
> 	libcontent.so => not found
> 	libgpu.so => not found
> 	libgles2_utils.so => not found
> 	librange.so => not found
> 	libgl_wrapper.so => not found
> 	libplatform.so => not found
> 	libgl_init.so => not found
> 	libipc.so => not found
> 	libmojo_public_system.so => not found
> 	libkeycodes_x11.so => not found
> 	libui_base_x.so => not found
> 	liburl.so => not found
> 	libmojo_common_lib.so => not found
> 	libcc.so => not found
> 	libaccessibility.so => not found
> 	liburl_ipc.so => not found
> 	libcompositor.so => not found

Turns out the necessary dependency was added in the past few days, so added
GetTimestamp() here.

Tom (Use chromium acct)

Patchset #10 (id:220001) has been deleted

4 years, 4 months ago (2016-08-19 00:29:58 UTC) #64

danakj

ok before I read the FocusIn/Out bit here's a few first things. https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc ...

4 years, 4 months ago (2016-08-19 21:43:45 UTC) #65

danakj

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode268 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:268: if (detail == NotifyInferior) On 2016/08/19 21:43:45, danakj wrote: ...

4 years, 4 months ago (2016-08-19 23:17:32 UTC) #66

danakj

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode283 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:283: if (detail == NotifyInferior) Can you say the same ...

4 years, 4 months ago (2016-08-19 23:29:58 UTC) #67

danakj

OK otherwise than the IsActive() thing I believe everything is correct and just would like ...

4 years, 4 months ago (2016-08-19 23:59:30 UTC) #68

OK otherwise than the IsActive() thing I believe everything is correct and just
would like more comments, and some readability suggestions.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:286: if (mode !=
NotifyWhileGrabbed && detail != NotifyPointer)
Also can you explain by ignore Pointer here in a comment, esp as I don't see
this explained well in the focus_tracking.txt. My understanding is:

NotifyPointer happens in cases where we are acquiring or losing pointer focus
(ie focus is moving between windows, and we gain/lose focus only due to the
pointer) which is tracked by has_pointer_focus_ so in all cases we don't want to
consider has_window_focus_ or has_focus_ (which both exclude has_pointer_focus_)
to change.

I think it'd help me structure this in my head a bit if you did

if (detail != Pointer) {
  if (mode != WhileGrabbed)
    has_focus_ = focus_in;
  if (mode != Grab && mode != Ungrab)
    has_window_focus_ = focus_in;
}

Now all the "window focus stuff" is together.

if (mode != Grab && mode != Ungrab && has_pointer_) {
  switch (detail) ...
}

And the "pointer focus stuff" is on its own.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:286: if (mode !=
NotifyWhileGrabbed && detail != NotifyPointer)
On 2016/08/19 23:29:58, danakj wrote:
> Can you throw some comments here kinda like the header file? Like If not
> NotifyWhileGrabbed then either we're gaining/losing focus without a grab or
> because of a grab starting/ending?

Maybe better would be to say that while grabbed we don't care about focus
changes? Events will continue going to the grab anyway, which we heard about via
NotifyGrab/Ungrab? I'm not actually sure how to generate a NotifyWhileGrabbed in
the wild.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:295: case
NotifyVirtual:
Can you leave a comment why NonLinear(Virtual) are ignored here? I think this is
something like:

In the case of NonLinear, focus was not in an ancestor or descendant but the
pointer is known to be in our window. So we must not have had pointer focus (as
that only goes to ancestors/dendendants) and we are not gaining pointer focus.
So we can ignore those cases.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1975:
OnFocusEvent(enter_event->evtype == XI_FocusIn, enter_event->mode,
Would you consider rewriting the mode to non-XI constants here so the
OnFocusEvent code doesn't have to deal with them?

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

4 years, 4 months ago (2016-08-22 20:26:35 UTC) #69

Tom (Use chromium acct)

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode249 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:249: OnHostLostWindowCapture(); On 2016/08/19 21:43:45, danakj wrote: > Why do ...

4 years, 4 months ago (2016-08-22 20:27:08 UTC) #70

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:249:
OnHostLostWindowCapture();
On 2016/08/19 21:43:45, danakj wrote:
> Why do we not do this if we had pointer focus but lose it? Think
> focus-follows-mouse WM behaviour.

You're right, I was unclear about the definition of capture.  It looks like the
cross-platform code uses the Windows definition of mouse capture, which is
"either when the mouse is over the capturing window, or when the mouse button
was pressed while the mouse was over the capturing window and the button is
still down."  I've updated the code to be exactly following this definition.

It's a little confusing having OnHostLostWindowCapture.  I think it should
instead be OnHostLostPointerCapture, especially since we now have variables
representing window focus and pointer focus in X11 code O_O

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:266: bool focus,
On 2016/08/19 21:43:45, danakj wrote:
> focus_in_window_or_ancestor?

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:268: if (detail ==
NotifyInferior)
On 2016/08/19 23:17:32, danakj wrote:
> On 2016/08/19 21:43:45, danakj wrote:
> > When the mouse moves from the root into |xwindow_| this would be an enter
> > NotifyInferior we care about, no? And when it moves from |xwindow_| to root
it
> > would be a leave NotifyInferior?
> 
> Oh I see.
> 
> EnterNotify with Inferior means we're coming from a child to |xwindow_|
> LeaveNotify with Inferior means we're going to a child from |xwindow_|.
> 
> Can you comment that?

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:274: if (focus &&
!has_window_focus_)
On 2016/08/19 21:43:45, danakj wrote:
> // If focus is not in an ancestor then this crossing event can't move pointer
> focus relative to our window. Pointer focus can only be on an inferior of the
> focused window, which we are not.  <- sound right?

Done.  I wrote something different though.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:283: if (detail ==
NotifyInferior)
On 2016/08/19 23:29:58, danakj wrote:
> Can you say the same thing here moreorless?

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:286: if (mode !=
NotifyWhileGrabbed && detail != NotifyPointer)
On 2016/08/19 23:59:30, danakj wrote:
> Also can you explain by ignore Pointer here in a comment, esp as I don't see
> this explained well in the focus_tracking.txt. My understanding is:
> 
> NotifyPointer happens in cases where we are acquiring or losing pointer focus
> (ie focus is moving between windows, and we gain/lose focus only due to the
> pointer) which is tracked by has_pointer_focus_ so in all cases we don't want
to
> consider has_window_focus_ or has_focus_ (which both exclude
has_pointer_focus_)
> to change.
> 
> I think it'd help me structure this in my head a bit if you did
> 
> if (detail != Pointer) {
>   if (mode != WhileGrabbed)
>     has_focus_ = focus_in;
>   if (mode != Grab && mode != Ungrab)
>     has_window_focus_ = focus_in;
> }
> 
> Now all the "window focus stuff" is together.
> 
> if (mode != Grab && mode != Ungrab && has_pointer_) {
>   switch (detail) ...
> }
> 
> And the "pointer focus stuff" is on its own.

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:286: if (mode !=
NotifyWhileGrabbed && detail != NotifyPointer)
On 2016/08/19 23:29:58, danakj wrote:
> Can you throw some comments here kinda like the header file? Like If not
> NotifyWhileGrabbed then either we're gaining/losing focus without a grab or
> because of a grab starting/ending?

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:295: case
NotifyVirtual:
On 2016/08/19 23:59:30, danakj wrote:
> Can you leave a comment why NonLinear(Virtual) are ignored here? I think this
is
> something like:
> 
> In the case of NonLinear, focus was not in an ancestor or descendant but the
> pointer is known to be in our window. So we must not have had pointer focus
(as
> that only goes to ancestors/dendendants) and we are not gaining pointer focus.
> So we can ignore those cases.

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:741: bool is_active
= (has_focus_ || has_pointer_focus_) && !ignore_input_;
On 2016/08/19 23:29:58, danakj wrote:
> FWIW normally an X window will show itself as visible active even tho it
doesn't
> have focus during a grab. This looks like it would not do that. I think you
want
> actually has_window_focus_ here if this is for display the focus state to the
> user (title bar color etc), but has_focus_ if this is for deciding if we
expect
> keyboard input.
> 

The cross-platform code seems to want a window to be active when it will receive
keyboard input, so that's the definition I went with here.

Like you said, we will receive keyboard input when has_focus_ == true.
However, we will also receive keyboard input when has_focus_ == false, but
has_pointer_focus_ == true.  An ancestor window has the focus, but the pointer
is over our window, so we will receive keystrokes.

A stripped-down demo of this is http://www.sbin.org/doc/Xlib/chapt_09.html,
under "Monitoring whether keyboard input will be available".

> Try alt-tabbing with most WMs and I think you should see a LeaveFocus with
> NotifyGrab while holding alt, and EnterFocus NotifyUngrab when letting it go
> (can see this in Openbox at least with this demo:
> https://github.com/danakj/openbox/blob/master/tests/focusout.c

Cool demo, I didn't know you had your own WM.  I'll try it out :)

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1922: case
FocusOut: {
On 2016/08/19 21:43:45, danakj wrote:
> nit: {} are not needed since there's no vars declared in the case

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1975:
OnFocusEvent(enter_event->evtype == XI_FocusIn, enter_event->mode,
On 2016/08/19 23:59:30, danakj wrote:
> Would you consider rewriting the mode to non-XI constants here so the
> OnFocusEvent code doesn't have to deal with them?

Done.

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:2062: case
UnmapNotify: {
On 2016/08/19 21:43:45, danakj wrote:
> jw do we care about DestroyNotify? Presumably no.. but checking

no

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/240001

4 years, 4 months ago (2016-08-22 20:27:26 UTC) #71

danakj

https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode741 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:741: bool is_active = (has_focus_ || has_pointer_focus_) && !ignore_input_; On ...

4 years, 4 months ago (2016-08-22 20:32:43 UTC) #72

Tom (Use chromium acct)

On 2016/08/22 20:32:43, danakj wrote: > https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): > > https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode741 > ...

4 years, 4 months ago (2016-08-22 20:38:15 UTC) #73

On 2016/08/22 20:32:43, danakj wrote:
>
https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):
> 
>
https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/deskto...
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:741: bool
is_active
> = (has_focus_ || has_pointer_focus_) && !ignore_input_;
> On 2016/08/22 20:27:07, Tom Anderson wrote:
> > On 2016/08/19 23:29:58, danakj wrote:
> > > FWIW normally an X window will show itself as visible active even tho it
> > doesn't
> > > have focus during a grab. This looks like it would not do that. I think
you
> > want
> > > actually has_window_focus_ here if this is for display the focus state to
> the
> > > user (title bar color etc), but has_focus_ if this is for deciding if we
> > expect
> > > keyboard input.
> > > 
> > 
> > The cross-platform code seems to want a window to be active when it will
> receive
> > keyboard input, so that's the definition I went with here.
> > 
> > Like you said, we will receive keyboard input when has_focus_ == true.
> > However, we will also receive keyboard input when has_focus_ == false, but
> > has_pointer_focus_ == true.  An ancestor window has the focus, but the
pointer
> > is over our window, so we will receive keystrokes.
> > 
> > A stripped-down demo of this is http://www.sbin.org/doc/Xlib/chapt_09.html,
> > under "Monitoring whether keyboard input will be available".
> > 
> > > Try alt-tabbing with most WMs and I think you should see a LeaveFocus with
> > > NotifyGrab while holding alt, and EnterFocus NotifyUngrab when letting it
go
> > > (can see this in Openbox at least with this demo:
> > > https://github.com/danakj/openbox/blob/master/tests/focusout.c
> > 
> > Cool demo, I didn't know you had your own WM.  I'll try it out :)
> 
> OK just to be clear then, when alt-tabbing does the active chrome window
change
> color to be unfocused now?

On unity it doesn't change color at all.  I'm guessing I have to use a WM that
chrome doesn't have a theme for?  I'll try on Openbox.

danakj

On Mon, Aug 22, 2016 at 1:38 PM, <thomasanderson@google.com> wrote: > On 2016/08/22 20:32:43, danakj ...

4 years, 4 months ago (2016-08-22 20:40:37 UTC) #74

On Mon, Aug 22, 2016 at 1:38 PM, <thomasanderson@google.com> wrote:

> On 2016/08/22 20:32:43, danakj wrote:
> >
> https://codereview.chromium.org/2165083002/diff/200001/ui/
> views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> (right):
> >
> >
> https://codereview.chromium.org/2165083002/diff/200001/ui/
> views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode741
> > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:741: bool
> is_active
> > = (has_focus_ || has_pointer_focus_) && !ignore_input_;
> > On 2016/08/22 20:27:07, Tom Anderson wrote:
> > > On 2016/08/19 23:29:58, danakj wrote:
> > > > FWIW normally an X window will show itself as visible active even
> tho it
> > > doesn't
> > > > have focus during a grab. This looks like it would not do that. I
> think
> you
> > > want
> > > > actually has_window_focus_ here if this is for display the focus
> state to
> > the
> > > > user (title bar color etc), but has_focus_ if this is for deciding
> if we
> > > expect
> > > > keyboard input.
> > > >
> > >
> > > The cross-platform code seems to want a window to be active when it
> will
> > receive
> > > keyboard input, so that's the definition I went with here.
> > >
> > > Like you said, we will receive keyboard input when has_focus_ == true.
> > > However, we will also receive keyboard input when has_focus_ == false,
> but
> > > has_pointer_focus_ == true. An ancestor window has the focus, but the
> pointer
> > > is over our window, so we will receive keystrokes.
> > >
> > > A stripped-down demo of this is http://www.sbin.org/doc/Xlib/
> chapt_09.html,
> > > under "Monitoring whether keyboard input will be available".
> > >
> > > > Try alt-tabbing with most WMs and I think you should see a
> LeaveFocus with
> > > > NotifyGrab while holding alt, and EnterFocus NotifyUngrab when
> letting it
> go
> > > > (can see this in Openbox at least with this demo:
> > > > https://github.com/danakj/openbox/blob/master/tests/focusout.c
> > >
> > > Cool demo, I didn't know you had your own WM. I'll try it out :)
> >
> > OK just to be clear then, when alt-tabbing does the active chrome window
> change
> > color to be unfocused now?
>
> On unity it doesn't change color at all. I'm guessing I have to use a WM
> that
> chrome doesn't have a theme for? I'll try on Openbox.
>

Oh weird. Maybe just try a custom chrome theme from the theme store. My
theme changes green shades when focused/not. Maybe the "gtk theme" doesn't
(that makes me sad).


>
> https://codereview.chromium.org/2165083002/
>

-- 
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.

Elliot Glaysher

On 2016/08/22 20:40:37, danakj wrote: > On Mon, Aug 22, 2016 at 1:38 PM, <mailto:thomasanderson@google.com> ...

4 years, 4 months ago (2016-08-22 20:42:52 UTC) #75

Tom (Use chromium acct)

On 2016/08/22 20:40:37, danakj wrote: > On Mon, Aug 22, 2016 at 1:38 PM, <mailto:thomasanderson@google.com> ...

4 years, 4 months ago (2016-08-22 20:48:51 UTC) #76

On 2016/08/22 20:40:37, danakj wrote:
> On Mon, Aug 22, 2016 at 1:38 PM, <mailto:thomasanderson@google.com> wrote:
> 
> > On 2016/08/22 20:32:43, danakj wrote:
> > >
> > https://codereview.chromium.org/2165083002/diff/200001/ui/
> > views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> > > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> > (right):
> > >
> > >
> > https://codereview.chromium.org/2165083002/diff/200001/ui/
> > views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode741
> > > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:741: bool
> > is_active
> > > = (has_focus_ || has_pointer_focus_) && !ignore_input_;
> > > On 2016/08/22 20:27:07, Tom Anderson wrote:
> > > > On 2016/08/19 23:29:58, danakj wrote:
> > > > > FWIW normally an X window will show itself as visible active even
> > tho it
> > > > doesn't
> > > > > have focus during a grab. This looks like it would not do that. I
> > think
> > you
> > > > want
> > > > > actually has_window_focus_ here if this is for display the focus
> > state to
> > > the
> > > > > user (title bar color etc), but has_focus_ if this is for deciding
> > if we
> > > > expect
> > > > > keyboard input.
> > > > >
> > > >
> > > > The cross-platform code seems to want a window to be active when it
> > will
> > > receive
> > > > keyboard input, so that's the definition I went with here.
> > > >
> > > > Like you said, we will receive keyboard input when has_focus_ == true.
> > > > However, we will also receive keyboard input when has_focus_ == false,
> > but
> > > > has_pointer_focus_ == true. An ancestor window has the focus, but the
> > pointer
> > > > is over our window, so we will receive keystrokes.
> > > >
> > > > A stripped-down demo of this is http://www.sbin.org/doc/Xlib/
> > chapt_09.html,
> > > > under "Monitoring whether keyboard input will be available".
> > > >
> > > > > Try alt-tabbing with most WMs and I think you should see a
> > LeaveFocus with
> > > > > NotifyGrab while holding alt, and EnterFocus NotifyUngrab when
> > letting it
> > go
> > > > > (can see this in Openbox at least with this demo:
> > > > > https://github.com/danakj/openbox/blob/master/tests/focusout.c
> > > >
> > > > Cool demo, I didn't know you had your own WM. I'll try it out :)
> > >
> > > OK just to be clear then, when alt-tabbing does the active chrome window
> > change
> > > color to be unfocused now?
> >
> > On unity it doesn't change color at all. I'm guessing I have to use a WM
> > that
> > chrome doesn't have a theme for? I'll try on Openbox.
> >
> 
> Oh weird. Maybe just try a custom chrome theme from the theme store. My
> theme changes green shades when focused/not. Maybe the "gtk theme" doesn't
> (that makes me sad).
> 

During an alt-tab, the active chrome window changes color to unfocused.

danakj

On Mon, Aug 22, 2016 at 1:48 PM, <thomasanderson@google.com> wrote: > On 2016/08/22 20:40:37, danakj ...

4 years, 4 months ago (2016-08-22 20:51:36 UTC) #77

On Mon, Aug 22, 2016 at 1:48 PM, <thomasanderson@google.com> wrote:

> On 2016/08/22 20:40:37, danakj wrote:
>
> > On Mon, Aug 22, 2016 at 1:38 PM, <mailto:thomasanderson@google.com>
> wrote:
> >
> > > On 2016/08/22 20:32:43, danakj wrote:
> > > >
> > > https://codereview.chromium.org/2165083002/diff/200001/ui/
> > > views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> > > > File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
> > > (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2165083002/diff/200001/ui/
> > > views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode741
> > > > ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:741:
> bool
> > > is_active
> > > > = (has_focus_ || has_pointer_focus_) && !ignore_input_;
> > > > On 2016/08/22 20:27:07, Tom Anderson wrote:
> > > > > On 2016/08/19 23:29:58, danakj wrote:
> > > > > > FWIW normally an X window will show itself as visible active even
> > > tho it
> > > > > doesn't
> > > > > > have focus during a grab. This looks like it would not do that. I
> > > think
> > > you
> > > > > want
> > > > > > actually has_window_focus_ here if this is for display the focus
> > > state to
> > > > the
> > > > > > user (title bar color etc), but has_focus_ if this is for
> deciding
> > > if we
> > > > > expect
> > > > > > keyboard input.
> > > > > >
> > > > >
> > > > > The cross-platform code seems to want a window to be active when it
> > > will
> > > > receive
> > > > > keyboard input, so that's the definition I went with here.
> > > > >
> > > > > Like you said, we will receive keyboard input when has_focus_ ==
> true.
> > > > > However, we will also receive keyboard input when has_focus_ ==
> false,
> > > but
> > > > > has_pointer_focus_ == true. An ancestor window has the focus, but
> the
> > > pointer
> > > > > is over our window, so we will receive keystrokes.
> > > > >
> > > > > A stripped-down demo of this is http://www.sbin.org/doc/Xlib/
> > > chapt_09.html,
> > > > > under "Monitoring whether keyboard input will be available".
> > > > >
> > > > > > Try alt-tabbing with most WMs and I think you should see a
> > > LeaveFocus with
> > > > > > NotifyGrab while holding alt, and EnterFocus NotifyUngrab when
> > > letting it
> > > go
> > > > > > (can see this in Openbox at least with this demo:
> > > > > > https://github.com/danakj/openbox/blob/master/tests/focusout.c
> > > > >
> > > > > Cool demo, I didn't know you had your own WM. I'll try it out :)
> > > >
> > > > OK just to be clear then, when alt-tabbing does the active chrome
> window
> > > change
> > > > color to be unfocused now?
> > >
> > > On unity it doesn't change color at all. I'm guessing I have to use a
> WM
> > > that
> > > chrome doesn't have a theme for? I'll try on Openbox.
> > >
> >
> > Oh weird. Maybe just try a custom chrome theme from the theme store. My
> > theme changes green shades when focused/not. Maybe the "gtk theme"
> doesn't
> > (that makes me sad).
> >
>
> During an alt-tab, the active chrome window changes color to unfocused.
>

Ok, that seems bad to me, it is at least inconsisent with toolkits/window
managers. Do you think we should choose the color from a new function
instead of IsActive() if IsActive() really is strictly if keys go to the
window? I kinda think we should just lie during Alt-tab unless that has
some weird bad sideeffects which I am not expecting.


>
>
> https://codereview.chromium.org/2165083002/
>

-- 
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.

danakj

On Mon, Aug 22, 2016 at 1:42 PM, <erg@chromium.org> wrote: > On 2016/08/22 20:40:37, danakj ...

4 years, 4 months ago (2016-08-22 20:52:15 UTC) #78

Tom (Use chromium acct)

On 2016/08/22 20:51:36, danakj wrote: > Ok, that seems bad to me, it is at ...

4 years, 4 months ago (2016-08-22 20:59:45 UTC) #79

danakj

On Mon, Aug 22, 2016 at 1:59 PM, <thomasanderson@google.com> wrote: > On 2016/08/22 20:51:36, danakj ...

4 years, 4 months ago (2016-08-22 21:10:55 UTC) #80

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-22 21:39:25 UTC) #81

commit-bot: I haz the power

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_rel_ng/builds/266214)

4 years, 4 months ago (2016-08-22 21:39:26 UTC) #82

Tom (Use chromium acct)

On 2016/08/22 21:10:55, danakj wrote: > On Mon, Aug 22, 2016 at 1:59 PM, <mailto:thomasanderson@google.com> ...

4 years, 3 months ago (2016-08-26 19:07:31 UTC) #83

danakj

LGTM https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right): https://codereview.chromium.org/2165083002/diff/200001/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc#newcode249 ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:249: OnHostLostWindowCapture(); On 2016/08/22 20:27:07, Tom Anderson wrote: > ...

4 years, 3 months ago (2016-08-26 19:58:44 UTC) #84

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com

4 years, 3 months ago (2016-08-26 20:01:27 UTC) #85

Tom (Use chromium acct)

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, sky@chromium.org Link to ...

4 years, 3 months ago (2016-08-26 20:01:27 UTC) #86

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/260001

4 years, 3 months ago (2016-08-26 20:03:05 UTC) #87

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-26 20:06:29 UTC) #88

commit-bot: I haz the power

Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/59341) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, ...

4 years, 3 months ago (2016-08-26 20:06:31 UTC) #89

Tom (Use chromium acct)

The CQ bit was checked by thomasanderson@google.com

4 years, 3 months ago (2016-08-26 20:20:17 UTC) #90

Tom (Use chromium acct)

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, erg@chromium.org, sky@chromium.org Link ...

4 years, 3 months ago (2016-08-26 20:20:17 UTC) #91

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2165083002/280001

4 years, 3 months ago (2016-08-26 20:21:39 UTC) #92

commit-bot: I haz the power

Description was changed from ========== * Remove all tracking of _NET_ACTIVE_WINDOW * Remove timestamp tracking ...

4 years, 3 months ago (2016-08-26 21:20:07 UTC) #93

commit-bot: I haz the power

Description was changed from ========== * Remove all tracking of _NET_ACTIVE_WINDOW * Remove timestamp tracking ...

4 years, 3 months ago (2016-08-26 21:22:29 UTC) #95

commit-bot: I haz the power

4 years, 3 months ago (2016-08-26 21:22:33 UTC) #96

Message was sent while issue was closed.

Patchset 12 (id:??) landed as
https://crrev.com/3b6aeffa075816129e32e57412b1c89ba619df10
Cr-Commit-Position: refs/heads/master@{#414807}

Issue 2165083002: Linux: Refactor X11DesktopHandler (Closed)

Description

Patch Set 1 #

Patch Set 2 : Refactor #

Patch Set 3 : Rebase #

Patch Set 4 : Fix tests #

Patch Set 5 : Fixes & handle XI2 events #

Patch Set 6 : Forgot to ignore XINotifyPassive(Un)Grab #

Patch Set 7 : Fix tests #

Patch Set 8 : window_mapped_ -> IsVisible() #

Patch Set 9 : Add GetTimestamp() in libgtk2ui files #

Patch Set 10 : Add lots of comments #

Patch Set 11 : Ignore keyboard grabs #

Patch Set 12 : Rebase to ToT #

Messages