Unified touch_device_aurax11 and touch_device_ozone.

Unified touch_device_aurax11 and touch_device_ozone.

This change was suggested in http://crrev.com/806693008

BUG=443667
Committed: https://crrev.com/52cfea1ccce19c424168a56843a98253bd505645
Cr-Commit-Position: refs/heads/master@{#312430}

[mustaq, 2014-12-19 16:06:45]

Patchset #1 (id:1) has been deleted

[mustaq, 2014-12-19 16:23:30]

mustaq@chromium.org changed reviewers:
+ sadrul@chromium.org

[mustaq, 2014-12-19 16:23:30]

sadrul@: Attempted the change you suggested in my other CL.

Few things are not clear to me:

- Linux build seems to include touch_device.cc, not any of touch_device_*.cc.
This is true both before and after my change. I guess this is because BUILD.gn
mistakenly used '&&' instead of '||'?

- ui_base.gyp: I was expecting direct rules to include exactly one of
touch_device*.cc for each platform. Am I missing something there?

[sadrul, 2014-12-19 21:48:44]

Thanks for the quick follow-up!

https://codereview.chromium.org/815983003/diff/20001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_linux.cc (right):

https://codereview.chromium.org/815983003/diff/20001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_linux.cc:19: return
ui::TouchFactory::GetInstance()->GetMaxTouchPoints();
I think this is X11 only, and isn't available on ozone. I have a plan to change
the impl of MaxTouchPoints() to make it go through DeviceDataManager. Le me take
a stab at that first. Once that is done, this should be ready to land.

[sadrul, 2015-01-13 22:02:35]

On 2014/12/19 21:48:44, sadrul wrote:
> Thanks for the quick follow-up!
> 
>
https://codereview.chromium.org/815983003/diff/20001/ui/base/touch/touch_devi...
> File ui/base/touch/touch_device_linux.cc (right):
> 
>
https://codereview.chromium.org/815983003/diff/20001/ui/base/touch/touch_devi...
> ui/base/touch/touch_device_linux.cc:19: return
> ui::TouchFactory::GetInstance()->GetMaxTouchPoints();
> I think this is X11 only, and isn't available on ozone. I have a plan to
change
> the impl of MaxTouchPoints() to make it go through DeviceDataManager. Le me
take
> a stab at that first. Once that is done, this should be ready to land.

The fix is now in (https://codereview.chromium.org/845413002/). So now, we can
use the implementation of MaxTouchPoints() in touch_device_aurax11 for both
aura-x11 and ozone in touch_device_linux.

[mustaq, 2015-01-15 20:38:57]

Patchset #3 (id:60001) has been deleted

[mustaq, 2015-01-15 20:42:19]

ptal.

[mustaq, 2015-01-15 20:50:39]

Forgot to add: I tried specifying touch_device_linux.cc for the OS==linux
condition in the gyp file, seems to break things. I think I need a better
understanding of the conditions use_aura==x and use_x11==x etc. Any pointers are
welcome.

[sadrul, 2015-01-15 22:07:21]

On 2015/01/15 20:50:39, mustaq wrote:
> Forgot to add: I tried specifying touch_device_linux.cc for the OS==linux
> condition in the gyp file, seems to break things. I think I need a better
> understanding of the conditions use_aura==x and use_x11==x etc. Any pointers
are
> welcome.

Do you remember what the failure was? Maybe you need to make sure the default
touch_device.cc gets excluded correctly for OS=linux?

FYI: for gyp, the default rules in //build/filename_rules.gypi takes care of
doing some of the filtering based on filenames, so touch_device_linux.cc should
automatically be excluded on non-linux platforms. We don't have such filters for
gn though, and so you will need to explicitly exclude _linux.cc on non-linux
platforms.

[sadrul, 2015-01-15 22:07:50]

https://codereview.chromium.org/815983003/diff/40001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_linux.cc (right):

https://codereview.chromium.org/815983003/diff/40001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_linux.cc:13: // TODO(sadrul@chromium.org): Support
evdev hotplugging.
We don't need this TODO anymore. You can remove this (thankfully :) )

[mustaq, 2015-01-20 15:03:42]

On 2015/01/15 22:07:21, sadrul wrote:
> On 2015/01/15 20:50:39, mustaq wrote:
> > Forgot to add: I tried specifying touch_device_linux.cc for the OS==linux
> > condition in the gyp file, seems to break things. I think I need a better
> > understanding of the conditions use_aura==x and use_x11==x etc. Any pointers
> are
> > welcome.
> 
> Do you remember what the failure was? Maybe you need to make sure the default
> touch_device.cc gets excluded correctly for OS=linux?
> 
> FYI: for gyp, the default rules in //build/filename_rules.gypi takes care of
> doing some of the filtering based on filenames, so touch_device_linux.cc
should
> automatically be excluded on non-linux platforms. We don't have such filters
for
> gn though, and so you will need to explicitly exclude _linux.cc on non-linux
> platforms.

I had breakages with mac builds etc. I have narrowed down to the gyp rule that
caused it, and then split it. ptal.

[mustaq, 2015-01-20 15:03:50]

https://codereview.chromium.org/815983003/diff/40001/ui/base/touch/touch_devi...
File ui/base/touch/touch_device_linux.cc (right):

https://codereview.chromium.org/815983003/diff/40001/ui/base/touch/touch_devi...
ui/base/touch/touch_device_linux.cc:13: // TODO(sadrul@chromium.org): Support
evdev hotplugging.
On 2015/01/15 22:07:50, sadrul wrote:
> We don't need this TODO anymore. You can remove this (thankfully :) )

Done.

[sadrul, 2015-01-20 18:44:08]

LGTM

https://codereview.chromium.org/815983003/diff/80001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/815983003/diff/80001/ui/base/BUILD.gn#newcode282
ui/base/BUILD.gn:282: } else if (is_linux || use_ozone || (use_aura && use_x11))
{
Just 'is_linux' should be sufficient (use_ozone is supported only on linux, and
so is use_x11)

https://codereview.chromium.org/815983003/diff/80001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/815983003/diff/80001/ui/base/ui_base.gyp#newc...
ui/base/ui_base.gyp:539: ['chromeos==1 or OS=="linux"', {
I think you can use just 'OS=="linux"' here.

[mustaq, 2015-01-21 17:07:35]

https://codereview.chromium.org/815983003/diff/80001/ui/base/BUILD.gn
File ui/base/BUILD.gn (right):

https://codereview.chromium.org/815983003/diff/80001/ui/base/BUILD.gn#newcode282
ui/base/BUILD.gn:282: } else if (is_linux || use_ozone || (use_aura && use_x11))
{
On 2015/01/20 18:44:07, sadrul wrote:
> Just 'is_linux' should be sufficient (use_ozone is supported only on linux,
and
> so is use_x11)

Done.

https://codereview.chromium.org/815983003/diff/80001/ui/base/ui_base.gyp
File ui/base/ui_base.gyp (right):

https://codereview.chromium.org/815983003/diff/80001/ui/base/ui_base.gyp#newc...
ui/base/ui_base.gyp:539: ['chromeos==1 or OS=="linux"', {
On 2015/01/20 18:44:08, sadrul wrote:
> I think you can use just 'OS=="linux"' here.

Done.

[mustaq, 2015-01-21 18:38:28]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2015-01-21 18:40:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/815983003/100001

[commit-bot: I haz the power, 2015-01-21 20:34:31]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-01-21 20:35:46]

Patchset 4 (id:??) landed as
https://crrev.com/52cfea1ccce19c424168a56843a98253bd505645
Cr-Commit-Position: refs/heads/master@{#312430}