Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
134773004:
Include external touchscreen vid/pid in UMA hardware profile (Closed)
|
Created:
6 years, 11 months ago by tdresser Modified:
6 years, 10 months ago Reviewers:
sadrul, jar (doing other things), jar1, oshima, Alexei Svitkine (slow), Ilya Sherman, battre CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInclude external touchscreen vid/pid in UMA hardware profile
Relanding https://codereview.chromium.org/103893005 after it was
reverted in https://codereview.chromium.org/135763002. When this
was first landed, it would try to read information on the
displays after ash::Shell had been destroyed, which caused
issues. This patch checks if the ash::Shell exists before
reading information on the displays.
This patch also fixes a bug in the previous patch in which we'd
log that the primary display was not a touch screen if we failed
to find the native screen.
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=249617
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address oshima's comments. #
Total comments: 2
Patch Set 3 : Method rename. #
Total comments: 4
Patch Set 4 : Include <utility> #
Total comments: 4
Patch Set 5 : Rebase onto refactor. #
Total comments: 4
Patch Set 6 : Address avitkine's comments. #
Total comments: 6
Patch Set 7 : Remove unused attribute from TouchFactory. #Patch Set 8 : Don't assume a screen exists - it doesn't in some tests. #
Messages
Total messages: 37 (0 generated)
oshima, does this look like a reasonable solution?
On second thought, i think it's better to support screen during shutdown because this is not the first time I saw the issue and better to fix for the rest. I have a fix and will upload soon, but no worry you can keep the most of the code the same. https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc File ui/events/event_utils.cc (right): https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... ui/events/event_utils.cc:34: if (!screen) This should not happen. I was thinking to change screen to return empty list and use it as the indicator, but as I mentioned above, I'll make CL so that it will return the last display info during the shutdown. https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... ui/events/event_utils.cc:41: display.touch_support() == gfx::Display::TOUCH_SUPPORT_AVAILABLE) you also need to check if it's UNKNOWN or not. https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h File ui/events/event_utils.h (right): https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h#newc... ui/events/event_utils.h:31: }; can you just use gfx::Display:TouchSupport?
On 2014/01/14 22:35:00, oshima wrote: > On second thought, i think it's better to support screen during shutdown because > this is not the first time I saw the issue and better to fix for the rest. > > I have a fix and will upload soon, FYI: https://codereview.chromium.org/138003007/ > but no worry you can keep the most of the > code the same. > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc > File ui/events/event_utils.cc (right): > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... > ui/events/event_utils.cc:34: if (!screen) > This should not happen. I was thinking to change screen to return empty list and > use it as the indicator, but as I mentioned above, I'll make CL so that it will > return the last display info during the shutdown. > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... > ui/events/event_utils.cc:41: display.touch_support() == > gfx::Display::TOUCH_SUPPORT_AVAILABLE) > you also need to check if it's UNKNOWN or not. > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h > File ui/events/event_utils.h (right): > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h#newc... > ui/events/event_utils.h:31: }; > can you just use gfx::Display:TouchSupport?
On 2014/01/15 03:01:38, oshima wrote: > On 2014/01/14 22:35:00, oshima wrote: > > On second thought, i think it's better to support screen during shutdown > because > > this is not the first time I saw the issue and better to fix for the rest. > > > > I have a fix and will upload soon, > > FYI: https://codereview.chromium.org/138003007/ > > > but no worry you can keep the most of the > > code the same. > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc > > File ui/events/event_utils.cc (right): > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... > > ui/events/event_utils.cc:34: if (!screen) > > This should not happen. I was thinking to change screen to return empty list > and > > use it as the indicator, but as I mentioned above, I'll make CL so that it > will > > return the last display info during the shutdown. > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... > > ui/events/event_utils.cc:41: display.touch_support() == > > gfx::Display::TOUCH_SUPPORT_AVAILABLE) > > you also need to check if it's UNKNOWN or not. > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h > > File ui/events/event_utils.h (right): > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h#newc... > > ui/events/event_utils.h:31: }; > > can you just use gfx::Display:TouchSupport? Looks good, I'll hold off until that's landed, and then update this patch. Thanks!
On 2014/01/15 13:30:08, tdresser wrote: > On 2014/01/15 03:01:38, oshima wrote: > > On 2014/01/14 22:35:00, oshima wrote: > > > On second thought, i think it's better to support screen during shutdown > > because > > > this is not the first time I saw the issue and better to fix for the rest. > > > > > > I have a fix and will upload soon, > > > > FYI: https://codereview.chromium.org/138003007/ > > > > > but no worry you can keep the most of the > > > code the same. > > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc > > > File ui/events/event_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... > > > ui/events/event_utils.cc:34: if (!screen) > > > This should not happen. I was thinking to change screen to return empty list > > and > > > use it as the indicator, but as I mentioned above, I'll make CL so that it > > will > > > return the last display info during the shutdown. > > > > > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... > > > ui/events/event_utils.cc:41: display.touch_support() == > > > gfx::Display::TOUCH_SUPPORT_AVAILABLE) > > > you also need to check if it's UNKNOWN or not. > > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h > > > File ui/events/event_utils.h (right): > > > > > > > > > https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h#newc... > > > ui/events/event_utils.h:31: }; > > > can you just use gfx::Display:TouchSupport? > > Looks good, I'll hold off until that's landed, and then update this patch. > Thanks! it's landed.
Thanks for the CL. Is this all that's necessary now? https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc File ui/events/event_utils.cc (right): https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.cc#new... ui/events/event_utils.cc:41: display.touch_support() == gfx::Display::TOUCH_SUPPORT_AVAILABLE) On 2014/01/14 22:35:01, oshima wrote: > you also need to check if it's UNKNOWN or not. Done. https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h File ui/events/event_utils.h (right): https://codereview.chromium.org/134773004/diff/1/ui/events/event_utils.h#newc... ui/events/event_utils.h:31: }; On 2014/01/14 22:35:01, oshima wrote: > can you just use gfx::Display:TouchSupport? Oh, absolutely, thanks. Done.
lgtm with nit https://codereview.chromium.org/134773004/diff/140001/ui/events/event_utils.cc File ui/events/event_utils.cc (right): https://codereview.chromium.org/134773004/diff/140001/ui/events/event_utils.c... ui/events/event_utils.cc:33: gfx::Display::TouchSupport InternalDisplaySupportsTouch() { GetInternalDisplayTouchSupport() ?
Relanding https://codereview.chromium.org/103893005 after fixing some issues with accessing information on displays after the ash::Shell was no longer valid. Getting jar@ to review since isherman@ is out. https://codereview.chromium.org/134773004/diff/140001/ui/events/event_utils.cc File ui/events/event_utils.cc (right): https://codereview.chromium.org/134773004/diff/140001/ui/events/event_utils.c... ui/events/event_utils.cc:33: gfx::Display::TouchSupport InternalDisplaySupportsTouch() { On 2014/01/17 20:49:47, oshima wrote: > GetInternalDisplayTouchSupport() ? Done.
sadrul@, can you take a look at ui/**? jar@, can you take a look at chrome/**? There are only minor changes since https://codereview.chromium.org/103893005.
ui/events/ lgtm
https://codereview.chromium.org/134773004/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/134773004/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/metrics_log.cc:395: touchscreen->set_vendor_id(it->first); Do we have privacy approval for uploading this data? https://codereview.chromium.org/134773004/diff/200001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.h (right): https://codereview.chromium.org/134773004/diff/200001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:10: #include <set> nit: For std::pair, should you #include <utility>
https://codereview.chromium.org/134773004/diff/200001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/134773004/diff/200001/chrome/browser/metrics/... chrome/browser/metrics/metrics_log.cc:395: touchscreen->set_vendor_id(it->first); On 2014/01/20 22:34:47, jar wrote: > Do we have privacy approval for uploading this data? See google internal review: cr/59273564 https://codereview.chromium.org/134773004/diff/200001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.h (right): https://codereview.chromium.org/134773004/diff/200001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:10: #include <set> On 2014/01/20 22:34:47, jar wrote: > nit: For std::pair, should you > #include <utility> Done.
jar@, can you take another look?
These values seem similar to the types of things we log in the UMA system profile already, so seems reasonable to me. But let's check with Chrome Privacy just in case. +battre
LGTM to the idea of collecting vid/pid (did not review the CL)
On 2014/01/27 14:03:16, battre wrote: > LGTM to the idea of collecting vid/pid (did not review the CL) asvitkine@, does this look good?
https://codereview.chromium.org/134773004/diff/280001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/134773004/diff/280001/chrome/browser/metrics/... chrome/browser/metrics/metrics_log.cc:11: #include "ash/shell.h" Is this header valid to include on all platforms? I thought ash was a ChromeOS (or Aura) only thing? Probably should be in an ifdef. https://codereview.chromium.org/134773004/diff/280001/chrome/browser/metrics/... chrome/browser/metrics/metrics_log.cc:820: gfx::Display::TouchSupport has_touch = ui::GetInternalDisplayTouchSupport(); I think this ChromeOS-specific code in this class has tipped to a point that it should be moved out of MetricsService into a helper class. That class can then be a member var of MetricsService (on ChromeOS) and have a single function - i.e. WriteChromeOSMetrics(). Can you do this refactoring? https://codereview.chromium.org/134773004/diff/280001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/134773004/diff/280001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.cc:290: &bytes_after_return, &prop_return) == Success) { According to the docs for this function: bytes_after_return Returns the number of bytes remaining to be read in the property if a partial read was performed. But it doesn't look like your code is handling the partial read case?
I have a patch up performing the refactor here: https://codereview.chromium.org/146913005. Once that lands, I'll rebase, which should address your other concerns. https://codereview.chromium.org/134773004/diff/280001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/134773004/diff/280001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.cc:290: &bytes_after_return, &prop_return) == Success) { On 2014/01/28 16:01:06, Alexei Svitkine wrote: > According to the docs for this function: > > bytes_after_return > Returns the number of bytes remaining to be read in the > property if a partial read was performed. > > But it doesn't look like your code is handling the partial read case? I ask to read 2 items, and if any number of items other than 2 is returned, I bail. Items past the first two don't matter.
asvitkine@, how's this?
Looks great, just a couple of comments. https://codereview.chromium.org/134773004/diff/340001/chrome/common/metrics/p... File chrome/common/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/134773004/diff/340001/chrome/common/metrics/p... chrome/common/metrics/proto/system_profile.proto:223: // Whether the internal display produces touch events. Expand comment to mention that if this is omitted if the value is unknown. Also, both this comment and the comments below should mention that this is only recorded on ChromeOS. https://codereview.chromium.org/134773004/diff/340001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/134773004/diff/340001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.cc:275: XDevice* device = XOpenDevice(display, device_id); This should probably check for an error.
https://codereview.chromium.org/134773004/diff/340001/chrome/common/metrics/p... File chrome/common/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/134773004/diff/340001/chrome/common/metrics/p... chrome/common/metrics/proto/system_profile.proto:223: // Whether the internal display produces touch events. On 2014/02/04 21:13:35, Alexei Svitkine wrote: > Expand comment to mention that if this is omitted if the value is unknown. Also, > both this comment and the comments below should mention that this is only > recorded on ChromeOS. Done. I'll land the google3 side of this change once this has landed. https://codereview.chromium.org/134773004/diff/340001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/134773004/diff/340001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.cc:275: XDevice* device = XOpenDevice(display, device_id); On 2014/02/04 21:13:35, Alexei Svitkine wrote: > This should probably check for an error. Done.
LGTM, thanks!
On 2014/02/04 22:09:55, Alexei Svitkine wrote: > LGTM, thanks! jar@, can you take a look at chrome/browser/metrics/metrics_log_chromeos.cc?
https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.cc (right): https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.cc:31: touch_events_disabled_(false), nit: initializer for internal_display_supports_touch_ https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.h (right): https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:121: bool touch_events_disabled_; nit/pet-peeve: It is a bad idea to have a negative in the name of a boolean. IT requires a lot more thought to read the meaning when you often have a double negative :-/. Much better is touch_events_enabled_. If you feel the same... it would be nice to clean up this name, maybe even during this CL. https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:124: bool internal_display_supports_touch_; I couldn't find where this was used... though I did see that it wasn't initialized in the constructor.
https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.h (right): https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:121: bool touch_events_disabled_; On 2014/02/05 00:10:13, jar wrote: > nit/pet-peeve: It is a bad idea to have a negative in the name of a boolean. IT > requires a lot more thought to read the meaning when you often have a double > negative :-/. > > Much better is touch_events_enabled_. > > If you feel the same... it would be nice to clean up this name, maybe even > during this CL. I definitely agree in general - not sure about this specific instance though. This boolean is true iff an action has been taken to disable touch events. This isn't the same as saying touch events have been enabled. If you negated the current meaning of this variable, you'd end up with a boolean which is true iff no action has been taken to disable touch events, which feels more confusing to me. What do you think? https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:124: bool internal_display_supports_touch_; On 2014/02/05 00:10:13, jar wrote: > I couldn't find where this was used... though I did see that it wasn't > initialized in the constructor. Yikes, that was supposed to have been removed long ago. Good catch.
lgtm https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... File ui/events/x/touch_factory_x11.h (right): https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... ui/events/x/touch_factory_x11.h:121: bool touch_events_disabled_; On 2014/02/05 19:29:58, tdresser wrote: > On 2014/02/05 00:10:13, jar wrote: > > nit/pet-peeve: It is a bad idea to have a negative in the name of a boolean. > IT > > requires a lot more thought to read the meaning when you often have a double > > negative :-/. > > > > Much better is touch_events_enabled_. > > > > If you feel the same... it would be nice to clean up this name, maybe even > > during this CL. > > I definitely agree in general - not sure about this specific instance though. > > This boolean is true iff an action has been taken to disable touch events. > > This isn't the same as saying touch events have been enabled. If you negated the > current meaning of this variable, you'd end up with a boolean which is true iff > no action has been taken to disable touch events, which feels more confusing to > me. > > What do you think? > IMO, if you intended this to have 3 states: Never set; Enabled, and Disabled, it would be better to have a tri-state (enum) than a bool. That said... I was just looking at a quick improvement (to change polarity). If you want to argue it is tri-state, masking as a bool, with special semantics... it would be IMO a bigger change than I'd ask to be mixed into the current CL. I'll leave it to you... but I think you should probably skip it at this point.
lgtm2, with a driveby request to make sure to update to internal .proto files to match the final state of these files.
On 2014/02/05 20:27:17, jar1 wrote: > lgtm > > https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... > File ui/events/x/touch_factory_x11.h (right): > > https://codereview.chromium.org/134773004/diff/360001/ui/events/x/touch_facto... > ui/events/x/touch_factory_x11.h:121: bool touch_events_disabled_; > On 2014/02/05 19:29:58, tdresser wrote: > > On 2014/02/05 00:10:13, jar wrote: > > > nit/pet-peeve: It is a bad idea to have a negative in the name of a > boolean. > > IT > > > requires a lot more thought to read the meaning when you often have a double > > > negative :-/. > > > > > > Much better is touch_events_enabled_. > > > > > > If you feel the same... it would be nice to clean up this name, maybe even > > > during this CL. > > > > I definitely agree in general - not sure about this specific instance though. > > > > This boolean is true iff an action has been taken to disable touch events. > > > > This isn't the same as saying touch events have been enabled. If you negated > the > > current meaning of this variable, you'd end up with a boolean which is true > iff > > no action has been taken to disable touch events, which feels more confusing > to > > me. > > > > What do you think? > > > > IMO, if you intended this to have 3 states: Never set; Enabled, and Disabled, it > would be better to have a tri-state (enum) than a bool. > > That said... I was just looking at a quick improvement (to change polarity). If > you want to argue it is tri-state, masking as a bool, with special semantics... > it would be IMO a bigger change than I'd ask to be mixed into the current CL. > > I'll leave it to you... but I think you should probably skip it at this point. Skipping it for now. I think you're right that it's unclear; however, there are only 2 states: Never Set, and Disabled. It feels like a 2 valued enum would probably be overkill, but maybe that's the clearest way to do it.
On 2014/02/05 20:48:33, Ilya Sherman (catching up...) wrote: > lgtm2, with a driveby request to make sure to update to internal .proto files to > match the final state of these files. Thanks for the reminder isherman@, I'll update the internal .proto as soon as this has stuck.
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/134773004/470001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by tdresser@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/134773004/630002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdresser@chromium.org/134773004/630002
Message was sent while issue was closed.
Change committed as 249617 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
