Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(911)

Unified Diff: chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc

Issue 2859393002: Report page load timing information for child frames. (Closed)
Patch Set: add comment Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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..f46e4a841785ef4ac8abcb5227349b39c4cbc338 100644
--- a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
+++ b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "base/command_line.h"
+#include "base/feature_list.h"
#include "base/files/scoped_temp_dir.h"
#include "base/macros.h"
#include "base/test/histogram_tester.h"
@@ -28,8 +30,10 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_view_host.h"
+#include "content/public/common/content_switches.h"
#include "content/public/test/browser_test_utils.h"
#include "content/public/test/download_test_observer.h"
+#include "net/dns/mock_host_resolver.h"
#include "net/http/failing_http_transaction_factory.h"
#include "net/http/http_cache.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
@@ -45,9 +49,11 @@ class PageLoadMetricsWaiter
public:
// A bitvector to express which timing fields to match on.
enum ExpectedTimingFields {
jkarlin 2017/05/10 16:55:28 enum class
Bryan McQuade 2017/05/10 18:06:15 thanks! i moved this whole change to https://coder
- FIRST_PAINT = 1 << 0,
- FIRST_CONTENTFUL_PAINT = 1 << 1,
- STYLE_UPDATE_BEFORE_FCP = 1 << 2
+ FIRST_LAYOUT = 1 << 0,
+ FIRST_PAINT = 1 << 1,
+ FIRST_CONTENTFUL_PAINT = 1 << 2,
+ STYLE_UPDATE_BEFORE_FCP = 1 << 3,
+ DOCUMENT_WRITE_BLOCK_RELOAD = 1 << 4
};
explicit PageLoadMetricsWaiter(content::WebContents* web_contents)
@@ -56,14 +62,11 @@ 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(ExpectedTimingFields fields) {
+ main_frame_expected_fields_ |= fields;
}
-
- // Instructs observer to also watch for |count|
- // WebLoadingBehaviorDocumentWriteBlockReload events.
- void ExpectDocumentWriteBlockReload(int count) {
- match_document_write_block_reload_ = count;
+ void AddSubFrameExpectation(ExpectedTimingFields fields) {
+ child_frame_expected_fields_ |= fields;
}
// Waits for a TimingUpdated IPC that matches the fields set by
@@ -81,26 +84,40 @@ class PageLoadMetricsWaiter
}
private:
+ static int GetMatchedBits(
+ const page_load_metrics::PageLoadTiming& timing,
+ const page_load_metrics::PageLoadMetadata& metadata) {
+ int matched_bits = 0;
+ if (timing.document_timing.first_layout)
+ matched_bits |= FIRST_LAYOUT;
+ if (timing.paint_timing.first_paint)
+ matched_bits |= FIRST_PAINT;
+ if (timing.paint_timing.first_contentful_paint)
+ matched_bits |= FIRST_CONTENTFUL_PAINT;
+ if (timing.style_sheet_timing.update_style_duration_before_fcp)
+ matched_bits |= STYLE_UPDATE_BEFORE_FCP;
+ if (metadata.behavior_flags &
+ blink::WebLoadingBehaviorFlag::
+ kWebLoadingBehaviorDocumentWriteBlockReload)
+ matched_bits |= 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)) {
+ int matched_bits = GetMatchedBits(timing, metadata);
+ if (is_main_frame)
+ main_frame_expected_fields_ &= ~matched_bits;
jkarlin 2017/05/10 16:55:28 Maybe add a comment that we're incrementally remov
Bryan McQuade 2017/05/10 18:06:15 i think the refactor i did in the other change mak
+ else
+ child_frame_expected_fields_ &= ~matched_bits;
+
+ if (main_frame_expected_fields_ == 0 && child_frame_expected_fields_ == 0) {
expectations_satisfied_ = true;
if (run_loop_)
run_loop_->Quit();
@@ -108,11 +125,26 @@ class PageLoadMetricsWaiter
}
std::unique_ptr<base::RunLoop> run_loop_;
- int matching_fields_ = 0; // A bitvector composed from ExpectedTimingFields.
+
+ // Bitvectors composed from ExpectedTimingFields.
+ int child_frame_expected_fields_ = 0;
+ int main_frame_expected_fields_ = 0;
+
bool expectations_satisfied_ = false;
jkarlin 2017/05/10 16:55:28 expectations_satisfied_ can be replaced with main_
Bryan McQuade 2017/05/10 18:06:15 done in other change
- int match_document_write_block_reload_ = 0;
};
+// Due to crbug/705315, paints in child frames are associated with the main
+// frame, unless the child frame is cross-origin and Chrome is running with out
+// of process cross-origin child frames. As a result, some tests wait for
+// different behavior to be observed depending on which mode we are in.
+// TODO(crbug/705315): remove this method once the bug is addressed.
+static bool AreCrossOriginChildFramesOutOfProcess() {
+ return base::CommandLine::ForCurrentProcess()->HasSwitch(
+ switches::kSitePerProcess) ||
+ base::CommandLine::ForCurrentProcess()->HasSwitch(
+ switches::kTopDocumentIsolation);
+}
+
} // namespace
class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
@@ -125,6 +157,15 @@ class PageLoadMetricsBrowserTest : public InProcessBrowserTest {
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
}
+ // TODO(crbug/705315): remove this method once the bug is addressed.
+ void SetUpOnMainThread() override {
+ InProcessBrowserTest::SetUpOnMainThread();
+ host_resolver()->AddRule("a.com", "127.0.0.1");
+ host_resolver()->AddRule("b.com", "127.0.0.1");
+ host_resolver()->AddRule("c.com", "127.0.0.1");
+ content::SetupCrossSiteRedirector(embedded_test_server());
+ }
+
bool NoPageLoadMetricsRecorded() {
// Determine whether any 'public' page load metrics are recorded. We exclude
// 'internal' metrics as these may be recorded for debugging purposes.
@@ -169,13 +210,18 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoNavigation) {
IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NewPage) {
ASSERT_TRUE(embedded_test_server()->Start());
+ std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
+ CreatePageLoadMetricsWaiter();
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/title1.html"));
+ timing_waiter->Wait();
NavigateToUntrackedUrl();
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);
@@ -190,6 +236,121 @@ 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> timing_waiter =
+ CreatePageLoadMetricsWaiter();
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::FIRST_LAYOUT);
+ ui_test_utils::NavigateToURL(browser(),
+ embedded_test_server()->GetURL("/empty.html"));
+ timing_waiter->Wait();
+
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 0);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+ 0);
+}
+
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
+ NoPaintForEmptyDocumentInChildFrame) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+
+ std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
+ CreatePageLoadMetricsWaiter();
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::FIRST_LAYOUT);
+ timing_waiter->AddSubFrameExpectation(PageLoadMetricsWaiter::FIRST_LAYOUT);
+ // TODO(crbug/705315): remove the a.com domain once the bug is addressed.
+ GURL a_url(embedded_test_server()->GetURL(
+ "a.com", "/page_load_metrics/empty_iframe.html"));
+ ui_test_utils::NavigateToURL(browser(), a_url);
+ timing_waiter->Wait();
+
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 0);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
+ 0);
+}
+
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PaintInChildFrame) {
jkarlin 2017/05/10 16:55:28 I'd like to see a set of tests w/ mocked navigatio
Bryan McQuade 2017/05/10 18:06:15 i'd like to do this in a unit test too, and we onc
+ ASSERT_TRUE(embedded_test_server()->Start());
+
+ std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
+ CreatePageLoadMetricsWaiter();
+ // TODO(crbug/705315): Once the bug is fixed, remove the else case and make
+ // the if case the default behavior.
+ if (AreCrossOriginChildFramesOutOfProcess()) {
+ timing_waiter->AddSubFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddSubFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ } else {
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ }
+ // TODO(crbug/705315): remove the a.com domain once the bug is addressed.
+ GURL a_url(embedded_test_server()->GetURL("a.com",
+ "/page_load_metrics/iframe.html"));
+ ui_test_utils::NavigateToURL(browser(), a_url);
+ timing_waiter->Wait();
+
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 1);
+}
+
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PaintInMultipleChildFrames) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+
+ std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
+ CreatePageLoadMetricsWaiter();
+ // TODO(crbug/705315): Once the bug is fixed, remove the else case and make
+ // the if case the default behavior.
+ if (AreCrossOriginChildFramesOutOfProcess()) {
+ timing_waiter->AddSubFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddSubFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ } else {
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ }
+
+ // TODO(crbug/705315): remove the a.com domain once the bug is addressed.
+ GURL a_url(embedded_test_server()->GetURL("a.com",
+ "/page_load_metrics/iframes.html"));
+ ui_test_utils::NavigateToURL(browser(), a_url);
+ timing_waiter->Wait();
+
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 1);
+}
+
+IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PaintInMainAndChildFrame) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+
+ std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
+ CreatePageLoadMetricsWaiter();
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ // TODO(crbug/705315): Once the bug is fixed, make the if case the default
+ // behavior.
+ if (AreCrossOriginChildFramesOutOfProcess()) {
+ timing_waiter->AddSubFrameExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddSubFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ }
+
+ // TODO(crbug/705315): remove the a.com domain once the bug is addressed.
+ GURL a_url(embedded_test_server()->GetURL(
+ "a.com", "/page_load_metrics/main_frame_with_iframe.html"));
+ ui_test_utils::NavigateToURL(browser(), a_url);
+ timing_waiter->Wait();
+
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 1);
+ histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 1);
+}
+
IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, SameDocumentNavigation) {
ASSERT_TRUE(embedded_test_server()->Start());
@@ -298,7 +459,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, PreloadDocumentWrite) {
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
@@ -326,7 +488,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, NoDocumentWrite) {
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/title1.html"));
@@ -344,7 +507,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteBlock) {
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
@@ -361,28 +525,37 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, DocumentWriteReload) {
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);
-
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/document_write_script_block.html"));
+ fcp_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(
+ PageLoadMetricsWaiter::DOCUMENT_WRITE_BLOCK_RELOAD);
+ reload_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/document_write_script_block.html"));
+ reload_waiter->Wait();
+ reload_waiter = CreatePageLoadMetricsWaiter();
+ reload_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::DOCUMENT_WRITE_BLOCK_RELOAD);
+ reload_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::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(
@@ -443,7 +616,7 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, MAYBE_BadXhtml) {
std::unique_ptr<PageLoadMetricsWaiter> timing_waiter =
CreatePageLoadMetricsWaiter();
- timing_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_PAINT);
+ timing_waiter->AddMainFrameExpectation(PageLoadMetricsWaiter::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
@@ -643,7 +816,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
@@ -668,7 +842,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/title1.html"));
@@ -687,7 +862,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest,
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/nostore.html"));
@@ -705,7 +881,8 @@ IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, CSSTiming) {
std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::STYLE_UPDATE_BEFORE_FCP);
+ fcp_waiter->AddMainFrameExpectation(
+ PageLoadMetricsWaiter::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

Powered by Google App Engine
This is Rietveld 408576698