Do not show client side previews when effective connection type is OFFLINE

This does not affect OFFLINE preview type.

Change-Id: Ia7dbec31a7eb1434fc1b46bc6895eab5d32f9086
Bug: 838969
Reviewed-on: https://chromium-review.googlesource.com/1039160
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555624}

components/previews/content/previews_io_data.cc

@@ -255,9 +255,17 @@ bool PreviewsIOData::ShouldAllowPreviewAtECT(
       net::EFFECTIVE_CONNECTION_TYPE_LAST) {
     net::NetworkQualityEstimator* network_quality_estimator =
         request.context()->network_quality_estimator();
-    if (!network_quality_estimator ||
-        network_quality_estimator->GetEffectiveConnectionType() <
-            net::EFFECTIVE_CONNECTION_TYPE_OFFLINE) {
+    const net::EffectiveConnectionType observed_effective_connection_type =
+        network_quality_estimator
+            ? network_quality_estimator->GetEffectiveConnectionType()
+            : net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN;
+    // Network quality estimator may sometimes return effective connection type
+    // as offline when the Android APIs incorrectly return device connectivity
+    // as null. See https://crbug.com/838969. So, we do not trigger previews
+    // when |observed_effective_connection_type| is
+    // net::EFFECTIVE_CONNECTION_TYPE_OFFLINE.
+    if (observed_effective_connection_type <=
+        net::EFFECTIVE_CONNECTION_TYPE_OFFLINE) {
       LogPreviewDecisionMade(
           PreviewsEligibilityReason::NETWORK_QUALITY_UNAVAILABLE, request.url(),
           base::Time::Now(), type, std::move(passed_reasons), page_id);
@@ -266,7 +274,7 @@ bool PreviewsIOData::ShouldAllowPreviewAtECT(
     passed_reasons.push_back(
         PreviewsEligibilityReason::NETWORK_QUALITY_UNAVAILABLE);
 
-    if (network_quality_estimator->GetEffectiveConnectionType() >
+    if (observed_effective_connection_type >
         effective_connection_type_threshold) {
       LogPreviewDecisionMade(PreviewsEligibilityReason::NETWORK_NOT_SLOW,
                              request.url(), base::Time::Now(), type,

components/previews/content/previews_io_data_unittest.cc

@@ -553,15 +553,35 @@ TEST_F(PreviewsIODataTest, TestAllowOffline) {
   scoped_feature_list.InitAndEnableFeature(features::kPreviews);
   InitializeUIService();
 
-  network_quality_estimator()->set_effective_connection_type(
-      net::EFFECTIVE_CONNECTION_TYPE_2G);
+  const struct {
+    net::EffectiveConnectionType effective_connection_type;
+    bool expected_offline_allowed;
+  } tests[] = {
+      {net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN, false},
+      {net::EFFECTIVE_CONNECTION_TYPE_OFFLINE, false},
+      {net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G, true},
+      {net::EFFECTIVE_CONNECTION_TYPE_2G, true},
+      {net::EFFECTIVE_CONNECTION_TYPE_3G, false},
+  };
+  for (const auto& test : tests) {
+    network_quality_estimator()->set_effective_connection_type(
+        test.effective_connection_type);
 
-  base::HistogramTester histogram_tester;
-  EXPECT_TRUE(
-      io_data()->ShouldAllowPreview(*CreateRequest(), PreviewsType::OFFLINE));
-  histogram_tester.ExpectUniqueSample(
-      "Previews.EligibilityReason.Offline",
-      static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+    base::HistogramTester histogram_tester;
+    EXPECT_EQ(
+        test.expected_offline_allowed,
+        io_data()->ShouldAllowPreview(*CreateRequest(), PreviewsType::OFFLINE))
+        << " effective_connection_type=" << test.effective_connection_type;
+    if (test.expected_offline_allowed) {
+      histogram_tester.ExpectUniqueSample(
+          "Previews.EligibilityReason.Offline",
+          static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+    } else {
+      histogram_tester.ExpectBucketCount(
+          "Previews.EligibilityReason.Offline",
+          static_cast<int>(PreviewsEligibilityReason::ALLOWED), 0);
+    }
+  }
 }
 
 TEST_F(PreviewsIODataTest, ClientLoFiDisallowedWhenFeatureDisabled) {
@@ -632,17 +652,39 @@ TEST_F(PreviewsIODataTest, ClientLoFiAllowed) {
 
   EXPECT_EQ(net::EFFECTIVE_CONNECTION_TYPE_2G,
             params::EffectiveConnectionTypeThresholdForClientLoFi());
-  network_quality_estimator()->set_effective_connection_type(
-      net::EFFECTIVE_CONNECTION_TYPE_2G);
 
-  base::HistogramTester histogram_tester;
-  EXPECT_TRUE(io_data()->ShouldAllowPreviewAtECT(
-      *CreateRequest(), PreviewsType::LOFI,
-      params::EffectiveConnectionTypeThresholdForClientLoFi(),
-      params::GetBlackListedHostsForClientLoFiFieldTrial()));
-  histogram_tester.ExpectUniqueSample(
-      "Previews.EligibilityReason.LoFi",
-      static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+  const struct {
+    net::EffectiveConnectionType effective_connection_type;
+    bool expected_client_lofi_allowed;
+  } tests[] = {
+      {net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN, false},
+      {net::EFFECTIVE_CONNECTION_TYPE_OFFLINE, false},
+      {net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G, true},
+      {net::EFFECTIVE_CONNECTION_TYPE_2G, true},
+      {net::EFFECTIVE_CONNECTION_TYPE_3G, false},
+  };
+
+  for (const auto& test : tests) {
+    network_quality_estimator()->set_effective_connection_type(
+        test.effective_connection_type);
+
+    base::HistogramTester histogram_tester;
+    EXPECT_EQ(test.expected_client_lofi_allowed,
+              io_data()->ShouldAllowPreviewAtECT(
+                  *CreateRequest(), PreviewsType::LOFI,
+                  params::EffectiveConnectionTypeThresholdForClientLoFi(),
+                  params::GetBlackListedHostsForClientLoFiFieldTrial()))
+        << " effective_connection_type=" << test.effective_connection_type;
+    if (test.expected_client_lofi_allowed) {
+      histogram_tester.ExpectUniqueSample(
+          "Previews.EligibilityReason.LoFi",
+          static_cast<int>(PreviewsEligibilityReason::ALLOWED), 1);
+    } else {
+      histogram_tester.ExpectBucketCount(
+          "Previews.EligibilityReason.LoFi",
+          static_cast<int>(PreviewsEligibilityReason::ALLOWED), 0);
+    }
+  }
 }
 
 TEST_F(PreviewsIODataTest, MissingHostDisallowed) {
@@ -759,20 +801,43 @@ TEST_F(PreviewsIODataTest, NoScriptAllowedByFeature) {
       {features::kPreviews, features::kNoScriptPreviews}, {});
   InitializeUIService();
 
-  network_quality_estimator()->set_effective_connection_type(
-      net::EFFECTIVE_CONNECTION_TYPE_2G);
+  const struct {
+    net::EffectiveConnectionType effective_connection_type;
+    bool expected_noscript_allowed;
+  } tests[] = {
+      {net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN, false},
+      {net::EFFECTIVE_CONNECTION_TYPE_OFFLINE, false},
+      {net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G, true},
+      {net::EFFECTIVE_CONNECTION_TYPE_2G, true},
+      {net::EFFECTIVE_CONNECTION_TYPE_3G, false},
+  };
 
-  base::HistogramTester histogram_tester;
-  EXPECT_TRUE(io_data()->ShouldAllowPreviewAtECT(
-      *CreateHttpsRequest(), PreviewsType::NOSCRIPT,
-      previews::params::GetECTThresholdForPreview(
-          previews::PreviewsType::NOSCRIPT),
-      std::vector<std::string>()));
-  histogram_tester.ExpectUniqueSample(
-      "Previews.EligibilityReason.NoScript",
-      static_cast<int>(
-          PreviewsEligibilityReason::ALLOWED_WITHOUT_OPTIMIZATION_HINTS),
-      1);
+  for (const auto& test : tests) {
+    network_quality_estimator()->set_effective_connection_type(
+        test.effective_connection_type);
+
+    base::HistogramTester histogram_tester;
+    EXPECT_EQ(test.expected_noscript_allowed,
+              io_data()->ShouldAllowPreviewAtECT(
+                  *CreateHttpsRequest(), PreviewsType::NOSCRIPT,
+                  previews::params::GetECTThresholdForPreview(
+                      previews::PreviewsType::NOSCRIPT),
+                  std::vector<std::string>()))
+        << " effective_connection_type=" << test.effective_connection_type;
+    if (test.expected_noscript_allowed) {
+      histogram_tester.ExpectUniqueSample(
+          "Previews.EligibilityReason.NoScript",
+          static_cast<int>(
+              PreviewsEligibilityReason::ALLOWED_WITHOUT_OPTIMIZATION_HINTS),
+          1);
+    } else {
+      histogram_tester.ExpectBucketCount(
+          "Previews.EligibilityReason.NoScript",
+          static_cast<int>(
+              PreviewsEligibilityReason::ALLOWED_WITHOUT_OPTIMIZATION_HINTS),
+          0);
+    }
+  }
 }
 
 TEST_F(PreviewsIODataTest, NoScriptAllowedByFeatureWithWhitelist) {

net/nqe/effective_connection_type.h

@@ -33,9 +33,12 @@ enum EffectiveConnectionType {
   // Effective connection type reported when the network quality is unknown.
   EFFECTIVE_CONNECTION_TYPE_UNKNOWN = 0,
 
-  // Effective connection type reported when the Internet is unreachable, either
-  // because the device does not have a connection or because the
-  // connection is too slow to be usable.
+  // Effective connection type reported when the Internet is unreachable
+  // because the device does not have a connection (as reported by underlying
+  // platform APIs). Note that due to rare but  potential bugs in the platform
+  // APIs, it is possible that effective connection type is reported as
+  // EFFECTIVE_CONNECTION_TYPE_OFFLINE. Callers must use caution when using
+  // acting on this.
   EFFECTIVE_CONNECTION_TYPE_OFFLINE,
 
   // Effective connection type reported when the network has the quality of a