ozone: evdev: Add gesture property provider

ozone: evdev: Add gesture property provider

The CL does the following things:

1. Add GesturePropertyProvider which parses xorg-conf files under
/etc/gesture/ upon chrome starts and sets the property values according
to the device touchpad/mouse type and the file content.

2. Provides GesturesPropProvider APIs so that libgesture can expose
its property values and allow Chrome to access it.

Contributed by sheckylin@chromium.org

BUG=400022
TEST=link_freon ChromeOS build
put appropriate xorg-conf files under /etc/gesture
restart chrome and check that:
1. the property values get loaded by manually logging.
2. the touchpad gesture is now smoother than before.

Committed: https://crrev.com/af98f0fa02a4cfc584291ea27aecda165bbdaddb
Cr-Commit-Position: refs/heads/master@{#300423}

[Shecky Lin, 2014-09-16 08:48:22]

sheckylin@chromium.org changed reviewers:
+ adlr@chromium.org, rjkroege@chromium.org, spang@chromium.org

[Shecky Lin, 2014-09-16 08:49:52]

Hey guys,

Please take a look of the patch. I am still doing some more detailed checking
but it seems to work fine now. Thanks!

[Shecky Lin, 2014-09-17 09:04:01]

Done some more checking today and it looks really well. Please review it and let
me know how you think. Thanks.

[alexst (slow to review), 2014-09-17 14:39:53]

alexst@chromium.org changed reviewers:
+ alexst@chromium.org

[alexst (slow to review), 2014-09-17 14:39:53]

https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:210: typedef
std::map<DeviceId, PropertyMap*> DevicePropertyMap;
Just a drive-by comment, could you use scoped_ptr_hash_map here instead to
simplify memory management and not have to manually delete them, or is there a
subtle reason you need to do it?

https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...

[Shecky Lin, 2014-09-18 09:44:26]

On 2014/09/17 14:39:53, alexst wrote:
>
https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h
(right):
> 
>
https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:210:
typedef
> std::map<DeviceId, PropertyMap*> DevicePropertyMap;
> Just a drive-by comment, could you use scoped_ptr_hash_map here instead to
> simplify memory management and not have to manually delete them, or is there a
> subtle reason you need to do it?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...

No, it is just that I don't know such a thing exists before. Thanks for the
suggestion!

[alexst (slow to review), 2014-09-18 16:13:29]

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:278: delete
configurations_[i].criterias[j];
Thank you for the scoped_ptr_hash map change. Can you more the rest of the
manually created/destroyed memory to use scoped_ptr or scoped_ref_ptr? Manual
memory management is harder to trace and tends to be more fragile.

[spang, 2014-09-18 18:06:17]

Thanks for doing this!

I think we'll need a bit of change to better match the chromium coding style.

https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:247: return
Singleton<GesturePropertyProvider>::get();
Instead of making it a singleton, please create this object in the
EventFactoryEvdev constructor, and plumb it to the gestures device constructor.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:35: friend
class GesturesPropFunctionsWrapper;
Put this under private:

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:60: struct
DeviceProperty {
Name makes me think it's only one property.

Maybe DeviceProperties?

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:85: void
GetDeviceIndices(const DeviceType type,
Maybe "GetDeviceIdsByType"

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:98: size_t*
count);
This interface concerns me.. why have we lost all of the types?

I think we should probably have separate, type-safe functions.

  GetIntProperty(DeviceId device_id, const std::string& name, int* value);
  GetShortProperty(DeviceId device_id, const std::string& name, short* value);
  GetBoolProperty (DeviceId device_id, const std::string& name, bool* value);
  GetStringProperty(DeviceId device_id, const std::string& name, std::string*
value);
  GetDoubleProperty(DeviceId device_id, const std::string& name, double *value);

[spang, 2014-09-18 21:23:16]

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:30: #define
INFO_SEVERITY INFO
Please don't rename this. You shouldn't use LOG(INFO), there's a presubmit that
will complain if you do it.

Use:

VLOG(n) if you want to support logging even in release builds
DVLOG(n) if you want to support logging only in debug

LOG(WARNING) and LOG(ERROR) are also OK for real problems.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:145: static
DeallocatorMap data_pointer_deallocator_map_;
static objects with nontrivial initialization aren't allowed. There's actually a
perf bot that will catch this and go red.

It looks like this could be done with a switch/case - no static object needed.

Then again, I think what you really want is a virtual destructor. Then you can
just do "delete my_prop" inside Free().

[Shecky Lin, 2014-09-19 03:24:18]

On 2014/09/18 18:06:17, spang wrote:
> Thanks for doing this!
> 
> I think we'll need a bit of change to better match the chromium coding style.
> 
>
https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> (right):
> 
>
https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:247:
return
> Singleton<GesturePropertyProvider>::get();
> Instead of making it a singleton, please create this object in the
> EventFactoryEvdev constructor, and plumb it to the gestures device
constructor.

Just a quick question: if we create the object in EventFactoryEvdev, how can we
let other classes access the property values? For example, if we want to allow
input_device_setting to 
set the property values, it seems like we have to make the object globally
available.

[Shecky Lin, 2014-09-19 09:31:09]

Address most comments. At the next patch, I will:

1. Finish the GetProperty API.
2. Finish GetDeviceIdsByType.
3. Move more pointers to scoped_ptr for better memory management.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:30: #define
INFO_SEVERITY INFO
On 2014/09/18 21:23:16, spang wrote:
> Please don't rename this. You shouldn't use LOG(INFO), there's a presubmit
that
> will complain if you do it.
> 
> Use:
> 
> VLOG(n) if you want to support logging even in release builds
> DVLOG(n) if you want to support logging only in debug
> 
> LOG(WARNING) and LOG(ERROR) are also OK for real problems.

Done.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:145: static
DeallocatorMap data_pointer_deallocator_map_;
On 2014/09/18 21:23:16, spang wrote:
> static objects with nontrivial initialization aren't allowed. There's actually
a
> perf bot that will catch this and go red.
> 
> It looks like this could be done with a switch/case - no static object needed.
> 
> Then again, I think what you really want is a virtual destructor. Then you can
> just do "delete my_prop" inside Free().

Done. Thanks for the great suggestion.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:278: delete
configurations_[i].criterias[j];
On 2014/09/18 16:13:29, alexst wrote:
> Thank you for the scoped_ptr_hash map change. Can you more the rest of the
> manually created/destroyed memory to use scoped_ptr or scoped_ref_ptr? Manual
> memory management is harder to trace and tends to be more fragile.

Yeah, I am still looking into this. Will do it at the next patch.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:35: friend
class GesturesPropFunctionsWrapper;
On 2014/09/18 18:06:16, spang wrote:
> Put this under private:

Done.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:60: struct
DeviceProperty {
On 2014/09/18 18:06:15, spang wrote:
> Name makes me think it's only one property.
> 
> Maybe DeviceProperties?

Done.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:85: void
GetDeviceIndices(const DeviceType type,
On 2014/09/18 18:06:15, spang wrote:
> Maybe "GetDeviceIdsByType"

Done.

https://codereview.chromium.org/545063006/diff/40001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:98: size_t*
count);
On 2014/09/18 18:06:15, spang wrote:
> This interface concerns me.. why have we lost all of the types?
> 
> I think we should probably have separate, type-safe functions.
> 
>   GetIntProperty(DeviceId device_id, const std::string& name, int* value);
>   GetShortProperty(DeviceId device_id, const std::string& name, short* value);
>   GetBoolProperty (DeviceId device_id, const std::string& name, bool* value);
>   GetStringProperty(DeviceId device_id, const std::string& name, std::string*
> value);
>   GetDoubleProperty(DeviceId device_id, const std::string& name, double
*value);

I agree. Will do it at the next patch. However, most of time it would just be
plain data copying though, and type doesn't really matter.

[spang, 2014-09-19 18:28:38]

On 2014/09/19 03:24:18, Shecky Lin wrote:
> On 2014/09/18 18:06:17, spang wrote:
> > Thanks for doing this!
> > 
> > I think we'll need a bit of change to better match the chromium coding
style.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/20001/ui/events/ozone/evdev/li...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:247:
> return
> > Singleton<GesturePropertyProvider>::get();
> > Instead of making it a singleton, please create this object in the
> > EventFactoryEvdev constructor, and plumb it to the gestures device
> constructor.
> 
> Just a quick question: if we create the object in EventFactoryEvdev, how can
we
> let other classes access the property values? For example, if we want to allow
> input_device_setting to 
> set the property values, it seems like we have to make the object globally
> available.

We'll need to use an interface for input settings so that all build configs
work. It will need to go in ui/events or ui/ozone/public.

There is a big object you can get statically called OzonePlatform - adding a
function like GetInputDeviceSettings() that returns the object is better than
creating a new static altogether.

[Shecky Lin, 2014-09-22 09:57:32]

Got it. Unfortunately, I found that I still need a globally available
object that I can call from in the APIs provided by
GesturePropFunctionsWrapper, which will in turn be called externally from
the gesture library. Do you have any idea how I can do this (without using
a singleton)?
2014/9/20 上午2:28 於 <spang@chromium.org> 寫道：

> On 2014/09/19 03:24:18, Shecky Lin wrote:
>
>> On 2014/09/18 18:06:17, spang wrote:
>> > Thanks for doing this!
>> >
>> > I think we'll need a bit of change to better match the chromium coding
>>
> style.
>
>> >
>> >
>>
>
> https://codereview.chromium.org/545063006/diff/20001/ui/
> events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
>
>> > File ui/events/ozone/evdev/libgestures_glue/gesture_
>> property_provider.cc
>> > (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/545063006/diff/20001/ui/
> events/ozone/evdev/libgestures_glue/gesture_property_provider.cc#
> newcode247
>
>> > ui/events/ozone/evdev/libgestures_glue/gesture_
>> property_provider.cc:247:
>> return
>> > Singleton<GesturePropertyProvider>::get();
>> > Instead of making it a singleton, please create this object in the
>> > EventFactoryEvdev constructor, and plumb it to the gestures device
>> constructor.
>>
>
>  Just a quick question: if we create the object in EventFactoryEvdev, how
>> can
>>
> we
>
>> let other classes access the property values? For example, if we want to
>> allow
>> input_device_setting to
>> set the property values, it seems like we have to make the object globally
>> available.
>>
>
> We'll need to use an interface for input settings so that all build configs
> work. It will need to go in ui/events or ui/ozone/public.
>
> There is a big object you can get statically called OzonePlatform - adding
> a
> function like GetInputDeviceSettings() that returns the object is better
> than
> creating a new static altogether.
>
> https://codereview.chromium.org/545063006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Shecky Lin, 2014-09-23 08:41:25]

I've uploaded a new patch which addressed all issues besides the singleton
problem. Please take a look.

I have also checked the OzonePlatform related codes and I think I finally
understand what you were talking about (oops!). Do you think I can put an
instance of GesturePropertyProvider in the OzonePlatform derived classes (e.g.,
OzonePlatformGbm) and let the users call a function like
GetGesturePropertyProvider()? Specifically, this is what I plan to do:

a) Split the gesture_property_provider into 3 files:
1. gesture_property{.h, .cc} - contains GesturesProp definition and related
functions (e.g., creators).
2. gesture_property_provider{.h, .cc} - contains GesturePropertyProvider.
3. gesture_property_utils{.h, .cc} - contains GesturesPropFunctionsWrapper and
an API object GesturesPropProvider which will be used by the gesture library.

b) Modify OzonePlatform:
1. Add a function GetGesturePropertyProvider() which returns a static
GesturePropertyProvider object.
2. Modify OzonePlatform derived classes to initialize the object during
InitializeUI().

c) Header include order:
gesture_property_provider.h includes gestures/gestures.h
gesture_property_provider.cc includes gesture_property.h
OzonePlatform derived classes' sources files include gesture_property_provider.h
gesture_property_utils.h includes gesture_property_provider.h
gesture_property_utils.cc includes gesture_property.h, ozone_platform.h
gesture_interpreter_libevdev_cros.h includes gesture_property_provider.h
gesture_interpreter_libevdev_cros.cc includes gesture_property_utils.h

d) For all other classes that needs to use gesture_property_provider (e.g.,
input_device_setting):
*.cc includes gesture_property_provider.h and ozone_platform.h

