Do not build unicode_to_keysym.cc when use_x11 == 0

Do not build unicode_to_keysym.cc when use_x11 == 0

BUG=608832
Committed: https://crrev.com/02dfd6d2723ed54559156a1b491c53dc596f898e
Cr-Commit-Position: refs/heads/master@{#395162}

[joth, 2016-05-19 00:36:11]

joth@chromium.org changed reviewers:
+ wez@chromium.org

[joth, 2016-05-19 01:22:09]

The CQ bit was checked by joth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 01:22:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997533002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997533002/1

[commit-bot: I haz the power, 2016-05-19 01:46:09]

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

[commit-bot: I haz the power, 2016-05-19 01:46:10]

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

[Wez, 2016-05-19 17:28:24]

wez@chromium.org changed reviewers:
+ lambroslambrou@chromium.org
- wez@chromium.org

[Wez, 2016-05-19 17:28:25]

wez->lambroslambrou

[joth, 2016-05-19 17:41:09]

Actually this isn't ready for detailed review: I think I've misunderstood the
use of xkbcommon vs X11 in ui/events/keycodes/xkb_keysym.h where I was trying to
copy the solution from.

keysymdef.h is not part of xkbcommon so this shouldn't be switching on
use_xkbcommon. What confused me is Rialto - CrOS devices that doesn't  use
xkbcommon - is missing the X11/keysymdef.h header file, so I assumed the two
come together.

[Lambros, 2016-05-19 17:48:13]

lambroslambrou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Lambros, 2016-05-19 17:48:13]

not lgtm, sorry.

I don't like the idea of having build flags that can break the remoting host in
subtle ways. If remoting host really needs the XKB functions, it should be an
error to try to build this component without those functions.

If this CL is needed to unbreak some buildbot somewhere, then I'd prefer for
that builder to be fixed to not build remoting host.

Sergey, WDYT? Is it possible/reasonable to allow host to be built this way?

[Lambros, 2016-05-19 18:12:21]

lambroslambrou@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Lambros, 2016-05-19 18:12:21]

Actually, I think we do build parts of the Linux host on CrOS ?

+kelvinp as well - he knows more about the CrOS stuff than I do.

In any case, if we have some configuration that doesn't build
the host, we should prefer to disable the host components for
that configuration, instead of applying dummy code to force it
to build.

I'm happy to take another look once you are ready for detailed
review, but maybe Sergey or Kelvin are better reviewers for
this?

[Sergey Ulanov, 2016-05-19 18:21:47]

I agree with Lambros. The version of GetKeySymsForUnicode() you are adding is
broken and we shouldn't add code that is known't to be broken.

Is that Chrome OS build? unicode_to_keysym.cc is specific to X11, but X11 is no
longer used on Chrome OS, so we shouldn't need to compile this file on Chrome
OS. I think the right fix is to update build files to exclude
unicode_to_keysym.cc here:
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/BUIL...

[Wez, 2016-05-19 20:03:17]

IIRC there are two remaining CrOS devices that still run X11, but the
Chromoting host will still inject directly via the Ozone interfaces on
those platforms.

On 19 May 2016 at 11:21, <sergeyu@chromium.org> wrote:

> I agree with Lambros. The version of GetKeySymsForUnicode() you are adding
> is
> broken and we shouldn't add code that is known't to be broken.
>
> Is that Chrome OS build? unicode_to_keysym.cc is specific to X11, but X11
> is no
> longer used on Chrome OS, so we shouldn't need to compile this file on
> Chrome
> OS. I think the right fix is to update build files to exclude
> unicode_to_keysym.cc here:
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/BUIL...
>
> https://codereview.chromium.org/1997533002/
>

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

[joth, 2016-05-19 21:51:20]

The CQ bit was checked by joth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 21:52:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997533002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997533002/40001

[commit-bot: I haz the power, 2016-05-19 21:58:37]

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

[commit-bot: I haz the power, 2016-05-19 21:58:38]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[joth, 2016-05-20 00:06:11]

The CQ bit was checked by joth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 00:07:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997533002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997533002/60001

[commit-bot: I haz the power, 2016-05-20 00:12:14]

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

[commit-bot: I haz the power, 2016-05-20 00:12:15]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[joth, 2016-05-20 01:03:46]

The CQ bit was checked by joth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 01:12:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997533002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997533002/100001

[commit-bot: I haz the power, 2016-05-20 01:18:18]

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

[commit-bot: I haz the power, 2016-05-20 01:18:20]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[joth, 2016-05-20 04:23:18]

The CQ bit was checked by joth@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-20 04:23:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997533002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997533002/120001

[joth, 2016-05-20 05:07:30]

Description was changed from

==========
Make remoting/host honor USE_XKBCOMMON flag

If xkbcommon is not in use, provide a dummy implementation of
GetKeySymsForUnicode

BUG=608832
==========

to

==========
Do not build unicode_to_keysym.cc when use_x11 == 0

BUG=608832
==========

[joth, 2016-05-20 05:09:49]

OK, please take another look.

Sergey Ulanov was right on the mark with suggestion to just not build this file.
Conditioning it on use_x11 means the legacy CrOS devices still using X11 (if
any) should be unaffected, afaict.

Thanks!

[commit-bot: I haz the power, 2016-05-20 05:23:15]

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

[commit-bot: I haz the power, 2016-05-20 05:23:17]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-05-20 19:23:14]

lgtm

[joth, 2016-05-20 19:38:14]

+Lambros Lambrou <lambroslambrou@chromium.org>   I'll also need lgtm from
you proceed.

Thanks!


On Fri, 20 May 2016 at 12:23 <sergeyu@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/1997533002/
>

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

[Lambros, 2016-05-20 20:12:51]

lgtm

[joth, 2016-05-20 20:53:59]

The CQ bit was checked by joth@chromium.org

[commit-bot: I haz the power, 2016-05-20 20:54:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1997533002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1997533002/120001

[commit-bot: I haz the power, 2016-05-20 20:59:11]

Description was changed from

==========
Do not build unicode_to_keysym.cc when use_x11 == 0

BUG=608832
==========

to

==========
Do not build unicode_to_keysym.cc when use_x11 == 0

BUG=608832
==========

[commit-bot: I haz the power, 2016-05-20 20:59:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-20 21:01:56]

Description was changed from

==========
Do not build unicode_to_keysym.cc when use_x11 == 0

BUG=608832
==========

to

==========
Do not build unicode_to_keysym.cc when use_x11 == 0

BUG=608832

Committed: https://crrev.com/02dfd6d2723ed54559156a1b491c53dc596f898e
Cr-Commit-Position: refs/heads/master@{#395162}
==========

[commit-bot: I haz the power, 2016-05-20 21:01:57]

Patchset 7 (id:??) landed as
https://crrev.com/02dfd6d2723ed54559156a1b491c53dc596f898e
Cr-Commit-Position: refs/heads/master@{#395162}