Various cleanups for page_load_metrics_browsertest

chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

Index: chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
diff --git a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
index 1b52ea8f37126e974d13ff08f527fb609c81d5e5..34221d7f5acbd0fba36a7e2066e32177b7eddaef 100644
--- a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
+++ b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
@@ -39,15 +39,32 @@
 
 namespace {
 
+void FailAllNetworkTransactions(net::URLRequestContextGetter* getter) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  net::HttpCache* cache(
+      getter->GetURLRequestContext()->http_transaction_factory()->GetCache());
+  DCHECK(cache);
+  std::unique_ptr<net::FailingHttpTransactionFactory> factory(
+      new net::FailingHttpTransactionFactory(cache->GetSession(),

[jkarlin, 2017/05/10 18:45:05]

base::MakeUnique while we're here :)

[Bryan McQuade, 2017/05/10 19:14:33]

Done

+                                             net::ERR_FAILED));

[jkarlin, 2017/05/10 18:45:04]

Include the net error header

[Bryan McQuade, 2017/05/10 19:14:33]

Done

+  // Throw away old version; since this is a browser test, there is no
+  // need to restore the old state.
+  cache->SetHttpNetworkTransactionFactoryForTesting(std::move(factory));
+}
+
 // Waits until specified timing and metadata expectations are satisfied.
 class PageLoadMetricsWaiter
     : public page_load_metrics::MetricsWebContentsObserver::TestingObserver {
  public:
   // A bitvector to express which timing fields to match on.
-  enum ExpectedTimingFields {
-    FIRST_PAINT = 1 << 0,
-    FIRST_CONTENTFUL_PAINT = 1 << 1,
-    STYLE_UPDATE_BEFORE_FCP = 1 << 2
+  enum class ExpectedTimingField : int {
+    FIRST_LAYOUT = 1 << 0,
+    FIRST_PAINT = 1 << 1,
+    FIRST_CONTENTFUL_PAINT = 1 << 2,
+    FIRST_MEANINGFUL_PAINT = 1 << 3,
+    STYLE_UPDATE_BEFORE_FCP = 1 << 4,
+    DOCUMENT_WRITE_BLOCK_RELOAD = 1 << 5,
+    LOAD_EVENT = 1 << 6
   };
 
   explicit PageLoadMetricsWaiter(content::WebContents* web_contents)
@@ -56,63 +73,102 @@ class PageLoadMetricsWaiter
   ~PageLoadMetricsWaiter() override { DCHECK_EQ(nullptr, run_loop_.get()); }
 
   // Add the given expectation to match on.
-  void AddExpectation(ExpectedTimingFields fields) {
-    matching_fields_ |= fields;
+  void AddMainFrameExpectation(ExpectedTimingField field) {
+    main_frame_expected_fields_.set(field);
   }
-
-  // Instructs observer to also watch for |count|
-  // WebLoadingBehaviorDocumentWriteBlockReload events.
-  void ExpectDocumentWriteBlockReload(int count) {
-    match_document_write_block_reload_ = count;
+  void AddSubFrameExpectation(ExpectedTimingField field) {
+    child_frame_expected_fields_.set(field);
   }
 
   // Waits for a TimingUpdated IPC that matches the fields set by
-  // |AddExpectation|.  All matching fields must be set in a TimingUpdated
-  // IPC for it to end this wait.
+  // |AddMainFrameExpectation| and |AddSubFrameExpectation|. All matching fields
+  // must be set in a TimingUpdated IPC for it to end this wait.
   void Wait() {
-    if (expectations_satisfied_)
+    if (expectations_satisfied())
       return;
 
     run_loop_.reset(new base::RunLoop());
     run_loop_->Run();
     run_loop_.reset(nullptr);
 
-    EXPECT_TRUE(expectations_satisfied_);
+    EXPECT_TRUE(expectations_satisfied());
   }
 
  private:
+  // Manages a bitset of ExpectedTimingFields.
+  class ExpectedTimingFieldBitSet {
+   public:
+    ExpectedTimingFieldBitSet() {}
+
+    // Returns whether this bitset has all bits unset.
+    bool empty() const { return bitmask_ == 0; }

[jkarlin, 2017/05/10 18:45:04]

Empty

[Bryan McQuade, 2017/05/10 19:14:32]

Done. I interpreted the style guide as indicating this is ok style, but perhaps
these aren't quite accessors / mutators:
https://google.github.io/styleguide/cppguide.html#Function_Names "accessors and
mutators may be named like variables."

[jkarlin, 2017/05/11 14:04:05]

I mostly agree with you that they could be lowercase, but clear and empty are
slightly more complex than getting and setting. Grep wars shows 3x as many
Empty() functions as empty().

+
+    // Sets the bit for the given |field|.
+    void set(ExpectedTimingField field) { bitmask_ |= static_cast<int>(field); }

[jkarlin, 2017/05/10 18:45:05]

Set

[Bryan McQuade, 2017/05/10 19:14:32]

Done

+
+    // Clears all bits set in the |other| bitset.
+    void clear(const ExpectedTimingFieldBitSet& other) {

[jkarlin, 2017/05/10 18:45:04]

Typically I think of clear as clearing everything. Perhaps, ClearMatchingBits?

[Bryan McQuade, 2017/05/10 19:14:33]

changed to ClearMatching.

+      bitmask_ &= ~other.bitmask_;
+    }
+
+   private:
+    int bitmask_ = 0;
+  };
+
+  static ExpectedTimingFieldBitSet GetMatchedBits(
+      const page_load_metrics::PageLoadTiming& timing,
+      const page_load_metrics::PageLoadMetadata& metadata) {
+    ExpectedTimingFieldBitSet matched_bits;
+    if (timing.document_timing.first_layout)
+      matched_bits.set(ExpectedTimingField::FIRST_LAYOUT);
+    if (timing.document_timing.load_event_start)
+      matched_bits.set(ExpectedTimingField::LOAD_EVENT);
+    if (timing.paint_timing.first_paint)
+      matched_bits.set(ExpectedTimingField::FIRST_PAINT);
+    if (timing.paint_timing.first_contentful_paint)
+      matched_bits.set(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
+    if (timing.paint_timing.first_meaningful_paint)
+      matched_bits.set(ExpectedTimingField::FIRST_MEANINGFUL_PAINT);
+    if (timing.style_sheet_timing.update_style_duration_before_fcp)
+      matched_bits.set(ExpectedTimingField::STYLE_UPDATE_BEFORE_FCP);
+    if (metadata.behavior_flags &
+        blink::WebLoadingBehaviorFlag::
+            kWebLoadingBehaviorDocumentWriteBlockReload)
+      matched_bits.set(ExpectedTimingField::DOCUMENT_WRITE_BLOCK_RELOAD);
+
+    return matched_bits;
+  }
+
   void OnTimingUpdated(
+      bool is_main_frame,
       const page_load_metrics::PageLoadTiming& timing,
       const page_load_metrics::PageLoadMetadata& metadata) override {
-    if (match_document_write_block_reload_ > 0 &&
-        metadata.behavior_flags &
-            blink::WebLoadingBehaviorFlag::
-                kWebLoadingBehaviorDocumentWriteBlockReload) {
-      --match_document_write_block_reload_;
-    }
-
-    if (match_document_write_block_reload_ > 0) {
+    if (expectations_satisfied())
       return;
-    }
 
-    if ((!(matching_fields_ & FIRST_PAINT) ||
-         timing.paint_timing.first_paint) &&
-        (!(matching_fields_ & FIRST_CONTENTFUL_PAINT) ||
-         timing.paint_timing.first_contentful_paint) &&
-        (!(matching_fields_ & STYLE_UPDATE_BEFORE_FCP) ||
-         timing.style_sheet_timing.update_style_duration_before_fcp)) {
-      expectations_satisfied_ = true;
-      if (run_loop_)
-        run_loop_->Quit();
-    }
+    ExpectedTimingFieldBitSet matched_bits = GetMatchedBits(timing, metadata);
+    if (is_main_frame)
+      main_frame_expected_fields_.clear(matched_bits);
+    else
+      child_frame_expected_fields_.clear(matched_bits);
+
+    if (expectations_satisfied() && run_loop_)
+      run_loop_->Quit();
+  }
+
+  bool expectations_satisfied() const {
+    return child_frame_expected_fields_.empty() &&
+           main_frame_expected_fields_.empty();
   }
 
   std::unique_ptr<base::RunLoop> run_loop_;
-  int matching_fields_ = 0;  // A bitvector composed from ExpectedTimingFields.
-  bool expectations_satisfied_ = false;
-  int match_document_write_block_reload_ = 0;
+
+  ExpectedTimingFieldBitSet child_frame_expected_fields_;
+  ExpectedTimingFieldBitSet main_frame_expected_fields_;
 };
 
+using ExpectedTimingField = PageLoadMetricsWaiter::ExpectedTimingField;
+
 }  // namespace
 
 class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
@@ -121,6 +177,10 @@ class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
   ~PageLoadMetricsBrowserTest() override {}
 
  protected:
+  // Force navigation to a new page, so the currently tracked page load runs its
+  // OnComplete callback. You should prefer to use PageLoadMetricsWaiter, and
+  // only use NavigateToUntrackedUrl for cases where the waiter isn't
+  // sufficient.
   void NavigateToUntrackedUrl() {
     ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
   }
@@ -148,19 +208,6 @@ class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
   DISALLOW_COPY_AND_ASSIGN(PageLoadMetricsBrowserTest);
 };
 
-void FailAllNetworkTransactions(net::URLRequestContextGetter* getter) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-  net::HttpCache* cache(
-      getter->GetURLRequestContext()->http_transaction_factory()->GetCache());
-  DCHECK(cache);
-  std::unique_ptr<net::FailingHttpTransactionFactory> factory(
-      new net::FailingHttpTransactionFactory(cache->GetSession(),
-                                             net::ERR_FAILED));
-  // Throw away old version; since this is a browser test, there is no
-  // need to restore the old state.
-  cache->SetHttpNetworkTransactionFactoryForTesting(std::move(factory));
-}
-
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoNavigation) {
   ASSERT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(NoPageLoadMetricsRecorded());
@@ -169,18 +216,25 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoNavigation) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NewPage) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();

