Serve offline page for online URL on disconnected or bad networks

chrome/browser/android/offline_pages/offline_page_request_handler.h

Index: chrome/browser/android/offline_pages/offline_page_request_handler.h
diff --git a/chrome/browser/android/offline_pages/offline_page_request_handler.h b/chrome/browser/android/offline_pages/offline_page_request_handler.h
index 350402899d25b40a2704c671ac3817d2a8851eb2..3b75bf46c914807970bb1de72d1a7ee4a0d2f547 100644
--- a/chrome/browser/android/offline_pages/offline_page_request_handler.h
+++ b/chrome/browser/android/offline_pages/offline_page_request_handler.h
@@ -6,7 +6,10 @@
 #define CHROME_BROWSER_ANDROID_OFFLINE_PAGES_OFFLINE_PAGE_REQUEST_HANDLER_H_
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/supports_user_data.h"
+#include "chrome/browser/android/offline_pages/offline_page_request_job.h"
+#include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_type.h"
 
 namespace net {
@@ -18,6 +21,62 @@ class URLRequestJob;
 
 namespace offline_pages {
 
+// This enum is used to tell current network state that will affect offline
+// page loading.
+enum class NetworkState {

[Dmitry Titov, 2016/08/16 20:02:55]

Some of values here are not the network state but rather presence of a header
etc. Perhaps the better name would be ReasonToUseOfflinePage, with
CONNECTED_NETWORK -> LOAD_ORIGINAL_PAGE? Or mayeb LoadDesision... Also, is there
any reason not to have it as internal in the OfflinePageRequestHandler?

[jianli, 2016/08/18 22:46:35]

The only value is FORCE_OFFLINE_ON_CONNECTED_NETWORK. Also CONNECTED_NETWORK
really means for connected network which we don't want to bring up the offline
page.

I think ReasonToUseOfflinePage is more confusing than NetworkState. Even when
the header is present to force loading offline page, we still check network
conditions. If disconnected, network state will be DISCONNECTED_NETWORK. 

Changed to move this inside .cc file.

+  // No network connection.
+  DISCONNECTED_NETWORK,
+  // Prohibitively slow means that the NetworkQualityEstimator reported a
+  // connection slow enough to warrant showing an offline page if available.
+  PROHIBITIVELY_SLOW_NETWORK,
+  // Network error received due to bad network, i.e. connected to a hotspot or
+  // proxy that does not have a working network.
+  FLAKY_NETWORK,
+  // Network is in working condition.
+  CONNECTED_NETWORK,
+  // Force to load the offline page if it is available, though network is in
+  // working condition.
+  FORCE_OFFLINE_ON_CONNECTED_NETWORK
+};
+
+// This enum is used for UMA reporting. It contains all possible outcomes of
+// handling requests that might service offline page in different network
+// conditions. Generally one of these outcomes will happen.
+// The fringe errors (like no OfflinePageModel, etc.) are not reported due
+// to their low probability.
+// NOTE: because this is used for UMA reporting, these values should not be
+// changed or reused; new values should be ended immediately before the MAX
+// value. Make sure to update the histogram enum
+// (OfflinePagesAggregatedRequestResult in histograms.xml) accordingly.
+// Public for testing.
+enum class AggregatedRequestResult {
+  SHOW_OFFLINE_ON_DISCONNECTED_NETWORK,
+  PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK,
+  SHOW_OFFLINE_ON_FLAKY_NETWORK,
+  PAGE_NOT_FOUND_ON_FLAKY_NETWORK,
+  SHOW_OFFLINE_ON_PROHIBITIVELY_SLOW_NETWORK,
+  PAGE_NOT_FOUND_ON_PROHIBITIVELY_SLOW_NETWORK,
+  PAGE_NOT_FRESH_ON_PROHIBITIVELY_SLOW_NETWORK,
+  SHOW_OFFLINE_ON_CONNECTED_NETWORK,
+  PAGE_NOT_FOUND_ON_CONNECTED_NETWORK,
+  NO_TAB_ID,
+  NO_WEB_CONTENTS,
+  SHOW_NET_ERROR_PAGE,
+  AGGREGATED_REQUEST_RESULT_MAX
+};
+
+// Header that indicates that the offline page should be loaded if it exists
+// regardless current network conditions. Its value is a comma/space separated
+// name-value pair that may provide reason or define custom behavior.
+extern const char kLoadingOfflinePageHeader[];
+

[Dmitry Titov, 2016/08/16 20:02:55]

can remove this empty line.

[jianli, 2016/08/18 22:46:35]

Done.

+// The name used in name-value pair of kLoadingOfflinePageHeader to denote the
+// reason for loading offline page.
+extern const char kLoadingOfflinePageReason[];
+// Possible values in name-value pair that denote the reason for loading offline
+// page.
+extern const char kLoadingOfflinePageDueToNetError[];
+
 // Class for servicing requests based on their offline information. Created one
 // per URLRequest and attached to each request.
 //
@@ -37,7 +96,8 @@ namespace offline_pages {
 //         2.1.3) Return a custom URLRequestJob that is put on hold to wait
 //                for the result of finding offline page.
 //    2.2) Otherwise, bail out without interception.
-class OfflinePageRequestHandler : public base::SupportsUserData::Data {
+class OfflinePageRequestHandler : public base::SupportsUserData::Data,
+                                  public OfflinePageRequestJob::Delegate {
  public:
   // Attaches a newly created handler if the given |request| needs to
   // be handled by offline pages.
@@ -55,15 +115,37 @@ class OfflinePageRequestHandler : public base::SupportsUserData::Data {
   static std::unique_ptr<net::URLRequestInterceptor> CreateInterceptor(
       void* profile_id);
 
+  static void ReportAggregatedRequestResult(AggregatedRequestResult result);
+
   ~OfflinePageRequestHandler() override;
 
+  // OfflinePageRequestJob::Delegate overrides:
+  void OnPrepareToRestart() override;
+
   net::URLRequestJob* MaybeCreateJob(
       net::URLRequest* request,
       net::NetworkDelegate* network_delegate,
       void* profile_id);
 
  private:
-  OfflinePageRequestHandler();
+  explicit OfflinePageRequestHandler(NetworkState network_state);
+
+  // Invoked from UI thread.
+  void GetOfflineFileURL(
+      const GURL& online_url,
+      void* profile_id,
+      content::ResourceRequestInfo::WebContentsGetter web_contents_getter);
+  void FailedToGetOfflineFileURL();
+
+  base::WeakPtr<OfflinePageRequestJob> offline_job_;
+
+  // True if the next time this request is started, the request should be
+  // serviced from the default handler.
+  bool use_default_ = false;
+
+  NetworkState network_state_;

[Dmitry Titov, 2016/08/16 20:02:55]

I see why you made this a member, you don't want to re-compute it again later.
Is this actually that slow to compute? If you compute it again when you created
an instance of the class, would it be something noticeable on perf? If not, we
can make code simpler and avoid optimization that is not yet necessary.

[jianli, 2016/08/18 22:46:35]

I don't think this is slow to compute. With refactoring per mmneke's feedback, I
don't need to cache it any more.

+
+  base::WeakPtrFactory<OfflinePageRequestHandler> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageRequestHandler);
 };