If this sounds good to you, please let me know so we can close this out soon.
Thanks!
On 2014/09/22 09:57:32, Shecky Lin wrote:
> Got it. Unfortunately, I found that I still need a globally available
> object that I can call from in the APIs provided by
> GesturePropFunctionsWrapper, which will in turn be called externally from
> the gesture library. Do you have any idea how I can do this (without using
> a singleton)?
> 2014/9/20 上午2:28 於 <mailto:spang@chromium.org> 寫道：
> 
> > On 2014/09/19 03:24:18, Shecky Lin wrote:
> >
> >> On 2014/09/18 18:06:17, spang wrote:
> >> > Thanks for doing this!
> >> >
> >> > I think we'll need a bit of change to better match the chromium coding
> >>
> > style.
> >
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/545063006/diff/20001/ui/
> > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> >
> >> > File ui/events/ozone/evdev/libgestures_glue/gesture_
> >> property_provider.cc
> >> > (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/545063006/diff/20001/ui/
> > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc#
> > newcode247
> >
> >> > ui/events/ozone/evdev/libgestures_glue/gesture_
> >> property_provider.cc:247:
> >> return
> >> > Singleton<GesturePropertyProvider>::get();
> >> > Instead of making it a singleton, please create this object in the
> >> > EventFactoryEvdev constructor, and plumb it to the gestures device
> >> constructor.
> >>
> >
> >  Just a quick question: if we create the object in EventFactoryEvdev, how
> >> can
> >>
> > we
> >
> >> let other classes access the property values? For example, if we want to
> >> allow
> >> input_device_setting to
> >> set the property values, it seems like we have to make the object globally
> >> available.
> >>
> >
> > We'll need to use an interface for input settings so that all build configs
> > work. It will need to go in ui/events or ui/ozone/public.
> >
> > There is a big object you can get statically called OzonePlatform - adding
> > a
> > function like GetInputDeviceSettings() that returns the object is better
> > than
> > creating a new static altogether.
> >
> > https://codereview.chromium.org/545063006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-09-23 16:49:55]

On 2014/09/22 09:57:32, Shecky Lin wrote:
> Got it. Unfortunately, I found that I still need a globally available
> object that I can call from in the APIs provided by
> GesturePropFunctionsWrapper, which will in turn be called externally from
> the gesture library. Do you have any idea how I can do this (without using
> a singleton)?

Pass the GestureInterpreterLibevdevCros* to GestureInterpreterSetPropProvider so
that you can recover the right object in the callbacks, and then add a pointer
to GesturePropertyProvider to GestureInterpreterLibevdevCros (set it in the
constructor).

> 2014/9/20 上午2:28 於 <mailto:spang@chromium.org> 寫道：
> 
> > On 2014/09/19 03:24:18, Shecky Lin wrote:
> >
> >> On 2014/09/18 18:06:17, spang wrote:
> >> > Thanks for doing this!
> >> >
> >> > I think we'll need a bit of change to better match the chromium coding
> >>
> > style.
> >
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/545063006/diff/20001/ui/
> > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> >
> >> > File ui/events/ozone/evdev/libgestures_glue/gesture_
> >> property_provider.cc
> >> > (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/545063006/diff/20001/ui/
> > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc#
> > newcode247
> >
> >> > ui/events/ozone/evdev/libgestures_glue/gesture_
> >> property_provider.cc:247:
> >> return
> >> > Singleton<GesturePropertyProvider>::get();
> >> > Instead of making it a singleton, please create this object in the
> >> > EventFactoryEvdev constructor, and plumb it to the gestures device
> >> constructor.
> >>
> >
> >  Just a quick question: if we create the object in EventFactoryEvdev, how
> >> can
> >>
> > we
> >
> >> let other classes access the property values? For example, if we want to
> >> allow
> >> input_device_setting to
> >> set the property values, it seems like we have to make the object globally
> >> available.
> >>
> >
> > We'll need to use an interface for input settings so that all build configs
> > work. It will need to go in ui/events or ui/ozone/public.
> >
> > There is a big object you can get statically called OzonePlatform - adding
> > a
> > function like GetInputDeviceSettings() that returns the object is better
> > than
> > creating a new static altogether.
> >
> > https://codereview.chromium.org/545063006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-09-23 16:58:05]

On 2014/09/23 08:41:25, Shecky Lin wrote:
> I've uploaded a new patch which addressed all issues besides the singleton
> problem. Please take a look.
> 
> I have also checked the OzonePlatform related codes and I think I finally
> understand what you were talking about (oops!). Do you think I can put an
> instance of GesturePropertyProvider in the OzonePlatform derived classes
(e.g.,
> OzonePlatformGbm) and let the users call a function like
> GetGesturePropertyProvider()? Specifically, this is what I plan to do:

Let's not include gestures stuff from the settings page - it's an implementation
detail.

The interface we expose should be "InputDeviceSettings" or similar, and contain
just enough to satisfy the settings page:

- bool HasTouchpad()
- bool HasMouse()
- bool HasKeyboard()

- void SetMouseAcceleration()
- void SetScrollDirection()

etc. Then implement this interface in EventFactoryEvdev and delegate to
GesturePropProvider for the touchpad parts.

> 
> a) Split the gesture_property_provider into 3 files:
> 1. gesture_property{.h, .cc} - contains GesturesProp definition and related
> functions (e.g., creators).
> 2. gesture_property_provider{.h, .cc} - contains GesturePropertyProvider.
> 3. gesture_property_utils{.h, .cc} - contains GesturesPropFunctionsWrapper and
> an API object GesturesPropProvider which will be used by the gesture library.
> 
> b) Modify OzonePlatform:
> 1. Add a function GetGesturePropertyProvider() which returns a static
> GesturePropertyProvider object.
> 2. Modify OzonePlatform derived classes to initialize the object during
> InitializeUI().
> 
> c) Header include order:
> gesture_property_provider.h includes gestures/gestures.h
> gesture_property_provider.cc includes gesture_property.h
> OzonePlatform derived classes' sources files include
gesture_property_provider.h
> gesture_property_utils.h includes gesture_property_provider.h
> gesture_property_utils.cc includes gesture_property.h, ozone_platform.h
> gesture_interpreter_libevdev_cros.h includes gesture_property_provider.h
> gesture_interpreter_libevdev_cros.cc includes gesture_property_utils.h
> 
> d) For all other classes that needs to use gesture_property_provider (e.g.,
> input_device_setting):
> *.cc includes gesture_property_provider.h and ozone_platform.h
> 
> If this sounds good to you, please let me know so we can close this out soon.
> Thanks!
> On 2014/09/22 09:57:32, Shecky Lin wrote:
> > Got it. Unfortunately, I found that I still need a globally available
> > object that I can call from in the APIs provided by
> > GesturePropFunctionsWrapper, which will in turn be called externally from
> > the gesture library. Do you have any idea how I can do this (without using
> > a singleton)?
> > 2014/9/20 上午2:28 於 <mailto:spang@chromium.org> 寫道：
> > 
> > > On 2014/09/19 03:24:18, Shecky Lin wrote:
> > >
> > >> On 2014/09/18 18:06:17, spang wrote:
> > >> > Thanks for doing this!
> > >> >
> > >> > I think we'll need a bit of change to better match the chromium coding
> > >>
> > > style.
> > >
> > >> >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/545063006/diff/20001/ui/
> > > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > >
> > >> > File ui/events/ozone/evdev/libgestures_glue/gesture_
> > >> property_provider.cc
> > >> > (right):
> > >> >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/545063006/diff/20001/ui/
> > > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc#
> > > newcode247
> > >
> > >> > ui/events/ozone/evdev/libgestures_glue/gesture_
> > >> property_provider.cc:247:
> > >> return
> > >> > Singleton<GesturePropertyProvider>::get();
> > >> > Instead of making it a singleton, please create this object in the
> > >> > EventFactoryEvdev constructor, and plumb it to the gestures device
> > >> constructor.
> > >>
> > >
> > >  Just a quick question: if we create the object in EventFactoryEvdev, how
> > >> can
> > >>
> > > we
> > >
> > >> let other classes access the property values? For example, if we want to
> > >> allow
> > >> input_device_setting to
> > >> set the property values, it seems like we have to make the object
globally
> > >> available.
> > >>
> > >
> > > We'll need to use an interface for input settings so that all build
configs
> > > work. It will need to go in ui/events or ui/ozone/public.
> > >
> > > There is a big object you can get statically called OzonePlatform - adding
> > > a
> > > function like GetInputDeviceSettings() that returns the object is better
> > > than
> > > creating a new static altogether.
> > >
> > > https://codereview.chromium.org/545063006/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Shecky Lin, 2014-09-24 08:34:46]

On 2014/09/23 16:49:55, spang wrote:
> On 2014/09/22 09:57:32, Shecky Lin wrote:
> > Got it. Unfortunately, I found that I still need a globally available
> > object that I can call from in the APIs provided by
> > GesturePropFunctionsWrapper, which will in turn be called externally from
> > the gesture library. Do you have any idea how I can do this (without using
> > a singleton)?
> 
> Pass the GestureInterpreterLibevdevCros* to GestureInterpreterSetPropProvider
so
> that you can recover the right object in the callbacks, and then add a pointer
> to GesturePropertyProvider to GestureInterpreterLibevdevCros (set it in the
> constructor).
Ah, this is so brilliant! Don't know why I didn't come to it at the first place.
Thanks a lot!
> 
> > 2014/9/20 上午2:28 於 <mailto:spang@chromium.org> 寫道：
> > 
> > > On 2014/09/19 03:24:18, Shecky Lin wrote:
> > >
> > >> On 2014/09/18 18:06:17, spang wrote:
> > >> > Thanks for doing this!
> > >> >
> > >> > I think we'll need a bit of change to better match the chromium coding
> > >>
> > > style.
> > >
> > >> >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/545063006/diff/20001/ui/
> > > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > >
> > >> > File ui/events/ozone/evdev/libgestures_glue/gesture_
> > >> property_provider.cc
> > >> > (right):
> > >> >
> > >> >
> > >>
> > >
> > > https://codereview.chromium.org/545063006/diff/20001/ui/
> > > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc#
> > > newcode247
> > >
> > >> > ui/events/ozone/evdev/libgestures_glue/gesture_
> > >> property_provider.cc:247:
> > >> return
> > >> > Singleton<GesturePropertyProvider>::get();
> > >> > Instead of making it a singleton, please create this object in the
> > >> > EventFactoryEvdev constructor, and plumb it to the gestures device
> > >> constructor.
> > >>
> > >
> > >  Just a quick question: if we create the object in EventFactoryEvdev, how
> > >> can
> > >>
> > > we
> > >
> > >> let other classes access the property values? For example, if we want to
> > >> allow
> > >> input_device_setting to
> > >> set the property values, it seems like we have to make the object
globally
> > >> available.
> > >>
> > >
> > > We'll need to use an interface for input settings so that all build
configs
> > > work. It will need to go in ui/events or ui/ozone/public.
> > >
> > > There is a big object you can get statically called OzonePlatform - adding
> > > a
> > > function like GetInputDeviceSettings() that returns the object is better
> > > than
> > > creating a new static altogether.
> > >
> > > https://codereview.chromium.org/545063006/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Shecky Lin, 2014-09-24 08:36:41]

On 2014/09/23 16:58:05, spang wrote:
> On 2014/09/23 08:41:25, Shecky Lin wrote:
> > I've uploaded a new patch which addressed all issues besides the singleton
> > problem. Please take a look.
> > 
> > I have also checked the OzonePlatform related codes and I think I finally
> > understand what you were talking about (oops!). Do you think I can put an
> > instance of GesturePropertyProvider in the OzonePlatform derived classes
> (e.g.,
> > OzonePlatformGbm) and let the users call a function like
> > GetGesturePropertyProvider()? Specifically, this is what I plan to do:
> 
> Let's not include gestures stuff from the settings page - it's an
implementation
> detail.
> 
> The interface we expose should be "InputDeviceSettings" or similar, and
contain
> just enough to satisfy the settings page:
> 
> - bool HasTouchpad()
> - bool HasMouse()
> - bool HasKeyboard()
> 
> - void SetMouseAcceleration()
> - void SetScrollDirection()
> 
> etc. Then implement this interface in EventFactoryEvdev and delegate to
> GesturePropProvider for the touchpad parts.
> 
This makes sense. I will proceed according to it once we get the patch in, or we
can discuss.

