Remove X11 and GTK dependencies from host_forwarder

Remove X11 and GTK dependencies from host_forwarder

Use libevent as the implementation of MessagePumpForUI.

BUG=346113
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257853

[Philippe, 2014-03-03 15:15:13]

On 2014/03/03 10:43:23, Zhenyu Liang wrote:

Thanks Zhenyu,

The linking error comes from the use of GTK in message_loop.cc, right? Is there
a way to force the libevent implementation of MessageLoop instead? I saw the
USE_OZONE and USE_AURA flags in particular (which are used on ChromeOS I
believe). Are you aware of them?

[Zhenyu Liang, 2014-03-04 11:06:26]

Previous patch is just a quick and dirty fix to make linker happy.
Actually, I'm not familiar with this component.
Please review the new patch which is following your hint :-)

[Philippe, 2014-03-07 09:14:06]

Thanks Zhenyu! The change looks great to me. You will need a base/ reviewer
though.

LGTM for changes in tools/android/.

[Zhenyu Liang, 2014-03-10 04:54:07]

Mark, could you please take a look on this CL? Thanks.

[Mark Mentovai, 2014-03-10 14:05:20]

https://codereview.chromium.org/176713005/diff/60001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/176713005/diff/60001/base/base.gypi#newcode819
base/base.gypi:819: 'USE_AURA=1',
I’m not sure about this. Android isn’t really an Aura platform, right?

https://codereview.chromium.org/176713005/diff/60001/base/base.gypi#newcode826
base/base.gypi:826: ['include', '^message_loop/message_pump_ozone\\.cc$'],
Do you really need message_pump_ozone.cc, or is message_pump_libevent.cc enough?

[Zhenyu Liang, 2014-03-12 08:39:06]

Mark, I did some more investigation on this problem.
We can not distinguish between the Android host platform and conventional Linux
platform in current build configuration. 
MessageLoop must have a MessagePumpForUI implementation on Linux platform (as
the android host here). There are three pumps, X11, Gtk and Ozone. We currently
use the Gtk pump. To get rid of the Gtk/X11 dependency, we have to use the Ozone
pump which use the libevent internally. That's why we need the USE_AURA and
USE_OZONE flags.

[Mark Mentovai, 2014-03-12 15:37:17]

What do you mean that “We can not distinguish between the Android host platform
and conventional Linux platform in current build configuration.” We have the GYP
variable OS="android" in GYP-land, and the C macro OS_ANDROID in C++-land. What
else do we need?

It seems that you really just want a libevent-backed message pump. Why aren’t
you using MessagePumpLibevent directly, instead of MessagePumpOzone? Android is
neither Aura nor Ozone, and pretending that it is just to make this work creates
a confusing mess. There must be a better way for this to work.

[Philippe, 2014-03-12 15:45:46]

On 2014/03/12 15:37:17, Mark Mentovai wrote:
> What do you mean that “We can not distinguish between the Android host
platform
> and conventional Linux platform in current build configuration.” We have the
GYP
> variable OS="android" in GYP-land, and the C macro OS_ANDROID in C++-land.
What
> else do we need?
> 
> It seems that you really just want a libevent-backed message pump. Why aren’t
> you using MessagePumpLibevent directly, instead of MessagePumpOzone? Android
is
> neither Aura nor Ozone, and pretending that it is just to make this work
creates
> a confusing mess. There must be a better way for this to work.

Thanks Mark, the message loop gets instantiated/linked in due to our use of
base::Thread so we don't directly control which message pump implementation we
use (other than through the flags). Note that the change that Zhenyu is doing
concerns the host as opposed to the target. The binary that we build here is for
the Linux host as part of the Android build. The idea here is that we don't want
to depend on GTK just because we are using base::Thread in a standalone
command-line binary. I think that GN had similar issues (there was a thread on
chromium-dev).

Does that make any sense?

[Mark Mentovai, 2014-03-12 16:03:47]

OK, so for the host, this is actually just about having something show up as
MessagePumpForUI, and you don’t actually use or care about the UI pump. And
you’re not ever actually using MessagePumpOzone, but are using
MessagePumpLibevent as your pump directly?

Then you absolutely, 100%, need a comment in base.gypi explaining what you’re
doing and why you’re doing it. If this was explained in the .gyp file, I think
we could tolerate it.

Still, I envision that having to haul in Aura and Ozone may one day be as
objectionable as depending on X11 and GTK. Ultimately, it would probably be best
for this “Android host” configuration to redirect MessagePumpForUI to be the
least harmless and dependency-having implementation possible, probably either a
dummy stub, MessagePumpDefault, or MessagePumpLibevent. If there’s no way to
detect this configuration from a C++ macro right now, you could easily introduce
a new macro here (instead of defining USE_AURA or USE_OZONE) and pick that up in
C++-land to choose the correct pump implementation.