[jkarlin, 2017/05/10 18:45:05]

optional: auto seems reasonable here

[Bryan McQuade, 2017/05/10 19:14:32]

done

+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_PAINT);

[jkarlin, 2017/05/10 18:45:04]

Seems like it would be useful to have a constructor like so:

auto waiter = CreatePageLoadMetricsWaiter(ExpectedTimingField::FIRST_PAINT);
then you could construct a waiter with all of the flags you want or'd together.

[Bryan McQuade, 2017/05/10 19:14:32]

I'm disinclined to do this since we have main and child frames & it's not
totally clear at the callsite whether the constructor is for main or child. I
think main is a logical default but overall I think the loss of clarity doesn't
make up for saving one line of code.

[jkarlin, 2017/05/11 14:04:05]

Fair point. The constructor could take two values, but then it wouldn't be clear
which is which.

   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
-  NavigateToUntrackedUrl();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(internal::kHistogramDomContentLoaded, 1);
   histogram_tester_.ExpectTotalCount(internal::kHistogramLoad, 1);
   histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 1);
   histogram_tester_.ExpectTotalCount(internal::kHistogramParseDuration, 1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramParseBlockedOnScriptLoad, 1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramParseBlockedOnScriptExecution, 1);
+
+  // Force navigation to another page, which should force logging of histograms
+  // persisted at the end of the page load lifetime.
+  NavigateToUntrackedUrl();
   histogram_tester_.ExpectTotalCount(internal::kHistogramTotalBytes, 1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramPageTimingForegroundDuration, 1);
