Run event executor on the ui thread to remove the need to explicitly XFlush() the XTest calls.

Run event executor on the ui thread to remove the need to explicitly XFlush() the XTest calls.


BUG=none
TEST=Connect to a host and try moving the mouse and typing. Both inputs should be reflected correctly.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75477

[Jamie, 2011-02-11 14:59:14]

Hi Albert,

Can you take a look at this changelist. I tried to stick as closely as possible
to the chat we had outlining this in which you suggested moving the UI loop into
the chromoting context. Unfortunately that's not quite possible because only one
such loop can exist on UNIX, so it was necessary to pass a reference to it to
the context. I've also had to modify message_loop_glib.cc to initialize gdk so
that event_executor_linux knows which Display to use (ie, the one that gdk will
pass to XPending or similar). I'm not an expert on GDK, so there might be a
better approach to ensuring that both code paths are using the same Display--I'm
open to suggestions. If it turns out that this gdk initialization is required
then we'll probably want to pull in another reviewer for that part of the
change.

Cheers,
Jamie

[awong, 2011-02-14 17:27:29]

Adding deanm to look at message_pump_glib.cc since his name seems to be peppered
all over it's revision history.

For the rest of the code, it looks solid.  I was wondering, though, if we could
avoid doing anything with grabbing or closing the display outside of the
messageloop/pump.

http://codereview.chromium.org/6489031/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_context_unittest.cc (right):

http://codereview.chromium.org/6489031/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_context_unittest.cc:14: ChromotingHostContext
context(0);
Please use NULL instead of 0.

http://codereview.chromium.org/6489031/diff/2001/remoting/host/event_executor...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/6489031/diff/2001/remoting/host/event_executor...
remoting/host/event_executor_linux.cc:244: display_ =
GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
hmm...can we poke a hole into the message loop such that it gives us the
associated display back? I'm leery of leaking an implicit GDK dependency here.

In the contenxt, we'd want to store a MessageLoopUI instead of just MessageLoop.

[Alpha Left Google, 2011-02-14 18:09:17]

Looks pretty good to me. I was going to suggest using the UI message loop as the
main message loop for chromoting, but I guess it doesn't matter as we can easily
switching threads and using an extra thread doesn't hurt.

[Jamie, 2011-02-14 19:12:38]

Needless to say, the description attached to the latest patch is a typo... I've
just fixed the first of ajwong's comments and responded to the second.

http://codereview.chromium.org/6489031/diff/2001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_context_unittest.cc (right):

http://codereview.chromium.org/6489031/diff/2001/remoting/host/chromoting_hos...
remoting/host/chromoting_host_context_unittest.cc:14: ChromotingHostContext
context(0);
On 2011/02/14 17:27:29, awong wrote:
> Please use NULL instead of 0.

Done.

http://codereview.chromium.org/6489031/diff/2001/remoting/host/event_executor...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/6489031/diff/2001/remoting/host/event_executor...
remoting/host/event_executor_linux.cc:244: display_ =
GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
On 2011/02/14 17:27:29, awong wrote:
> hmm...can we poke a hole into the message loop such that it gives us the
> associated display back? I'm leery of leaking an implicit GDK dependency here.
> 
> In the contenxt, we'd want to store a MessageLoopUI instead of just
MessageLoop.

I agree, but I couldn't see any clean way of doing this. Passing a MessageLoopUI
into the context means also passing one into ServiceProcess::Initialize, which
made me nervous. Even if we do do that, it's not clear how we would get at a
platform-specific Display pointer from a cross-platform interface.

[Wez, 2011-02-15 11:06:47]

We can pull the display name out of GDK into the string form and re-open the
display from within EventExecutorLinux, which would avoid that dependency in
the interface.  On other platforms we either don't need that information and
can pass an empty or NULL identifier, or if we do need it we can synthesise
something suitable.

