Offline pages using NQE 2G Slow

chrome/browser/android/offline_pages/offline_page_tab_helper.cc

Index: chrome/browser/android/offline_pages/offline_page_tab_helper.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_tab_helper.cc b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
index b56b7d1c9b1900edebc3161908adc05a9fe83519..cbc56b978265c182f9804d92f4afdf574e1cf0b0 100644
--- a/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
@@ -3,32 +3,40 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/offline_page_tab_helper.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
+#include "base/time/time.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/offline_page_utils.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/profiles/profile.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/offline_pages/client_namespace_constants.h"
 #include "components/offline_pages/offline_page_model.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_change_notifier.h"
+#include "net/nqe/network_quality_estimator.h"
 #include "ui/base/page_transition_types.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinePageTabHelper);
 
 namespace offline_pages {
 namespace {
 
 void ReportAccessedOfflinePage(content::BrowserContext* browser_context,
                                const GURL& navigated_url,
                                const GURL& online_url) {
@@ -49,20 +57,21 @@ class DefaultDelegate : public OfflinePageTabHelper::Delegate {
       return false;
     *tab_id = base::IntToString(temp_tab_id);
     return true;
   }
 };
 }  // namespace
 
 OfflinePageTabHelper::OfflinePageTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       delegate_(new DefaultDelegate()),
+      clock_(new base::DefaultClock),
       weak_ptr_factory_(this) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 }
 
 OfflinePageTabHelper::~OfflinePageTabHelper() {}
 
 void OfflinePageTabHelper::SetDelegateForTesting(
     std::unique_ptr<OfflinePageTabHelper::Delegate> delegate) {
   DCHECK(delegate);
   delegate_ = std::move(delegate);
@@ -88,27 +97,46 @@ void OfflinePageTabHelper::DidStartNavigation(
   // which are not at the head of the stack.
   // TODO(dimich): Not sure this is needed. Clarify and remove. Bug 624216.
   const content::NavigationController& controller =
       web_contents()->GetController();
   if (controller.GetEntryCount() > 0 &&
       controller.GetCurrentEntryIndex() != -1 &&
       controller.GetCurrentEntryIndex() < controller.GetEntryCount() - 1) {
     return;
   }
 
-  content::BrowserContext* context = web_contents()->GetBrowserContext();
   if (net::NetworkChangeNotifier::IsOffline()) {
     GetPagesForRedirectToOffline(
         RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK, navigated_url);
     return;
   }
 
+  content::BrowserContext* context = web_contents()->GetBrowserContext();
+
+  // If using offline pages for slow networks is allowed, and the network is
+  // currently estimated to be prohibitivley slow, attempt to load an offline
+  // page.
+  if (data_reduction_proxy::params::

[tbansal1, 2016/07/25 21:43:22]

Might be cleaner to move this to a separate function.

[RyanSturm, 2016/07/26 19:48:11]

Done.

+          IsIncludedInOfflinePagesSlowConnectionFieldTrial()) {
+    Profile* profile = Profile::FromBrowserContext(context);
+    UINetworkQualityEstimatorService* nqe_service =
+        UINetworkQualityEstimatorServiceFactory::GetForProfile(profile);
+    if (nqe_service &&
+        nqe_service->GetEffectiveConnectionType() ==
+            net::NetworkQualityEstimator::EFFECTIVE_CONNECTION_TYPE_SLOW_2G) {

[tbansal1, 2016/07/25 21:43:22]

Instead of doing this, I will do something like
ect = nqe::GetEct();
if(ect >=_OFFLINE && ect <= _SLOW_2G) {
// do something
}

[RyanSturm, 2016/07/26 19:48:11]

I'm using the NCN::IsOffline above; either way I'd have to change the
RedirectResult, so I will do (ect > offline && ect <= slow_2g)

+      GetPagesForRedirectToOffline(
+          RedirectResult::REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK,
+          navigated_url);
+      return;
+    }
+  }
+
   OfflinePageModel* offline_page_model =
       OfflinePageModelFactory::GetForBrowserContext(context);
   if (!offline_page_model)
     return;
 
   offline_page_model->GetPageByOfflineURL(
       navigated_url, base::Bind(&OfflinePageTabHelper::RedirectToOnline,
                                 weak_ptr_factory_.GetWeakPtr(), navigated_url));
 }
 
@@ -192,21 +220,22 @@ void OfflinePageTabHelper::GetPagesForRedirectToOffline(
       online_url,
       base::Bind(&OfflinePageTabHelper::SelectBestPageForRedirectToOffline,
                  weak_ptr_factory_.GetWeakPtr(), result, online_url));
 }
 
 void OfflinePageTabHelper::SelectBestPageForRedirectToOffline(
     RedirectResult result,
     const GURL& online_url,
     const MultipleOfflinePageItemResult& pages) {
   DCHECK(result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ||
-         result == RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK);
+         result == RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK ||
+         result == RedirectResult::REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK);

