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

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics_unittest.cc

+// Copyright 2015 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 "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/histogram_tester.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/events/devices/device_data_manager.h"
+#include "ui/events/test/device_data_manager_test_api.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/screen.h"
+
+namespace {
+
+const char kTouchEventsEnabledHistogramName[] =
+    "Touchscreen.TouchEventsEnabled";
+
+// A simple test double that allows gfx::DisplayObserver to be added and
+// removed.
+class TestScreen : public gfx::Screen {

[Alexei Svitkine (slow), 2015/07/07 22:00:58]

Seems like we have a number of these TestScreen subclasses already in the
codebase.

e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Can we move it to its own file and share the same class from different tests?

[bruthig, 2015/07/08 20:29:52]

I've created a simple gfx::test::TestScreen and updated two existing tests to
use it.  There are still two custom test Screens, including the one you linked
to, that are more complex and not functionally equivalent.

+ public:
+  TestScreen() {}
+  ~TestScreen() override {}
+
+  // Overridden from gfx::Screen:
+  gfx::Point GetCursorScreenPoint() override {
+    NOTREACHED()
+        << "Function not implemented!!! 'TestScreen::GetCursorScreenPoint()'";

[Alexei Svitkine (slow), 2015/07/07 22:00:58]

Nit: No need for these messages. I think when NOTREACHED() is hit, you get the
stack trace. So no need to mention the function name. And then, "Function not
implemented" is redundant since it's a NOTREACHED() already.

[bruthig, 2015/07/08 20:29:52]

Done.

+    return gfx::Point();
+  }
+
+  gfx::NativeWindow GetWindowUnderCursor() override {
+    NOTREACHED()
+        << "Function not implemented!!! 'TestScreen::GetWindowUnderCursor()'";
+    return NULL;

[Ilya Sherman, 2015/07/07 22:06:22]

nit: nullptr (pretty much everywhere where you're using NULL now)

[bruthig, 2015/07/08 20:29:52]

Done.

+  }
+
+  gfx::NativeWindow GetWindowAtScreenPoint(const gfx::Point& point) override {
+    NOTREACHED() << "Function not implemented!!! "
+                    "'TestScreen::GetWindowAtScreenPoint(...)'";
+    return NULL;
+  }
+
+  int GetNumDisplays() const override { return 0; }
+
+  std::vector<gfx::Display> GetAllDisplays() const override {
+    NOTREACHED()
+        << "Function not implemented!!! 'TestScreen::GetAllDisplays()'";
+    return std::vector<gfx::Display>();
+  }
+
+  gfx::Display GetDisplayNearestWindow(gfx::NativeView view) const override {
+    NOTREACHED() << "Function not implemented!!! "
+                    "'TestScreen::GetDisplayNearestWindow(...)'";
+    return gfx::Display();
+  }
+
+  gfx::Display GetDisplayNearestPoint(const gfx::Point& point) const override {
+    NOTREACHED() << "Function not implemented!!! "
+                    "'TestScreen::GetDisplayNearestPoint(...)'";
+    return gfx::Display();
+  }
+
+  gfx::Display GetDisplayMatching(const gfx::Rect& match_rect) const override {
+    NOTREACHED()
+        << "Function not implemented!!! 'TestScreen::GetDisplayMatching(...)'";
+    return gfx::Display();
+  }
+
+  gfx::Display GetPrimaryDisplay() const override {
+    NOTREACHED()
+        << "Function not implemented!!! 'TestScreen::GetPrimaryDisplay()'";
+    return gfx::Display();
+  }
+
+  void AddObserver(gfx::DisplayObserver* observer) override {}
+
+  void RemoveObserver(gfx::DisplayObserver* observer) override {}
+
+  void AddDisplay(const gfx::Rect& bounds, const gfx::Rect& work_area) {
+    NOTREACHED() << "Function not implemented!!! 'TestScreen::AddDisplay(...)'";
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestScreen);
+};
+
+}  // namespace
+
+class ChromeBrowserMainExtraPartsMetricsTest : public testing::Test {
+ public:
+  ChromeBrowserMainExtraPartsMetricsTest();
+  ~ChromeBrowserMainExtraPartsMetricsTest() override;
+
+ protected:
+#if defined(USE_OZONE) || defined(USE_X11)
+  // A newly constructed DeviceDataManger. Accessible via
+  // DeviceDataManager::GetInstance().
+  scoped_ptr<ui::DeviceDataManager> device_data_manager_;
+
+  // Test API wrapping |device_data_manager_|.
+  scoped_ptr<ui::test::DeviceDataManagerTestAPI> device_data_manager_test_api_;
+#endif  // defined(USE_OZONE) || defined(USE_X11)

[Alexei Svitkine (slow), 2015/07/07 22:00:58]

This is a lot of extra complexity being put into the chrome/ code. Can this
complexity be tucked away in the ui test helper?

i.e. can ui::test::DeviceDataManagerTestAPI own the device_data_manager and also
have the ifdefs about ozone? So that this code can just have its instance around
without ifdefs?

[bruthig, 2015/07/08 20:29:52]

Done.

+
+ private:
+  // Required by a ChromeBrowserMainExtraPartsMetrics test target.
+  base::MessageLoop message_loop_;
+
+  // Dummy screen required by a ChromeBrowserMainExtraPartsMetrics test target.
+  scoped_ptr<::TestScreen> test_screen_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainExtraPartsMetricsTest);
+};
+
+ChromeBrowserMainExtraPartsMetricsTest::ChromeBrowserMainExtraPartsMetricsTest()
+    :
+#if defined(USE_OZONE) || defined(USE_X11)
+      device_data_manager_(
+          ui::test::DeviceDataManagerTestAPI::CreateDeviceDataManager()),
+      device_data_manager_test_api_(
+          new ui::test::DeviceDataManagerTestAPI(device_data_manager_.get())),
+#endif  // defined(USE_OZONE) || defined(USE_X11)
+      test_screen_(new ::TestScreen) {
+  gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, test_screen_.get());
+
+#if defined(USE_OZONE) || defined(USE_X11)
+  ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);