On 14 February 2011 19:12, <jamiewalch@chromium.org> wrote:
>
>
>
http://codereview.chromium.org/6489031/diff/2001/remoting/host/event_executor...
> File remoting/host/event_executor_linux.cc (right):
>
>
>
http://codereview.chromium.org/6489031/diff/2001/remoting/host/event_executor...
> remoting/host/event_executor_linux.cc:244: display_ =
> GDK_DISPLAY_XDISPLAY(gdk_display_get_default());
> On 2011/02/14 17:27:29, awong wrote:
>
>> hmm...can we poke a hole into the message loop such that it gives us
>>
> the
>
>> associated display back? I'm leery of leaking an implicit GDK
>>
> dependency here.
>
>  In the contenxt, we'd want to store a MessageLoopUI instead of just
>>
> MessageLoop.
>
> I agree, but I couldn't see any clean way of doing this. Passing a
> MessageLoopUI into the context means also passing one into
> ServiceProcess::Initialize, which made me nervous. Even if we do do
> that, it's not clear how we would get at a platform-specific Display
> pointer from a cross-platform interface.
>
>
> http://codereview.chromium.org/6489031/
>

[Jamie, 2011-02-15 12:02:13]

On 2011/02/15 11:06:47, Wez wrote:
> We can pull the display name out of GDK into the string form and re-open the
> display from within EventExecutorLinux, which would avoid that dependency in
> the interface.

I don't think that re-opening the display string will work because it will make
a separate socket connection. You need the actual Display* reference that gdk is
internally passing to XPending (or equivalent).

[Wez, 2011-02-15 12:46:45]

Doesn't the act of opening a display with gdk_display_open() cause it to be
added to the set of sources the Glib run-loop works with?

OTOH, working with the "default" display isn't necessarily a problem.  On
other platforms it's unlikely that we'll be able to support remoting for
multiple "displays" concurrently within a single process - we'll need a
separate process per-display in general - so perhaps having the Linux
implementation limited in this way is not really an issue at all?


On 15 February 2011 12:02, <jamiewalch@chromium.org> wrote:

> On 2011/02/15 11:06:47, Wez wrote:
>
>> We can pull the display name out of GDK into the string form and re-open
>> the
>> display from within EventExecutorLinux, which would avoid that dependency
>> in
>> the interface.
>>
>
> I don't think that re-opening the display string will work because it will
> make
> a separate socket connection. You need the actual Display* reference that
> gdk is
> internally passing to XPending (or equivalent).
>
>
> http://codereview.chromium.org/6489031/
>

[Dean McNamee, 2011-02-15 17:06:26]

Thanks...

http://codereview.chromium.org/6489031/diff/9002/base/message_pump_glib.cc
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/6489031/diff/9002/base/message_pump_glib.cc#ne...
base/message_pump_glib.cc:176: // Initialize gdk.
This is the classic style of comment:

// Add one to x.
x += 1;

Why are we initializing GDK?  I haven't looked that deeply at the patch, but I
definitely don't know why it's being added.

[Jamie, 2011-02-15 17:17:03]

http://codereview.chromium.org/6489031/diff/9002/base/message_pump_glib.cc
File base/message_pump_glib.cc (right):

http://codereview.chromium.org/6489031/diff/9002/base/message_pump_glib.cc#ne...
base/message_pump_glib.cc:176: // Initialize gdk.
On 2011/02/15 17:06:26, Dean McNamee wrote:
> This is the classic style of comment:
> 
> // Add one to x.
> x += 1;

Yes, I put the comment in before I factored out the function. I'll remove it.

> Why are we initializing GDK?  I haven't looked that deeply at the patch, but I
> definitely don't know why it's being added.

I need to be able to get the Display reference that GDK is internally passing to
XPending (or equivalent) so that another class can use it (actually, in a
revision to this CL it will most likely be pulled into this class).
MessagePumpGlibX uses gdk_display_get_default(), but that only works if gdk_init
has been called. Also, it seems that if we don't initialize gdk then we're not
respecting command-line arguments such as "-display".

[Dean McNamee, 2011-02-15 18:51:45]

On 2011/02/15 17:17:03, jamiewalch wrote:
> http://codereview.chromium.org/6489031/diff/9002/base/message_pump_glib.cc
> File base/message_pump_glib.cc (right):
> 
>
http://codereview.chromium.org/6489031/diff/9002/base/message_pump_glib.cc#ne...
> base/message_pump_glib.cc:176: // Initialize gdk.
> On 2011/02/15 17:06:26, Dean McNamee wrote:
> > This is the classic style of comment:
> > 
> > // Add one to x.
> > x += 1;
> 
> Yes, I put the comment in before I factored out the function. I'll remove it.
> 
> > Why are we initializing GDK?  I haven't looked that deeply at the patch, but
I
> > definitely don't know why it's being added.
> 
> I need to be able to get the Display reference that GDK is internally passing
to
> XPending (or equivalent) so that another class can use it (actually, in a
> revision to this CL it will most likely be pulled into this class).
> MessagePumpGlibX uses gdk_display_get_default(), but that only works if
gdk_init
> has been called. Also, it seems that if we don't initialize gdk then we're not
> respecting command-line arguments such as "-display".

