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/aura/test/test_screen.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 {

[Ilya Sherman, 2015/06/24 23:46:51]

nit: Please leave a blank line below this one.

[bruthig, 2015/06/25 17:26:52]

Done.

+const char kTouchEventsEnabledHistogramName[] =
+    "Touchscreen.TouchEventsEnabled";

[Ilya Sherman, 2015/06/24 23:46:51]

nit: Please leave a blank line below this one.

[bruthig, 2015/06/25 17:26:52]

Done.

+}  // namespace
+
+// Test fixture for the ChromeBrowserMainExtraPartsMetricsTest class.

[Ilya Sherman, 2015/06/24 23:46:51]

Optional nit: This comment seems unnecessary to me, in that it is redundant with
the code.  OK to leave if you really prefer it, though.

[bruthig, 2015/06/25 17:26:52]

Done.

+class ChromeBrowserMainExtraPartsMetricsTest : public testing::Test {
+ public:
+  ChromeBrowserMainExtraPartsMetricsTest();
+  ~ChromeBrowserMainExtraPartsMetricsTest() override;
+
+  // testing::Test:
+  void SetUp() override;
+  void TearDown() override;

[Ilya Sherman, 2015/06/24 23:46:51]

nit: Please inline all of the method definitions for this class.  In my
experience, that's the common style in unit test files.

[bruthig, 2015/06/25 17:26:52]

The reason I don't do that is because it implies an 'inline' request to the
compiler which will bloats the code. Do you know the reason why the common style
is to inline them?

[Ilya Sherman, 2015/06/25 20:31:47]

I think that for test suites, it just doesn't matter that much, and so people
prefer the increased readability.

+
+ protected:
+  // Test API wrapping |device_data_manager_|.
+  scoped_ptr<ui::test::DeviceDataManagerTestAPI> device_data_manager_test_api_;
+
+ private:
+  // A newly constructed DeviceDataManger. Accessible via
+  // DeviceDataManager::GetInstance().
+  scoped_ptr<ui::DeviceDataManager> device_data_manager_;
+
+  // Required by a ChromeBrowserMainExtraPartsMetrics test target.
+  base::MessageLoop message_loop_;
+
+  // Dummy screen required by a ChromeBrowserMainExtraPartsMetrics test target.
+  scoped_ptr<aura::TestScreen> test_screen_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainExtraPartsMetricsTest);
+};
+
+ChromeBrowserMainExtraPartsMetricsTest::
+    ChromeBrowserMainExtraPartsMetricsTest() {
+}
+
+ChromeBrowserMainExtraPartsMetricsTest::
+    ~ChromeBrowserMainExtraPartsMetricsTest() {
+}
+
+void ChromeBrowserMainExtraPartsMetricsTest::SetUp() {
+  test_screen_.reset(aura::TestScreen::Create(gfx::Size()));
+  gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, test_screen_.get());
+
+  device_data_manager_ =
+      ui::test::DeviceDataManagerTestAPI::CreateDeviceDataManager().Pass();
+  ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(
+      device_data_manager_.get());
+
+  device_data_manager_test_api_.reset(
+      new ui::test::DeviceDataManagerTestAPI(device_data_manager_.get()));

[Ilya Sherman, 2015/06/24 23:46:51]

Can you move all of this work to the constructor, rather than to the SetUp()
method?

[bruthig, 2015/06/25 17:26:52]

Done.

+}
+
+void ChromeBrowserMainExtraPartsMetricsTest::TearDown() {
+  gfx::Screen::SetScreenInstance(gfx::SCREEN_TYPE_NATIVE, nullptr);
+
+  device_data_manager_test_api_.reset();

[Ilya Sherman, 2015/06/24 23:46:50]

Are you writing this explicitly because the order matters?  If so, please
document that.

[bruthig, 2015/06/25 17:26:52]

Removed.

+
+  ui::test::DeviceDataManagerTestAPI::SetDeviceDataManagerInstance(nullptr);

[Ilya Sherman, 2015/06/24 23:46:51]

Likewise, can you move all of this to the destructor?

[bruthig, 2015/06/25 17:26:52]

Done.

+}
+
+// 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)