Revert "Report save-data savings to LiteMode site-breakdown"

This reverts commit 733e2fb1.

Reason for revert: This CL caused a CHECK failure in the following
browser_tests:

LazyLoadDisabledBrowserTest.LazyLoadFrame_DisabledInBackgroundTab
LazyLoadDisabledBrowserTest.LazyLoadFrame_DisabledInIncognito

Sample run: https://ci.chromium.org/p/chromium/builders/ci/Linux%20Tests%20%28dbg%29%281%29/85969

CHECK failure log:
> [67654:67654:0110/151505.705450:FATAL:embedded_test_server.cc(389)]
> Check failed: !io_thread_.get(). Handlers must be registered before
> starting the server.

Original change's description:
> Report save-data savings to LiteMode site-breakdown
> 
> This CL gets the estimated data savings due to SaveData and updates as
> LiteMode savings. The savings are retrieved from field trial as json.
> 
> Bug: 923551
> Change-Id: Ia7c5518edfeaedce33e957c19224f6a2350df6c2
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1990989
> Commit-Queue: rajendrant <rajendrant@chromium.org>
> Reviewed-by: Tarun Bansal <tbansal@chromium.org>
> Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#729972}

TBR=tbansal@chromium.org,rajendrant@chromium.org,ryansturm@chromium.org

Change-Id: I79ebf0d0a25d218ddbe644d708ba65d57061c92a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 923551
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1994850Reviewed-by: Yuta Kitamura <yutak@chromium.org>
Commit-Queue: Yuta Kitamura <yutak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730072}

chrome/browser/page_load_metrics/observers/data_saver_site_breakdown_metrics_observer.cc

@@ -50,10 +50,6 @@ DataSaverSiteBreakdownMetricsObserver::OnCommit(
   committed_host_ = navigation_handle->GetWebContents()
                         ->GetLastCommittedURL()
                         .HostNoBrackets();
-  committed_origin_ = navigation_handle->GetWebContents()
-                          ->GetLastCommittedURL()
-                          .GetOrigin()
-                          .spec();
   return CONTINUE_OBSERVING;
 }
 