> > 
> > a) Split the gesture_property_provider into 3 files:
> > 1. gesture_property{.h, .cc} - contains GesturesProp definition and related
> > functions (e.g., creators).
> > 2. gesture_property_provider{.h, .cc} - contains GesturePropertyProvider.
> > 3. gesture_property_utils{.h, .cc} - contains GesturesPropFunctionsWrapper
and
> > an API object GesturesPropProvider which will be used by the gesture
library.
> > 
> > b) Modify OzonePlatform:
> > 1. Add a function GetGesturePropertyProvider() which returns a static
> > GesturePropertyProvider object.
> > 2. Modify OzonePlatform derived classes to initialize the object during
> > InitializeUI().
> > 
> > c) Header include order:
> > gesture_property_provider.h includes gestures/gestures.h
> > gesture_property_provider.cc includes gesture_property.h
> > OzonePlatform derived classes' sources files include
> gesture_property_provider.h
> > gesture_property_utils.h includes gesture_property_provider.h
> > gesture_property_utils.cc includes gesture_property.h, ozone_platform.h
> > gesture_interpreter_libevdev_cros.h includes gesture_property_provider.h
> > gesture_interpreter_libevdev_cros.cc includes gesture_property_utils.h
> > 
> > d) For all other classes that needs to use gesture_property_provider (e.g.,
> > input_device_setting):
> > *.cc includes gesture_property_provider.h and ozone_platform.h
> > 
> > If this sounds good to you, please let me know so we can close this out
soon.
> > Thanks!
> > On 2014/09/22 09:57:32, Shecky Lin wrote:
> > > Got it. Unfortunately, I found that I still need a globally available
> > > object that I can call from in the APIs provided by
> > > GesturePropFunctionsWrapper, which will in turn be called externally from
> > > the gesture library. Do you have any idea how I can do this (without using
> > > a singleton)?
> > > 2014/9/20 上午2:28 於 <mailto:spang@chromium.org> 寫道：
> > > 
> > > > On 2014/09/19 03:24:18, Shecky Lin wrote:
> > > >
> > > >> On 2014/09/18 18:06:17, spang wrote:
> > > >> > Thanks for doing this!
> > > >> >
> > > >> > I think we'll need a bit of change to better match the chromium
coding
> > > >>
> > > > style.
> > > >
> > > >> >
> > > >> >
> > > >>
> > > >
> > > > https://codereview.chromium.org/545063006/diff/20001/ui/
> > > > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > > >
> > > >> > File ui/events/ozone/evdev/libgestures_glue/gesture_
> > > >> property_provider.cc
> > > >> > (right):
> > > >> >
> > > >> >
> > > >>
> > > >
> > > > https://codereview.chromium.org/545063006/diff/20001/ui/
> > > > events/ozone/evdev/libgestures_glue/gesture_property_provider.cc#
> > > > newcode247
> > > >
> > > >> > ui/events/ozone/evdev/libgestures_glue/gesture_
> > > >> property_provider.cc:247:
> > > >> return
> > > >> > Singleton<GesturePropertyProvider>::get();
> > > >> > Instead of making it a singleton, please create this object in the
> > > >> > EventFactoryEvdev constructor, and plumb it to the gestures device
> > > >> constructor.
> > > >>
> > > >
> > > >  Just a quick question: if we create the object in EventFactoryEvdev,
how
> > > >> can
> > > >>
> > > > we
> > > >
> > > >> let other classes access the property values? For example, if we want
to
> > > >> allow
> > > >> input_device_setting to
> > > >> set the property values, it seems like we have to make the object
> globally
> > > >> available.
> > > >>
> > > >
> > > > We'll need to use an interface for input settings so that all build
> configs
> > > > work. It will need to go in ui/events or ui/ozone/public.
> > > >
> > > > There is a big object you can get statically called OzonePlatform -
adding
> > > > a
> > > > function like GetInputDeviceSettings() that returns the object is better
> > > > than
> > > > creating a new static altogether.
> > > >
> > > > https://codereview.chromium.org/545063006/
> > > >
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.

[Shecky Lin, 2014-09-24 08:39:05]

Hey guys, I've uploaded a new patch which addressed all comments before. Could
you take another look of it? Thanks!