@@ -190,11 +244,38 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NewPage) {
   EXPECT_FALSE(NoPageLoadMetricsRecorded());
 }
 
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoPaintForEmptyDocument) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();

[jkarlin, 2017/05/10 18:45:05]

What if you could also create a set of unexpected matching bits that the test
would fail on if they were detected?

[Bryan McQuade, 2017/05/10 19:14:33]

Sure, I added an API for this.

+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_LAYOUT);
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/empty.html"));
+  waiter->Wait();
+
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramLoad, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 0);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     0);
+}
+
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, SameDocumentNavigation) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_LAYOUT);
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
+  waiter->Wait();
+
+  histogram_tester_.ExpectTotalCount(internal::kHistogramDomContentLoaded, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramLoad, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+
+  // Perform a same-document navigation. No additional metrics should be logged.
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL("/title1.html#hash"));
   NavigateToUntrackedUrl();
@@ -207,11 +288,23 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, SameDocumentNavigation) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, SameUrlNavigation) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_LAYOUT);
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
+  waiter->Wait();
+
+  histogram_tester_.ExpectTotalCount(internal::kHistogramDomContentLoaded, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramLoad, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+
+  waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_LAYOUT);
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
-  NavigateToUntrackedUrl();
+  waiter->Wait();
 
   // We expect one histogram sample for each navigation to title1.html.
   histogram_tester_.ExpectTotalCount(internal::kHistogramDomContentLoaded, 2);
