Persist EffectiveConnectionType in UKM on navigation start.

This change emits a UKM PageLoad metric for the EffectiveConnectionType
observed at navigation start.

This change also corrects an issue where we logged some information in
OnStart. When OnStart is invoked, we don't yet know if we're observing
a page load, so we move all logging to later in the flow, when we know
we're observing a page load.

BUG=700537

Review-Url: https://codereview.chromium.org/2740403002
Cr-Commit-Position: refs/heads/master@{#456219}

chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc

@@ -4,9 +4,13 @@
 
 #include "chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/net/nqe/ui_network_quality_estimator_service.h"
+#include "chrome/browser/net/nqe/ui_network_quality_estimator_service_factory.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
+#include "chrome/browser/profiles/profile.h"
 #include "components/ukm/ukm_entry_builder.h"
 #include "components/ukm/ukm_service.h"
+#include "content/public/browser/web_contents.h"
 
 namespace internal {
 
@@ -20,21 +24,39 @@ const char kUkmFirstContentfulPaintName[] =
 const char kUkmFirstMeaningfulPaintName[] =
     "Experimental.PaintTiming.NavigationToFirstMeaningfulPaint";
 const char kUkmForegroundDurationName[] = "PageTiming.ForegroundDuration";
+const char kUkmEffectiveConnectionType[] =
+    "Net.EffectiveConnectionType.OnNavigationStart";
 
 }  // namespace internal
 
+namespace {
+
+UINetworkQualityEstimatorService* GetNQEService(
+    content::WebContents* web_contents) {
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents->GetBrowserContext());
+  if (!profile)
+    return nullptr;
+  return UINetworkQualityEstimatorServiceFactory::GetForProfile(profile);
+}
+
+}  // namespace
+
 // static
 std::unique_ptr<page_load_metrics::PageLoadMetricsObserver>
