Do not record startup metrics when non-browser UI was displayed

chrome/browser/ui/profile_error_browsertest.cc

Index: chrome/browser/ui/profile_error_browsertest.cc
diff --git a/chrome/browser/ui/profile_error_browsertest.cc b/chrome/browser/ui/profile_error_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6ff7cdc59099eb5341e8ebaa61b2bf0634d39cf3
--- /dev/null
+++ b/chrome/browser/ui/profile_error_browsertest.cc
@@ -0,0 +1,93 @@
+// 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 <string>
+
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/strings/string_util.h"
+#include "base/test/histogram_tester.h"
+#include "chrome/browser/ui/simple_message_box_internal.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/ui_test_utils.h"
+
+namespace {
+
+class ProfileErrorBrowserTest : public InProcessBrowserTest,
+                                public testing::WithParamInterface<bool> {
+  // A fixture that allows testing histograms reporting when faced with a
+  // corrupted profile. The boolean parameter forces the creation of an empty or
+  // corrupted profile, allowing to test both the corruption case and that what
+  // it is testing indeed happens differently when not under corruption.
+ public:
+  ProfileErrorBrowserTest() : do_corrupt_(GetParam()) {}
+
+  bool SetUpUserDataDirectory() override {
+    CreateProfileOnDisk();
+    return InProcessBrowserTest::SetUpUserDataDirectory();

[gab, 2015/07/06 15:52:54]

Actually this doesn't do any setup
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...),
I think it semantically makes more sense to override it completely (don't call
the super class' version of this method).

In that light I think the code for CreateProfileOnDisk() should be inlined here
and on failure you'll want to both ADD_FAILURE() and return false.

e.g. for all EXPECTs, instead of
EXPECT_TRUE(base::CreateDirectory(profile_dir));
:
if (!base::CreateDirectory(profile_dir)) {
  ADD_FAILURE();
  return false;
}

[tiany, 2015/07/06 21:49:54]

Done.

+  }
+
+  void SetUpInProcessBrowserTestFixture() override {
+    InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
+
+    // Skip showing the error message box in order to avoid freezing the main
+    // thread.
+    chrome::internal::g_should_skip_message_box_for_test = true;
+  }
+
+ protected:
+  void CreateProfileOnDisk() {
+    base::FilePath profile_dir;
+    EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &profile_dir));
+    profile_dir = profile_dir.AppendASCII(TestingProfile::kTestUserProfileDir);
+    EXPECT_TRUE(base::CreateDirectory(profile_dir));
+    const base::FilePath pref_file =
+        profile_dir.Append(chrome::kPreferencesFilename);
+
+    // Write either an empty or a invalid string to user profile, determined by

[gab, 2015/07/06 15:52:53]

s/,determined/as determined/

[gab, 2015/07/06 15:52:54]

s/a invalid/an invalid/

[gab, 2015/07/06 15:52:54]

s/to user profile/ to the user profile/

[tiany, 2015/07/06 21:49:54]

Done.

[tiany, 2015/07/06 21:49:54]

Done.

[tiany, 2015/07/06 21:49:54]

Done.

[tiany, 2015/07/06 21:49:54]

Done.

+    // the boolean parameter

[gab, 2015/07/06 15:52:54]

End sentence with '.'

[tiany, 2015/07/06 21:49:54]

Done.

+    std::string data(do_corrupt_ ? "invalid json" : "{}");

[gab, 2015/07/06 15:52:54]

const char[]

(instead of putting it in a string and then extracting its c_str() -- use
arraysize() from base/macros.h for the size parameter)

[tiany, 2015/07/06 21:49:54]

Discussed with gab, used const std::string instead.

+    EXPECT_TRUE(base::WriteFile(pref_file, data.c_str(), data.size()));
+  }
+
+  // Histogram value verifier.
+  const base::HistogramTester histogram_tester_;
+
+  // Decide to create an empty or a corrupted profile depending on the params
+  // passed into the test

[gab, 2015/07/06 15:52:53]

// Whether the test fixture and test should set up a corrupted profile and
expect a reaction to one.

[tiany, 2015/07/06 21:49:54]

Done.

+  const bool do_corrupt_;
+};
+
+#if defined(OS_CHROMEOS)
+// Disable the test on chromos since kernel controls the user profile thus we
+// won't be able to corrupt it.
+#define MAYBE(test) DISABLED_##test

[gab, 2015/07/06 15:52:54]

No need for advanced MAYBE macro anymore, i.e.:

MAYBE_CorruptedProfile DISABLED_CorruptedProfile

[tiany, 2015/07/06 21:49:54]

Done.

+#else
+#define MAYBE(test) test
+#endif
+
+IN_PROC_BROWSER_TEST_P(ProfileErrorBrowserTest, MAYBE(CorruptedProfile)) {
+  const char kPaintHistogram[] = "Startup.FirstWebContents.NonEmptyPaint";
+  const char kLoadHistogram[] = "Startup.FirstWebContents.MainFrameLoad";
+
+  // Navigate to a URL so the first non-empty paint is registered.
+  ui_test_utils::NavigateToURL(browser(), GURL("http://www.example.com/"));
+  if (do_corrupt_) {
+    histogram_tester_.ExpectTotalCount(kPaintHistogram, 0);
+    histogram_tester_.ExpectTotalCount(kLoadHistogram, 0);
+  } else {
+    histogram_tester_.ExpectTotalCount(kPaintHistogram, 1);
+    histogram_tester_.ExpectTotalCount(kLoadHistogram, 1);
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(ProfileErrorBrowserTestInstance,
+                        ProfileErrorBrowserTest,
+                        testing::Bool());
+
+}  // namespace