[tbansal1, 2016/07/25 21:43:22]

May be: s/PROHIBITVELY_//
bengr likes PROHIBITVELY_ but I do not. I think it is redundant :)

[RyanSturm, 2016/07/26 19:48:11]

I think slow is <4G, but prohibitively slow is the point where I would not
bother using my phone anymore (slow 2g), so I don't find it redundant.

 
   // When there is no valid tab android there is nowhere to show the offline
   // page, so we can leave.
   std::string tab_id;
   if (!delegate_->GetTabId(web_contents(), &tab_id)) {
     ReportRedirectResultUMA(RedirectResult::NO_TAB_ID);
     return;
   }
 
   const OfflinePageItem* selected_page = nullptr;
@@ -216,24 +245,46 @@ void OfflinePageTabHelper::SelectBestPageForRedirectToOffline(
         (offline_page.client_id.name_space == kLastNNamespace &&
          offline_page.client_id.id == tab_id)) {
       if (!selected_page ||
           offline_page.creation_time > selected_page->creation_time) {
         selected_page = &offline_page;
       }
     }
   }
 
   if (!selected_page) {
+    switch (result) {
+      case RedirectResult::REDIRECTED_ON_FLAKY_NETWORK:
+        ReportRedirectResultUMA(
+            RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK);
+        break;

[tbansal1, 2016/07/25 21:43:22]

Why not return here?

[RyanSturm, 2016/07/26 19:48:11]

Done.

+      case RedirectResult::REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK:
+        ReportRedirectResultUMA(
+            RedirectResult::PAGE_NOT_FOUND_ON_PROHIBITVELY_SLOW_NETWORK);
+        break;
+      case RedirectResult::REDIRECTED_ON_DISCONNECTED_NETWORK:
+        ReportRedirectResultUMA(
+            RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK);
+        break;
+      default:
+        NOTREACHED();
+    }
+    return;
+  }
+
+  // If the page is being loaded on a slow network, only use the offline page
+  // if it was created within the past day.
+  if (result == RedirectResult::REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK &&
+      clock_->Now() - selected_page->creation_time >
+          base::TimeDelta::FromDays(1)) {
     ReportRedirectResultUMA(
-        result == RedirectResult::REDIRECTED_ON_FLAKY_NETWORK ?
-        RedirectResult::PAGE_NOT_FOUND_ON_FLAKY_NETWORK :
-        RedirectResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK);
+        RedirectResult::PAGE_NOT_FRESH_ON_PROHIBITVELY_SLOW_NETWORK);

[tbansal1, 2016/07/25 21:35:26]

Why is this needed?

[RyanSturm, 2016/07/26 19:48:11]

It gives us the amount of times that we had a slow network and the page was not
fresh enough. Every other case is accounted for in this UMA, and I think there
is a difference between page not found and page not fresh enough for slow
network. I'm willing to turn this into PAGE_NOT_FOUND_ON_SLOW_NETWORK if you
think that is more sound.

     return;
   }
 
   TryRedirectToOffline(result, online_url, *selected_page);
 }
 
 void OfflinePageTabHelper::TryRedirectToOffline(
     RedirectResult result,
     const GURL& from_url,
     const OfflinePageItem& offline_page) {
@@ -266,11 +317,17 @@ bool OfflinePageTabHelper::IsInRedirectLoop(const GURL& to_url) const {
   return entry &&
          !entry->GetRedirectChain().empty() &&
          entry->GetRedirectChain().back() == to_url;
 }
 
 void OfflinePageTabHelper::ReportRedirectResultUMA(RedirectResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.RedirectResult",
       static_cast<int>(result),
       static_cast<int>(RedirectResult::REDIRECT_RESULT_MAX));
 }
+
+void OfflinePageTabHelper::SetClockForTesting(
+    std::unique_ptr<base::Clock> clock) {
+  clock_ = std::move(clock);
+}
+
 }  // namespace offline_pages