Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
49383011:
Build fix linux aura without USE_XI2_MT. (Closed)
|
Created:
7 years, 1 month ago by dshwang Modified:
7 years, 1 month ago CC:
chromium-reviews Base URL:
https://git.chromium.org/chromium/src.git@ime Visibility:
Public. |
DescriptionBuild fix linux aura without USE_XI2_MT.
touch_factory_x11.cc:96 must not use XScopedString because it is defined ui/base.
ui/base depends on ui/events.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232256
Patch Set 1 #
Total comments: 1
Patch Set 2 : Don't use XScopedString #
Total comments: 1
Patch Set 3 : Clean up for-loop #
Total comments: 2
Patch Set 4 : #Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/49383011/diff/1/ui/events/DEPS File ui/events/DEPS (right): https://codereview.chromium.org/49383011/diff/1/ui/events/DEPS#newcode2 ui/events/DEPS:2: "+ui/base", events can't depend on base (base depends on events, and that would cause a dependency cycle).
On 2013/10/31 14:56:39, sadrul wrote: > https://codereview.chromium.org/49383011/diff/1/ui/events/DEPS > File ui/events/DEPS (right): > > https://codereview.chromium.org/49383011/diff/1/ui/events/DEPS#newcode2 > ui/events/DEPS:2: "+ui/base", > events can't depend on base (base depends on events, and that would cause a > dependency cycle). Thank you for advice. In 2nd CL, ui/events doesn't depend on ui/base.
I would also encourage to look into switching to XI2, because we will completely switch to that at some point. https://codereview.chromium.org/49383011/diff/90001/ui/events/x/touch_factory... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/49383011/diff/90001/ui/events/x/touch_factory... ui/events/x/touch_factory_x11.cc:103: } Can you do this like the following instead: dev_list = ...; Atom xi_touchscreen = XInternAtom(display, XI_TOUCHSCREEN, false); for (....) { if (dev_list[i].type == xi_touchscreen) { ... I think that would be cleaner.
On 2013/10/31 15:30:36, sadrul wrote: > I would also encourage to look into switching to XI2, because we will completely > switch to that at some point. Yes, I agree. However, some embedded enviornment does not support XI2.2. for example, Tizen supports XI2.1. btw, Could I change chromium to support XInput2.1 also? USE_XI2_MT does not care of XInput2.1. > https://codereview.chromium.org/49383011/diff/90001/ui/events/x/touch_factory... > File ui/events/x/touch_factory_x11.cc (right): > > https://codereview.chromium.org/49383011/diff/90001/ui/events/x/touch_factory... > ui/events/x/touch_factory_x11.cc:103: } > Can you do this like the following instead: > > dev_list = ...; > Atom xi_touchscreen = XInternAtom(display, XI_TOUCHSCREEN, false); > for (....) { > if (dev_list[i].type == xi_touchscreen) { > ... > > I think that would be cleaner. Thx, Done.
https://codereview.chromium.org/49383011/diff/170001/ui/events/x/touch_factor... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/49383011/diff/170001/ui/events/x/touch_factor... ui/events/x/touch_factory_x11.cc:96: if (devtype) { Do you think thin if-statement is needed? I want to remove "char* devtype = XGetAtomName(display, xi_touchscreen);" because devtype is not used except for if-statement.
XI2.1 (and earlier) has incomplete touch support, and so we don't plan on maintaining support for that for long. https://codereview.chromium.org/49383011/diff/170001/ui/events/x/touch_factor... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/49383011/diff/170001/ui/events/x/touch_factor... ui/events/x/touch_factory_x11.cc:96: if (devtype) { On 2013/10/31 15:47:26, dshwang wrote: > Do you think thin if-statement is needed? I want to remove "char* devtype = > XGetAtomName(display, xi_touchscreen);" because devtype is not used except for > if-statement. Yes. Remove devtype. It's not necessary here.
On 2013/10/31 15:49:21, sadrul wrote: > XI2.1 (and earlier) has incomplete touch support, and so we don't plan on > maintaining support for that for long. How do you think if Intel maintains XInput2.1? Intel maintains XInput2.1 in its own github now: https://github.com/crosswalk-project/chromium-crosswalk If chromium-dev is ok, Intel can contribute to code related to supporting XInput2.1. > https://codereview.chromium.org/49383011/diff/170001/ui/events/x/touch_factor... > File ui/events/x/touch_factory_x11.cc (right): > > https://codereview.chromium.org/49383011/diff/170001/ui/events/x/touch_factor... > ui/events/x/touch_factory_x11.cc:96: if (devtype) { > On 2013/10/31 15:47:26, dshwang wrote: > > Do you think thin if-statement is needed? I want to remove "char* devtype = > > XGetAtomName(display, xi_touchscreen);" because devtype is not used except for > > if-statement. > > Yes. Remove devtype. It's not necessary here. Done.
LGTM. Thanks! On 2013/10/31 16:08:16, dshwang wrote: > On 2013/10/31 15:49:21, sadrul wrote: > > XI2.1 (and earlier) has incomplete touch support, and so we don't plan on > > maintaining support for that for long. > > How do you think if Intel maintains XInput2.1? Intel maintains XInput2.1 in its > own github now: https://github.com/crosswalk-project/chromium-crosswalk > If chromium-dev is ok, Intel can contribute to code related to supporting > XInput2.1. Depends on the patches. But unlikely to be upstreamed. All the buildbot/trybots are on XI2.2, and so the XI2.1 code will regress without anyone noticing (like this one). So this would be best maintained out-of-tree (like the github repo).
On 2013/10/31 16:15:39, sadrul wrote: > LGTM. Thanks! > > On 2013/10/31 16:08:16, dshwang wrote: > > On 2013/10/31 15:49:21, sadrul wrote: > > > XI2.1 (and earlier) has incomplete touch support, and so we don't plan on > > > maintaining support for that for long. > > > > How do you think if Intel maintains XInput2.1? Intel maintains XInput2.1 in > its > > own github now: https://github.com/crosswalk-project/chromium-crosswalk > > If chromium-dev is ok, Intel can contribute to code related to supporting > > XInput2.1. > > Depends on the patches. But unlikely to be upstreamed. All the buildbot/trybots > are on XI2.2, and so the XI2.1 code will regress without anyone noticing (like > this one). So this would be best maintained out-of-tree (like the github repo). Thank you for your opinion! :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/49383011/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/49383011/240001
Message was sent while issue was closed.
Change committed as 232256 |
