PageLoadMetrics: Make TranslatePLMO support Prerender2
This patch introduces `is_in_primary_page_` flag to track if the
observer is bound with the primary page. This is set at `OnStart`
and `DidActivatePrerenderedPage`, and `TranslateMetricsLogger::OnPageLoadStart`
is called at both places after the flag is set.
This patch also modifies the mock in unit tests to initialize
SetUkmSourceId's expectation. This is for suppressing warnings
that have existed before this change.
Bug: 1317494
Change-Id: I95a64ea5635dd8297825305f9bd6c85d6328b2fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3608048
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1005299}
diff --git a/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc b/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
index f0b3038..5c4be8c 100644
--- a/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
+++ b/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc
@@ -10,6 +10,7 @@
#include "base/callback_helpers.h"
#include "components/ukm/content/source_url_recorder.h"
#include "content/public/browser/web_contents.h"
+#include "third_party/blink/public/common/features.h"
#include "url/gurl.h"
namespace page_load_metrics {
@@ -31,6 +32,20 @@
web_contents()->WasShown();
}
+void PageLoadMetricsObserverTestHarness::InitializeFeatureList() {
+ scoped_feature_list_.InitWithFeaturesAndParameters(
+ {
+ {blink::features::kPrerender2, {}},
+ {blink::features::kFencedFrames, {{"implementation_type", "mparch"}}},
+ {blink::features::kInitialNavigationEntry, {}},
+ },
+ {
+ // Disable the memory requirement of Prerender2
+ // so the test can run on any bot.
+ {blink::features::kPrerender2MemoryControls},
+ });
+}
+
const char PageLoadMetricsObserverTestHarness::kResourceUrl[] =
"https://www.example.com/resource";
diff --git a/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h b/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
index d8c9459..621af27 100644
--- a/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
+++ b/chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.h
@@ -7,6 +7,7 @@
#include <memory>
+#include "base/test/scoped_feature_list.h"
#include "chrome/test/base/chrome_render_view_host_test_harness.h"
#include "components/page_load_metrics/browser/observers/page_load_metrics_observer_tester.h"
@@ -29,7 +30,9 @@
template <typename... TaskEnvironmentTraits>
explicit PageLoadMetricsObserverTestHarness(TaskEnvironmentTraits&&... traits)
: ChromeRenderViewHostTestHarness(
- std::forward<TaskEnvironmentTraits>(traits)...) {}
+ std::forward<TaskEnvironmentTraits>(traits)...) {
+ InitializeFeatureList();
+ }
PageLoadMetricsObserverTestHarness(
const PageLoadMetricsObserverTestHarness&) = delete;
@@ -46,7 +49,10 @@
const PageLoadMetricsObserverTester* tester() const { return tester_.get(); }
private:
+ void InitializeFeatureList();
+
std::unique_ptr<PageLoadMetricsObserverTester> tester_;
+ base::test::ScopedFeatureList scoped_feature_list_;
};
} // namespace page_load_metrics
diff --git a/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.cc
index 10b5618..d477912 100644
--- a/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.cc
@@ -38,11 +38,22 @@
content::NavigationHandle* navigation_handle,
const GURL& currently_committed_url,
bool started_in_foreground) {
+ DCHECK(!is_in_primary_page_);
+ is_in_primary_page_ = true;
translate_metrics_logger_->OnPageLoadStart(started_in_foreground);
return CONTINUE_OBSERVING;
}
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+TranslatePageLoadMetricsObserver::OnPrerenderStart(
+ content::NavigationHandle* navigation_handle,
+ const GURL& currently_committed_url) {
+ // Continue running, but don't communicate via `translate_metrics_logger_`
+ // until the prerendered page activation.
+ return CONTINUE_OBSERVING;
+}
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
TranslatePageLoadMetricsObserver::OnFencedFramesStart(
content::NavigationHandle* navigation_handle,
const GURL& currently_committed_url) {
@@ -54,31 +65,51 @@
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
TranslatePageLoadMetricsObserver::OnCommit(
content::NavigationHandle* navigation_handle) {
- translate_metrics_logger_->SetUkmSourceId(GetDelegate().GetPageUkmSourceId());
+ if (is_in_primary_page_) {
+ translate_metrics_logger_->SetUkmSourceId(
+ GetDelegate().GetPageUkmSourceId());
+ }
return CONTINUE_OBSERVING;
}
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
TranslatePageLoadMetricsObserver::OnHidden(
const page_load_metrics::mojom::PageLoadTiming& timing) {
- translate_metrics_logger_->OnForegroundChange(false);
+ if (is_in_primary_page_) {
+ translate_metrics_logger_->OnForegroundChange(false);
+ }
return CONTINUE_OBSERVING;
}
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
TranslatePageLoadMetricsObserver::OnShown() {
- translate_metrics_logger_->OnForegroundChange(true);
+ if (is_in_primary_page_) {
+ translate_metrics_logger_->OnForegroundChange(true);
+ }
return CONTINUE_OBSERVING;
}
page_load_metrics::PageLoadMetricsObserver::ObservePolicy
TranslatePageLoadMetricsObserver::FlushMetricsOnAppEnterBackground(
const page_load_metrics::mojom::PageLoadTiming& timing) {
- translate_metrics_logger_->RecordMetrics(false);
+ if (is_in_primary_page_) {
+ translate_metrics_logger_->RecordMetrics(false);
+ }
return CONTINUE_OBSERVING;
}
void TranslatePageLoadMetricsObserver::OnComplete(
const page_load_metrics::mojom::PageLoadTiming& timing) {
- translate_metrics_logger_->RecordMetrics(true);
+ if (is_in_primary_page_) {
+ translate_metrics_logger_->RecordMetrics(true);
+ }
+}
+
+void TranslatePageLoadMetricsObserver::DidActivatePrerenderedPage(
+ content::NavigationHandle* navigation_handle) {
+ DCHECK(!is_in_primary_page_);
+ is_in_primary_page_ = true;
+ translate_metrics_logger_->OnPageLoadStart(
+ GetDelegate().GetVisibilityTracker().currently_in_foreground());
+ translate_metrics_logger_->SetUkmSourceId(GetDelegate().GetPageUkmSourceId());
}
diff --git a/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.h b/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.h
index 822a86c..952350f 100644
--- a/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.h
+++ b/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer.h
@@ -38,13 +38,11 @@
const TranslatePageLoadMetricsObserver&) = delete;
// page_load_metrics::PageLoadMetricsObserver
- // TODO(https://crbug.com/1317494): Consider prerendering support. Now a page
- // started with prerendering doesn't collect metrics. This class should return
- // CONTINUE_OBSERVING for OnPrerenderStart() but keep being silent until
- // DidActivatePrerenderedPage is called.
ObservePolicy OnStart(content::NavigationHandle* navigation_handle,
const GURL& currently_committed_url,
bool started_in_foreground) override;
+ ObservePolicy OnPrerenderStart(content::NavigationHandle* navigation_handle,
+ const GURL& currently_committed_url) override;
ObservePolicy OnFencedFramesStart(
content::NavigationHandle* navigation_handle,
const GURL& currently_committed_url) override;
@@ -56,8 +54,11 @@
const page_load_metrics::mojom::PageLoadTiming& timing) override;
void OnComplete(
const page_load_metrics::mojom::PageLoadTiming& timing) override;
+ void DidActivatePrerenderedPage(
+ content::NavigationHandle* navigation_handle) override;
private:
+ bool is_in_primary_page_ = false;
std::unique_ptr<translate::TranslateMetricsLogger> translate_metrics_logger_;
};
diff --git a/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer_unittest.cc b/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer_unittest.cc
index d6daf947..d692c24 100644
--- a/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer_unittest.cc
+++ b/chrome/browser/page_load_metrics/observers/translate_page_load_metrics_observer_unittest.cc
@@ -10,6 +10,7 @@
#include "components/page_load_metrics/browser/page_load_tracker.h"
#include "components/translate/core/browser/mock_translate_metrics_logger.h"
#include "components/translate/core/browser/translate_metrics_logger.h"
+#include "content/public/test/web_contents_tester.h"
#include "testing/gmock/include/gmock/gmock.h"
// Wraps a MockTranslateMetricsLogger so that test can retain a pointer to the
@@ -155,19 +156,26 @@
class TranslatePageLoadMetricsObserverTest
: public page_load_metrics::PageLoadMetricsObserverTestHarness {
- public:
- void SetUp() override {
- PageLoadMetricsObserverTestHarness::SetUp();
-
- // Creates the MockTranslateMetricsLogger that will be used for this test.
- mock_translate_metrics_logger_ =
- std::make_unique<translate::testing::MockTranslateMetricsLogger>();
+ protected:
+ void PrepareMock(size_t n = 1) {
+ for (size_t i = 0; i < n; ++i) {
+ // Creates the MockTranslateMetricsLogger that will be used for this test.
+ mock_translate_metrics_loggers_.emplace_back(
+ std::make_unique<translate::testing::MockTranslateMetricsLogger>());
+ }
+ }
+ const std::vector<
+ std::unique_ptr<translate::testing::MockTranslateMetricsLogger>>&
+ mock_translate_metrics_loggers() const {
+ return mock_translate_metrics_loggers_;
}
+ private:
void RegisterObservers(page_load_metrics::PageLoadTracker* tracker) override {
+ EXPECT_LT(used_mock_count_, mock_translate_metrics_loggers_.size());
translate::testing::MockTranslateMetricsLogger*
raw_mock_translate_metrics_logger =
- mock_translate_metrics_logger_.get();
+ mock_translate_metrics_loggers_[used_mock_count_++].get();
// Wraps the raw pointer in a container.
std::unique_ptr<MockTranslateMetricsLoggerContainer>
@@ -179,37 +187,50 @@
std::move(mock_translate_metrics_logger_container)));
}
- translate::testing::MockTranslateMetricsLogger&
- mock_translate_metrics_logger() const {
- return *mock_translate_metrics_logger_;
- }
-
- private:
// This is the TranslateMetricsLoggers used in a test.It is owned by the
// TranslatePageLoadMetricsObserverTest.
- std::unique_ptr<translate::testing::MockTranslateMetricsLogger>
- mock_translate_metrics_logger_;
+ std::vector<std::unique_ptr<translate::testing::MockTranslateMetricsLogger>>
+ mock_translate_metrics_loggers_;
+ size_t used_mock_count_ = 0u;
};
TEST_F(TranslatePageLoadMetricsObserverTest, SinglePageLoad) {
- EXPECT_CALL(mock_translate_metrics_logger(), OnPageLoadStart(true)).Times(1);
- EXPECT_CALL(mock_translate_metrics_logger(), OnPageLoadStart(false)).Times(0);
- EXPECT_CALL(mock_translate_metrics_logger(), OnForegroundChange(testing::_))
+ PrepareMock();
+
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(false))
.Times(0);
- EXPECT_CALL(mock_translate_metrics_logger(), RecordMetrics(true)).Times(1);
- EXPECT_CALL(mock_translate_metrics_logger(), RecordMetrics(false)).Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0],
+ OnForegroundChange(testing::_))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], SetUkmSourceId(testing::_))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(false))
+ .Times(0);
NavigateAndCommit(GURL("https://www.example.com"));
tester()->NavigateToUntrackedUrl();
}
TEST_F(TranslatePageLoadMetricsObserverTest, AppEntersBackground) {
- EXPECT_CALL(mock_translate_metrics_logger(), OnPageLoadStart(true)).Times(1);
- EXPECT_CALL(mock_translate_metrics_logger(), OnPageLoadStart(false)).Times(0);
- EXPECT_CALL(mock_translate_metrics_logger(), OnForegroundChange(testing::_))
+ PrepareMock();
+
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(false))
.Times(0);
- EXPECT_CALL(mock_translate_metrics_logger(), RecordMetrics(true)).Times(1);
- EXPECT_CALL(mock_translate_metrics_logger(), RecordMetrics(false)).Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0],
+ OnForegroundChange(testing::_))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], SetUkmSourceId(testing::_))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(false))
+ .Times(1);
NavigateAndCommit(GURL("https://www.example.com"));
tester()->SimulateAppEnterBackground();
@@ -217,14 +238,22 @@
}
TEST_F(TranslatePageLoadMetricsObserverTest, RepeatedAppEntersBackground) {
+ PrepareMock();
+
int num_times_enter_background = 100;
- EXPECT_CALL(mock_translate_metrics_logger(), OnPageLoadStart(true)).Times(1);
- EXPECT_CALL(mock_translate_metrics_logger(), OnPageLoadStart(false)).Times(0);
- EXPECT_CALL(mock_translate_metrics_logger(), OnForegroundChange(testing::_))
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(false))
.Times(0);
- EXPECT_CALL(mock_translate_metrics_logger(), RecordMetrics(true)).Times(1);
- EXPECT_CALL(mock_translate_metrics_logger(), RecordMetrics(false))
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0],
+ OnForegroundChange(testing::_))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], SetUkmSourceId(testing::_))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(false))
.Times(num_times_enter_background);
NavigateAndCommit(GURL("https://www.example.com"));
@@ -234,5 +263,50 @@
tester()->NavigateToUntrackedUrl();
}
+TEST_F(TranslatePageLoadMetricsObserverTest, PrerenderAndActivation) {
+ PrepareMock(2);
+
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], OnPageLoadStart(false))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0],
+ OnForegroundChange(testing::_))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], SetUkmSourceId(testing::_))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[0], RecordMetrics(false))
+ .Times(0);
+
+ EXPECT_CALL(*mock_translate_metrics_loggers()[1], OnPageLoadStart(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[1], OnPageLoadStart(false))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[1],
+ OnForegroundChange(testing::_))
+ .Times(0);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[1], SetUkmSourceId(testing::_))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[1], RecordMetrics(true))
+ .Times(1);
+ EXPECT_CALL(*mock_translate_metrics_loggers()[1], RecordMetrics(false))
+ .Times(0);
+
+ // Navigate to the initial page to set the initiator page's origin explicitly.
+ NavigateAndCommit(GURL("https://www.example.com"));
+
+ const GURL kPrerenderingUrl("https://www.example.com/prerender");
+ content::WebContentsTester::For(web_contents())
+ ->AddPrerenderAndCommitNavigation(kPrerenderingUrl);
+
+ // Activation
+ content::WebContentsTester::For(web_contents())
+ ->ActivatePrerenderedPage(kPrerenderingUrl);
+
+ tester()->NavigateToUntrackedUrl();
+}
+
// TODO(curranmax): Add unit tests that confirm behavior when the hidden/shown.
// status of the tab changes. https://crbug.com/1114868.