Can you put all of this in a comment?  I was less requesting you to remove the
comment, but instead to give a nice explanation about why gdk should be
initialized, and why it should be initialized at that point.

I know that once upon a time there was gdk initialization, it's either still
there somewhere, or if it's not there is a reason for it.  I would ask evanm or
estade.

[awong, 2011-02-16 23:07:41]

I wanted to vet our ability to expose the X display out of the message loop, so
I asked on IRC.

14:40 <+awong> any linux guys around willing to help look over a
message_pump_glib.cc change?
14:49 <+awong>
http://codereview.chromium.org/6489031/diff/14002/base/message_loop.h
14:51 < sadrulhc> awong: ui/base/x/x11_util.h:GetXDisplay
14:53 <+awong> sadrulhc: man, I wish I had known about that earlier. Thanks
14:55 <+awong> sadrulhc: so we assume that the default display is the one
associated with the main UI message loop?
14:55 < sadrulhc> yep. (I don't know if using x11_util it would involve adding
ui/base/x to DEPS, though)
14:57 <+awong> sadrulhc: we'd be wanting to take a dependency onto it from
remoting. Is this the correct location for these utility functions?  Or should
some sit in base?
14:58 < sadrulhc> I can see that ui/gfx/ is used from remoting/, so perhaps
ui/base/x/ would also be OK
9 <+awong> sadrulhc: Conceptually, what is the "ui" directory supposed to be?  A
bag of code related to talking with the UI subsystem of each platform?
15:00 < sadrulhc> awong: ui/README.chromium ;)
15:02 <+awong> sadrulhc: hah! thanks. :)  I'm slightly worried about our usage
here since we're not really building UI; we're taking screen grabs. But
whatever. Sounds good 
enough for now.