[Alexei Svitkine (slow), 2015/07/07 22:00:58]

Can this be handled in the
DeviceDataManagerTestAPI::SetDeviceDataManagerInstance() implementation, since
it's already a helper for testing?

[bruthig, 2015/07/08 20:29:52]

Done.

+  ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(
+      device_data_manager_.get());
+#endif  // defined(USE_OZONE) || defined(USE_X11)
+}
+
+ChromeBrowserMainExtraPartsMetricsTest::
+    ~ChromeBrowserMainExtraPartsMetricsTest() {
+  gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, nullptr);
+
+#if defined(USE_OZONE) || defined(USE_X11)
+  ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);
+#endif  // defined(USE_OZONE) || defined(USE_X11)
+}
+
+// Verify a TouchEventsEnabled value isn't recorded during construction.
+TEST_F(ChromeBrowserMainExtraPartsMetricsTest,
+       VerifyTouchEventsEnabledIsNotRecordedAfterConstruction) {
+  base::HistogramTester histogram_tester;
+  ChromeBrowserMainExtraPartsMetrics test_target;
+  histogram_tester.ExpectTotalCount(kTouchEventsEnabledHistogramName, 0);
+}
+
+#if defined(USE_OZONE) || defined(USE_X11)
+
+// Verify a TouchEventsEnabled value isn't recorded during PostBrowserStart if
+// the device scan hasn't completed yet.
+TEST_F(ChromeBrowserMainExtraPartsMetricsTest,
+       VerifyTouchEventsEnabledIsNotRecordedAfterPostBrowserStart) {
+  base::HistogramTester histogram_tester;
+
+  ChromeBrowserMainExtraPartsMetrics test_target;
+
+  test_target.PostBrowserStart();
+  histogram_tester.ExpectTotalCount(kTouchEventsEnabledHistogramName, 0);
+}
+
+// Verify a TouchEventsEnabled value is recorded during PostBrowserStart if the
+// device scan has already completed.
+TEST_F(ChromeBrowserMainExtraPartsMetricsTest,
+       VerifyTouchEventsEnabledIsRecordedAfterPostBrowserStart) {
+  base::HistogramTester histogram_tester;
+
+  device_data_manager_test_api_->OnDeviceListsComplete();
+
+  ChromeBrowserMainExtraPartsMetrics test_target;
+
+  test_target.PostBrowserStart();
+  histogram_tester.ExpectTotalCount(kTouchEventsEnabledHistogramName, 1);
+}
+
+// Verify a TouchEventsEnabled value is recorded when an asynchronous device
+// scan completes.
+TEST_F(ChromeBrowserMainExtraPartsMetricsTest,
+       VerifyTouchEventsEnabledIsRecordedWhenDeviceListsComplete) {
+  base::HistogramTester histogram_tester;
+  ChromeBrowserMainExtraPartsMetrics test_target;
+
+  test_target.PostBrowserStart();
+  device_data_manager_test_api_->NotifyObserversDeviceListsComplete();
+  histogram_tester.ExpectTotalCount(kTouchEventsEnabledHistogramName, 1);
+}
+
+// Verify a TouchEventsEnabled value is only recorded once if multiple
+// asynchronous device scans happen.
+TEST_F(ChromeBrowserMainExtraPartsMetricsTest,
+       VerifyTouchEventsEnabledIsOnlyRecordedOnce) {
+  base::HistogramTester histogram_tester;
+  ChromeBrowserMainExtraPartsMetrics test_target;
+
+  test_target.PostBrowserStart();
+  device_data_manager_test_api_->NotifyObserversDeviceListsComplete();
+  device_data_manager_test_api_->NotifyObserversDeviceListsComplete();
+  histogram_tester.ExpectTotalCount(kTouchEventsEnabledHistogramName, 1);
+}
+
+#else
+
+// Verify a TouchEventsEnabled value is recorded during PostBrowserStart.
+TEST_F(ChromeBrowserMainExtraPartsMetricsTest,
+       VerifyTouchEventsEnabledIsRecordedAfterPostBrowserStart) {
+  base::HistogramTester histogram_tester;
+  ChromeBrowserMainExtraPartsMetrics test_target;
+
+  test_target.PostBrowserStart();
+  histogram_tester.ExpectTotalCount(kTouchEventsEnabledHistogramName, 1);
+}
+
+#endif  // defined(USE_OZONE) || defined(USE_X11)