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..acfa38216d793e569c89aa803848cb040cf8a540
--- /dev/null
+++ b/chrome/browser/ui/profile_error_browsertest.cc
@@ -0,0 +1,108 @@
+// 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> {
+ public:
+  ProfileErrorBrowserTest() : do_corrupt_(GetParam()) {}
+
+  void SetUpInProcessBrowserTestFixture() override {
+    InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
+    // Imitate the behavior in the main test but do not show the message box so

[gab, 2015/06/16 19:55:34]

nit: +empty line above.

[tiany, 2015/06/23 18:36:35]

Done.

+    // it can terminate

[gab, 2015/06/16 19:55:34]

How about something like:

// In the main test, skip showing the error message box in order to avoid
freezing the main thread.

[tiany, 2015/06/23 18:36:35]

Done.

+    if (!IsPRETest())
+      chrome::internal::g_should_skip_message_box_for_test = true;
+  }
+
+  bool SetUpUserDataDirectory() override {

[gab, 2015/06/16 19:55:34]

nit: Put this override first (since it happens first in the logic and thus makes
more sense when reading IMO..), i.e. just above
SetUpInProcessBrowserTestFixture()

[tiany, 2015/06/23 18:36:35]

Done.

+    // Setup normally in the PRE test and corrupt user profile in the main test.
+    if (IsPRETest())
+      return InProcessBrowserTest::SetUpUserDataDirectory();
+

[gab, 2015/06/16 19:55:34]

Remove empty line here (since the comment on line 35 is about everything).

[tiany, 2015/06/23 18:36:35]

Done.

+    if (do_corrupt_)
+      CorruptProfileOnDisk();
+    return true;
+  }
+
+ protected:
+  void CorruptProfileOnDisk() {
+    base::FilePath profile_dir;
+    EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DATA, &profile_dir));
+    profile_dir = profile_dir.AppendASCII(TestingProfile::kTestUserProfileDir);
+
+    const base::FilePath pref_file =
+        profile_dir.Append(chrome::kPreferencesFilename);
+    EXPECT_TRUE(base::PathExists(pref_file));
+
+    // Corrupt the user profile.
+    std::string junk("junk");
+    EXPECT_TRUE(base::AppendToFile(pref_file, junk.c_str(), junk.size()));
+  }
+
+  base::HistogramTester& histograms() { return histograms_tester_; }

[gab, 2015/06/16 19:55:34]

In Chromium we never (almost never) use non-const references.

So here you'd want const& or *.

In this specific case however I think putting |histograms_tester_| in the
protected section and having tests access it directly is fine (you can make the
member variable const too since all the methods you're using are const AFAICT).

PS: Didn't know about HistogramTester, seems useful :-).

[tiany, 2015/06/23 18:36:35]

Done.

+
+  // Decide to corrupt or not depending on the params passed into the test
+  const bool do_corrupt_;
+
+ private:
+  // Returns true if this is the PRE_ phase of the test.
+  bool IsPRETest() {
+    return base::StartsWithASCII(
+        testing::UnitTest::GetInstance()->current_test_info()->name(), "PRE_",
+        true /* case_sensitive */);
+  }
+
+  // Histogram value verifier.
+  base::HistogramTester histograms_tester_;
+};
+
+#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
+#else
+#define MAYBE(test) test
+#endif
+
+IN_PROC_BROWSER_TEST_P(ProfileErrorBrowserTest, MAYBE(PRE_CorruptedProfile)) {
+  // Nothing to do, the purpose of this PRE test is only to bring up a default
+  // User Data directory.
+}
+
+IN_PROC_BROWSER_TEST_P(ProfileErrorBrowserTest, MAYBE(CorruptedProfile)) {
+  const char* kPaintHistogram = "Startup.FirstWebContents.NonEmptyPaint";

[gab, 2015/06/16 19:55:34]

const char kFoo[] instead of const char* kFoo for string literals.

[tiany, 2015/06/23 18:36:35]

Done.

+  const char* kFrameHistogram = "Startup.FirstWebContents.MainFrameLoad";
+
+  // Navigate to a URL so the first non-empty paint is registered.
+  ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/"));

[gab, 2015/06/16 19:55:34]

Best practice is to use http://example.com as a dummy but real site URL.

[tiany, 2015/06/23 18:36:35]

Done.

+  if (do_corrupt_) {
+    histograms().ExpectTotalCount(kPaintHistogram, 0);
+    histograms().ExpectTotalCount(kFrameHistogram, 0);
+  } else {
+    histograms().ExpectTotalCount(kPaintHistogram, 1);
+    histograms().ExpectTotalCount(kFrameHistogram, 1);
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(ProfileErrorBrowserTestInstance,
+                        ProfileErrorBrowserTest,
+                        testing::Bool());
+
+}  // namespace