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

Issues Search
    My Issues | Starred     Open | Closed | All

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

Issue 2847803002: Revert of Break page load metrics test dependency on IPC. (Closed)
Patch Set: Created 3 years, 8 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
« no previous file with comments | « chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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 2e54df31d741d14370389f4d6555a932de1fbe4d..8d69c71e8ddf51c0cebb4c566fda09100271e219 100644
--- a/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
+++ b/chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc
@@ -38,9 +38,9 @@
namespace {
-// Waits until specified timing and metadata expectations are satisfied.
-class PageLoadMetricsWaiter
- : public page_load_metrics::MetricsWebContentsObserver::TestingObserver {
+// Waits until a PageLoadMetricsMsg_TimingUpdated message IPC is received
+// matching a PageLoadTiming. See WaitForMatchingIPC for details.
+class TimingUpdatedObserver : public content::BrowserMessageFilter {
public:
// A bitvector to express which timing fields to match on.
enum ExpectedTimingFields {
@@ -49,40 +49,57 @@
STYLE_UPDATE_BEFORE_FCP = 1 << 2
};
- explicit PageLoadMetricsWaiter(content::WebContents* web_contents)
- : TestingObserver(web_contents) {}
-
- ~PageLoadMetricsWaiter() override { DCHECK_EQ(nullptr, run_loop_.get()); }
-
- // Add the given expectation to match on.
- void AddExpectation(ExpectedTimingFields fields) {
- matching_fields_ |= fields;
- }
-
- // Instructs observer to also watch for |count|
- // WebLoadingBehaviorDocumentWriteBlockReload events.
- void ExpectDocumentWriteBlockReload(int count) {
- match_document_write_block_reload_ = count;
- }
-
- // 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.
- void Wait() {
- if (expectations_satisfied_)
- return;
-
+ explicit TimingUpdatedObserver(content::RenderWidgetHost* render_widget_host)
+ : content::BrowserMessageFilter(PageLoadMetricsMsgStart) {
+ render_widget_host->GetProcess()->AddFilter(this);
+
+ // Roundtrip to the IO thread, to ensure that the filter is properly
+ // installed.
+ content::BrowserThread::PostTaskAndReply(
+ content::BrowserThread::IO, FROM_HERE, base::BindOnce(&base::DoNothing),
+ base::BindOnce(&TimingUpdatedObserver::Quit, this));
run_loop_.reset(new base::RunLoop());
run_loop_->Run();
run_loop_.reset(nullptr);
-
- EXPECT_TRUE(expectations_satisfied_);
+ }
+
+ // Add the given timing fields to the set of fields to match on.
+ void AddMatchingFields(ExpectedTimingFields fields) {
+ matching_fields_ |= fields;
+ }
+
+ // Instructs observer to also watch for |count|
+ // WebLoadingBehaviorDocumentWriteBlockReload events.
+ void MatchDocumentWriteBlockReload(int count) {
+ match_document_write_block_reload_ = count;
+ }
+
+ // Waits for a TimingUpdated IPC that matches the fields set by
+ // |AddMatchingFields|. All matching fields must be set in a TimingUpdated
+ // IPC for it to end this wait.
+ void WaitForMatchingIPC() {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ if (matched_timing_update_)
+ return;
+
+ run_loop_.reset(new base::RunLoop());
+ run_loop_->Run();
+ run_loop_.reset(nullptr);
}
private:
- void OnTimingUpdated(
- const page_load_metrics::PageLoadTiming& timing,
- const page_load_metrics::PageLoadMetadata& metadata) override {
+ bool OnMessageReceived(const IPC::Message& message) override {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+ IPC_BEGIN_MESSAGE_MAP(TimingUpdatedObserver, message)
+ IPC_MESSAGE_HANDLER(PageLoadMetricsMsg_TimingUpdated, OnTimingUpdated)
+ IPC_END_MESSAGE_MAP()
+
+ return false;
+ }
+
+ bool OnTimingUpdated(const page_load_metrics::PageLoadTiming& timing,
+ const page_load_metrics::PageLoadMetadata& metadata) {
if (match_document_write_block_reload_ > 0 &&
metadata.behavior_flags &
blink::WebLoadingBehaviorFlag::
@@ -91,7 +108,7 @@
}
if (match_document_write_block_reload_ > 0) {
- return;
+ return true;
}
if ((!(matching_fields_ & FIRST_PAINT) ||
@@ -100,15 +117,41 @@
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();
+ // Ensure that any other handlers of this message, for example the real
+ // PageLoadMetric observers, get a chance to handle this message before
+ // this waiter unblocks.
+ content::BrowserThread::PostTask(
+ content::BrowserThread::IO, FROM_HERE,
+ base::BindOnce(&TimingUpdatedObserver::BounceTimingUpdate, this,
+ timing, metadata));
}
- }
+ return true;
+ }
+
+ void BounceTimingUpdate(const page_load_metrics::PageLoadTiming& timing,
+ const page_load_metrics::PageLoadMetadata& metadata) {
+ content::BrowserThread::PostTask(
+ content::BrowserThread::UI, FROM_HERE,
+ base::BindOnce(&TimingUpdatedObserver::SetTimingUpdatedAndQuit, this));
+ }
+
+ void Quit() {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ if (run_loop_)
+ run_loop_->Quit();
+ }
+
+ void SetTimingUpdatedAndQuit() {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ matched_timing_update_ = true;
+ Quit();
+ }
+
+ ~TimingUpdatedObserver() override {}
std::unique_ptr<base::RunLoop> run_loop_;
int matching_fields_ = 0; // A bitvector composed from ExpectedTimingFields.
- bool expectations_satisfied_ = false;
+ bool matched_timing_update_ = false;
int match_document_write_block_reload_ = 0;
};
@@ -135,10 +178,12 @@
return total_pageload_histograms - total_internal_histograms == 0;
}
- std::unique_ptr<PageLoadMetricsWaiter> CreatePageLoadMetricsWaiter() {
+ scoped_refptr<TimingUpdatedObserver> CreateTimingUpdatedObserver() {
content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents();
- return base::MakeUnique<PageLoadMetricsWaiter>(web_contents);
+ scoped_refptr<TimingUpdatedObserver> observer(new TimingUpdatedObserver(
+ web_contents->GetRenderViewHost()->GetWidget()));
+ return observer;
}
base::HistogramTester histogram_tester_;
@@ -294,14 +339,15 @@
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);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/document_write_external_script.html"));
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(
internal::kHistogramDocWriteParseStartToFirstContentfulPaint, 1);
@@ -322,13 +368,14 @@
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);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/title1.html"));
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(
internal::kHistogramDocWriteParseStartToFirstContentfulPaint, 0);
@@ -340,14 +387,15 @@
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);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/document_write_script_block.html"));
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(
internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 1);
@@ -357,12 +405,13 @@
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);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
+ scoped_refptr<TimingUpdatedObserver> reload_observer =
+ CreateTimingUpdatedObserver();
+ reload_observer->MatchDocumentWriteBlockReload(2);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
@@ -380,8 +429,8 @@
histogram_tester_.ExpectTotalCount(
internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 1);
- fcp_waiter->Wait();
- reload_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
+ reload_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(
internal::kHistogramDocWriteBlockParseStartToFirstContentfulPaint, 1);
@@ -439,9 +488,9 @@
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);
+ scoped_refptr<TimingUpdatedObserver> timing_observer =
+ CreateTimingUpdatedObserver();
+ timing_observer->AddMatchingFields(TimingUpdatedObserver::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
@@ -452,7 +501,7 @@
browser(),
embedded_test_server()->GetURL("/page_load_metrics/badxml.xhtml"));
- timing_waiter->Wait();
+ timing_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(internal::kHistogramFirstLayout, 0);
histogram_tester_.ExpectTotalCount(internal::kHistogramFirstPaint, 0);
@@ -639,14 +688,15 @@
FirstMeaningfulPaintNotRecorded) {
ASSERT_TRUE(embedded_test_server()->Start());
- std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
- CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL(
"/page_load_metrics/page_with_active_connections.html"));
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
// Navigate away before a FMP is reported.
NavigateToUntrackedUrl();
@@ -664,14 +714,15 @@
NoStatePrefetchObserverCacheable) {
ASSERT_TRUE(embedded_test_server()->Start());
- std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
- CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/title1.html"));
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(
"Prerender.none_PrefetchTTFCP.Reference.NoStore.Visible", 0);
@@ -683,14 +734,15 @@
NoStatePrefetchObserverNoStore) {
ASSERT_TRUE(embedded_test_server()->Start());
- std::unique_ptr<PageLoadMetricsWaiter> fcp_waiter =
- CreatePageLoadMetricsWaiter();
- fcp_waiter->AddExpectation(PageLoadMetricsWaiter::FIRST_CONTENTFUL_PAINT);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::FIRST_CONTENTFUL_PAINT);
ui_test_utils::NavigateToURL(browser(),
embedded_test_server()->GetURL("/nostore.html"));
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(
"Prerender.none_PrefetchTTFCP.Reference.NoStore.Visible", 1);
@@ -701,9 +753,10 @@
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);
+ scoped_refptr<TimingUpdatedObserver> fcp_observer =
+ CreateTimingUpdatedObserver();
+ fcp_observer->AddMatchingFields(
+ TimingUpdatedObserver::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
@@ -712,7 +765,7 @@
browser(),
embedded_test_server()->GetURL("/page_load_metrics/page_with_css.html"));
NavigateToUntrackedUrl();
- fcp_waiter->Wait();
+ fcp_observer->WaitForMatchingIPC();
histogram_tester_.ExpectTotalCount(internal::kHistogramFirstContentfulPaint,
1);
« no previous file with comments | « chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698