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

chrome/browser/ui/profile_error_browsertest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.

[huangs, 2015/06/12 18:16:14]

2015

[tiany, 2015/06/16 14:37:18]

Done.

+// 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/metrics/histogram_base.h"

[huangs, 2015/06/12 18:16:14]

#include "base/memory/scoped_ptr.h"

[tiany, 2015/06/16 14:37:18]

good catch! thanks.

+#include "base/metrics/histogram_samples.h"
+#include "base/metrics/statistics_recorder.h"
+#include "base/path_service.h"
+#include "base/prefs/pref_service.h"
+#include "base/strings/string_util.h"
+#include "base/values.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/simple_message_box_internal.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.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 {
+

[huangs, 2015/06/12 18:16:14]

Nit: End sentence with "."

[tiany, 2015/06/16 14:37:18]

Done.

+// Returns the number of times |histogram_name| was reported so far;
+bool StartupHistogramRecorded(const char* histogram_name) {
+  const base::HistogramBase* histogram =
+      base::StatisticsRecorder::FindHistogram(histogram_name);
+  if (!histogram)
+    return false;
+
+  scoped_ptr<base::HistogramSamples> samples(histogram->SnapshotSamples());
+  return (samples->TotalCount() > 0);

[gab, 2015/06/12 18:12:38]

@asvitkine: it's not clear to me from the
base::StatisticsRecorder::FindHistogram API, if it succeeds, is it guaranteed
that there are >= 1 samples (i.e. we could replace this entire method with
base::StatisticsRecorder::FindHistogram(histogram_name))?

[Alexei Svitkine (slow), 2015/06/15 20:06:40]

Chatted with Alex offline - suggested to use HistogramTester helper class.

+}
+
+class ProfileErrorBrowserTest : public InProcessBrowserTest,
+                                public testing::WithParamInterface<bool> {
+ public:
+  ProfileErrorBrowserTest() : do_corrupt_(GetParam()) {}
+
+  void SetUp() override {
+    // Imitate the behavior but do not show the message box so the test can
+    // terminate
+    chrome::internal::g_should_skip_message_box_for_test = true;

[gab, 2015/06/12 18:12:38]

Only do this if !IsPRETest(); it shouldn't matter in practice, but this makes it
even more explicit what this is intended for.

+    InProcessBrowserTest::SetUp();

[gab, 2015/06/12 18:12:38]

Do this first (i.e. in general when overriding SetUp(), call the super class'
SetUp then do yours; and on TearDown() you'd do yours first then call theirs --
just like the construction/destruction stack).

+  }
+
+  bool SetUpUserDataDirectory() override {
+    // Setup normally in the PRE test and corrupt user profile in the main test.
+    if (IsPRETest())
+      return InProcessBrowserTest::SetUpUserDataDirectory();
+
+    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()));
+  }
+
+  bool do_corrupt_;

[gab, 2015/06/12 18:12:38]

Make this const and add a comment.

+
+ private:
+  // Returns true if this is the PRE_ phase of the test.
+  bool IsPRETest() {
+    return StartsWithASCII(
+        testing::UnitTest::GetInstance()->current_test_info()->name(), "PRE_",
+        true /* case_sensitive */);
+  }
+};
+
+#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)) {
+}

[gab, 2015/06/12 18:12:38]

Add a comment in the body to explain the purpose to a reader, e.g.:

// Nothing to do, the purpose of this PRE test is only to bring up a default a 
// default User Data directory.

+
+IN_PROC_BROWSER_TEST_P(ProfileErrorBrowserTest, MAYBE(CorruptedProfile)) {
+  // Navigate to a URL so the first non-empty paint is registered.
+  ui_test_utils::NavigateToURL(browser(), GURL("http://www.google.com/"));
+  if (do_corrupt_) {
+    EXPECT_FALSE(StartupHistogramRecorded(
+        "Startup.FirstWebContents.NonEmptyPaint"));
+    EXPECT_FALSE(StartupHistogramRecorded(
+        "Startup.FirstWebContents.MainFrameLoad"));
+  } else {
+    EXPECT_TRUE(StartupHistogramRecorded(
+        "Startup.FirstWebContents.NonEmptyPaint"));
+    EXPECT_TRUE(StartupHistogramRecorded(
+        "Startup.FirstWebContents.MainFrameLoad"));
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(ProfileErrorBrowserTestInstance,
+  ProfileErrorBrowserTest, testing::Bool());
+
+}  // namespace