@@ -296,14 +389,13 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, IgnoreDownloads) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PreloadDocumentWrite) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
 
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_external_script.html"));
-  fcp_waiter->Wait();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteParseStartToFirstContentfulPaint, 1);
@@ -312,11 +404,15 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PreloadDocumentWrite) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoPreloadDocumentWrite) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_no_script.html"));
-  NavigateToUntrackedUrl();
+  waiter->Wait();
 
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteParseStartToFirstContentfulPaint, 0);
 }
@@ -324,14 +420,15 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoPreloadDocumentWrite) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoDocumentWrite) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
 
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
-  fcp_waiter->Wait();
+  waiter->Wait();
 
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteParseStartToFirstContentfulPaint, 0);
   histogram_tester_.ExpectTotalCount(
@@ -342,14 +439,13 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoDocumentWrite) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteBlock) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
 
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_script_block.html"));
-  fcp_waiter->Wait();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 1);
@@ -359,30 +455,40 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteBlock) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteReload) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
-  std::unique_ptr<PageLoadMetricsWaiter> reload_waiter =
-      CreatePageLoadMetricsWaiter();
-  reload_waiter->ExpectDocumentWriteBlockReload(2);
-
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_script_block.html"));
+  waiter->Wait();
+
+  histogram_tester_.ExpectTotalCount(
+      internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 1);
+  histogram_tester_.ExpectTotalCount(internal::kHistogramDocWriteBlockCount, 1);
 
   // Reload should not log the histogram as the script is not blocked.
+  std::unique_ptr<PageLoadMetricsWaiter> reload_waiter =
+      CreatePageLoadMetricsWaiter();
+  reload_waiter->AddMainFrameExpectation(
+      ExpectedTimingField::DOCUMENT_WRITE_BLOCK_RELOAD);
+  reload_waiter->AddMainFrameExpectation(
+      ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_script_block.html"));
+  reload_waiter->Wait();
+
+  histogram_tester_.ExpectTotalCount(
+      internal::kHistogramDocWriteBlockReloadCount, 1);
 
+  reload_waiter = CreatePageLoadMetricsWaiter();
+  reload_waiter->AddMainFrameExpectation(
+      ExpectedTimingField::DOCUMENT_WRITE_BLOCK_RELOAD);
+  reload_waiter->AddMainFrameExpectation(
+      ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_script_block.html"));
-
-  histogram_tester_.ExpectTotalCount(
-      internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 1);
-
-  fcp_waiter->Wait();
   reload_waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(
@@ -396,11 +502,15 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteReload) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteAsync) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
-                     "/page_load_metrics/document_write_script_async.html"));
-  NavigateToUntrackedUrl();
+                     "/page_load_metrics/document_write_async_script.html"));
+  waiter->Wait();
 
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 0);
   histogram_tester_.ExpectTotalCount(internal::kHistogramDocWriteBlockCount, 0);