-UkmPageLoadMetricsObserver::CreateIfNeeded() {
+UkmPageLoadMetricsObserver::CreateIfNeeded(content::WebContents* web_contents) {
   if (!g_browser_process->ukm_service()) {
     return nullptr;
   }
-
-  return base::MakeUnique<UkmPageLoadMetricsObserver>();
+  return base::MakeUnique<UkmPageLoadMetricsObserver>(
+      GetNQEService(web_contents));
 }
 
-UkmPageLoadMetricsObserver::UkmPageLoadMetricsObserver()
-    : source_id_(ukm::UkmService::GetNewSourceID()) {}
+UkmPageLoadMetricsObserver::UkmPageLoadMetricsObserver(
+    net::NetworkQualityEstimator::NetworkQualityProvider*
+        network_quality_provider)
+    : network_quality_provider_(network_quality_provider),
+      source_id_(ukm::UkmService::GetNewSourceID()) {}
 
 UkmPageLoadMetricsObserver::~UkmPageLoadMetricsObserver() = default;
 
@@ -45,8 +67,17 @@ UkmPageLoadMetricsObserver::ObservePolicy UkmPageLoadMetricsObserver::OnStart(
   if (!started_in_foreground)
     return STOP_OBSERVING;
 
-  ukm::UkmService* ukm_service = g_browser_process->ukm_service();
-  ukm_service->UpdateSourceURL(source_id_, navigation_handle->GetURL());
+  // When OnStart is invoked, we don't yet know whether we're observing a web
+  // page load, vs another kind of load (e.g. a download or a PDF). Thus,
+  // metrics and source information should not be recorded here. Instead, we
+  // store data we might want to persist in member variables below, and later
+  // record UKM metrics for that data once we've confirmed that we're observing
+  // a web page load.
+
+  if (network_quality_provider_) {
+    effective_connection_type_ =
+        network_quality_provider_->GetEffectiveConnectionType();
+  }
   return CONTINUE_OBSERVING;
 }
 
@@ -54,17 +85,17 @@ UkmPageLoadMetricsObserver::ObservePolicy
 UkmPageLoadMetricsObserver::FlushMetricsOnAppEnterBackground(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RecordTimingMetrics(timing);
   RecordPageLoadExtraInfoMetrics(info, base::TimeTicks::Now());
+  RecordTimingMetrics(timing);
   return STOP_OBSERVING;
 }
 
 UkmPageLoadMetricsObserver::ObservePolicy UkmPageLoadMetricsObserver::OnHidden(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RecordTimingMetrics(timing);
   RecordPageLoadExtraInfoMetrics(
       info, base::TimeTicks() /* no app_background_time */);
+  RecordTimingMetrics(timing);
   return STOP_OBSERVING;
 }
 
@@ -78,9 +109,9 @@ void UkmPageLoadMetricsObserver::OnFailedProvisionalLoad(
 void UkmPageLoadMetricsObserver::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RecordTimingMetrics(timing);
   RecordPageLoadExtraInfoMetrics(
       info, base::TimeTicks() /* no app_background_time */);
+  RecordTimingMetrics(timing);
 }
 
 void UkmPageLoadMetricsObserver::RecordTimingMetrics(
@@ -115,16 +146,20 @@ void UkmPageLoadMetricsObserver::RecordPageLoadExtraInfoMetrics(
     const page_load_metrics::PageLoadExtraInfo& info,
     base::TimeTicks app_background_time) {
   ukm::UkmService* ukm_service = g_browser_process->ukm_service();
+  ukm_service->UpdateSourceURL(source_id_, info.start_url);
   ukm_service->UpdateSourceURL(source_id_, info.url);
 
+  std::unique_ptr<ukm::UkmEntryBuilder> builder =
+      ukm_service->GetEntryBuilder(source_id_, internal::kUkmPageLoadEventName);
   base::Optional<base::TimeDelta> foreground_duration =
       page_load_metrics::GetInitialForegroundDuration(info,
                                                       app_background_time);
   if (foreground_duration) {
-    std::unique_ptr<ukm::UkmEntryBuilder> builder =
-        ukm_service->GetEntryBuilder(source_id_,
-                                     internal::kUkmPageLoadEventName);
     builder->AddMetric(internal::kUkmForegroundDurationName,
                        foreground_duration.value().InMilliseconds());
   }
+  if (effective_connection_type_ != net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN) {
+    builder->AddMetric(internal::kUkmEffectiveConnectionType,
+                       static_cast<int64_t>(effective_connection_type_));
+  }
 }

chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h

@@ -7,6 +7,11 @@
 
 #include "base/macros.h"
 #include "chrome/browser/page_load_metrics/page_load_metrics_observer.h"
+#include "net/nqe/network_quality_estimator.h"
+
+namespace content {
+class WebContents;
+}
 
 namespace internal {
 
@@ -18,6 +23,7 @@ extern const char kUkmLoadEventName[];
 extern const char kUkmFirstContentfulPaintName[];
 extern const char kUkmFirstMeaningfulPaintName[];
 extern const char kUkmForegroundDurationName[];
+extern const char kUkmEffectiveConnectionType[];
 
 }  // namespace internal
 
@@ -28,9 +34,11 @@ class UkmPageLoadMetricsObserver
  public:
   // Returns a UkmPageLoadMetricsObserver, or nullptr if it is not needed.
   static std::unique_ptr<page_load_metrics::PageLoadMetricsObserver>
-  CreateIfNeeded();
+  CreateIfNeeded(content::WebContents* web_contents);
 
-  UkmPageLoadMetricsObserver();
+  explicit UkmPageLoadMetricsObserver(
+      net::NetworkQualityEstimator::NetworkQualityProvider*
+          network_quality_provider);
   ~UkmPageLoadMetricsObserver() override;
 
   // page_load_metrics::PageLoadMetricsObserver implementation:
@@ -65,9 +73,15 @@ class UkmPageLoadMetricsObserver
       const page_load_metrics::PageLoadExtraInfo& info,
       base::TimeTicks app_background_time);
 
+  net::NetworkQualityEstimator::NetworkQualityProvider* const
+      network_quality_provider_;
+
   // Unique UKM identifier for the page load we are recording metrics for.
   const int32_t source_id_;
 
+  net::EffectiveConnectionType effective_connection_type_ =
+      net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+
   DISALLOW_COPY_AND_ASSIGN(UkmPageLoadMetricsObserver);
 };
 

chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc

@@ -12,24 +12,47 @@
 #include "components/ukm/test_ukm_service.h"
 #include "components/ukm/ukm_entry.h"
 #include "components/ukm/ukm_source.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using testing::AnyNumber;