[spang, 2014-09-26 15:15:32]

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:36: struct
GesturesProp {
This interface needs work to avoid lots of the casts in the current
implementation. We try to avoid casts whenever possible. So, can we have
different types for each storage type we need to support? The base class can
have a variant-ish interface.

The interface exposed by libgestures doesn't seem ideal, I guess since it was
made for cmt. We can deal, but let's try to be as safe as possible when writing
through the pointers into gestures' memory.

Here's a way to do it I'd be much more comfortable with:

class GesturesProp { 
 public:
  GesturesProp() : handler_data_(NULL), get_(NULL), set_(NULL) {}
  virtual ~GesturesProp() {}
  
  virtual int size() = 0;

  virtual int GetIntValueAt(size_t index) {
    NOTREACHED();
    return -1;
  }
  virtual bool SetIntValueAt(size_t index, int value) {
    NOTREACHED();
    return false;
  }

  virtual short GetShortValueAt(size_t index) {
    NOTREACHED();
    return -1;
  }
  virtual bool SetShortValueAt(size_t index, short value) {
    NOTREACHED();
    return false;
  }

  virtual bool GetBoolValueAt(size_t index) {
    NOTREACHED();
    return false;
  }
  virtual bool SetBoolValueAt(size_t index, bool value) {
    NOTREACHED();
    return false;
  }

  virtual double GetDoubleValueAt(size_t index) {
    NOTREACHED();
    return -1;
  }
  virtual bool SetDoubleValueAt(size_t index, double value) {
    NOTREACHED();
    return false;
  }

  virtual bool SetStringValue(const std::string& string) {
    NOTREACHED();
    return false;
  }
  virtual std::string GetStringValue() {
    NOTREACHED();
    return "";
  }

  void SetHandlers(GesturesPropGetHandler get, GesturesPropSetHandler set, void
*data) {
    // Get is not used by the library and not supported.
    set_ = set;    
    handler_data_ = data;
  }

  void OnSet() {
    if (set_)
      set_(handler_data_);
  }

 private:
  void* handler_data_;
  GesturesPropSetHandler set_;
};

namespace ui {

class GesturesPropIntArray : public GesturesProp {
public:
  GesturesPropInt(size_t size, const int* init, int* write_back)
    : write_back_(write_back) {
    val_.reset(new int[size]);
    if (init)
      memcpy(val_.get(), init, data_size());
    else
      memset(val_.get(), 0, data_size());
  }

  size_t size() { return size_; }

  size_t data_size() { return size_ * sizeof(val_[0]); }
  
  virtual int GetIntValueAt(size_t index) OVERRIDE {
    DCHECK_LT(index, size());
    return val_index];
  }

  virtual bool SetIntValueAt(size_t index, int value) OVERRIDE {
    DCHECK_LT(index, size());
    val_[index] = value;
    if (write_back_)
      write_back_[index] = value;
    OnSet();
  }

 private:
   size_t size;
   scoped_ptr<int[]> val_;
   int* write_back_;
};

class GesturesPropString : public GesturesProp {
public:
  GesturesPropString(const char* init, const char** write_back)
    : write_back_(write_back) {
    val_.reset(new std::string(init));
    *write_back = val_->c_str();
  }

  virtual std::string GetStringValue() OVERRIDE {
    return *val_;
  }

  virtual bool SetStringValue(const std::string& value) OVERRIDE {
    // This creates a new string because libgestures still has a pointer into
the old one that
    // we are about to invalidate.
    scoped_ptr<const std::string> new_val(new string(value));
    if (write_back_)
      write_back_[index] = new_val.c_str();
    OnSet();

    // The internal pointer is now updated, so the old string can be freed.
    val_ = new_val.Pass();
  }

 private:
   scoped_ptr<const std::string> val_;
   char** write_back_;
};

// and so on

}

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:56: return
reinterpret_cast<T*>(val);
This allows arbitrary unsafe casts of the pointer...

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:402: return
true;
Doesn't this mean if all of the patterns are invalid it will match? More
generally, it looks like malformed configs are silently accepted as matches.
Shouldn't we generate an error at least?

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:972:
GPROP_LOG(3) << "Found old property, deleting it ...";
Does the lib ever create differently types properties with the same name?

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1184:
reinterpret_cast<short*>(&(dev->info.id.vendor)));
This isn't allowed with strict aliasing, please don't reinterpret_cast T* to U*
(unless U is char).

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:178:
MatchCriteria* AllocateMatchCriteria(const std::string& arg) {
Could you move this to the cc file? There shouldn't be anything in the header
file that doesn't need to be there. Looks like they can even be inside an
anonymous namespace.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:186: class
MatchProduct : public MatchCriteria {
Move all matchers to .cc file.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:274: static
bool IsDeviceOfType(DevicePtr device, const DeviceType type);
All of these private statics should be moved to the .cc file unless they need
access to class internals.

So move IsDeviceOfType, kMaxDeviceNum, IsMatch*, ParseBooleanKeyword.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:278: DeviceId
GetDeviceId(DevicePtr dev, const bool do_create = false);
Remove the default argument (they aren't allowed by the style guide).

[Shecky Lin, 2014-09-26 16:13:37]

I agree with other comments so I just reply to this one quickly to avoid
being delayed by time zone difference.

On Sep 26, 2014 11:15 PM, <spang@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> (right):
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:36:
> struct GesturesProp {
> This interface needs work to avoid lots of the casts in the current
> implementation. We try to avoid casts whenever possible. So, can we have
> different types for each storage type we need to support? The base class
> can have a variant-ish interface.
>
> The interface exposed by libgestures doesn't seem ideal, I guess since
> it was made for cmt. We can deal, but let's try to be as safe as
> possible when writing through the pointers into gestures' memory.
>
> Here's a way to do it I'd be much more comfortable with:

The reason I chose not to do it this way is because we will duplicate a lot
of similar codes for all 5 data types (e.g., SetValue,GetValue,Create). If
we use the cast-approach, most of them can then be combined using templates
and thus reducing our maintainance effort. I actually wrote something
similar to this and changed to the cast approach before sending it for the
1st review.

So, are you good with the possible code duplication this approach may
imply? I am fine with either ways.
>
> class GesturesProp {
>  public:
>   GesturesProp() : handler_data_(NULL), get_(NULL), set_(NULL) {}
>   virtual ~GesturesProp() {}
>
>   virtual int size() = 0;
>
>   virtual int GetIntValueAt(size_t index) {
>     NOTREACHED();
>     return -1;
>   }
>   virtual bool SetIntValueAt(size_t index, int value) {
>     NOTREACHED();
>     return false;
>   }
>
>   virtual short GetShortValueAt(size_t index) {
>     NOTREACHED();
>     return -1;
>   }
>   virtual bool SetShortValueAt(size_t index, short value) {
>     NOTREACHED();
>     return false;
>   }
>
>   virtual bool GetBoolValueAt(size_t index) {
>     NOTREACHED();
>     return false;
>   }
>   virtual bool SetBoolValueAt(size_t index, bool value) {
>     NOTREACHED();
>     return false;
>   }
>
>   virtual double GetDoubleValueAt(size_t index) {
>     NOTREACHED();
>     return -1;
>   }
>   virtual bool SetDoubleValueAt(size_t index, double value) {
>     NOTREACHED();
>     return false;
>   }
>
>   virtual bool SetStringValue(const std::string& string) {
>     NOTREACHED();
>     return false;
>   }
>   virtual std::string GetStringValue() {
>     NOTREACHED();
>     return "";
>   }
>
>   void SetHandlers(GesturesPropGetHandler get, GesturesPropSetHandler
> set, void *data) {
>     // Get is not used by the library and not supported.
>     set_ = set;
>     handler_data_ = data;
>   }
>
>   void OnSet() {
>     if (set_)
>       set_(handler_data_);
>   }
>
>  private:
>   void* handler_data_;
>   GesturesPropSetHandler set_;
> };
>
> namespace ui {
>
> class GesturesPropIntArray : public GesturesProp {
> public:
>   GesturesPropInt(size_t size, const int* init, int* write_back)
>     : write_back_(write_back) {
>     val_.reset(new int[size]);
>     if (init)
>       memcpy(val_.get(), init, data_size());
>     else
>       memset(val_.get(), 0, data_size());
>   }
>
>   size_t size() { return size_; }
>
>   size_t data_size() { return size_ * sizeof(val_[0]); }
>
>   virtual int GetIntValueAt(size_t index) OVERRIDE {
>     DCHECK_LT(index, size());
>     return val_index];
>   }
>
>   virtual bool SetIntValueAt(size_t index, int value) OVERRIDE {
>     DCHECK_LT(index, size());
>     val_[index] = value;
>     if (write_back_)
>       write_back_[index] = value;
>     OnSet();
>   }
>
>  private:
>    size_t size;
>    scoped_ptr<int[]> val_;
>    int* write_back_;
> };
>
> class GesturesPropString : public GesturesProp {
> public:
>   GesturesPropString(const char* init, const char** write_back)
>     : write_back_(write_back) {
>     val_.reset(new std::string(init));
>     *write_back = val_->c_str();
>   }
>
>   virtual std::string GetStringValue() OVERRIDE {
>     return *val_;
>   }
>
>   virtual bool SetStringValue(const std::string& value) OVERRIDE {
>     // This creates a new string because libgestures still has a pointer
> into the old one that
>     // we are about to invalidate.
>     scoped_ptr<const std::string> new_val(new string(value));
>     if (write_back_)
>       write_back_[index] = new_val.c_str();
>     OnSet();
>
>     // The internal pointer is now updated, so the old string can be
> freed.
>     val_ = new_val.Pass();
>   }
>
>  private:
>    scoped_ptr<const std::string> val_;
>    char** write_back_;
> };
>
> // and so on
>
> }
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:56:
> return reinterpret_cast<T*>(val);
> This allows arbitrary unsafe casts of the pointer...
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:402:
> return true;
> Doesn't this mean if all of the patterns are invalid it will match? More
> generally, it looks like malformed configs are silently accepted as
> matches. Shouldn't we generate an error at least?
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:972:
> GPROP_LOG(3) << "Found old property, deleting it ...";
> Does the lib ever create differently types properties with the same
> name?
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1184:
> reinterpret_cast<short*>(&(dev->info.id.vendor)));
> This isn't allowed with strict aliasing, please don't reinterpret_cast
> T* to U* (unless U is char).
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h
> (right):
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:178:
> MatchCriteria* AllocateMatchCriteria(const std::string& arg) {
> Could you move this to the cc file? There shouldn't be anything in the
> header file that doesn't need to be there. Looks like they can even be
> inside an anonymous namespace.
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:186:
> class MatchProduct : public MatchCriteria {
> Move all matchers to .cc file.
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:274:
> static bool IsDeviceOfType(DevicePtr device, const DeviceType type);
> All of these private statics should be moved to the .cc file unless they
> need access to class internals.
>
> So move IsDeviceOfType, kMaxDeviceNum, IsMatch*, ParseBooleanKeyword.
>
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:278:
> DeviceId GetDeviceId(DevicePtr dev, const bool do_create = false);
> Remove the default argument (they aren't allowed by the style guide).
>
> https://codereview.chromium.org/545063006/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spang, 2014-09-26 16:29:53]

On 2014/09/26 16:13:37, Shecky Lin wrote:
> I agree with other comments so I just reply to this one quickly to avoid
> being delayed by time zone difference.
> 
> On Sep 26, 2014 11:15 PM, <mailto:spang@chromium.org> wrote:
> >
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:36:
> > struct GesturesProp {
> > This interface needs work to avoid lots of the casts in the current
> > implementation. We try to avoid casts whenever possible. So, can we have
> > different types for each storage type we need to support? The base class
> > can have a variant-ish interface.
> >
> > The interface exposed by libgestures doesn't seem ideal, I guess since
> > it was made for cmt. We can deal, but let's try to be as safe as
> > possible when writing through the pointers into gestures' memory.
> >
> > Here's a way to do it I'd be much more comfortable with:
> 
> The reason I chose not to do it this way is because we will duplicate a lot
> of similar codes for all 5 data types (e.g., SetValue,GetValue,Create). If
> we use the cast-approach, most of them can then be combined using templates
> and thus reducing our maintainance effort. I actually wrote something
> similar to this and changed to the cast approach before sending it for the
> 1st review.
> 
> So, are you good with the possible code duplication this approach may
> imply? I am fine with either ways.

I don't think the current deduplication strategy of relying on casts fits into
our coding style.

You could still write a GesturesPropScalarArray<T> to dedup big large parts of
the class for int/short/etc and then create subclasseses:

class GesturesPropIntArray : GesturesPropScalarArray<int> {
  virtual int GetIntValueAt(size_t index) OVERRIDE {
    return GetScalarValueAt(index);
  }
  virtual bool SetIntValueAt(size_t index, int value) OVERRIDE {
    return SetScalarValueAt(index, value);
  }
}

The style guide doesn't really encourage template metaprogramming, but this
example is probably OK.

Duplication isn't really a problem from a style perspective, we're totally OK
with this if it means the resulting code is easier to read than the
metaprogramming or macro solution.

> >
> > class GesturesProp {
> >  public:
> >   GesturesProp() : handler_data_(NULL), get_(NULL), set_(NULL) {}
> >   virtual ~GesturesProp() {}
> >
> >   virtual int size() = 0;
> >
> >   virtual int GetIntValueAt(size_t index) {
> >     NOTREACHED();
> >     return -1;
> >   }
> >   virtual bool SetIntValueAt(size_t index, int value) {
> >     NOTREACHED();
> >     return false;
> >   }
> >
> >   virtual short GetShortValueAt(size_t index) {
> >     NOTREACHED();
> >     return -1;
> >   }
> >   virtual bool SetShortValueAt(size_t index, short value) {
> >     NOTREACHED();
> >     return false;
> >   }
> >
> >   virtual bool GetBoolValueAt(size_t index) {
> >     NOTREACHED();
> >     return false;
> >   }
> >   virtual bool SetBoolValueAt(size_t index, bool value) {
> >     NOTREACHED();
> >     return false;
> >   }
> >
> >   virtual double GetDoubleValueAt(size_t index) {
> >     NOTREACHED();
> >     return -1;
> >   }
> >   virtual bool SetDoubleValueAt(size_t index, double value) {
> >     NOTREACHED();
> >     return false;
> >   }
> >
> >   virtual bool SetStringValue(const std::string& string) {
> >     NOTREACHED();
> >     return false;
> >   }
> >   virtual std::string GetStringValue() {
> >     NOTREACHED();
> >     return "";
> >   }
> >
> >   void SetHandlers(GesturesPropGetHandler get, GesturesPropSetHandler
> > set, void *data) {
> >     // Get is not used by the library and not supported.
> >     set_ = set;
> >     handler_data_ = data;
> >   }
> >
> >   void OnSet() {
> >     if (set_)
> >       set_(handler_data_);
> >   }
> >
> >  private:
> >   void* handler_data_;
> >   GesturesPropSetHandler set_;
> > };
> >
> > namespace ui {
> >
> > class GesturesPropIntArray : public GesturesProp {
> > public:
> >   GesturesPropInt(size_t size, const int* init, int* write_back)
> >     : write_back_(write_back) {
> >     val_.reset(new int[size]);
> >     if (init)
> >       memcpy(val_.get(), init, data_size());
> >     else
> >       memset(val_.get(), 0, data_size());
> >   }
> >
> >   size_t size() { return size_; }
> >
> >   size_t data_size() { return size_ * sizeof(val_[0]); }
> >
> >   virtual int GetIntValueAt(size_t index) OVERRIDE {
> >     DCHECK_LT(index, size());
> >     return val_index];
> >   }
> >
> >   virtual bool SetIntValueAt(size_t index, int value) OVERRIDE {
> >     DCHECK_LT(index, size());
> >     val_[index] = value;
> >     if (write_back_)
> >       write_back_[index] = value;
> >     OnSet();
> >   }
> >
> >  private:
> >    size_t size;
> >    scoped_ptr<int[]> val_;
> >    int* write_back_;
> > };
> >
> > class GesturesPropString : public GesturesProp {
> > public:
> >   GesturesPropString(const char* init, const char** write_back)
> >     : write_back_(write_back) {
> >     val_.reset(new std::string(init));
> >     *write_back = val_->c_str();
> >   }
> >
> >   virtual std::string GetStringValue() OVERRIDE {
> >     return *val_;
> >   }
> >
> >   virtual bool SetStringValue(const std::string& value) OVERRIDE {
> >     // This creates a new string because libgestures still has a pointer
> > into the old one that
> >     // we are about to invalidate.
> >     scoped_ptr<const std::string> new_val(new string(value));
> >     if (write_back_)
> >       write_back_[index] = new_val.c_str();
> >     OnSet();
> >
> >     // The internal pointer is now updated, so the old string can be
> > freed.
> >     val_ = new_val.Pass();
> >   }
> >
> >  private:
> >    scoped_ptr<const std::string> val_;
> >    char** write_back_;
> > };
> >
> > // and so on
> >
> > }
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:56:
> > return reinterpret_cast<T*>(val);
> > This allows arbitrary unsafe casts of the pointer...
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:402:
> > return true;
> > Doesn't this mean if all of the patterns are invalid it will match? More
> > generally, it looks like malformed configs are silently accepted as
> > matches. Shouldn't we generate an error at least?
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:972:
> > GPROP_LOG(3) << "Found old property, deleting it ...";
> > Does the lib ever create differently types properties with the same
> > name?
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1184:
> > reinterpret_cast<short*>(&(dev->info.id.vendor)));
> > This isn't allowed with strict aliasing, please don't reinterpret_cast
> > T* to U* (unless U is char).
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h
> > (right):
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:178:
> > MatchCriteria* AllocateMatchCriteria(const std::string& arg) {
> > Could you move this to the cc file? There shouldn't be anything in the
> > header file that doesn't need to be there. Looks like they can even be
> > inside an anonymous namespace.
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:186:
> > class MatchProduct : public MatchCriteria {
> > Move all matchers to .cc file.
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:274:
> > static bool IsDeviceOfType(DevicePtr device, const DeviceType type);
> > All of these private statics should be moved to the .cc file unless they
> > need access to class internals.
> >
> > So move IsDeviceOfType, kMaxDeviceNum, IsMatch*, ParseBooleanKeyword.
> >
> >
>
https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:278:
> > DeviceId GetDeviceId(DevicePtr dev, const bool do_create = false);
> > Remove the default argument (they aren't allowed by the style guide).
> >
> > https://codereview.chromium.org/545063006/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Shecky Lin, 2014-10-03 03:58:55]

Hey guys, please take another look of the new patch. Thanks!

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:36: struct
GesturesProp {
I've tried to comply with this as much as possible and revamped the interfaces
so that no pointer type-casting happens anymore. However, please note that we
can't scope the data pointer as they usually points to the external memory in
the gesture library. Also, I've tried to avoid the code duplication at the same
time. Anyway, feel free to let me know how you think.

On 2014/09/26 15:15:32, spang wrote:
> This interface needs work to avoid lots of the casts in the current
> implementation. We try to avoid casts whenever possible. So, can we have
> different types for each storage type we need to support? The base class can
> have a variant-ish interface.
> 
> The interface exposed by libgestures doesn't seem ideal, I guess since it was
> made for cmt. We can deal, but let's try to be as safe as possible when
writing
> through the pointers into gestures' memory.
> 
> Here's a way to do it I'd be much more comfortable with:
> 
> class GesturesProp { 
>  public:
>   GesturesProp() : handler_data_(NULL), get_(NULL), set_(NULL) {}
>   virtual ~GesturesProp() {}
>   
>   virtual int size() = 0;
> 
>   virtual int GetIntValueAt(size_t index) {
>     NOTREACHED();
>     return -1;
>   }
>   virtual bool SetIntValueAt(size_t index, int value) {
>     NOTREACHED();
>     return false;
>   }
> 
>   virtual short GetShortValueAt(size_t index) {
>     NOTREACHED();
>     return -1;
>   }
>   virtual bool SetShortValueAt(size_t index, short value) {
>     NOTREACHED();
>     return false;
>   }
> 
>   virtual bool GetBoolValueAt(size_t index) {
>     NOTREACHED();
>     return false;
>   }
>   virtual bool SetBoolValueAt(size_t index, bool value) {
>     NOTREACHED();
>     return false;
>   }
> 
>   virtual double GetDoubleValueAt(size_t index) {
>     NOTREACHED();
>     return -1;
>   }
>   virtual bool SetDoubleValueAt(size_t index, double value) {
>     NOTREACHED();
>     return false;
>   }
> 
>   virtual bool SetStringValue(const std::string& string) {
>     NOTREACHED();
>     return false;
>   }
>   virtual std::string GetStringValue() {
>     NOTREACHED();
>     return "";
>   }
> 
>   void SetHandlers(GesturesPropGetHandler get, GesturesPropSetHandler set,
void
> *data) {
>     // Get is not used by the library and not supported.
>     set_ = set;    
>     handler_data_ = data;
>   }
> 
>   void OnSet() {
>     if (set_)
>       set_(handler_data_);
>   }
> 
>  private:
>   void* handler_data_;
>   GesturesPropSetHandler set_;
> };
> 
> namespace ui {
> 
> class GesturesPropIntArray : public GesturesProp {
> public:
>   GesturesPropInt(size_t size, const int* init, int* write_back)
>     : write_back_(write_back) {
>     val_.reset(new int[size]);
>     if (init)
>       memcpy(val_.get(), init, data_size());
>     else
>       memset(val_.get(), 0, data_size());
>   }
> 
>   size_t size() { return size_; }
> 
>   size_t data_size() { return size_ * sizeof(val_[0]); }
>   
>   virtual int GetIntValueAt(size_t index) OVERRIDE {
>     DCHECK_LT(index, size());
>     return val_index];
>   }
> 
>   virtual bool SetIntValueAt(size_t index, int value) OVERRIDE {
>     DCHECK_LT(index, size());
>     val_[index] = value;
>     if (write_back_)
>       write_back_[index] = value;
>     OnSet();
>   }
> 
>  private:
>    size_t size;
>    scoped_ptr<int[]> val_;
>    int* write_back_;
> };
> 
> class GesturesPropString : public GesturesProp {
> public:
>   GesturesPropString(const char* init, const char** write_back)
>     : write_back_(write_back) {
>     val_.reset(new std::string(init));
>     *write_back = val_->c_str();
>   }
> 
>   virtual std::string GetStringValue() OVERRIDE {
>     return *val_;
>   }
> 
>   virtual bool SetStringValue(const std::string& value) OVERRIDE {
>     // This creates a new string because libgestures still has a pointer into
> the old one that
>     // we are about to invalidate.
>     scoped_ptr<const std::string> new_val(new string(value));
>     if (write_back_)
>       write_back_[index] = new_val.c_str();
>     OnSet();
> 
>     // The internal pointer is now updated, so the old string can be freed.
>     val_ = new_val.Pass();
>   }
> 
>  private:
>    scoped_ptr<const std::string> val_;
>    char** write_back_;
> };
> 
> // and so on
> 
> }

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:56: return
reinterpret_cast<T*>(val);
On 2014/09/26 15:15:32, spang wrote:
> This allows arbitrary unsafe casts of the pointer...

Removed.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:402: return
true;
On 2014/09/26 15:15:32, spang wrote:
> Doesn't this mean if all of the patterns are invalid it will match? More
> generally, it looks like malformed configs are silently accepted as matches.
> Shouldn't we generate an error at least?

Done.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:972:
GPROP_LOG(3) << "Found old property, deleting it ...";
On 2014/09/26 15:15:32, spang wrote:
> Does the lib ever create differently types properties with the same name? 

In general, it shouldn't, but in case it happens I want to be able to handle it.
This check is also part of the original CMT implementation. Also, it doesn't
have to be different types - it can be the exactly same property being
re-created.

I've changed the log message to WARNING to signify its importance.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1184:
reinterpret_cast<short*>(&(dev->info.id.vendor)));
On 2014/09/26 15:15:32, spang wrote:
> This isn't allowed with strict aliasing, please don't reinterpret_cast T* to
U*
> (unless U is char).

Done.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:178:
MatchCriteria* AllocateMatchCriteria(const std::string& arg) {
On 2014/09/26 15:15:32, spang wrote:
> Could you move this to the cc file? There shouldn't be anything in the header
> file that doesn't need to be there. Looks like they can even be inside an
> anonymous namespace.

I couldn't made it in an anonymous namespace because I need to use these types
in the function definition and also some variables. Forward declaration can't
work with anonymous namespace. I put them under namespace internal instead as
this seems to be a convention in the Chromium codebase.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:186: class
MatchProduct : public MatchCriteria {
On 2014/09/26 15:15:32, spang wrote:
> Move all matchers to .cc file.

Done.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:274: static
bool IsDeviceOfType(DevicePtr device, const DeviceType type);
On 2014/09/26 15:15:32, spang wrote:
> All of these private statics should be moved to the .cc file unless they need
> access to class internals.
> 
> So move IsDeviceOfType, kMaxDeviceNum, IsMatch*, ParseBooleanKeyword.
> 

Done.

https://codereview.chromium.org/545063006/diff/100001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:278: DeviceId
GetDeviceId(DevicePtr dev, const bool do_create = false);
On 2014/09/26 15:15:32, spang wrote:
> Remove the default argument (they aren't allowed by the style guide).

Done.

[spang, 2014-10-03 21:40:11]

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:36: struct
GesturesProp {
This is nontrivial and should not be a struct. Change to "class" and privatize /
protect the member variables (and add underscores).

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:57: virtual
bool GetIntValue(int* v) const {
You've lost all bounds checking here (in my suggestion I was returning one
element at a time and checking it inside).

It's important to get this right - overrunning the client's buffer is really
bad.

If you want to get/set the whole thing in one go, how about using a vector:

virtual std::vector<int> GetIntVector() const {
  NOTREACHED();
  return std::vector<int>();
}

virtual void SetIntVector(const std::vector<int>& vec) {
   NOTREACHED();
 }

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:176: if
(count > 1)
I think you can remove the if{} here (and below). An array with one element
works just fine.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:197: bool
GetNumericalValue(U* v) const {
If you use vectors:

std::vector<U> GetNumericalVector()

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:209: bool
SetNumericalValue(const U* v) {
void SetNumericalVector(const std::vector<U>& v)

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:218: bool
GetNumericalPropertyValue(U* v) const {
std::vector<U> GetNumericalPropertyVector()

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:225: bool
SetNumericalPropertyValue(const U* v) {
void SetNumericalPropertyVector(const std::vector<U>& v)

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:97: bool
GetPropertyType(const DeviceId device_id,
I think you can replace these with just one function to return the object? That
is, instead of:

  bool GetProperty
  bool GetIntProperty
  bool GetShortProperty
  ...

you can just have

  GesturesProp* GetProperty(DeviceId device_id,
                            const std::string& name);

The client would do

  GesturesProp* prop = provider->GetProperty(id, name);
  std::vector<int> val = prop->GetIntVector();

or

  std::vector<int> vec;
  vec.push_back(1);
  vec.push_back(2);
  vec.push_back(3);
  vec.push_back(4);

  GesturesProp* prop = provider->GetProperty(id, name);
  prop->SetIntVector(vec);

[alexst (slow to review), 2014-10-03 22:32:02]

Hi Shecky, thank you for working through this!

I made some comments on code style related things. Chromium owners tend to be
very strict about about following the style guide. I noticed a number of things
I didn't comment on, so I'll leave a general note here. Please read through
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html and do a pass on
all the cosmetic things in your patch.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h
(right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:61:
GesturePropertyProvider* GetPropertyProvider() { return prop_provider_; }
Per google code style
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

Please make this

GesturePropertyProvider* property_provider() { return property_provider_; }

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:64:
Evdev* GetDevicePointer() { return dev_; }
Evdev* evdev() { return dev_; }

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:89:
GesturePropertyProvider* prop_provider_;
Use full names please, property_provider_

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:98:
Evdev* dev_;
Full name here as well, evdev_

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:101:
scoped_ptr<GestureDeviceProperties> device_props_;
device_properties_

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1379:
GesturesProp* GesturesPropFunctionsWrapper::CreateProperty(void* priv,
Please define parameter names in full without abbreviations.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:263: static
GesturesProp* CreateReal(void*,
Please add names to all the parameters

[Shecky Lin, 2014-10-08 10:51:00]

Hey guys,

I have addressed your previous comments and re-factored the code per the style
guideline. Can you take another look of this? Thanks!

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h
(right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:61:
GesturePropertyProvider* GetPropertyProvider() { return prop_provider_; }
On 2014/10/03 22:32:02, alexst wrote:
> Per google code style
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names
> 
> Please make this
> 
> GesturePropertyProvider* property_provider() { return property_provider_; }

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:64:
Evdev* GetDevicePointer() { return dev_; }
On 2014/10/03 22:32:02, alexst wrote:
> Evdev* evdev() { return dev_; }

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:89:
GesturePropertyProvider* prop_provider_;
On 2014/10/03 22:32:02, alexst wrote:
> Use full names please, property_provider_

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:98:
Evdev* dev_;
On 2014/10/03 22:32:02, alexst wrote:
> Full name here as well, evdev_

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.h:101:
scoped_ptr<GestureDeviceProperties> device_props_;
On 2014/10/03 22:32:02, alexst wrote:
> device_properties_

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:36: struct
GesturesProp {
On 2014/10/03 21:40:11, spang wrote:
> This is nontrivial and should not be a struct. Change to "class" and privatize
/
> protect the member variables (and add underscores).

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:57: virtual
bool GetIntValue(int* v) const {
On 2014/10/03 21:40:11, spang wrote:
> You've lost all bounds checking here (in my suggestion I was returning one
> element at a time and checking it inside).
> 
> It's important to get this right - overrunning the client's buffer is really
> bad.
> 
> If you want to get/set the whole thing in one go, how about using a vector:
> 
> virtual std::vector<int> GetIntVector() const {
>   NOTREACHED();
>   return std::vector<int>();
> }
> 
> virtual void SetIntVector(const std::vector<int>& vec) {
>    NOTREACHED();
>  }

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:176: if
(count > 1)
On 2014/10/03 21:40:11, spang wrote:
> I think you can remove the if{} here (and below). An array with one element
> works just fine.

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:197: bool
GetNumericalValue(U* v) const {
On 2014/10/03 21:40:11, spang wrote:
> If you use vectors:
> 
> std::vector<U> GetNumericalVector()

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:209: bool
SetNumericalValue(const U* v) {
On 2014/10/03 21:40:11, spang wrote:
> void SetNumericalVector(const std::vector<U>& v)

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:218: bool
GetNumericalPropertyValue(U* v) const {
On 2014/10/03 21:40:11, spang wrote:
> std::vector<U> GetNumericalPropertyVector()

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:225: bool
SetNumericalPropertyValue(const U* v) {
On 2014/10/03 21:40:11, spang wrote:
> void SetNumericalPropertyVector(const std::vector<U>& v)

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1379:
GesturesProp* GesturesPropFunctionsWrapper::CreateProperty(void* priv,
On 2014/10/03 22:32:02, alexst wrote:
> Please define parameter names in full without abbreviations.

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:97: bool
GetPropertyType(const DeviceId device_id,
On 2014/10/03 21:40:11, spang wrote:
> I think you can replace these with just one function to return the object?
That
> is, instead of:
> 
>   bool GetProperty
>   bool GetIntProperty
>   bool GetShortProperty
>   ...
> 
> you can just have
> 
>   GesturesProp* GetProperty(DeviceId device_id,
>                             const std::string& name);
> 
> The client would do
> 
>   GesturesProp* prop = provider->GetProperty(id, name);
>   std::vector<int> val = prop->GetIntVector();
> 
> or
> 
>   std::vector<int> vec;
>   vec.push_back(1);
>   vec.push_back(2);
>   vec.push_back(3);
>   vec.push_back(4);
> 
>   GesturesProp* prop = provider->GetProperty(id, name);
>   prop->SetIntVector(vec);

Done.

https://codereview.chromium.org/545063006/diff/120001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:263: static
GesturesProp* CreateReal(void*,
On 2014/10/03 22:32:02, alexst wrote:
> Please add names to all the parameters

Done.

[spang, 2014-10-08 19:09:15]

This is looking much better. I do have a few more concerns.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:107: virtual
bool is_read_only() const = 0;
IsReadOnly()

(lower case with underscores is for trivial accessors, never virtuals)

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:270: virtual
bool is_allocated() const OVERRIDE { return is_allocated_; }
same here.. if it's virtual please use mixed case: IsAllocated()

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:742:
std::string path = GetDeviceNodePath(device);
Do we actually use the path for any matches?

I think depending on path is inherently racy - they are assigned in probe order
so the particular path assigned is not stable.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1069:
if(property)
whitespace: if ()

I use "git clang-format" to fix whitespace issues automatically.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:169:
ScopedDevicePropertyMap default_properties_maps_;
I think I managed to figure out what all the typedefs & maps are for. Maps
nested inside maps is very difficult to understand. This would be a lot clearer:

typedef base::hash_map<std::string, GesturesProp*> PropertiesMap;
typedef base::ScopedPtrHashMap<std::string, GesturesProp> ScopedPropertiesMap;

struct DevicePropertyData {

  // Owned properties.
  ScopedPropertiesMap properties;

  // Unowned default properties (owned by configuration).
  PropertiesMap default_properties;
};


You could then declare your shared map like this:

base::ScopedPtrHashMap<DeviceId, DevicePropertyData> device_data_;

However, do you need it? Can't we just add

  DevicePropertyData properties_;

to GestureInterpreterLibevdevCros, and pass that along? It doesn't seem like a
shared map is necessary.

Doing that would also fix the cleanup issue / leak.

[alexst (slow to review), 2014-10-08 20:21:47]

just a nit.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.cc:160: gesture_prop_provider_(new
GesturePropertyProvider),
please use full name here as well, gesture_property_provider_

[Shecky Lin, 2014-10-09 09:19:31]

Done the comments. Responses inline. Please take another look, thanks!

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.cc:160: gesture_prop_provider_(new
GesturePropertyProvider),
On 2014/10/08 20:21:47, alexst wrote:
> please use full name here as well, gesture_property_provider_

Done.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:107: virtual
bool is_read_only() const = 0;
On 2014/10/08 19:09:15, spang wrote:
> IsReadOnly()
> 
> (lower case with underscores is for trivial accessors, never virtuals)

Done.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:270: virtual
bool is_allocated() const OVERRIDE { return is_allocated_; }
On 2014/10/08 19:09:15, spang wrote:
> same here.. if it's virtual please use mixed case: IsAllocated()

Done.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:742:
std::string path = GetDeviceNodePath(device);
On 2014/10/08 19:09:15, spang wrote:
> Do we actually use the path for any matches?
> 
> I think depending on path is inherently racy - they are assigned in probe
order
> so the particular path assigned is not stable.

Yes, we do. If you search for "MatchDevicePath" under the xorg-conf directory in
the CrOS tree, you may probably see dozens of example. It is mainly used to rule
out malicious devices that are not actually input ones (e.g., whose device node
doesn't start with /dev/input/event*). We wouldn't specify a node index in the
conf file because just like you said, the index is dependent on the probe order
and isn't stable.

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1069:
if(property)
On 2014/10/08 19:09:15, spang wrote:
> whitespace: if ()
> 
> I use "git clang-format" to fix whitespace issues automatically.
Nice tip. Thanks!

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h (right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:169:
ScopedDevicePropertyMap default_properties_maps_;
On 2014/10/08 19:09:15, spang wrote:
> I think I managed to figure out what all the typedefs & maps are for. Maps
> nested inside maps is very difficult to understand. This would be a lot
clearer:
> 
> typedef base::hash_map<std::string, GesturesProp*> PropertiesMap;
> typedef base::ScopedPtrHashMap<std::string, GesturesProp> ScopedPropertiesMap;
> 
> struct DevicePropertyData {
> 
>   // Owned properties.
>   ScopedPropertiesMap properties;
> 
>   // Unowned default properties (owned by configuration).
>   PropertiesMap default_properties;
> };
> 
> 
> You could then declare your shared map like this:
> 
> base::ScopedPtrHashMap<DeviceId, DevicePropertyData> device_data_;
> 
> However, do you need it? Can't we just add
> 
>   DevicePropertyData properties_;
> 
> to GestureInterpreterLibevdevCros, and pass that along? It doesn't seem like a
> shared map is necessary.
> 
> Doing that would also fix the cleanup issue / leak.

I have combined both maps to a struct per your suggestion. Note that I have to
forward declare it and moving its definition to the source file because
otherwise ScopedPtrHashMap will require me to expose GesturesProp's destructor,
which will complicate the header even more.

I tend not to put the properties in GestureInterpreterLibevdevCros for a few
reasons. 

1. Part of GesturePropertyProvider's function is to replace the xinput utility
in the non-X world. A typical usage of xinput (and the inputcontrol script) is
to get a list of device ids that match a device class (e.g., mouse, touchpad,
multi-touch mouse) and then change some property values for all of these
devices. For example, we get a list of all touchpads and change their pointer
sensitivity properties when the user changes the pointer sensitivity in the
settings page. Similar usage pattern arises during the touch log collection and
the gesture property setting page that we want to implement.

In the current design, one may simply call GetDeviceIdsByType to get a list of
device ids and then call GetProperty on each of them to access the property. If
we move the properties to GestureInterpreterLibevdevCros, someone will need to
maintain a list of all currently active GestureInterpreterLibevdevCros'es so
that we can check each of them when being requested. Although we can still track
them in GesturePropertyProvider, this kind of bi-directional reference just
seems more complicated to me (as compared to the current design using
map-of-struct).

2. Access control.
The idea of GesturePropertyProvider is to hide implementation details such as
the GesturesProp object as much as possible. The GetProperty function (returning
GesturesProp objects) kind of breaks the rule, but it might still be acceptable
given that we open only limited interface for the GesturesProp class and that it
does dedup the code quite a bit. However, if we put the properties in
GestureInterpreterLibevdevCros directly, there won't be any protection to
prevent innocent developers from adding/deleting/replacing the properties to
their own wish. The GesturesProp objects are linked to the data in the gesture
library and should only be created/deleted from the gesture library side.
Putting the properties in GestureInterpreterLibevdevCros and expose it seems
like to say one may modify the properties as much as they want. 

Besides, even if we put properties there, we still need to call out to
GesturePropertyProvider for all related tasks (e.g., to setup the default
properties). It also seems weird to me to have a member in a class that one
needs to rely on another class every time he/she wants to interact with it.

Anyway, I will let you decide on it. If you want to put properties in
GestureInterpreterLibevdevCros, I will just change it accordingly.

BTW, I think we don't have any cleanup issue now after we scoped every pointer
we allocate (correct me if I am wrong ...).

[spang, 2014-10-09 16:57:19]

On 2014/10/09 09:19:31, Shecky Lin wrote:
> Done the comments. Responses inline. Please take another look, thanks!
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
> File ui/events/ozone/evdev/event_factory_evdev.cc (right):
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
> ui/events/ozone/evdev/event_factory_evdev.cc:160: gesture_prop_provider_(new
> GesturePropertyProvider),
> On 2014/10/08 20:21:47, alexst wrote:
> > please use full name here as well, gesture_property_provider_
> 
> Done.
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> (right):
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:107:
virtual
> bool is_read_only() const = 0;
> On 2014/10/08 19:09:15, spang wrote:
> > IsReadOnly()
> > 
> > (lower case with underscores is for trivial accessors, never virtuals)
> 
> Done.
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:270:
virtual
> bool is_allocated() const OVERRIDE { return is_allocated_; }
> On 2014/10/08 19:09:15, spang wrote:
> > same here.. if it's virtual please use mixed case: IsAllocated()
> 
> Done.
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:742:
> std::string path = GetDeviceNodePath(device);
> On 2014/10/08 19:09:15, spang wrote:
> > Do we actually use the path for any matches?
> > 
> > I think depending on path is inherently racy - they are assigned in probe
> order
> > so the particular path assigned is not stable.
> 
> Yes, we do. If you search for "MatchDevicePath" under the xorg-conf directory
in
> the CrOS tree, you may probably see dozens of example. It is mainly used to
rule
> out malicious devices that are not actually input ones (e.g., whose device
node
> doesn't start with /dev/input/event*). We wouldn't specify a node index in the
> conf file because just like you said, the index is dependent on the probe
order
> and isn't stable.
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1069:
> if(property)
> On 2014/10/08 19:09:15, spang wrote:
> > whitespace: if ()
> > 
> > I use "git clang-format" to fix whitespace issues automatically.
> Nice tip. Thanks!
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h
(right):
> 
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:169:
> ScopedDevicePropertyMap default_properties_maps_;
> On 2014/10/08 19:09:15, spang wrote:
> > I think I managed to figure out what all the typedefs & maps are for. Maps
> > nested inside maps is very difficult to understand. This would be a lot
> clearer:
> > 
> > typedef base::hash_map<std::string, GesturesProp*> PropertiesMap;
> > typedef base::ScopedPtrHashMap<std::string, GesturesProp>
ScopedPropertiesMap;
> > 
> > struct DevicePropertyData {
> > 
> >   // Owned properties.
> >   ScopedPropertiesMap properties;
> > 
> >   // Unowned default properties (owned by configuration).
> >   PropertiesMap default_properties;
> > };
> > 
> > 
> > You could then declare your shared map like this:
> > 
> > base::ScopedPtrHashMap<DeviceId, DevicePropertyData> device_data_;
> > 
> > However, do you need it? Can't we just add
> > 
> >   DevicePropertyData properties_;
> > 
> > to GestureInterpreterLibevdevCros, and pass that along? It doesn't seem like
a
> > shared map is necessary.
> > 
> > Doing that would also fix the cleanup issue / leak.
> 
> I have combined both maps to a struct per your suggestion. Note that I have to
> forward declare it and moving its definition to the source file because
> otherwise ScopedPtrHashMap will require me to expose GesturesProp's
destructor,
> which will complicate the header even more.
> 
> I tend not to put the properties in GestureInterpreterLibevdevCros for a few
> reasons. 
> 
> 1. Part of GesturePropertyProvider's function is to replace the xinput utility
> in the non-X world. A typical usage of xinput (and the inputcontrol script) is
> to get a list of device ids that match a device class (e.g., mouse, touchpad,
> multi-touch mouse) and then change some property values for all of these
> devices. For example, we get a list of all touchpads and change their pointer
> sensitivity properties when the user changes the pointer sensitivity in the
> settings page. Similar usage pattern arises during the touch log collection
and
> the gesture property setting page that we want to implement.
> 
> In the current design, one may simply call GetDeviceIdsByType to get a list of
> device ids and then call GetProperty on each of them to access the property.
If
> we move the properties to GestureInterpreterLibevdevCros, someone will need to
> maintain a list of all currently active GestureInterpreterLibevdevCros'es so
> that we can check each of them when being requested. Although we can still
track
> them in GesturePropertyProvider, this kind of bi-directional reference just
> seems more complicated to me (as compared to the current design using
> map-of-struct).
> 
> 2. Access control.
> The idea of GesturePropertyProvider is to hide implementation details such as
> the GesturesProp object as much as possible. The GetProperty function
(returning
> GesturesProp objects) kind of breaks the rule, but it might still be
acceptable
> given that we open only limited interface for the GesturesProp class and that
it
> does dedup the code quite a bit. However, if we put the properties in
> GestureInterpreterLibevdevCros directly, there won't be any protection to
> prevent innocent developers from adding/deleting/replacing the properties to
> their own wish. The GesturesProp objects are linked to the data in the gesture
> library and should only be created/deleted from the gesture library side.
> Putting the properties in GestureInterpreterLibevdevCros and expose it seems
> like to say one may modify the properties as much as they want. 
> 
> Besides, even if we put properties there, we still need to call out to
> GesturePropertyProvider for all related tasks (e.g., to setup the default
> properties). It also seems weird to me to have a member in a class that one
> needs to rely on another class every time he/she wants to interact with it.
> 
> Anyway, I will let you decide on it. If you want to put properties in
> GestureInterpreterLibevdevCros, I will just change it accordingly.
> 
> BTW, I think we don't have any cleanup issue now after we scoped every pointer
> we allocate (correct me if I am wrong ...).

Okay, how about just wrapping the pre-device data in a new object (e.g.
DevicePropertyData) for now. I think we might nighto move the map later

The cleanup issue I was referring to was your TODO:

  // TODO(sheckylin): Replace this id generation scheme with a better one.
  // The current way may result in memory leaks.

Can you allocate the property data strictly instead of lazily, and make sure to
clean it up properly when the GestureInterpreterLibevdevCros is destroyed?

[spang, 2014-10-09 17:00:06]

On 2014/10/09 16:57:19, spang wrote:
> On 2014/10/09 09:19:31, Shecky Lin wrote:
> > Done the comments. Responses inline. Please take another look, thanks!
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
> > File ui/events/ozone/evdev/event_factory_evdev.cc (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
> > ui/events/ozone/evdev/event_factory_evdev.cc:160: gesture_prop_provider_(new
> > GesturePropertyProvider),
> > On 2014/10/08 20:21:47, alexst wrote:
> > > please use full name here as well, gesture_property_provider_
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:107:
> virtual
> > bool is_read_only() const = 0;
> > On 2014/10/08 19:09:15, spang wrote:
> > > IsReadOnly()
> > > 
> > > (lower case with underscores is for trivial accessors, never virtuals)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:270:
> virtual
> > bool is_allocated() const OVERRIDE { return is_allocated_; }
> > On 2014/10/08 19:09:15, spang wrote:
> > > same here.. if it's virtual please use mixed case: IsAllocated()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:742:
> > std::string path = GetDeviceNodePath(device);
> > On 2014/10/08 19:09:15, spang wrote:
> > > Do we actually use the path for any matches?
> > > 
> > > I think depending on path is inherently racy - they are assigned in probe
> > order
> > > so the particular path assigned is not stable.
> > 
> > Yes, we do. If you search for "MatchDevicePath" under the xorg-conf
directory
> in
> > the CrOS tree, you may probably see dozens of example. It is mainly used to
> rule
> > out malicious devices that are not actually input ones (e.g., whose device
> node
> > doesn't start with /dev/input/event*). We wouldn't specify a node index in
the
> > conf file because just like you said, the index is dependent on the probe
> order
> > and isn't stable.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1069:
> > if(property)
> > On 2014/10/08 19:09:15, spang wrote:
> > > whitespace: if ()
> > > 
> > > I use "git clang-format" to fix whitespace issues automatically.
> > Nice tip. Thanks!
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h
> (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:169:
> > ScopedDevicePropertyMap default_properties_maps_;
> > On 2014/10/08 19:09:15, spang wrote:
> > > I think I managed to figure out what all the typedefs & maps are for. Maps
> > > nested inside maps is very difficult to understand. This would be a lot
> > clearer:
> > > 
> > > typedef base::hash_map<std::string, GesturesProp*> PropertiesMap;
> > > typedef base::ScopedPtrHashMap<std::string, GesturesProp>
> ScopedPropertiesMap;
> > > 
> > > struct DevicePropertyData {
> > > 
> > >   // Owned properties.
> > >   ScopedPropertiesMap properties;
> > > 
> > >   // Unowned default properties (owned by configuration).
> > >   PropertiesMap default_properties;
> > > };
> > > 
> > > 
> > > You could then declare your shared map like this:
> > > 
> > > base::ScopedPtrHashMap<DeviceId, DevicePropertyData> device_data_;
> > > 
> > > However, do you need it? Can't we just add
> > > 
> > >   DevicePropertyData properties_;
> > > 
> > > to GestureInterpreterLibevdevCros, and pass that along? It doesn't seem
like
> a
> > > shared map is necessary.
> > > 
> > > Doing that would also fix the cleanup issue / leak.
> > 
> > I have combined both maps to a struct per your suggestion. Note that I have
to
> > forward declare it and moving its definition to the source file because
> > otherwise ScopedPtrHashMap will require me to expose GesturesProp's
> destructor,
> > which will complicate the header even more.
> > 
> > I tend not to put the properties in GestureInterpreterLibevdevCros for a few
> > reasons. 
> > 
> > 1. Part of GesturePropertyProvider's function is to replace the xinput
utility
> > in the non-X world. A typical usage of xinput (and the inputcontrol script)
is
> > to get a list of device ids that match a device class (e.g., mouse,
touchpad,
> > multi-touch mouse) and then change some property values for all of these
> > devices. For example, we get a list of all touchpads and change their
pointer
> > sensitivity properties when the user changes the pointer sensitivity in the
> > settings page. Similar usage pattern arises during the touch log collection
> and
> > the gesture property setting page that we want to implement.
> > 
> > In the current design, one may simply call GetDeviceIdsByType to get a list
of
> > device ids and then call GetProperty on each of them to access the property.
> If
> > we move the properties to GestureInterpreterLibevdevCros, someone will need
to
> > maintain a list of all currently active GestureInterpreterLibevdevCros'es so
> > that we can check each of them when being requested. Although we can still
> track
> > them in GesturePropertyProvider, this kind of bi-directional reference just
> > seems more complicated to me (as compared to the current design using
> > map-of-struct).
> > 
> > 2. Access control.
> > The idea of GesturePropertyProvider is to hide implementation details such
as
> > the GesturesProp object as much as possible. The GetProperty function
> (returning
> > GesturesProp objects) kind of breaks the rule, but it might still be
> acceptable
> > given that we open only limited interface for the GesturesProp class and
that
> it
> > does dedup the code quite a bit. However, if we put the properties in
> > GestureInterpreterLibevdevCros directly, there won't be any protection to
> > prevent innocent developers from adding/deleting/replacing the properties to
> > their own wish. The GesturesProp objects are linked to the data in the
gesture
> > library and should only be created/deleted from the gesture library side.
> > Putting the properties in GestureInterpreterLibevdevCros and expose it seems
> > like to say one may modify the properties as much as they want. 
> > 
> > Besides, even if we put properties there, we still need to call out to
> > GesturePropertyProvider for all related tasks (e.g., to setup the default
> > properties). It also seems weird to me to have a member in a class that one
> > needs to rely on another class every time he/she wants to interact with it.
> > 
> > Anyway, I will let you decide on it. If you want to put properties in
> > GestureInterpreterLibevdevCros, I will just change it accordingly.
> > 
> > BTW, I think we don't have any cleanup issue now after we scoped every
pointer
> > we allocate (correct me if I am wrong ...).
> 
> Okay, how about just wrapping the pre-device data in a new object (e.g.
> DevicePropertyData) for now. I think we might nighto move the map later
> 

.. might need to move the map of devices by type later (to make it not
gestures-specific). Possibly soon, when we implement the settings page.

> The cleanup issue I was referring to was your TODO:
> 
>   // TODO(sheckylin): Replace this id generation scheme with a better one.
>   // The current way may result in memory leaks.
> 
> Can you allocate the property data strictly instead of lazily, and make sure
to
> clean it up properly when the GestureInterpreterLibevdevCros is destroyed?

[spang, 2014-10-09 17:06:47]

On 2014/10/09 17:00:06, spang wrote:
> On 2014/10/09 16:57:19, spang wrote:
> > On 2014/10/09 09:19:31, Shecky Lin wrote:
> > > Done the comments. Responses inline. Please take another look, thanks!
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
> > > File ui/events/ozone/evdev/event_factory_evdev.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/e...
> > > ui/events/ozone/evdev/event_factory_evdev.cc:160:
gesture_prop_provider_(new
> > > GesturePropertyProvider),
> > > On 2014/10/08 20:21:47, alexst wrote:
> > > > please use full name here as well, gesture_property_provider_
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:107:
> > virtual
> > > bool is_read_only() const = 0;
> > > On 2014/10/08 19:09:15, spang wrote:
> > > > IsReadOnly()
> > > > 
> > > > (lower case with underscores is for trivial accessors, never virtuals)
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:270:
> > virtual
> > > bool is_allocated() const OVERRIDE { return is_allocated_; }
> > > On 2014/10/08 19:09:15, spang wrote:
> > > > same here.. if it's virtual please use mixed case: IsAllocated()
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:742:
> > > std::string path = GetDeviceNodePath(device);
> > > On 2014/10/08 19:09:15, spang wrote:
> > > > Do we actually use the path for any matches?
> > > > 
> > > > I think depending on path is inherently racy - they are assigned in
probe
> > > order
> > > > so the particular path assigned is not stable.
> > > 
> > > Yes, we do. If you search for "MatchDevicePath" under the xorg-conf
> directory
> > in
> > > the CrOS tree, you may probably see dozens of example. It is mainly used
to
> > rule
> > > out malicious devices that are not actually input ones (e.g., whose device
> > node
> > > doesn't start with /dev/input/event*). We wouldn't specify a node index in
> the
> > > conf file because just like you said, the index is dependent on the probe
> > order
> > > and isn't stable.
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:1069:
> > > if(property)
> > > On 2014/10/08 19:09:15, spang wrote:
> > > > whitespace: if ()
> > > > 
> > > > I use "git clang-format" to fix whitespace issues automatically.
> > > Nice tip. Thanks!
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
> > > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.h:169:
> > > ScopedDevicePropertyMap default_properties_maps_;
> > > On 2014/10/08 19:09:15, spang wrote:
> > > > I think I managed to figure out what all the typedefs & maps are for.
Maps
> > > > nested inside maps is very difficult to understand. This would be a lot
> > > clearer:
> > > > 
> > > > typedef base::hash_map<std::string, GesturesProp*> PropertiesMap;
> > > > typedef base::ScopedPtrHashMap<std::string, GesturesProp>
> > ScopedPropertiesMap;
> > > > 
> > > > struct DevicePropertyData {
> > > > 
> > > >   // Owned properties.
> > > >   ScopedPropertiesMap properties;
> > > > 
> > > >   // Unowned default properties (owned by configuration).
> > > >   PropertiesMap default_properties;
> > > > };
> > > > 
> > > > 
> > > > You could then declare your shared map like this:
> > > > 
> > > > base::ScopedPtrHashMap<DeviceId, DevicePropertyData> device_data_;
> > > > 
> > > > However, do you need it? Can't we just add
> > > > 
> > > >   DevicePropertyData properties_;
> > > > 
> > > > to GestureInterpreterLibevdevCros, and pass that along? It doesn't seem
> like
> > a
> > > > shared map is necessary.
> > > > 
> > > > Doing that would also fix the cleanup issue / leak.
> > > 
> > > I have combined both maps to a struct per your suggestion. Note that I
have
> to
> > > forward declare it and moving its definition to the source file because
> > > otherwise ScopedPtrHashMap will require me to expose GesturesProp's
> > destructor,
> > > which will complicate the header even more.
> > > 
> > > I tend not to put the properties in GestureInterpreterLibevdevCros for a
few
> > > reasons. 
> > > 
> > > 1. Part of GesturePropertyProvider's function is to replace the xinput
> utility
> > > in the non-X world. A typical usage of xinput (and the inputcontrol
script)
> is
> > > to get a list of device ids that match a device class (e.g., mouse,
> touchpad,
> > > multi-touch mouse) and then change some property values for all of these
> > > devices. For example, we get a list of all touchpads and change their
> pointer
> > > sensitivity properties when the user changes the pointer sensitivity in
the
> > > settings page. Similar usage pattern arises during the touch log
collection
> > and
> > > the gesture property setting page that we want to implement.
> > > 
> > > In the current design, one may simply call GetDeviceIdsByType to get a
list
> of
> > > device ids and then call GetProperty on each of them to access the
property.
> > If
> > > we move the properties to GestureInterpreterLibevdevCros, someone will
need
> to
> > > maintain a list of all currently active GestureInterpreterLibevdevCros'es
so
> > > that we can check each of them when being requested. Although we can still
> > track
> > > them in GesturePropertyProvider, this kind of bi-directional reference
just
> > > seems more complicated to me (as compared to the current design using
> > > map-of-struct).
> > > 
> > > 2. Access control.
> > > The idea of GesturePropertyProvider is to hide implementation details such
> as
> > > the GesturesProp object as much as possible. The GetProperty function
> > (returning
> > > GesturesProp objects) kind of breaks the rule, but it might still be
> > acceptable
> > > given that we open only limited interface for the GesturesProp class and
> that
> > it
> > > does dedup the code quite a bit. However, if we put the properties in
> > > GestureInterpreterLibevdevCros directly, there won't be any protection to
> > > prevent innocent developers from adding/deleting/replacing the properties
to
> > > their own wish. The GesturesProp objects are linked to the data in the
> gesture
> > > library and should only be created/deleted from the gesture library side.
> > > Putting the properties in GestureInterpreterLibevdevCros and expose it
seems
> > > like to say one may modify the properties as much as they want. 
> > > 
> > > Besides, even if we put properties there, we still need to call out to
> > > GesturePropertyProvider for all related tasks (e.g., to setup the default
> > > properties). It also seems weird to me to have a member in a class that
one
> > > needs to rely on another class every time he/she wants to interact with
it.
> > > 
> > > Anyway, I will let you decide on it. If you want to put properties in
> > > GestureInterpreterLibevdevCros, I will just change it accordingly.
> > > 
> > > BTW, I think we don't have any cleanup issue now after we scoped every
> pointer
> > > we allocate (correct me if I am wrong ...).
> > 
> > Okay, how about just wrapping the pre-device data in a new object (e.g.
> > DevicePropertyData) for now. I think we might nighto move the map later
> > 
> 
> .. might need to move the map of devices by type later (to make it not
> gestures-specific). Possibly soon, when we implement the settings page.
> 
> > The cleanup issue I was referring to was your TODO:
> > 
> >   // TODO(sheckylin): Replace this id generation scheme with a better one.
> >   // The current way may result in memory leaks.
> > 
> > Can you allocate the property data strictly instead of lazily, and make sure
> to
> > clean it up properly when the GestureInterpreterLibevdevCros is destroyed?

Oh, you already moved it to the struct. Thanks.

Can you just fix it so its created & destroyed at the right time - not lazily
inside GetDeviceId. After that I think this is in good enough shape to land, and
we can iterate from there.

[spang, 2014-10-09 17:20:48]

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/140001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:742:
std::string path = GetDeviceNodePath(device);
On 2014/10/09 09:19:31, Shecky Lin wrote:
> On 2014/10/08 19:09:15, spang wrote:
> > Do we actually use the path for any matches?
> > 
> > I think depending on path is inherently racy - they are assigned in probe
> order
> > so the particular path assigned is not stable.
> 
> Yes, we do. If you search for "MatchDevicePath" under the xorg-conf directory
in
> the CrOS tree, you may probably see dozens of example. It is mainly used to
rule
> out malicious devices that are not actually input ones (e.g., whose device
node
> doesn't start with /dev/input/event*). We wouldn't specify a node index in the
> conf file because just like you said, the index is dependent on the probe
order
> and isn't stable.

Ah.. ok. We only open devices that match /dev/input/event* in the first place,
so if that's the only match used then it doesn't make any difference in our
case.

[Shecky Lin, 2014-10-13 09:12:55]

Done fixing the memory leak. Please take another look. Thanks!

[Shecky Lin, 2014-10-14 07:16:20]

I've re-based the code but unfortunately we now reached the number limit of
template parameters for base::Bind in EventFactoryEvdev::OnDeviceEvent when
posting tasks of OpenInputDevice. OpenInputDevice now takes 8 parameters while
base::Bind allows up to 7. 

Michael, could you suggest what I should do here? I am tempted to wrap object
pointers (e.g., EventModifiersEvdev, CursorDelegateEvdev, etc.) in a struct to
save parameters but I feel that I may consult your opinion first.

BTW, I've noticed that dnicoara@ has recently implemented a simple device id
generation routine in EventFactoryEvdev. I think we should unify this with the
device ids used by GesturePropertyProvider for the obvious benefit. So if you
are fine with this, I would like to move my id generation code to
EventFactoryEvdev so that we can all use a same unique, non-negative device id
to identify input devices.

Let me know how you think about it. Thanks.

[spang, 2014-10-14 13:41:19]

On 2014/10/14 07:16:20, Shecky Lin wrote:
> I've re-based the code but unfortunately we now reached the number limit of
> template parameters for base::Bind in EventFactoryEvdev::OnDeviceEvent when
> posting tasks of OpenInputDevice. OpenInputDevice now takes 8 parameters while
> base::Bind allows up to 7. 
> 
> Michael, could you suggest what I should do here? I am tempted to wrap object
> pointers (e.g., EventModifiersEvdev, CursorDelegateEvdev, etc.) in a struct to
> save parameters but I feel that I may consult your opinion first.

I've actually already added a struct that in another patch. I'll land it so you
can rebase on top.

> 
> BTW, I've noticed that dnicoara@ has recently implemented a simple device id
> generation routine in EventFactoryEvdev. I think we should unify this with the
> device ids used by GesturePropertyProvider for the obvious benefit. So if you
> are fine with this, I would like to move my id generation code to
> EventFactoryEvdev so that we can all use a same unique, non-negative device id
> to identify input devices.
> 
> Let me know how you think about it. Thanks.

Yes, I thought tsame thing.

[Shecky Lin, 2014-10-15 09:28:30]

On 2014/10/14 13:41:19, spang wrote:
> On 2014/10/14 07:16:20, Shecky Lin wrote:
> > I've re-based the code but unfortunately we now reached the number limit of
> > template parameters for base::Bind in EventFactoryEvdev::OnDeviceEvent when
> > posting tasks of OpenInputDevice. OpenInputDevice now takes 8 parameters
while
> > base::Bind allows up to 7. 
> > 
> > Michael, could you suggest what I should do here? I am tempted to wrap
object
> > pointers (e.g., EventModifiersEvdev, CursorDelegateEvdev, etc.) in a struct
to
> > save parameters but I feel that I may consult your opinion first.
> 
> I've actually already added a struct that in another patch. I'll land it so
you
> can rebase on top.
> 
> > 
> > BTW, I've noticed that dnicoara@ has recently implemented a simple device id
> > generation routine in EventFactoryEvdev. I think we should unify this with
the
> > device ids used by GesturePropertyProvider for the obvious benefit. So if
you
> > are fine with this, I would like to move my id generation code to
> > EventFactoryEvdev so that we can all use a same unique, non-negative device
id
> > to identify input devices.
> > 
> > Let me know how you think about it. Thanks.
> 
> Yes, I thought tsame thing.

Both done. Could you take another look? Thanks.

[spang, 2014-10-15 11:36:42]

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.cc:310: id = (id + 1) & kMaxDeviceNum;
This is way overkill. If 2 billion devices over the lifetime of the browser
isn't enough, we can solve that by switching to int64_t. Realistically, I think
int is plenty - let's keep it simple.

So: All these changes to ID generation are unnecessary for this CL, please drop
them.

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.h (right):

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.h:84: DeviceIdMap device_ids_map_;
What is this for? I don't think you need it.

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:658: virtual
~MatchProduct(){};
whitespace. Run 

$ git clang-format HEAD^

[Shecky Lin, 2014-10-15 11:51:32]

On 2014/10/15 11:36:42, spang wrote:
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> File ui/events/ozone/evdev/event_factory_evdev.cc (right):
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> ui/events/ozone/evdev/event_factory_evdev.cc:310: id = (id + 1) &
kMaxDeviceNum;
> This is way overkill. If 2 billion devices over the lifetime of the browser
> isn't enough, we can solve that by switching to int64_t. Realistically, I
think
> int is plenty - let's keep it simple.
> 
> So: All these changes to ID generation are unnecessary for this CL, please
drop
> them.
I was just thinking that there could be some malicious devices that repetitively
attach/detach itself (e.g., due to a driver problem or something) and then we
may run out of ids quite soon. But you might be right, I will revert that part
of change.
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> File ui/events/ozone/evdev/event_factory_evdev.h (right):
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> ui/events/ozone/evdev/event_factory_evdev.h:84: DeviceIdMap device_ids_map_;
> What is this for? I don't think you need it.
> 
This is used to recycle id when a device is detached. Will remove it.
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> (right):
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:658:
virtual
> ~MatchProduct(){};
> whitespace. Run 
> 
> $ git clang-format HEAD^
Sorry, I thought I ran it before. Not sure how it comes back...

Will update the patch tomorrow. Let me know if you have any other comment.

[Shecky Lin, 2014-10-16 04:53:53]

Hey guys,

Could you take another look and see if this is good to land? Let's get it in
soon before it goes stale again. Thanks!

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.cc:310: id = (id + 1) & kMaxDeviceNum;
On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> This is way overkill. If 2 billion devices over the lifetime of the browser
> isn't enough, we can solve that by switching to int64_t. Realistically, I
think
> int is plenty - let's keep it simple.
> 
> So: All these changes to ID generation are unnecessary for this CL, please
drop
> them.

Done.

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory_evdev.h (right):

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory_evdev.h:84: DeviceIdMap device_ids_map_;
On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> What is this for? I don't think you need it.

Done.

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:658: virtual
~MatchProduct(){};
On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> whitespace. Run 
> 
> $ git clang-format HEAD^

I ran it again and it turns out this is what clang format wants (no space
between ")" and "{"). You can try it out yourself. I didn't change the code
thereby.

[spang, 2014-10-16 13:58:55]

On 2014/10/16 04:53:53, Shecky Lin wrote:
> Hey guys,
> 
> Could you take another look and see if this is good to land? Let's get it in
> soon before it goes stale again. Thanks!
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> File ui/events/ozone/evdev/event_factory_evdev.cc (right):
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> ui/events/ozone/evdev/event_factory_evdev.cc:310: id = (id + 1) &
kMaxDeviceNum;
> On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> > This is way overkill. If 2 billion devices over the lifetime of the browser
> > isn't enough, we can solve that by switching to int64_t. Realistically, I
> think
> > int is plenty - let's keep it simple.
> > 
> > So: All these changes to ID generation are unnecessary for this CL, please
> drop
> > them.
> 
> Done.
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> File ui/events/ozone/evdev/event_factory_evdev.h (right):
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> ui/events/ozone/evdev/event_factory_evdev.h:84: DeviceIdMap device_ids_map_;
> On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> > What is this for? I don't think you need it.
> 
> Done.
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> (right):
> 
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:658:
virtual
> ~MatchProduct(){};
> On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> > whitespace. Run 
> > 
> > $ git clang-format HEAD^
> 
> I ran it again and it turns out this is what clang format wants (no space
> between ")" and "{"). You can try it out yourself. I didn't change the code
> thereby.

Er, sorry, the right command is:

 git-clang-format --style=Chromium HEAD^

[alexst (slow to review), 2014-10-16 14:03:24]

git cl format does the trick too.

[Shecky Lin, 2014-10-17 04:26:03]

On 2014/10/16 13:58:55, spang (OOO Oct 15 - Oct 17) wrote:
> On 2014/10/16 04:53:53, Shecky Lin wrote:
> > Hey guys,
> > 
> > Could you take another look and see if this is good to land? Let's get it in
> > soon before it goes stale again. Thanks!
> > 
> >
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> > File ui/events/ozone/evdev/event_factory_evdev.cc (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> > ui/events/ozone/evdev/event_factory_evdev.cc:310: id = (id + 1) &
> kMaxDeviceNum;
> > On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> > > This is way overkill. If 2 billion devices over the lifetime of the
browser
> > > isn't enough, we can solve that by switching to int64_t. Realistically, I
> > think
> > > int is plenty - let's keep it simple.
> > > 
> > > So: All these changes to ID generation are unnecessary for this CL, please
> > drop
> > > them.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> > File ui/events/ozone/evdev/event_factory_evdev.h (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/e...
> > ui/events/ozone/evdev/event_factory_evdev.h:84: DeviceIdMap device_ids_map_;
> > On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> > > What is this for? I don't think you need it.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/330001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:658:
> virtual
> > ~MatchProduct(){};
> > On 2014/10/15 11:36:41, spang (OOO Oct 15 - Oct 17) wrote:
> > > whitespace. Run 
> > > 
> > > $ git clang-format HEAD^
> > 
> > I ran it again and it turns out this is what clang format wants (no space
> > between ")" and "{"). You can try it out yourself. I didn't change the code
> > thereby.
> 
> Er, sorry, the right command is:
> 
>  git-clang-format --style=Chromium HEAD^

Thanks. I tried both commands you guys provided. They do format the code
differently from the old one. However, they still discard the spaces between ")"
and "{". I again didn't change that part of code.

[Shecky Lin, 2014-10-20 06:42:02]

I've rebased the code again. Could you take a look? Thanks.

[alexst (slow to review), 2014-10-20 14:22:33]

lgtm, thank you!

[spang, 2014-10-20 14:32:57]

lgtm with a nit

please remove the extra semicolons that are causing clang-format to screw up the
whitespace before submitting.

https://codereview.chromium.org/545063006/diff/390001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
(right):

https://codereview.chromium.org/545063006/diff/390001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:647: virtual
~MatchCriteria(){};
Your problem is the semicolon. Remove it and add the space.

[spang, 2014-10-20 15:40:55]

On 2014/10/20 14:32:57, spang wrote:
> lgtm with a nit
> 
> please remove the extra semicolons that are causing clang-format to screw up
the
> whitespace before submitting.
> 
>
https://codereview.chromium.org/545063006/diff/390001/ui/events/ozone/evdev/l...
> File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> (right):
> 
>
https://codereview.chromium.org/545063006/diff/390001/ui/events/ozone/evdev/l...
> ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:647:
virtual
> ~MatchCriteria(){};
> Your problem is the semicolon. Remove it and add the space.

There's also some missing ifdefs (see the red try runs on latest patch).

[alexst (slow to review), 2014-10-20 20:36:41]

On 2014/10/20 15:40:55, spang wrote:
> On 2014/10/20 14:32:57, spang wrote:
> > lgtm with a nit
> > 
> > please remove the extra semicolons that are causing clang-format to screw up
> the
> > whitespace before submitting.
> > 
> >
>
https://codereview.chromium.org/545063006/diff/390001/ui/events/ozone/evdev/l...
> > File ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/545063006/diff/390001/ui/events/ozone/evdev/l...
> > ui/events/ozone/evdev/libgestures_glue/gesture_property_provider.cc:647:
> virtual
> > ~MatchCriteria(){};
> > Your problem is the semicolon. Remove it and add the space.
> 
> There's also some missing ifdefs (see the red try runs on latest patch).

And if you need try server access, you can get it here
http://www.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access

[Shecky Lin, 2014-10-21 04:56:52]

The CQ bit was checked by sheckylin@chromium.org

[commit-bot: I haz the power, 2014-10-21 04:57:59]

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

[commit-bot: I haz the power, 2014-10-21 05:39:14]

Committed patchset #16 (id:410001)

[commit-bot: I haz the power, 2014-10-21 05:40:04]

Patchset 16 (id:??) landed as
https://crrev.com/af98f0fa02a4cfc584291ea27aecda165bbdaddb
Cr-Commit-Position: refs/heads/master@{#300423}