[Zhenyu Liang, 2014-03-12 17:38:27]

On 2014/03/12 16:03:47, Mark Mentovai wrote:
> OK, so for the host, this is actually just about having something show up as
> MessagePumpForUI, and you don’t actually use or care about the UI pump. And
> you’re not ever actually using MessagePumpOzone, but are using
> MessagePumpLibevent as your pump directly?
> 
> Then you absolutely, 100%, need a comment in base.gypi explaining what you’re
> doing and why you’re doing it. If this was explained in the .gyp file, I think
> we could tolerate it.
> 
> Still, I envision that having to haul in Aura and Ozone may one day be as
> objectionable as depending on X11 and GTK. Ultimately, it would probably be
best
> for this “Android host” configuration to redirect MessagePumpForUI to be
the
> least harmless and dependency-having implementation possible, probably either
a
> dummy stub, MessagePumpDefault, or MessagePumpLibevent. If there’s no way to
> detect this configuration from a C++ macro right now, you could easily
introduce
> a new macro here (instead of defining USE_AURA or USE_OZONE) and pick that up
in
> C++-land to choose the correct pump implementation.

I have to agree to Mark. It's not good to abuse the flags. And the change is
simple.

[Zhenyu Liang, 2014-03-12 17:38:33]

On 2014/03/12 16:03:47, Mark Mentovai wrote:
> OK, so for the host, this is actually just about having something show up as
> MessagePumpForUI, and you don’t actually use or care about the UI pump. And
> you’re not ever actually using MessagePumpOzone, but are using
> MessagePumpLibevent as your pump directly?
> 
> Then you absolutely, 100%, need a comment in base.gypi explaining what you’re
> doing and why you’re doing it. If this was explained in the .gyp file, I think
> we could tolerate it.
> 
> Still, I envision that having to haul in Aura and Ozone may one day be as
> objectionable as depending on X11 and GTK. Ultimately, it would probably be
best
> for this “Android host” configuration to redirect MessagePumpForUI to be
the
> least harmless and dependency-having implementation possible, probably either
a
> dummy stub, MessagePumpDefault, or MessagePumpLibevent. If there’s no way to
> detect this configuration from a C++ macro right now, you could easily
introduce
> a new macro here (instead of defining USE_AURA or USE_OZONE) and pick that up
in
> C++-land to choose the correct pump implementation.

I have to agree with Mark. It's not good to abuse the flags. And the change is
simple.

[Zhenyu Liang, 2014-03-12 17:44:05]

Add Jói to reviewers since introduce a new "OS_ANDROID_HOST" macro.

[Jói, 2014-03-12 19:50:28]

PRESUBMIT.py LGTM

[Mark Mentovai, 2014-03-12 19:52:14]

LGTM. This macro may need to migrate elsewhere if it becomes useful outside of
base.

[Zhenyu Liang, 2014-03-13 03:10:45]

The CQ bit was checked by zhenyu.liang@intel.com

[commit-bot: I haz the power, 2014-03-13 03:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/176713005/80001

[commit-bot: I haz the power, 2014-03-13 03:18:53]

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

[commit-bot: I haz the power, 2014-03-13 03:18:54]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Philippe, 2014-03-13 12:47:49]

On 2014/03/13 03:18:54, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on win_chromium_compile_dbg

The change looks much nicer that way indeed, thanks!

[Zhenyu Liang, 2014-03-13 16:32:54]

The CQ bit was checked by zhenyu.liang@intel.com

[commit-bot: I haz the power, 2014-03-13 16:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/176713005/80001

[commit-bot: I haz the power, 2014-03-13 17:09:02]

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

[commit-bot: I haz the power, 2014-03-13 17:09:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Zhenyu Liang, 2014-03-15 20:43:45]

The CQ bit was checked by zhenyu.liang@intel.com

[commit-bot: I haz the power, 2014-03-15 20:43:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/176713005/80001

[commit-bot: I haz the power, 2014-03-15 21:07:25]

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

[commit-bot: I haz the power, 2014-03-15 21:07:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Zhenyu Liang, 2014-03-19 02:47:09]

The CQ bit was checked by zhenyu.liang@intel.com

[commit-bot: I haz the power, 2014-03-19 02:47:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhenyu.liang@intel.com/176713005/80001

[commit-bot: I haz the power, 2014-03-19 05:47:05]

Change committed as 257853