Fixed the Touchscreen.TouchEventsEnabled histogram to record the correct values on X11 and Ozone ba…

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/cpu.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/sys_info.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time/time.h"
 #include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_browser_main.h"
 #include "chrome/browser/mac/bluetooth_utility.h"
 #include "chrome/browser/pref_service_flags_storage.h"
 #include "chrome/browser/shell_integration.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/base/touch/touch_device.h"
 #include "ui/base/ui_base_switches.h"
+#include "ui/events/devices/device_data_manager.h"
 #include "ui/events/event_switches.h"
 #include "ui/gfx/screen.h"
 
 #if defined(OS_ANDROID) && defined(__arm__)
 #include <cpu-features.h>
 #endif  // defined(OS_ANDROID) && defined(__arm__)
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 #include <gnu/libc-version.h>
 
@@ skipping 193 matching lines @@
     state = UMA_TOUCH_EVENTS_ENABLED;
   } else if (touch_enabled_switch == switches::kTouchEventsAuto) {
     state = ui::IsTouchDevicePresent() ?
         UMA_TOUCH_EVENTS_AUTO_ENABLED : UMA_TOUCH_EVENTS_AUTO_DISABLED;
   } else if (touch_enabled_switch == switches::kTouchEventsDisabled) {
     state = UMA_TOUCH_EVENTS_DISABLED;
   } else {
     NOTREACHED();
     return;
   }
-
   UMA_HISTOGRAM_ENUMERATION("Touchscreen.TouchEventsEnabled", state,
                             UMA_TOUCH_EVENTS_STATE_COUNT);
 }
 
 }  // namespace
 
 ChromeBrowserMainExtraPartsMetrics::ChromeBrowserMainExtraPartsMetrics()
     : display_count_(0), is_screen_observer_(false) {
 }
 
 ChromeBrowserMainExtraPartsMetrics::~ChromeBrowserMainExtraPartsMetrics() {
   if (is_screen_observer_)
     gfx::Screen::GetNativeScreen()->RemoveObserver(this);
+#if defined(USE_OZONE) || defined(USE_X11)
+  ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
+#endif  // defined(USE_OZONE) || defined(USE_X11)
 }
 
 void ChromeBrowserMainExtraPartsMetrics::PreProfileInit() {
   RecordMicroArchitectureStats();
 }
 
 void ChromeBrowserMainExtraPartsMetrics::PreBrowserStart() {
   about_flags::PrefServiceFlagsStorage flags_storage_(
       g_browser_process->local_state());
   about_flags::RecordUMAStatistics(&flags_storage_);
 }
 
 void ChromeBrowserMainExtraPartsMetrics::PostBrowserStart() {
   RecordLinuxGlibcVersion();
 #if defined(USE_X11) && !defined(OS_CHROMEOS)
   UMA_HISTOGRAM_ENUMERATION("Linux.WindowManager",
                             GetLinuxWindowManager(),
                             UMA_LINUX_WINDOW_MANAGER_COUNT);
 #endif
+
+#if defined(USE_OZONE) || defined(USE_X11)

[Rick Byers, 2015/06/16 14:45:47]

Rather than use an #if, couldn't we encapsulate this into DeviceDataManager? 
Eg. a 'areDeviceListsComplete' method?  Wouldn't that also prevent a race on
OZONE/X11 where OnDeviceListsComplete occurred before we happened to get our
observer installed?

[bruthig, 2015/06/16 15:52:45]

Agreed I'm not happy about the 'potential' race condition either, but IIUC, and
spang@ can confirm, the race condition does not exist in the current
implementation.  It would only be introduced if the point at which attaching the
observer changed or if the spot where the device scan is started is changed.

Having said that, from what I can tell the DeviceDataManager is not used (or
created) on the Windows or Android platforms (see IsTouchDevicePresent()
implementations here
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...
and here
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/touch/touc...).
 So adding a DeviceDataManager::AreDeviceListsComplete() method is not
sufficient.

IMO the check to determine if the return value for IsTouchDevicePresent() is
valid should live in the same spot as IsTouchDevicePresent() not in
DeviceDataManager.  But this is not trivial to implement since
IsTouchDevicePresent() is a static/global function and for the X11 and Ozone
platforms it requires the asynchronous observer.

If we want to address this concern I think the right thing to do would be to
turn touch_devices.h into a proper class and each platform can have have their
own implementation.

Thoughts?

[spang, 2015/06/16 15:55:40]

A race free way to do it is

  if (ui::DeviceDataManager::GetInstance()->device_lists_complete())
    OnDeviceListsComplete();
  else
    ui::DeviceDataManager::GetInstance()->AddObserver(this);

+  // The touch event state for X11 and Ozone based event sub-systems are based
+  // on device scans that happen asynchronously so we attach an observer to
+  // wait until these scans complete.
+  ui::DeviceDataManager::GetInstance()->AddObserver(this);
+#else
   RecordTouchEventState();
+#endif  // defined(USE_OZONE) || defined(USE_X11)
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   RecordMacMetrics();
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
   const int kStartupMetricsGatheringDelaySeconds = 45;
   content::BrowserThread::GetBlockingPool()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&RecordStartupMetricsOnBlockingPool),
       base::TimeDelta::FromSeconds(kStartupMetricsGatheringDelaySeconds));
@@ skipping 21 matching lines @@
 
 void ChromeBrowserMainExtraPartsMetrics::OnDisplayMetricsChanged(
     const gfx::Display& display,
     uint32_t changed_metrics) {
 }
 
 void ChromeBrowserMainExtraPartsMetrics::ProfilerFinishedCollectingMetrics() {
   first_web_contents_profiler_.reset();
 }
 
+void ChromeBrowserMainExtraPartsMetrics::OnDeviceListsComplete() {
+  ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
+  RecordTouchEventState();
+}
+
 void ChromeBrowserMainExtraPartsMetrics::EmitDisplaysChangedMetric() {
   int display_count = gfx::Screen::GetNativeScreen()->GetNumDisplays();
   if (display_count != display_count_) {
     display_count_ = display_count;
     UMA_HISTOGRAM_COUNTS_100("Hardware.Display.Count.OnChange", display_count_);
   }
 }
 
 namespace chrome {
 
 void AddMetricsExtraParts(ChromeBrowserMainParts* main_parts) {
   main_parts->AddParts(new ChromeBrowserMainExtraPartsMetrics());
 }
 
 }  // namespace chrome