@@ -409,11 +519,15 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteAsync) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteSameDomain) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_external_script.html"));
-  NavigateToUntrackedUrl();
+  waiter->Wait();
 
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 0);
   histogram_tester_.ExpectTotalCount(internal::kHistogramDocWriteBlockCount, 0);
@@ -422,11 +536,15 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteSameDomain) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoDocumentWriteScript) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/document_write_no_script.html"));
-  NavigateToUntrackedUrl();
+  waiter->Wait();
 
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     1);
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 0);
   histogram_tester_.ExpectTotalCount(internal::kHistogramDocWriteBlockCount, 0);
@@ -441,9 +559,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoDocumentWriteScript) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, MAYBE_BadXhtml) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
-      CreatePageLoadMetricsWaiter();
-  timing_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_PAINT);
 
   // When an XHTML page contains invalid XML, it causes a paint of the error
   // message without a layout. Page load metrics currently treats this as an
@@ -454,7 +571,7 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, MAYBE_BadXhtml) {
       browser(),
       embedded_test_server()->GetURL("/page_load_metrics/badxml.xhtml"));
 
-  timing_waiter->Wait();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 0);
   histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 0);
@@ -488,11 +605,12 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, AbortNewNavigation) {
   GURL url2(embedded_test_server()->GetURL("/title2.html"));
   chrome::NavigateParams params2(browser(), url2,
                                  ui::PAGE_TRANSITION_FROM_ADDRESS_BAR);
-  content::TestNavigationManager manager2(
-      browser()->tab_strip_model()->GetActiveWebContents(), url2);
+
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   chrome::Navigate(&params2);
+  waiter->Wait();
 
-  manager2.WaitForNavigationFinished();
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramAbortNewNavigationBeforeCommit, 1);
 }
@@ -509,11 +627,12 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, AbortReload) {
   EXPECT_TRUE(manager.WaitForRequestStart());
 
   chrome::NavigateParams params2(browser(), url, ui::PAGE_TRANSITION_RELOAD);
-  content::TestNavigationManager manager2(
-      browser()->tab_strip_model()->GetActiveWebContents(), url);
+
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   chrome::Navigate(&params2);
+  waiter->Wait();
 
-  manager2.WaitForNavigationFinished();
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramAbortReloadBeforeCommit, 1);
 }
@@ -565,15 +684,14 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, AbortMultiple) {
 
   GURL url3(embedded_test_server()->GetURL("/title3.html"));
   chrome::NavigateParams params3(browser(), url3, ui::PAGE_TRANSITION_TYPED);
-  content::TestNavigationManager manager3(
-      browser()->tab_strip_model()->GetActiveWebContents(), url3);
+
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   chrome::Navigate(&params3);
+  waiter->Wait();
 
-  EXPECT_TRUE(manager3.WaitForRequestStart());
   manager2.WaitForNavigationFinished();
 
-  manager3.WaitForNavigationFinished();
-
   histogram_tester_.ExpectTotalCount(
       internal::kHistogramAbortNewNavigationBeforeCommit, 2);
 }
