Include external touchscreen vid/pid in UMA hardware profile

Include external touchscreen vid/pid in UMA hardware profile

Previously we looked at including the touchscreen name in the UMA
hardware profile: https://codereview.chromium.org/23619085/.

As that posed a privacy concern, this patch includes the vid/pid
instead.

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

[tdresser, 2013-12-12 17:42:18]

sadrul, can you take a look at ui/**?
isherman, can you take a look at chrome/**?

isherman, I assume this will also require going through a privacy review, and
getting approval from the logs collection team.

Should I start a thread with sumit@ for the privacy review?

[sadrul, 2013-12-12 18:22:19]

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
File ui/events/x/touch_factory_x11.cc (right):

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
ui/events/x/touch_factory_x11.cc:292:
touchscreen_ids_.insert(std::make_pair(ptr[0], ptr[1]));
How do you interpret these numbers?

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x11.h
File ui/events/x/touch_factory_x11.h (right):

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
ui/events/x/touch_factory_x11.h:107: static const int kDeviceProductIdAtom =
249;
Where does this number come from?

[tdresser, 2013-12-12 19:05:00]

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
File ui/events/x/touch_factory_x11.cc (right):

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
ui/events/x/touch_factory_x11.cc:292:
touchscreen_ids_.insert(std::make_pair(ptr[0], ptr[1]));
On 2013/12/12 18:22:19, sadrul wrote:
> How do you interpret these numbers?

Comment added.

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x11.h
File ui/events/x/touch_factory_x11.h (right):

https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
ui/events/x/touch_factory_x11.h:107: static const int kDeviceProductIdAtom =
249;
On 2013/12/12 18:22:19, sadrul wrote:
> Where does this number come from?

I've switched to a magic string instead of a magic number, which should be more
robust.

I haven't found any documentation which states that "Device Product ID" is
standard though...

[Yufeng Shen (Slow to review), 2013-12-12 19:14:41]

On 2013/12/12 19:05:00, tdresser wrote:
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
> File ui/events/x/touch_factory_x11.cc (right):
> 
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
> ui/events/x/touch_factory_x11.cc:292:
> touchscreen_ids_.insert(std::make_pair(ptr[0], ptr[1]));
> On 2013/12/12 18:22:19, sadrul wrote:
> > How do you interpret these numbers?
> 
> Comment added.
> 
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x11.h
> File ui/events/x/touch_factory_x11.h (right):
> 
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
> ui/events/x/touch_factory_x11.h:107: static const int kDeviceProductIdAtom =
> 249;
> On 2013/12/12 18:22:19, sadrul wrote:
> > Where does this number come from?
> 
> I've switched to a magic string instead of a magic number, which should be
more
> robust.
> 
> I haven't found any documentation which states that "Device Product ID" is
> standard though...

check this out
http://who-t.blogspot.ca/2011/06/linking-kernel-and-x-devices.html
http://code.metager.de/source/s?refs=XI_PROP_PRODUCT_ID&project=xorg

it seems the common practice is to use
#ifndef XI_PROP_PRODUCT_ID
#define XI_PROP_PRODUCT_ID "Device Product ID"
#endif

[tdresser, 2013-12-12 19:24:19]

On 2013/12/12 19:14:41, Yufeng Shen wrote:
> On 2013/12/12 19:05:00, tdresser wrote:
> >
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
> > File ui/events/x/touch_factory_x11.cc (right):
> > 
> >
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
> > ui/events/x/touch_factory_x11.cc:292:
> > touchscreen_ids_.insert(std::make_pair(ptr[0], ptr[1]));
> > On 2013/12/12 18:22:19, sadrul wrote:
> > > How do you interpret these numbers?
> > 
> > Comment added.
> > 
> >
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x11.h
> > File ui/events/x/touch_factory_x11.h (right):
> > 
> >
>
https://codereview.chromium.org/103893005/diff/1/ui/events/x/touch_factory_x1...
> > ui/events/x/touch_factory_x11.h:107: static const int kDeviceProductIdAtom =
> > 249;
> > On 2013/12/12 18:22:19, sadrul wrote:
> > > Where does this number come from?
> > 
> > I've switched to a magic string instead of a magic number, which should be
> more
> > robust.
> > 
> > I haven't found any documentation which states that "Device Product ID" is
> > standard though...
> 
> check this out
> http://who-t.blogspot.ca/2011/06/linking-kernel-and-x-devices.html
> http://code.metager.de/source/s?refs=XI_PROP_PRODUCT_ID&project=xorg
> 
> it seems the common practice is to use
> #ifndef XI_PROP_PRODUCT_ID
> #define XI_PROP_PRODUCT_ID "Device Product ID"
> #endif

The style guide prefers constant variables to macros though, so I think what
I've got here is probably preferable.

[sadrul, 2013-12-12 19:28:58]

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
File ui/events/x/touch_factory_x11.cc (right):

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:11: #include <X11/Xatom.h>
sort

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:139: touch_device_lookup_[devinfo->deviceid] =
true;
It should be in here, I think?

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:276: void
TouchFactory::CacheTouchscreenIds(Display* display, int id) {
Call this XID device_id

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:283: 
define kDeviceProductIdString in here.

(For X11 atoms, the atom name is the proper identifer, not the int-id (e.g. it's
257 on my workstation, 250 on my laptop etc.))

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
File ui/events/x/touch_factory_x11.h (right):

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.h:76: const std::set<std::pair<int, int> >
GetTouchscreenIds() const {
&

[tdresser, 2013-12-12 21:24:22]

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
File ui/events/x/touch_factory_x11.cc (right):

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:11: #include <X11/Xatom.h>
On 2013/12/12 19:28:59, sadrul wrote:
> sort

Done.

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:139: touch_device_lookup_[devinfo->deviceid] =
true;
On 2013/12/12 19:28:59, sadrul wrote:
> It should be in here, I think?

Done.

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:276: void
TouchFactory::CacheTouchscreenIds(Display* display, int id) {
On 2013/12/12 19:28:59, sadrul wrote:
> Call this XID device_id

Done.

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.cc:283: 
On 2013/12/12 19:28:59, sadrul wrote:
> define kDeviceProductIdString in here.
> 
> (For X11 atoms, the atom name is the proper identifer, not the int-id (e.g.
it's
> 257 on my workstation, 250 on my laptop etc.))

Done.

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
File ui/events/x/touch_factory_x11.h (right):

https://codereview.chromium.org/103893005/diff/20001/ui/events/x/touch_factor...
ui/events/x/touch_factory_x11.h:76: const std::set<std::pair<int, int> >
GetTouchscreenIds() const {
On 2013/12/12 19:28:59, sadrul wrote:
> &

Done.

[Ilya Sherman, 2013-12-12 22:49:43]

You'll need to commit the .proto changes to google3 prior to landing this CL. 
Feel free to ping me or the uma-team@ if you're not sure how to go about doing
that.

https://codereview.chromium.org/103893005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/103893005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:392: std::set<std::pair<int, int>
>::iterator it;
nit: Please declare this within the scope of the for loop rather than outside of
it.  (Also, I'm really looking forward to the days of C++11 and range-based for
loops...)

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:223: optional bool
is_touchscreen_present = 14;
Please add documentation for all fields.  For very clearly named fields like
this one, brief documentation suffices.

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:223: optional bool
is_touchscreen_present = 14;
Why is this needed given external_touchscreen below?  Can't you just check the
length of the repeated field?

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:225: // vids and pids of
external touchscreens.
nit: Please spell out these acronyms.

[tdresser, 2013-12-13 15:29:00]

Thanks, I'll take a look at making the google3 change.

https://codereview.chromium.org/103893005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/103893005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:392: std::set<std::pair<int, int>
>::iterator it;
On 2013/12/12 22:49:43, Ilya Sherman wrote:
> nit: Please declare this within the scope of the for loop rather than outside
of
> it.  (Also, I'm really looking forward to the days of C++11 and range-based
for
> loops...)

Done.

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:223: optional bool
is_touchscreen_present = 14;
On 2013/12/12 22:49:43, Ilya Sherman wrote:
> Why is this needed given external_touchscreen below?  Can't you just check the
> length of the repeated field?

is_touchscreen_present includes internal touchscreens, which don't have usb
vendor or product ids.

I've made this clearer and reduced the redundancy by replacing
|is_touchscreen_present| with |internal_display_supports_touch|.

I've added documentation for this field.

https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
chrome/common/metrics/proto/system_profile.proto:225: // vids and pids of
external touchscreens.
On 2013/12/12 22:49:43, Ilya Sherman wrote:
> nit: Please spell out these acronyms.

Done.

[tdresser, 2013-12-13 15:54:15]

On 2013/12/13 15:29:00, tdresser wrote:
> Thanks, I'll take a look at making the google3 change.
> 
>
https://codereview.chromium.org/103893005/diff/40001/chrome/browser/metrics/m...
> File chrome/browser/metrics/metrics_log.cc (right):
> 
>
https://codereview.chromium.org/103893005/diff/40001/chrome/browser/metrics/m...
> chrome/browser/metrics/metrics_log.cc:392: std::set<std::pair<int, int>
> >::iterator it;
> On 2013/12/12 22:49:43, Ilya Sherman wrote:
> > nit: Please declare this within the scope of the for loop rather than
outside
> of
> > it.  (Also, I'm really looking forward to the days of C++11 and range-based
> for
> > loops...)
> 
> Done.
> 
>
https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
> File chrome/common/metrics/proto/system_profile.proto (right):
> 
>
https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
> chrome/common/metrics/proto/system_profile.proto:223: optional bool
> is_touchscreen_present = 14;
> On 2013/12/12 22:49:43, Ilya Sherman wrote:
> > Why is this needed given external_touchscreen below?  Can't you just check
the
> > length of the repeated field?
> 
> is_touchscreen_present includes internal touchscreens, which don't have usb
> vendor or product ids.
> 
> I've made this clearer and reduced the redundancy by replacing
> |is_touchscreen_present| with |internal_display_supports_touch|.
> 
> I've added documentation for this field.
> 
>
https://codereview.chromium.org/103893005/diff/40001/chrome/common/metrics/pr...
> chrome/common/metrics/proto/system_profile.proto:225: // vids and pids of
> external touchscreens.
> On 2013/12/12 22:49:43, Ilya Sherman wrote:
> > nit: Please spell out these acronyms.
> 
> Done.

rbyers pointed out that there are better ways to determine if the internal
screen supports touch or not. I'll upload a better patch shortly.

[Ilya Sherman, 2013-12-14 00:12:12]

Thanks, LG.  Happy to officially approve once the google3 CL lands.

https://codereview.chromium.org/103893005/diff/100001/chrome/common/metrics/p...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/103893005/diff/100001/chrome/common/metrics/p...
chrome/common/metrics/proto/system_profile.proto:226: // vendor ids and product
ids of external touchscreens.
nit: Please start the comment with a capital letter.

[tdresser, 2014-01-06 15:44:45]

https://codereview.chromium.org/103893005/diff/100001/chrome/common/metrics/p...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/103893005/diff/100001/chrome/common/metrics/p...
chrome/common/metrics/proto/system_profile.proto:226: // vendor ids and product
ids of external touchscreens.
On 2013/12/14 00:12:12, Ilya Sherman (Away thru Jan 6) wrote:
> nit: Please start the comment with a capital letter.

Done.

[tdresser, 2014-01-08 19:36:06]

On 2014/01/06 15:44:45, tdresser wrote:
>
https://codereview.chromium.org/103893005/diff/100001/chrome/common/metrics/p...
> File chrome/common/metrics/proto/system_profile.proto (right):
> 
>
https://codereview.chromium.org/103893005/diff/100001/chrome/common/metrics/p...
> chrome/common/metrics/proto/system_profile.proto:226: // vendor ids and
product
> ids of external touchscreens.
> On 2013/12/14 00:12:12, Ilya Sherman (Away thru Jan 6) wrote:
> > nit: Please start the comment with a capital letter.
> 
> Done.

google3 CL landed: https://critique.corp.google.com/#review/59273564

[Ilya Sherman, 2014-01-08 21:45:32]

Thanks!  Metrics changes LGTM with a nit:

https://codereview.chromium.org/103893005/diff/110001/chrome/common/metrics/p...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/103893005/diff/110001/chrome/common/metrics/p...
chrome/common/metrics/proto/system_profile.proto:229: optional uint32 product_id
= 2;
nit: Please update these to be commented, to match the google3 version.  In
general, make sure the edits exactly match what landed in google3.

[sadrul, 2014-01-09 13:04:24]

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
File ui/events/x/touch_factory_x11.cc (right):

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
ui/events/x/touch_factory_x11.cc:282: static const char* kDeviceProductIdString
= "Device Product ID";
(non-static) const char ...[]

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
ui/events/x/touch_factory_x11.cc:286: if (XGetDeviceProperty(display, device,
device_product_id_atom, 0, 2,
Do you want to check device_product_id_atom != None first?

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
ui/events/x/touch_factory_x11.cc:293: long* ptr =
reinterpret_cast<long*>(prop_return);
Do you need to verify the expected values of actual_type_return,
actual_format_return, and nitems_return, before trying to read from prop_return?

[sadrul, 2014-01-10 16:57:28]

LGTM

[tdresser, 2014-01-10 16:59:03]

https://codereview.chromium.org/103893005/diff/110001/chrome/common/metrics/p...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/103893005/diff/110001/chrome/common/metrics/p...
chrome/common/metrics/proto/system_profile.proto:229: optional uint32 product_id
= 2;
On 2014/01/08 21:45:33, Ilya Sherman (Away Jan11-Feb2) wrote:
> nit: Please update these to be commented, to match the google3 version.  In
> general, make sure the edits exactly match what landed in google3.

Done.

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
File ui/events/x/touch_factory_x11.cc (right):

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
ui/events/x/touch_factory_x11.cc:282: static const char* kDeviceProductIdString
= "Device Product ID";
On 2014/01/09 13:04:25, sadrul wrote:
> (non-static) const char ...[]

Done.

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
ui/events/x/touch_factory_x11.cc:286: if (XGetDeviceProperty(display, device,
device_product_id_atom, 0, 2,
On 2014/01/09 13:04:25, sadrul wrote:
> Do you want to check device_product_id_atom != None first?

Done.

https://codereview.chromium.org/103893005/diff/110001/ui/events/x/touch_facto...
ui/events/x/touch_factory_x11.cc:293: long* ptr =
reinterpret_cast<long*>(prop_return);
On 2014/01/09 13:04:25, sadrul wrote:
> Do you need to verify the expected values of actual_type_return,
> actual_format_return, and nitems_return, before trying to read from
prop_return?

Done.

[commit-bot: I haz the power, 2014-01-10 17:04:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/103893005/230001

[commit-bot: I haz the power, 2014-01-10 18:53:52]

Change committed as 244210