+using testing::Mock;
+using testing::Return;
 
 namespace {
 
 const char kTestUrl1[] = "https://www.google.com/";
 const char kTestUrl2[] = "https://www.example.com/";
 
+class MockNetworkQualityProvider
+    : public net::NetworkQualityEstimator::NetworkQualityProvider {
+ public:
+  MOCK_CONST_METHOD0(GetEffectiveConnectionType,
+                     net::EffectiveConnectionType());
+  MOCK_METHOD1(
+      AddEffectiveConnectionTypeObserver,
+      void(net::NetworkQualityEstimator::EffectiveConnectionTypeObserver*));
+  MOCK_METHOD1(
+      RemoveEffectiveConnectionTypeObserver,
+      void(net::NetworkQualityEstimator::EffectiveConnectionTypeObserver*));
+};
+
 }  // namespace
 
 class UkmPageLoadMetricsObserverTest
     : public page_load_metrics::PageLoadMetricsObserverTestHarness {
  protected:
   void RegisterObservers(page_load_metrics::PageLoadTracker* tracker) override {
-    tracker->AddObserver(base::MakeUnique<UkmPageLoadMetricsObserver>());
+    tracker->AddObserver(base::MakeUnique<UkmPageLoadMetricsObserver>(
+        &mock_network_quality_provider_));
   }
 
   void SetUp() override {
     page_load_metrics::PageLoadMetricsObserverTestHarness::SetUp();
 
+    EXPECT_CALL(mock_network_quality_provider_, GetEffectiveConnectionType())
+        .Times(AnyNumber())
+        .WillRepeatedly(Return(net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN));
+
     TestingBrowserProcess::GetGlobal()->SetUkmService(
         ukm_service_test_harness_.test_ukm_service());
   }
@@ -42,6 +65,10 @@ class UkmPageLoadMetricsObserverTest
     return ukm_service_test_harness_.test_ukm_service()->entries_count();
   }
 
+  MockNetworkQualityProvider& mock_network_quality_provider() {
+    return mock_network_quality_provider_;
+  }
+
   const ukm::UkmSource* GetUkmSource(size_t source_index) {
     return ukm_service_test_harness_.test_ukm_service()->GetSource(
         source_index);
@@ -118,6 +145,7 @@ class UkmPageLoadMetricsObserverTest
   }
 
  private:
+  MockNetworkQualityProvider mock_network_quality_provider_;
   ukm::UkmServiceTestingHarness ukm_service_test_harness_;
 };
 
@@ -250,3 +278,26 @@ TEST_F(UkmPageLoadMetricsObserverTest, MultiplePageLoads) {
   EXPECT_TRUE(
       HasMetric(internal::kUkmForegroundDurationName, entry2_proto.metrics()));
 }
+
+TEST_F(UkmPageLoadMetricsObserverTest, EffectiveConnectionType) {
+  EXPECT_CALL(mock_network_quality_provider(), GetEffectiveConnectionType())
+      .WillRepeatedly(Return(net::EFFECTIVE_CONNECTION_TYPE_3G));
+
+  NavigateAndCommit(GURL(kTestUrl1));
+
+  // Simulate closing the tab.
+  DeleteContents();
+
+  EXPECT_EQ(1ul, ukm_source_count());
+  const ukm::UkmSource* source = GetUkmSource(0);
+  EXPECT_EQ(GURL(kTestUrl1), source->url());
+
+  EXPECT_GE(ukm_entry_count(), 1ul);
+  ukm::Entry entry_proto = GetMergedEntryProtoForSourceID(source->id());
+  EXPECT_EQ(entry_proto.source_id(), source->id());
+  EXPECT_EQ(entry_proto.event_hash(),
+            base::HashMetricName(internal::kUkmPageLoadEventName));
+  EXPECT_FALSE(entry_proto.metrics().empty());
+  ExpectMetric(internal::kUkmEffectiveConnectionType,
+               net::EFFECTIVE_CONNECTION_TYPE_3G, entry_proto.metrics());
+}

chrome/browser/page_load_metrics/page_load_metrics_initialize.cc

@@ -92,7 +92,7 @@ void PageLoadMetricsEmbedder::RegisterObservers(
     tracker->AddObserver(base::MakeUnique<TabRestorePageLoadMetricsObserver>());
 
     std::unique_ptr<page_load_metrics::PageLoadMetricsObserver> ukm_observer =
-        UkmPageLoadMetricsObserver::CreateIfNeeded();
+        UkmPageLoadMetricsObserver::CreateIfNeeded(web_contents_);
     if (ukm_observer)
       tracker->AddObserver(std::move(ukm_observer));