So...I think this avoids us needing the message loop change since we can just
use this utility function...Does that sound right to you Jamie?  If so, sorry
for the runaround about the messageloop API. :(

We'll need to add a line into DEPS for remoting if we depend on a header in
ui/base/x, but that's easy.

[jamiewalch, 2011-02-17 15:54:34]

I've looked into that and I think it will work, but aren't we going
round in circles here? In a previous incarnation, event_executor_linux
got the default display via much the same mechanism as GetXDisplay
does, but we felt that we didn't want to pollute it with a GDK
dependency. Putting that dependency behind a function call doesn't
seem much better, especially as we'd have to pull in app_base to get
it. An alternative that I've just discussed with Alpha is to pull just
the code file defining GetXDisplay into remoting.gyp, but then we're
*really* close to the previous incarnation.

Allowing the Display pointer to be obtained from the message pump
that's using it seems better, even if it does entail a
platform-specific #ifdef (although I might feel differently about that
point if there weren't already several of those in this area already).

Cheers,
Jamie

On 17 February 2011 00:07,  <ajwong@chromium.org> wrote:
> I wanted to vet our ability to expose the X display out of the message loop,
> so
> I asked on IRC.
>
> 14:40 <+awong> any linux guys around willing to help look over a
> message_pump_glib.cc change?
> 14:49 <+awong>
> http://codereview.chromium.org/6489031/diff/14002/base/message_loop.h
> 14:51 < sadrulhc> awong: ui/base/x/x11_util.h:GetXDisplay
> 14:53 <+awong> sadrulhc: man, I wish I had known about that earlier. Thanks
> 14:55 <+awong> sadrulhc: so we assume that the default display is the one
> associated with the main UI message loop?
> 14:55 < sadrulhc> yep. (I don't know if using x11_util it would involve
> adding
> ui/base/x to DEPS, though)
> 14:57 <+awong> sadrulhc: we'd be wanting to take a dependency onto it from
> remoting. Is this the correct location for these utility functions?  Or
> should
> some sit in base?
> 14:58 < sadrulhc> I can see that ui/gfx/ is used from remoting/, so perhaps
> ui/base/x/ would also be OK
> 9 <+awong> sadrulhc: Conceptually, what is the "ui" directory supposed to
> be?  A
> bag of code related to talking with the UI subsystem of each platform?
> 15:00 < sadrulhc> awong: ui/README.chromium ;)
> 15:02 <+awong> sadrulhc: hah! thanks. :)  I'm slightly worried about our
> usage
> here since we're not really building UI; we're taking screen grabs. But
> whatever. Sounds good
> enough for now.
>
>
> So...I think this avoids us needing the message loop change since we can
> just
> use this utility function...Does that sound right to you Jamie?  If so,
> sorry
> for the runaround about the messageloop API. :(
>
> We'll need to add a line into DEPS for remoting if we depend on a header in
> ui/base/x, but that's easy.
>
> http://codereview.chromium.org/6489031/
>

[awong, 2011-02-17 16:08:37]

Yeah, it is a circle. :(

I think the main concern with throwing GetDisplay into the messagepump/loop
will actually not be the platform ifdef, but the leakage of the
un-namedspaced, "Display" type into the header of messageloop.h.

As for the GDK dependency, it's more of a source dependency; app/base/x is
already designed as an interface layer according to the header file comments
in x_util.h

That being said, you make a convincing argument, and I think agree with
you...

Since we both agree it'd be cleaner to expose the display out of the
pump/loop, let's go with that and I'll see if I can't find someone more
familiar with the effects of leaking the type out of message loop before
landing.  It might take a couple of days though since the main linux guys
are out on vacation.

-Albert



On Thu, Feb 17, 2011 at 7:54 AM, James Walch <jamiewalch@google.com> wrote:

> I've looked into that and I think it will work, but aren't we going
> round in circles here? In a previous incarnation, event_executor_linux
> got the default display via much the same mechanism as GetXDisplay
> does, but we felt that we didn't want to pollute it with a GDK
> dependency. Putting that dependency behind a function call doesn't
> seem much better, especially as we'd have to pull in app_base to get
> it. An alternative that I've just discussed with Alpha is to pull just
> the code file defining GetXDisplay into remoting.gyp, but then we're
> *really* close to the previous incarnation.
>
> Allowing the Display pointer to be obtained from the message pump
> that's using it seems better, even if it does entail a
> platform-specific #ifdef (although I might feel differently about that
> point if there weren't already several of those in this area already).
>
> Cheers,
> Jamie
>
> On 17 February 2011 00:07,  <ajwong@chromium.org> wrote:
> > I wanted to vet our ability to expose the X display out of the message
> loop,
> > so
> > I asked on IRC.
> >
> > 14:40 <+awong> any linux guys around willing to help look over a
> > message_pump_glib.cc change?
> > 14:49 <+awong>
> > http://codereview.chromium.org/6489031/diff/14002/base/message_loop.h
> > 14:51 < sadrulhc> awong: ui/base/x/x11_util.h:GetXDisplay
> > 14:53 <+awong> sadrulhc: man, I wish I had known about that earlier.
> Thanks
> > 14:55 <+awong> sadrulhc: so we assume that the default display is the one
> > associated with the main UI message loop?
> > 14:55 < sadrulhc> yep. (I don't know if using x11_util it would involve
> > adding
> > ui/base/x to DEPS, though)
> > 14:57 <+awong> sadrulhc: we'd be wanting to take a dependency onto it
> from
> > remoting. Is this the correct location for these utility functions?  Or
> > should
> > some sit in base?
> > 14:58 < sadrulhc> I can see that ui/gfx/ is used from remoting/, so
> perhaps
> > ui/base/x/ would also be OK
> > 9 <+awong> sadrulhc: Conceptually, what is the "ui" directory supposed to
> > be?  A
> > bag of code related to talking with the UI subsystem of each platform?
> > 15:00 < sadrulhc> awong: ui/README.chromium ;)
> > 15:02 <+awong> sadrulhc: hah! thanks. :)  I'm slightly worried about our
> > usage
> > here since we're not really building UI; we're taking screen grabs. But
> > whatever. Sounds good
> > enough for now.
> >
> >
> > So...I think this avoids us needing the message loop change since we can
> > just
> > use this utility function...Does that sound right to you Jamie?  If so,
> > sorry
> > for the runaround about the messageloop API. :(
> >
> > We'll need to add a line into DEPS for remoting if we depend on a header
> in
> > ui/base/x, but that's easy.
> >
> > http://codereview.chromium.org/6489031/
> >
>

[Wez, 2011-02-17 16:19:39]

FWIW, the GLib message pump already has forward typedefs for GdkEvent, etc,
outside a namespace, doesn't it?

Wez


On 17 February 2011 16:07, Albert J. Wong (王重傑) <ajwong@chromium.org> wrote:

> Yeah, it is a circle. :(
>
> I think the main concern with throwing GetDisplay into the messagepump/loop
> will actually not be the platform ifdef, but the leakage of the
> un-namedspaced, "Display" type into the header of messageloop.h.
>
> As for the GDK dependency, it's more of a source dependency; app/base/x is
> already designed as an interface layer according to the header file comments
> in x_util.h
>
> That being said, you make a convincing argument, and I think agree with
> you...
>
> Since we both agree it'd be cleaner to expose the display out of the
> pump/loop, let's go with that and I'll see if I can't find someone more
> familiar with the effects of leaking the type out of message loop before
> landing.  It might take a couple of days though since the main linux guys
> are out on vacation.
>
> -Albert
>
>
>
> On Thu, Feb 17, 2011 at 7:54 AM, James Walch <jamiewalch@google.com>wrote:
>
>> I've looked into that and I think it will work, but aren't we going
>> round in circles here? In a previous incarnation, event_executor_linux
>> got the default display via much the same mechanism as GetXDisplay
>> does, but we felt that we didn't want to pollute it with a GDK
>> dependency. Putting that dependency behind a function call doesn't
>> seem much better, especially as we'd have to pull in app_base to get
>> it. An alternative that I've just discussed with Alpha is to pull just
>> the code file defining GetXDisplay into remoting.gyp, but then we're
>> *really* close to the previous incarnation.
>>
>> Allowing the Display pointer to be obtained from the message pump
>> that's using it seems better, even if it does entail a
>> platform-specific #ifdef (although I might feel differently about that
>> point if there weren't already several of those in this area already).
>>
>> Cheers,
>> Jamie
>>
>> On 17 February 2011 00:07,  <ajwong@chromium.org> wrote:
>> > I wanted to vet our ability to expose the X display out of the message
>> loop,
>> > so
>> > I asked on IRC.
>> >
>> > 14:40 <+awong> any linux guys around willing to help look over a
>> > message_pump_glib.cc change?
>> > 14:49 <+awong>
>> > http://codereview.chromium.org/6489031/diff/14002/base/message_loop.h
>> > 14:51 < sadrulhc> awong: ui/base/x/x11_util.h:GetXDisplay
>> > 14:53 <+awong> sadrulhc: man, I wish I had known about that earlier.
>> Thanks
>> > 14:55 <+awong> sadrulhc: so we assume that the default display is the
>> one
>> > associated with the main UI message loop?
>> > 14:55 < sadrulhc> yep. (I don't know if using x11_util it would involve
>> > adding
>> > ui/base/x to DEPS, though)
>> > 14:57 <+awong> sadrulhc: we'd be wanting to take a dependency onto it
>> from
>> > remoting. Is this the correct location for these utility functions?  Or
>> > should
>> > some sit in base?
>> > 14:58 < sadrulhc> I can see that ui/gfx/ is used from remoting/, so
>> perhaps
>> > ui/base/x/ would also be OK
>> > 9 <+awong> sadrulhc: Conceptually, what is the "ui" directory supposed
>> to
>> > be?  A
>> > bag of code related to talking with the UI subsystem of each platform?
>> > 15:00 < sadrulhc> awong: ui/README.chromium ;)
>> > 15:02 <+awong> sadrulhc: hah! thanks. :)  I'm slightly worried about our
>> > usage
>> > here since we're not really building UI; we're taking screen grabs. But
>> > whatever. Sounds good
>> > enough for now.
>> >
>> >
>> > So...I think this avoids us needing the message loop change since we can
>> > just
>> > use this utility function...Does that sound right to you Jamie?  If so,
>> > sorry
>> > for the runaround about the messageloop API. :(
>> >
>> > We'll need to add a line into DEPS for remoting if we depend on a header
>> in
>> > ui/base/x, but that's easy.
>> >
>> > http://codereview.chromium.org/6489031/
>> >
>>
>
>

[awong, 2011-02-17 16:50:34]

Yeah, but those are all mangled with a GDK prefix.

Maybe I'm just being paranoid...

On Thu, Feb 17, 2011 at 8:19 AM, Wez <wez@chromium.org> wrote:

> FWIW, the GLib message pump already has forward typedefs for GdkEvent, etc,
> outside a namespace, doesn't it?
>
> Wez
>
>
> On 17 February 2011 16:07, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote:
>
>> Yeah, it is a circle. :(
>>
>> I think the main concern with throwing GetDisplay into the
>> messagepump/loop will actually not be the platform ifdef, but the leakage of
>> the un-namedspaced, "Display" type into the header of messageloop.h.
>>
>> As for the GDK dependency, it's more of a source dependency; app/base/x is
>> already designed as an interface layer according to the header file comments
>> in x_util.h
>>
>> That being said, you make a convincing argument, and I think agree with
>> you...
>>
>> Since we both agree it'd be cleaner to expose the display out of the
>> pump/loop, let's go with that and I'll see if I can't find someone more
>> familiar with the effects of leaking the type out of message loop before
>> landing.  It might take a couple of days though since the main linux guys
>> are out on vacation.
>>
>> -Albert
>>
>>
>>
>> On Thu, Feb 17, 2011 at 7:54 AM, James Walch <jamiewalch@google.com>wrote:
>>
>>> I've looked into that and I think it will work, but aren't we going
>>> round in circles here? In a previous incarnation, event_executor_linux
>>> got the default display via much the same mechanism as GetXDisplay
>>> does, but we felt that we didn't want to pollute it with a GDK
>>> dependency. Putting that dependency behind a function call doesn't
>>> seem much better, especially as we'd have to pull in app_base to get
>>> it. An alternative that I've just discussed with Alpha is to pull just
>>> the code file defining GetXDisplay into remoting.gyp, but then we're
>>> *really* close to the previous incarnation.
>>>
>>> Allowing the Display pointer to be obtained from the message pump
>>> that's using it seems better, even if it does entail a
>>> platform-specific #ifdef (although I might feel differently about that
>>> point if there weren't already several of those in this area already).
>>>
>>> Cheers,
>>> Jamie
>>>
>>> On 17 February 2011 00:07,  <ajwong@chromium.org> wrote:
>>> > I wanted to vet our ability to expose the X display out of the message
>>> loop,
>>> > so
>>> > I asked on IRC.
>>> >
>>> > 14:40 <+awong> any linux guys around willing to help look over a
>>> > message_pump_glib.cc change?
>>> > 14:49 <+awong>
>>> > http://codereview.chromium.org/6489031/diff/14002/base/message_loop.h
>>> > 14:51 < sadrulhc> awong: ui/base/x/x11_util.h:GetXDisplay
>>> > 14:53 <+awong> sadrulhc: man, I wish I had known about that earlier.
>>> Thanks
>>> > 14:55 <+awong> sadrulhc: so we assume that the default display is the
>>> one
>>> > associated with the main UI message loop?
>>> > 14:55 < sadrulhc> yep. (I don't know if using x11_util it would involve
>>> > adding
>>> > ui/base/x to DEPS, though)
>>> > 14:57 <+awong> sadrulhc: we'd be wanting to take a dependency onto it
>>> from
>>> > remoting. Is this the correct location for these utility functions?  Or
>>> > should
>>> > some sit in base?
>>> > 14:58 < sadrulhc> I can see that ui/gfx/ is used from remoting/, so
>>> perhaps
>>> > ui/base/x/ would also be OK
>>> > 9 <+awong> sadrulhc: Conceptually, what is the "ui" directory supposed
>>> to
>>> > be?  A
>>> > bag of code related to talking with the UI subsystem of each platform?
>>> > 15:00 < sadrulhc> awong: ui/README.chromium ;)
>>> > 15:02 <+awong> sadrulhc: hah! thanks. :)  I'm slightly worried about
>>> our
>>> > usage
>>> > here since we're not really building UI; we're taking screen grabs. But
>>> > whatever. Sounds good
>>> > enough for now.
>>> >
>>> >
>>> > So...I think this avoids us needing the message loop change since we
>>> can
>>> > just
>>> > use this utility function...Does that sound right to you Jamie?  If so,
>>> > sorry
>>> > for the runaround about the messageloop API. :(
>>> >
>>> > We'll need to add a line into DEPS for remoting if we depend on a
>>> header in
>>> > ui/base/x, but that's easy.
>>> >
>>> > http://codereview.chromium.org/6489031/
>>> >
>>>
>>
>>
>

[Wez, 2011-02-17 17:24:02]

No, that's a fair point.

I think "Display" is relatively safe, though, for precisely the reason that
one generally has to assume that Unix apps will be polluted with X defines
of that sort [walks away to sound of build breaking due to Display
definition clashes... ;)].