@@ -594,11 +712,15 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
   EXPECT_TRUE(manager.WaitForRequestStart());
 
   {
+    std::unique_ptr<PageLoadMetricsWaiter> waiter =
+        CreatePageLoadMetricsWaiter();
+    waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
     content::TestNavigationManager reload_manager(
         browser()->tab_strip_model()->GetActiveWebContents(), first_url);
     EXPECT_TRUE(content::ExecuteScript(
         browser()->tab_strip_model()->GetActiveWebContents(),
         "window.location.reload();"));
+    waiter->Wait();
   }
 
   manager.WaitForNavigationFinished();
@@ -612,9 +734,13 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
                        FirstMeaningfulPaintRecorded) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_MEANINGFUL_PAINT);
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
+  waiter->Wait();
 
+#if 0
   // Wait until the renderer finishes observing layouts.
   const int kNetworkIdleTime = 3000;
   const int kMargin = 500;
@@ -626,8 +752,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
       browser()->tab_strip_model()->GetActiveWebContents(),
       javascript, &result));
   EXPECT_TRUE(result);
+#endif
 
-  NavigateToUntrackedUrl();
   histogram_tester_.ExpectUniqueSample(
       internal::kHistogramFirstMeaningfulPaintStatus,
       internal::FIRST_MEANINGFUL_PAINT_RECORDED, 1);
@@ -641,18 +767,19 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
                        FirstMeaningfulPaintNotRecorded) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
 
   ui_test_utils::NavigateToURL(
       browser(), embedded_test_server()->GetURL(
                      "/page_load_metrics/page_with_active_connections.html"));
-  fcp_waiter->Wait();
+  waiter->Wait();
 
   // Navigate away before a FMP is reported.
   NavigateToUntrackedUrl();
 
+  histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+                                     1);
   histogram_tester_.ExpectUniqueSample(
       internal::kHistogramFirstMeaningfulPaintStatus,
       internal::FIRST_MEANINGFUL_PAINT_DID_NOT_REACH_NETWORK_STABLE, 1);
@@ -666,14 +793,13 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
                        NoStatePrefetchObserverCacheable) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
 
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/title1.html"));
 
-  fcp_waiter->Wait();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(
       "Prerender.none_PrefetchTTFCP.Reference.NoStore.Visible", 0);
@@ -685,14 +811,13 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
                        NoStatePrefetchObserverNoStore) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::FIRST_CONTENTFUL_PAINT);
 
   ui_test_utils::NavigateToURL(browser(),
                                embedded_test_server()->GetURL("/nostore.html"));
 
-  fcp_waiter->Wait();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(
       "Prerender.none_PrefetchTTFCP.Reference.NoStore.Visible", 1);
@@ -703,9 +828,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, CSSTiming) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
-      CreatePageLoadMetricsWaiter();
-  fcp_waiter->AddExpectation(PageLoadMetricsWaiter::STYLE_UPDATE_BEFORE_FCP);
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::STYLE_UPDATE_BEFORE_FCP);
 
   // Careful: Blink code clamps timestamps to 5us, so any CSS parsing we do here
   // must take >> 5us, otherwise we'll log 0 for the value and it will remain
@@ -713,8 +837,7 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, CSSTiming) {
   ui_test_utils::NavigateToURL(
       browser(),
       embedded_test_server()->GetURL("/page_load_metrics/page_with_css.html"));
-  NavigateToUntrackedUrl();
-  fcp_waiter->Wait();
+  waiter->Wait();
 
   histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
                                      1);
@@ -729,8 +852,14 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, CSSTiming) {
 IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PayloadSize) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
+  std::unique_ptr<PageLoadMetricsWaiter> waiter = CreatePageLoadMetricsWaiter();
+  waiter->AddMainFrameExpectation(ExpectedTimingField::LOAD_EVENT);
   ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL(
                                               "/page_load_metrics/large.html"));
+  waiter->Wait();
+
+  // Payload histograms are only logged when a page load terminates, so force
+  // navigation to another page.
   NavigateToUntrackedUrl();
 
   histogram_tester_.ExpectTotalCount(internal::kHistogramTotalBytes, 1);