@@ -69,7 +65,6 @@ void DataSaverSiteBreakdownMetricsObserver::OnResourceDataUseObserved(
   if (data_reduction_proxy_settings &&
       data_reduction_proxy_settings->data_reduction_proxy_service()) {
     DCHECK(!committed_host_.empty());
-    DCHECK(!committed_origin_.empty());
     int64_t received_data_length = 0;
     int64_t data_reduction_proxy_bytes_saved = 0;
     for (auto const& resource : resources) {
@@ -88,14 +83,6 @@ void DataSaverSiteBreakdownMetricsObserver::OnResourceDataUseObserved(
             (resource->data_reduction_proxy_compression_ratio_estimate - 1.0);
       }
     }
-    double origin_save_data_savings =
-        data_reduction_proxy_settings->data_reduction_proxy_service()
-            ->GetSaveDataSavingsPercentEstimate(committed_origin_);
-    if (origin_save_data_savings) {
-      data_reduction_proxy_bytes_saved +=
-          received_data_length * origin_save_data_savings / 100;
-    }
-
     data_reduction_proxy_settings->data_reduction_proxy_service()
         ->UpdateDataUseForHost(
             received_data_length,

chrome/browser/page_load_metrics/observers/data_saver_site_breakdown_metrics_observer.h

@@ -42,7 +42,6 @@ class DataSaverSiteBreakdownMetricsObserver
       const std::string& mime_type) const override;
 
   std::string committed_host_;
-  std::string committed_origin_;
 
   // The browser context this navigation is operating in.
   content::BrowserContext* browser_context_ = nullptr;

chrome/browser/page_load_metrics/observers/data_saver_site_breakdown_metrics_observer_browsertest.cc

@@ -63,6 +63,18 @@ HandleResourceRequestWithPlaintextMimeType(
 // Browser tests with Lite mode not enabled.
 class DataSaverSiteBreakdownMetricsObserverBrowserTestBase
     : public InProcessBrowserTest {
+  void SetUp() override {
+    scoped_feature_list_.InitWithFeaturesAndParameters(
+        {{features::kLazyImageLoading,
+          {{"automatic-lazy-load-images-enabled", "true"},
+           {"enable-lazy-load-images-metadata-fetch", "true"},
+           {"lazy_image_first_k_fully_load", "4G:0"}}},
+         {features::kLazyFrameLoading,
+          {{"automatic-lazy-load-frames-enabled", "true"}}}},
+        {});
+    InProcessBrowserTest::SetUp();
+  }
+
  protected:
   // Gets the data usage recorded against the host the embedded server runs on.
   uint64_t GetDataUsage(const std::string& host) {
@@ -102,6 +114,9 @@ class DataSaverSiteBreakdownMetricsObserverBrowserTestBase
         ->PostTask(FROM_HERE, run_loop.QuitClosure());
     run_loop.Run();
   }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 // Browser tests with Lite mode enabled.
@@ -122,39 +137,14 @@ class DataSaverSiteBreakdownMetricsObserverBrowserTest
         data_reduction_proxy::switches::kEnableDataReductionProxy);
     command_line->AppendSwitch(previews::switches::kIgnorePreviewsBlacklist);
   }
-};
 
-class LazyLoadWithoutLiteModeBrowserTest
-    : public DataSaverSiteBreakdownMetricsObserverBrowserTestBase {
-  void SetUp() override {
-    scoped_feature_list_.InitWithFeaturesAndParameters(
-        {{features::kLazyImageLoading,
-          {{"automatic-lazy-load-images-enabled", "true"},
-           {"enable-lazy-load-images-metadata-fetch", "true"},
-           {"lazy_image_first_k_fully_load", "4G:0"}}},
-         {features::kLazyFrameLoading,
-          {{"automatic-lazy-load-frames-enabled", "true"}}}},
-        {});
-    DataSaverSiteBreakdownMetricsObserverBrowserTestBase::SetUp();
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
-
-class LazyLoadBrowserTest
-    : public DataSaverSiteBreakdownMetricsObserverBrowserTest {
- public:
-  void SetUp() override {
-    scoped_feature_list_.InitWithFeaturesAndParameters(
-        {{features::kLazyImageLoading,
-          {{"automatic-lazy-load-images-enabled", "true"},
-           {"enable-lazy-load-images-metadata-fetch", "true"},
-           {"lazy_image_first_k_fully_load", "4G:0"}}},
-         {features::kLazyFrameLoading,
-          {{"automatic-lazy-load-frames-enabled", "true"}}}},
-        {});
-    DataSaverSiteBreakdownMetricsObserverBrowserTest::SetUp();
+  void ScrollToAndWaitForScroll(unsigned int scroll_offset) {
+    ASSERT_TRUE(content::ExecuteScript(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        base::StringPrintf("window.scrollTo(0, %d);", scroll_offset)));
+    content::RenderFrameSubmissionObserver observer(
+        browser()->tab_strip_model()->GetActiveWebContents());
+    observer.WaitForScrollOffset(gfx::Vector2dF(0, scroll_offset));
   }
 
   // Navigates to |url| waiting until |expected_resources| are received and then
@@ -221,126 +211,8 @@ class LazyLoadBrowserTest
     return GetDataSavings(test_url.HostNoBrackets()) -
            data_savings_before_navigation;
   }
-
- private:
-  void ScrollToAndWaitForScroll(unsigned int scroll_offset) {
-    ASSERT_TRUE(content::ExecuteScript(
-        browser()->tab_strip_model()->GetActiveWebContents(),
-        base::StringPrintf("window.scrollTo(0, %d);", scroll_offset)));
-    content::RenderFrameSubmissionObserver observer(
-        browser()->tab_strip_model()->GetActiveWebContents());
-    observer.WaitForScrollOffset(gfx::Vector2dF(0, scroll_offset));
-  }
-
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
-
-struct SaveDataSavingsEstimate {
-  std::string host;
-  std::string data_savings_percent;
-};
-
-struct SaveDataSingleTestCase {
-  std::string test_host;
-  double expected_savings_percent;
-};
-
-std::string ConvertSaveDataSavingsEstimateToJson(
-    std::vector<SaveDataSavingsEstimate> estimates,
-    const net::EmbeddedTestServer& embedded_test_server) {
-  std::string origin_savings_estimate_json;
-  for (const auto& estimate : estimates) {
-    base::StringAppendF(&origin_savings_estimate_json, "\"%s\": %s,",
-                        embedded_test_server.GetURL(estimate.host, "/")
-                            .GetOrigin()
-                            .spec()
-                            .c_str(),
-                        estimate.data_savings_percent.c_str());
-  }
-  origin_savings_estimate_json.pop_back();
-  return "{" + origin_savings_estimate_json + "}";
-}
-
-struct SaveDataTestCase {
-  // One of the origin_savings_estimate fields will be populated.
-  std::string origin_savings_estimate_raw_json;
-  std::vector<SaveDataSavingsEstimate> origin_savings_estimate_list;
-
-  std::vector<SaveDataSingleTestCase> tests;
-} kSaveDataTestCases[] = {
-    // No savings recorded without field trial config.
-    {"", {}, {{"foo.com", 0.0}}},
-
-    // No savings recorded with invalid field trial parameter.
-    {"invalid json", {}, {{"foo.com", 0.0}}},
-
-    {"",
-     {{"saving.com", "10"}},
-     {{{"saving.com", 10.0}, {"notsaving.com", 0.0}}}},
-
-    {"",
-     {{"saving.com", "20"}, {"savingfloatingpoint.edu", "15.7"}},
-     {{{"saving.com", 20.0},
-       {"savingfloatingpoint.edu", 15.7},
-       {"notsaving.com", 0.0}}}}
-
 };
 
-// Browser tests with Lite mode not enabled.
-class SaveDataSavingsEstimateBrowserTest
-    : public DataSaverSiteBreakdownMetricsObserverBrowserTest,
-      public ::testing::WithParamInterface<SaveDataTestCase> {
- public:
-  void SetUp() override {
-    ASSERT_TRUE(embedded_test_server()->Start());
-    const std::string estimates_json =
-        !GetParam().origin_savings_estimate_raw_json.empty()
-            ? GetParam().origin_savings_estimate_raw_json
-            : (!GetParam().origin_savings_estimate_list.empty()
-                   ? ConvertSaveDataSavingsEstimateToJson(
-                         GetParam().origin_savings_estimate_list,
-                         *embedded_test_server())
-                   : "");
-    if (!estimates_json.empty()) {
-      scoped_feature_list_.InitWithFeaturesAndParameters(
-          {{data_reduction_proxy::features::kReportSaveDataSavings,
-            {{"origin_savings_estimate", estimates_json}}}},
-          {});
-    }
-    DataSaverSiteBreakdownMetricsObserverBrowserTest::SetUp();
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-};
-
-INSTANTIATE_TEST_SUITE_P(SaveDataSavingsEstimateBrowserTest,
-                         SaveDataSavingsEstimateBrowserTest,
-                         ::testing::ValuesIn(kSaveDataTestCases));
-
-IN_PROC_BROWSER_TEST_P(SaveDataSavingsEstimateBrowserTest,
-                       NavigateToSimplePage) {
-  WaitForDBToInitialize();
-
-  for (const auto& test : GetParam().tests) {
-    GURL test_url(
-        embedded_test_server()->GetURL(test.test_host, "/google/google.html"));
-    std::string host = test_url.HostNoBrackets();
-    uint64_t data_usage_before_navigation = GetDataUsage(host);
-    uint64_t data_savings_before_navigation = GetDataSavings(host);
-    ui_test_utils::NavigateToURL(browser(), test_url);
-
-    base::RunLoop().RunUntilIdle();
-    // Navigate away to force the histogram recording.
-    ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));
-
-    double data_savings_percent =
-        100.0 * (GetDataSavings(host) - data_savings_before_navigation) /
-        (GetDataUsage(host) - data_usage_before_navigation);
-    EXPECT_NEAR(data_savings_percent, test.expected_savings_percent, 0.01);
-  }
-}
-
 IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
                        NavigateToSimplePage) {
   const struct {
@@ -406,14 +278,15 @@ IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
                       data_usage_before_navigation);
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest, LazyLoadImagesCSSBackgroundImage) {
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
+                       LazyLoadImagesCSSBackgroundImage) {
   // 2 deferred images.
   EXPECT_EQ(10000 * 2,
             NavigateAndGetDataSavings("/lazyload/css-background-image.html",
                                       2 /* main html, favicon */));
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
                        LazyLoadImagesCSSBackgroundImageScrollRemovesSavings) {
   // Scrolling should remove the savings.
   EXPECT_EQ(0u, NavigateAndGetDataSavingsAfterScroll(
@@ -421,7 +294,8 @@ IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
                     2 /* lazyloaded images */));
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest, LazyLoadImagesImgElement) {
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
+                       LazyLoadImagesImgElement) {
   // Choose reasonable minimum, any savings is indicative of the mechanism
   // working.
   EXPECT_LE(
@@ -431,7 +305,7 @@ IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest, LazyLoadImagesImgElement) {
         10 /* main html, favicon, 8 images (2 eager, 4 placeholder, 2 full)*/));
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
                        LazyLoadImagesImgElementScrollRemovesSavings) {
   // Choose reasonable minimum, any savings is indicative of the mechanism
   // working.
@@ -440,14 +314,15 @@ IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
                                                     2 /* lazyloaded image */));
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest, LazyLoadImagesImgWithDimension) {
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
+                       LazyLoadImagesImgWithDimension) {
   // 1 deferred image.
   EXPECT_EQ(10000,
             NavigateAndGetDataSavings("/lazyload/img-with-dimension.html",
                                       3 /* main html, favicon, full image */));
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
                        LazyLoadImagesImgWithDimensionScrollRemovesSavings) {
   // Scrolling should remove the savings.
   EXPECT_EQ(0u, NavigateAndGetDataSavingsAfterScroll(
@@ -455,7 +330,7 @@ IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
                     1 /* lazyloaded image */));
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadWithoutLiteModeBrowserTest,
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTestBase,
                        NoSavingsRecordedWithoutLiteMode) {
   std::vector<std::string> test_urls = {
       "/google/google.html",
@@ -479,7 +354,8 @@ IN_PROC_BROWSER_TEST_F(LazyLoadWithoutLiteModeBrowserTest,
   }
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest, LazyLoadImageDisabledInReload) {
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
+                       LazyLoadImageDisabledInReload) {
   ASSERT_TRUE(embedded_test_server()->Start());
   WaitForDBToInitialize();
   GURL test_url(
@@ -520,7 +396,7 @@ IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest, LazyLoadImageDisabledInReload) {
   }
 }
 
-IN_PROC_BROWSER_TEST_F(LazyLoadBrowserTest,
+IN_PROC_BROWSER_TEST_F(DataSaverSiteBreakdownMetricsObserverBrowserTest,
                        DISABLED_LazyLoadFrameDisabledInReload) {
   net::EmbeddedTestServer cross_origin_server;
   cross_origin_server.ServeFilesFromSourceDirectory(GetChromeTestDataDir());

components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc

@@ -8,7 +8,6 @@
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
-#include "base/json/json_reader.h"
 #include "base/location.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_macros.h"
@@ -40,22 +39,6 @@
 
 namespace data_reduction_proxy {
 
-namespace {
-
-base::Optional<base::Value> GetSaveDataSavingsPercentEstimateFromFieldTrial() {
-  if (!base::FeatureList::IsEnabled(features::kReportSaveDataSavings))
-    return base::nullopt;
-  const auto origin_savings_estimate_json =
-      base::GetFieldTrialParamValueByFeature(features::kReportSaveDataSavings,
-                                             "origin_savings_estimate");
-  if (origin_savings_estimate_json.empty())
-    return base::nullopt;
-
-  return base::JSONReader::Read(origin_savings_estimate_json);
-}
-
-}  // namespace
-
 DataReductionProxyService::DataReductionProxyService(
     DataReductionProxySettings* settings,
     PrefService* prefs,
@@ -79,9 +62,7 @@ DataReductionProxyService::DataReductionProxyService(
       data_use_measurement_(data_use_measurement),
       effective_connection_type_(net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN),
       client_(client),
-      channel_(channel),
-      save_data_savings_estimate_dict_(
-          GetSaveDataSavingsPercentEstimateFromFieldTrial()) {
+      channel_(channel) {
   DCHECK(settings);
   DCHECK(network_quality_tracker_);
   DCHECK(network_connection_tracker_);
@@ -424,6 +405,8 @@ void DataReductionProxyService::MarkProxiesAsBad(
   // process (renderer).
 
   if (bypass_duration < base::TimeDelta()) {
+    LOG(ERROR) << "Received bad MarkProxiesAsBad() -- invalid bypass_duration: "
+               << bypass_duration;
     std::move(callback).Run();
     return;
   }
@@ -437,6 +420,8 @@ void DataReductionProxyService::MarkProxiesAsBad(
   // received (FindConfiguredDataReductionProxy() searches recent proxies too).
   for (const auto& proxy : bad_proxies.GetAll()) {
     if (!config_->FindConfiguredDataReductionProxy(proxy)) {
+      LOG(ERROR) << "Received bad MarkProxiesAsBad() -- not a DRP server: "
+                 << proxy.ToURI();
       std::move(callback).Run();
       return;
     }
@@ -565,17 +550,4 @@ void DataReductionProxyService::SetDependenciesForTesting(
     config_client_->Initialize(url_loader_factory_);
 }
 
-double DataReductionProxyService::GetSaveDataSavingsPercentEstimate(
-    const std::string& origin) const {
-  if (origin.empty() || !save_data_savings_estimate_dict_ ||
-      !save_data_savings_estimate_dict_->is_dict()) {
-    return 0;
-  }
-  const auto savings_percent =
-      save_data_savings_estimate_dict_->FindDoubleKey(origin);
-  if (!savings_percent)
-    return 0;
-  return *savings_percent;
-}
-
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/browser/data_reduction_proxy_service.h

@@ -16,7 +16,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
-#include "base/values.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_util.h"
 #include "components/data_reduction_proxy/core/browser/db_data_owner.h"
@@ -165,10 +164,6 @@ class DataReductionProxyService
   void Clone(
       mojo::PendingReceiver<mojom::DataReductionProxy> receiver) override;
 
-  // Returns the percentage of data savings estimate provided by save-data for
-  // an origin.
-  double GetSaveDataSavingsPercentEstimate(const std::string& origin) const;
-
   // Accessor methods.
   DataReductionProxyCompressionStats* compression_stats() const {
     return compression_stats_.get();
@@ -314,9 +309,6 @@ class DataReductionProxyService
   // is unavailable, then the destruction will happen on the UI thread.
   std::unique_ptr<NetworkPropertiesManager> network_properties_manager_;
 
-  // Dictionary of save-data savings estimates by origin.
-  const base::Optional<base::Value> save_data_savings_estimate_dict_;
-
   // The set of clients that will get updates about changes to the proxy config.
   mojo::RemoteSet<network::mojom::CustomProxyConfigClient>
       proxy_config_clients_;

components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc

@@ -74,10 +74,5 @@ const base::Feature kDataReductionProxyAggressiveConfigFetch{
     "DataReductionProxyAggressiveConfigFetch",
     base::FEATURE_ENABLED_BY_DEFAULT};
 
-// Reports estimated data savings due to save-data request header and JS API, as
-// savings provided by DataSaver.
-const base::Feature kReportSaveDataSavings{"ReportSaveDataSavings",
-                                           base::FEATURE_DISABLED_BY_DEFAULT};
-
 }  // namespace features
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/data_reduction_proxy_features.h

@@ -21,7 +21,6 @@ extern const base::Feature kDataReductionProxyPopulatePreviewsPageIDToPingback;
 extern const base::Feature kDataReductionProxyDisableProxyFailedWarmup;
 extern const base::Feature kDataReductionProxyServerExperiments;
 extern const base::Feature kDataReductionProxyAggressiveConfigFetch;
-extern const base::Feature kReportSaveDataSavings;
 
 }  // namespace features
 }  // namespace data_reduction_proxy