On 17 February 2011 16:50, Albert J. Wong (王重傑) <ajwong@chromium.org> wrote:

> Yeah, but those are all mangled with a GDK prefix.
>
> Maybe I'm just being paranoid...
>
>
> On Thu, Feb 17, 2011 at 8:19 AM, Wez <wez@chromium.org> wrote:
>
>> FWIW, the GLib message pump already has forward typedefs for GdkEvent,
>> etc, outside a namespace, doesn't it?
>>
>> Wez
>>
>>
>> On 17 February 2011 16:07, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote:
>>
>>> Yeah, it is a circle. :(
>>>
>>> I think the main concern with throwing GetDisplay into the
>>> messagepump/loop will actually not be the platform ifdef, but the leakage of
>>> the un-namedspaced, "Display" type into the header of messageloop.h.
>>>
>>> As for the GDK dependency, it's more of a source dependency; app/base/x
>>> is already designed as an interface layer according to the header file
>>> comments in x_util.h
>>>
>>> That being said, you make a convincing argument, and I think agree with
>>> you...
>>>
>>> Since we both agree it'd be cleaner to expose the display out of the
>>> pump/loop, let's go with that and I'll see if I can't find someone more
>>> familiar with the effects of leaking the type out of message loop before
>>> landing.  It might take a couple of days though since the main linux guys
>>> are out on vacation.
>>>
>>> -Albert
>>>
>>>
>>>
>>> On Thu, Feb 17, 2011 at 7:54 AM, James Walch <jamiewalch@google.com>wrote:
>>>
>>>> I've looked into that and I think it will work, but aren't we going
>>>> round in circles here? In a previous incarnation, event_executor_linux
>>>> got the default display via much the same mechanism as GetXDisplay
>>>> does, but we felt that we didn't want to pollute it with a GDK
>>>> dependency. Putting that dependency behind a function call doesn't
>>>> seem much better, especially as we'd have to pull in app_base to get
>>>> it. An alternative that I've just discussed with Alpha is to pull just
>>>> the code file defining GetXDisplay into remoting.gyp, but then we're
>>>> *really* close to the previous incarnation.
>>>>
>>>> Allowing the Display pointer to be obtained from the message pump
>>>> that's using it seems better, even if it does entail a
>>>> platform-specific #ifdef (although I might feel differently about that
>>>> point if there weren't already several of those in this area already).
>>>>
>>>> Cheers,
>>>> Jamie
>>>>
>>>> On 17 February 2011 00:07,  <ajwong@chromium.org> wrote:
>>>> > I wanted to vet our ability to expose the X display out of the message
>>>> loop,
>>>> > so
>>>> > I asked on IRC.
>>>> >
>>>> > 14:40 <+awong> any linux guys around willing to help look over a
>>>> > message_pump_glib.cc change?
>>>> > 14:49 <+awong>
>>>> > http://codereview.chromium.org/6489031/diff/14002/base/message_loop.h
>>>> > 14:51 < sadrulhc> awong: ui/base/x/x11_util.h:GetXDisplay
>>>> > 14:53 <+awong> sadrulhc: man, I wish I had known about that earlier.
>>>> Thanks
>>>> > 14:55 <+awong> sadrulhc: so we assume that the default display is the
>>>> one
>>>> > associated with the main UI message loop?
>>>> > 14:55 < sadrulhc> yep. (I don't know if using x11_util it would
>>>> involve
>>>> > adding
>>>> > ui/base/x to DEPS, though)
>>>> > 14:57 <+awong> sadrulhc: we'd be wanting to take a dependency onto it
>>>> from
>>>> > remoting. Is this the correct location for these utility functions?
>>>>  Or
>>>> > should
>>>> > some sit in base?
>>>> > 14:58 < sadrulhc> I can see that ui/gfx/ is used from remoting/, so
>>>> perhaps
>>>> > ui/base/x/ would also be OK
>>>> > 9 <+awong> sadrulhc: Conceptually, what is the "ui" directory supposed
>>>> to
>>>> > be?  A
>>>> > bag of code related to talking with the UI subsystem of each platform?
>>>> > 15:00 < sadrulhc> awong: ui/README.chromium ;)
>>>> > 15:02 <+awong> sadrulhc: hah! thanks. :)  I'm slightly worried about
>>>> our
>>>> > usage
>>>> > here since we're not really building UI; we're taking screen grabs.
>>>> But
>>>> > whatever. Sounds good
>>>> > enough for now.
>>>> >
>>>> >
>>>> > So...I think this avoids us needing the message loop change since we
>>>> can
>>>> > just
>>>> > use this utility function...Does that sound right to you Jamie?  If
>>>> so,
>>>> > sorry
>>>> > for the runaround about the messageloop API. :(
>>>> >
>>>> > We'll need to add a line into DEPS for remoting if we depend on a
>>>> header in
>>>> > ui/base/x, but that's easy.
>>>> >
>>>> > http://codereview.chromium.org/6489031/
>>>> >
>>>>
>>>
>>>
>>
>

[Jamie, 2011-02-18 13:08:28]

I think this is ready for re-review. To summarise, since this has been going for
a while now:

* The chrome service process now uses a MessageLoopForUI instead of a plain
MessageLoop.
* MessageLoopForUI on Linux allows the underlying Display* to be accessed
(requires the GDK has been initialized).
* event_executor_linux uses this Display* instead of opening its own, and runs
on the ui thread.
* The chrome service process and chromoting_simple_host now both have to
initialise GTK.

[Alpha Left Google, 2011-02-18 13:12:53]

LGTM.

[awong, 2011-02-18 23:10:27]

LGTM

Would still like to get Evan Martin's opinion on whether or not exposing Display
in message_loop.h is okay on unix platforms.  But he's out.  Going to
optimistically commit, and then ask him when he's back if we should revert part
of this CL.

[commit-bot: I haz the power, 2011-02-18 23:11:41]

Can't apply patch for file chrome/service/service_process.cc.
error: patch failed: chrome/service/service_process.cc:26
error: chrome/service/service_process.cc: patch does not apply

[sadrul, 2011-02-19 07:02:38]

http://codereview.chromium.org/6489031/diff/24001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6489031/diff/24001/base/message_loop.h#newcode513
base/message_loop.h:513: Display* get_display();
I see that this has already landed, but ... is there a reason for the horrific
name for the function (i.e. why 'get_display' instead of 'GetDisplay'?)

[Jamie, 2011-02-19 08:09:06]

http://codereview.chromium.org/6489031/diff/24001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6489031/diff/24001/base/message_loop.h#newcode513
base/message_loop.h:513: Display* get_display();
On 2011/02/19 07:02:39, sadrul wrote:
> I see that this has already landed, but ... is there a reason for the horrific
> name for the function (i.e. why 'get_display' instead of 'GetDisplay'?)

I was aiming for consistency with other getter-like functions in this header,
for example pump_win and pump_libevent, but just calling it display() didn't
feel descriptive enough.

[sadrul, 2011-02-19 08:22:50]

http://codereview.chromium.org/6489031/diff/24001/base/message_loop.h
File base/message_loop.h (right):

http://codereview.chromium.org/6489031/diff/24001/base/message_loop.h#newcode513
base/message_loop.h:513: Display* get_display();
On 2011/02/19 08:09:06, jamiewalch wrote:
> On 2011/02/19 07:02:39, sadrul wrote:
> > I see that this has already landed, but ... is there a reason for the
horrific
> > name for the function (i.e. why 'get_display' instead of 'GetDisplay'?)
> 
> I was aiming for consistency with other getter-like functions in this header,
> for example pump_win and pump_libevent, but just calling it display() didn't
> feel descriptive enough.

Simple accessor functions are named like this, so if there was a |display_|
attribute, the accessor would be display(). But for this, I believe GetDisplay
would have been more appropriate. 

But, I would suggest you wait for Evan Martin to weigh in on whether this is the
right way of exposing the Display first before renaming the function (if you
wanted to).