Support serving offline page by offline ID

chrome/browser/android/offline_pages/offline_page_request_job.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/offline_page_request_job.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/offline_page_tab_helper.h"
 #include "chrome/browser/android/offline_pages/offline_page_utils.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "components/offline_pages/offline_page_model.h"
 #include "components/previews/previews_experiments.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/resource_type.h"
 #include "net/base/network_change_notifier.h"
 #include "net/http/http_request_headers.h"
 #include "net/nqe/network_quality_estimator.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 
 namespace offline_pages {
 
-const char kLoadingOfflinePageHeader[] = "X-Chrome-offline";
-const char kLoadingOfflinePageReason[] = "reason=";
-const char kLoadingOfflinePageDueToNetError[] = "error";
+const char kOfflinePageHeader[] = "X-Chrome-offline";
+const char kOfflinePageHeaderReasonKey[] = "reason";
+const char kOfflinePageHeaderReasonValueDueToNetError[] = "error";
+const char kOfflinePageHeaderPersistKey[] = "persist";
+const char kOfflinePageHeaderNamespaceKey[] = "namespace";
+const char kOfflinePageHeaderIDKey[] = "id";
 
 namespace {
 
 enum class NetworkState {
   // 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
@@ skipping 57 matching lines @@
   }
 
  private:
   static bool GetTabId(content::WebContents* web_contents, int* tab_id) {
     return OfflinePageUtils::GetTabId(web_contents, tab_id);
   }
 
   DISALLOW_COPY_AND_ASSIGN(DefaultDelegate);
 };
 
-// Returns true if custom offline header is present.
-// |reason| may be set with the reason to trigger the offline page loading.
-bool ParseOfflineHeader(net::URLRequest* request, std::string* reason) {
-  std::string value;
-  if (!request->extra_request_headers().GetHeader(kLoadingOfflinePageHeader,
-                                                  &value)) {
-    return false;
+class OfflinePageHeader {

[Dmitry Titov, 2016/09/09 04:33:06]

Nice to add a class for that. 
Please add a class comment and comments to public members.

[jianli, 2016/09/10 00:56:03]

Done.

+ public:
+  explicit OfflinePageHeader(const std::string& header_string);
+  ~OfflinePageHeader() {}
+
+  bool succeeded_to_parse() const { return succeeded_to_parse_; }

[Dmitry Titov, 2016/09/09 04:33:06]

Naming suggestion: successfully_parsed_

[jianli, 2016/09/10 00:56:03]

Done.

+  bool need_to_persist() const { return need_to_persist_; }

[Dmitry Titov, 2016/09/09 04:33:06]

Shouldn't this always be 'true' ?
Again, if persistence is not a part of this CL, this probably should not be
here.

[jianli, 2016/09/10 00:56:04]

I think it should always be false because we should only persist if needed.
Yes, persistence is in another CL that will be landed before this one. So I
would like this patch to also contain the parsing for this field.

+  const std::string& reason() const { return reason_; }

[Dmitry Titov, 2016/09/09 04:33:06]

this looks like enum that this class should expose, to avoid comparing strings
later in the code. That would encapsulate string comparisons in this class.

[jianli, 2016/09/10 00:56:03]

Done.

+  const std::string& name_space() const { return name_space_; }
+  const std::string& id() const { return id_; }
+
+ private:
+  bool succeeded_to_parse_;
+

[Dmitry Titov, 2016/09/09 04:33:06]

extra empty line

[jianli, 2016/09/10 00:56:03]

Done.

+  bool need_to_persist_;
+  std::string reason_;
+  std::string name_space_;
+  std::string id_;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflinePageHeader);
+};
+
+OfflinePageHeader::OfflinePageHeader(const std::string& header_string)
+    : succeeded_to_parse_(false), need_to_persist_(false) {
+  if (header_string.empty()) {
+    succeeded_to_parse_ = true;
+    return;
   }
 
-  // Currently we only support reason field.
-  base::StringTokenizer tokenizer(value, ", ");
+  base::StringTokenizer tokenizer(header_string, ", ");
   while (tokenizer.GetNext()) {
-    if (base::StartsWith(tokenizer.token(),
-                         kLoadingOfflinePageReason,
-                         base::CompareCase::INSENSITIVE_ASCII)) {
-      *reason = tokenizer.token().substr(
-          arraysize(kLoadingOfflinePageReason) - 1);
-      break;
-    }
+    std::string pair = tokenizer.token();
+    std::size_t pos = pair.find('=');
+    if (pos == std::string::npos)
+      return;
+    std::string key = pair.substr(0, pos);
+    std::string value = pair.substr(pos + 1);
+    if (key == kOfflinePageHeaderPersistKey)

[fgorski, 2016/09/08 02:25:02]

is there a place where you call toLower? Or should you make comparisons case
insensitive, perhaps?
This applies to all key comparisons.

[jianli, 2016/09/10 00:56:03]

Done.

+      need_to_persist_ = (value == "1");
+    else if (key == kOfflinePageHeaderReasonKey)
+      reason_ = value;
+    else if (key == kOfflinePageHeaderNamespaceKey)
+      name_space_ = value;
+    else if (key == kOfflinePageHeaderIDKey)
+      id_ = value;
   }
-  return true;
+
+  succeeded_to_parse_ = true;

[Dmitry Titov, 2016/09/09 04:33:06]

Are there valid combinations and invalid combinations? For example, setting id
but not namespace?

[jianli, 2016/09/10 00:56:03]

After I remove namespace field, all remaining 3 fields are orthogonal. 

 }
 
 bool IsNetworkProhibitivelySlow(net::URLRequest* request) {
   // NetworkQualityEstimator only works when it is enabled.
   if (!previews::IsOfflinePreviewsEnabled())
     return false;
 
   if (!request->context())
     return false;
 
   net::NetworkQualityEstimator* network_quality_estimator =
       request->context()->network_quality_estimator();
   if (!network_quality_estimator)
     return false;
 
   net::EffectiveConnectionType effective_connection_type =
       network_quality_estimator->GetEffectiveConnectionType();
   return effective_connection_type >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE &&
          effective_connection_type <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G;
 }
 
 NetworkState GetNetworkState(net::URLRequest* request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  std::string reason;
-  bool has_offline_header = ParseOfflineHeader(request, &reason);
-  if (has_offline_header &&
-      base::EqualsCaseInsensitiveASCII(reason,
-                                       kLoadingOfflinePageDueToNetError)) {
+  std::string offline_header_string;
+  request->extra_request_headers().GetHeader(
+      kOfflinePageHeader, &offline_header_string);
+  OfflinePageHeader offline_header(offline_header_string);
+  if (base::EqualsCaseInsensitiveASCII(
+        offline_header.reason(), kOfflinePageHeaderReasonValueDueToNetError)) {
     return NetworkState::FLAKY_NETWORK;
   }
 
   if (net::NetworkChangeNotifier::IsOffline())
     return NetworkState::DISCONNECTED_NETWORK;
 
   if (IsNetworkProhibitivelySlow(request))
     return NetworkState::PROHIBITIVELY_SLOW_NETWORK;
 
-  // The presence of custom offline header will force to load an offline page
-  // even when network is connected.
-  return has_offline_header ? NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK
-                            : NetworkState::CONNECTED_NETWORK;
+  // If custom offline header is present and contains the reason field, the
+  // offline page should be forced to load even when the network is connected.
+  return offline_header.succeeded_to_parse() && !offline_header.reason().empty()
+      ? NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK
+      : NetworkState::CONNECTED_NETWORK;
 }
 
 OfflinePageRequestJob::AggregatedRequestResult
 RequestResultToAggregatedRequestResult(
     RequestResult request_result, NetworkState network_state) {
   if (request_result == RequestResult::NO_TAB_ID)
     return OfflinePageRequestJob::AggregatedRequestResult::NO_TAB_ID;
 
   if (request_result == RequestResult::NO_WEB_CONTENTS)
     return OfflinePageRequestJob::AggregatedRequestResult::NO_WEB_CONTENTS;
@@ skipping 45 matching lines @@
    return OfflinePageRequestJob::AggregatedRequestResult::
        AGGREGATED_REQUEST_RESULT_MAX;
 }
 
 void ReportRequestResult(
     RequestResult request_result, NetworkState network_state) {
   OfflinePageRequestJob::ReportAggregatedRequestResult(
       RequestResultToAggregatedRequestResult(request_result, network_state));
 }
 
+OfflinePageModel* GetOfflinePageModel(
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  content::WebContents* web_contents = web_contents_getter.Run();
+  return web_contents ?
+      OfflinePageModelFactory::GetForBrowserContext(
+          web_contents->GetBrowserContext()) :
+      nullptr;
+}
+
 void NotifyOfflineFilePathOnIO(base::WeakPtr<OfflinePageRequestJob> job,
                                const base::FilePath& offline_file_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
   if (!job)
     return;
   job->OnOfflineFilePathAvailable(offline_file_path);
 }
 
 // Notifies OfflinePageRequestJob about the offline file path. Note that the
@@ skipping 53 matching lines @@
       OfflinePageTabHelper::FromWebContents(web_contents);
   DCHECK(tab_helper);
   tab_helper->SetOfflinePage(
       *offline_page, network_state == NetworkState::PROHIBITIVELY_SLOW_NETWORK);
 
   *offline_file_path = offline_page->file_path;
   return RequestResult::OFFLINE_PAGE_SERVED;
 }
 
 // Handles the result of finding an offline page.
-void SelectPageForOnlineURLDone(
+void SucceededToFindOfflinePage(
     NetworkState network_state,
     base::WeakPtr<OfflinePageRequestJob> job,
     content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
     const OfflinePageItem* offline_page) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   base::FilePath offline_file_path;
   RequestResult request_result = AccessOfflineFile(
       network_state, job, web_contents_getter, offline_page,
       &offline_file_path);
 
   ReportRequestResult(request_result, network_state);
 
   // NotifyOfflineFilePathOnUI should always be called regardless the failure
   // result and empty file path such that OfflinePageRequestJob will be notified
   // on failure.
   NotifyOfflineFilePathOnUI(job, offline_file_path);
 }
 
-void FailedToSelectOfflinePage(base::WeakPtr<OfflinePageRequestJob> job) {
+void FailedToFindOfflinePage(base::WeakPtr<OfflinePageRequestJob> job) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Proceed with empty file path in order to notify the OfflinePageRequestJob
   // about the failure.
   base::FilePath empty_file_path;
   NotifyOfflineFilePathOnUI(job, empty_file_path);
 }
 
 // Tries to find the offline page to serve for |online_url|.
-void SelectOfflinePage(
+void SelectPageForOnlineURL(
     const GURL& online_url,
     NetworkState network_state,
-    void* profile_id,
     content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
     OfflinePageRequestJob::Delegate::TabIdGetter tab_id_getter,
     base::WeakPtr<OfflinePageRequestJob> job) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // |profile_id| needs to be checked with ProfileManager::IsValidProfile
-  // before using it.
-  if (!g_browser_process->profile_manager()->IsValidProfile(profile_id)) {
-    FailedToSelectOfflinePage(job);
-    return;
-  }
-  Profile* profile = reinterpret_cast<Profile*>(profile_id);
-
   content::WebContents* web_contents = web_contents_getter.Run();
   if (!web_contents){
     ReportRequestResult(RequestResult::NO_WEB_CONTENTS, network_state);
-    FailedToSelectOfflinePage(job);
+    FailedToFindOfflinePage(job);
     return;
   }
   int tab_id;
   if (!tab_id_getter.Run(web_contents, &tab_id)) {
     ReportRequestResult(RequestResult::NO_TAB_ID, network_state);
-    FailedToSelectOfflinePage(job);
+    FailedToFindOfflinePage(job);
     return;
   }
 
   OfflinePageUtils::SelectPageForOnlineURL(
-      profile,
+      web_contents->GetBrowserContext(),
       online_url,
       tab_id,
-      base::Bind(&SelectPageForOnlineURLDone,
+      base::Bind(&SucceededToFindOfflinePage,
                  network_state,
                  job,
                  web_contents_getter));
 }
 
+void FindPageWithOfflineIDDone(
+    const GURL& online_url,

[fgorski, 2016/09/08 02:25:02]

nit: since we no longer have the concept of offline_url, perhaps calling it
simply url makes sense?

[jianli, 2016/09/10 00:56:04]

Agreed. But I rather make another patch after landing removing offline URL to
replace all occurrences of online_url to url since we still have quite a few
references in offline API.

+    NetworkState network_state,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
+    OfflinePageRequestJob::Delegate::TabIdGetter tab_id_getter,
+    base::WeakPtr<OfflinePageRequestJob> job,
+    const OfflinePageItem* offline_page) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // If the found offline page does not has same online URL as the request URL,

[fgorski, 2016/09/08 02:25:02]

nit: does not have the same URL as the online URL

[jianli, 2016/09/10 00:56:03]

Done.

+  // fall back to find the offline page based on the online URL.
+  if (offline_page->url != online_url) {
+    SelectPageForOnlineURL(
+        online_url, network_state, web_contents_getter, tab_id_getter, job);
+    return;
+  }
+
+  SucceededToFindOfflinePage(
+      network_state, job, web_contents_getter, offline_page);
+}
+
+// Tries to find an offline page associated with |offline_id|.
+void FindPageWithOfflineID(
+    const GURL& online_url,
+    int64_t offline_id,
+    NetworkState network_state,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
+    OfflinePageRequestJob::Delegate::TabIdGetter tab_id_getter,
+    base::WeakPtr<OfflinePageRequestJob> job) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  OfflinePageModel* offline_page_model =
+      GetOfflinePageModel(web_contents_getter);
+  if (!offline_page_model) {
+    FailedToFindOfflinePage(job);
+    return;
+  }
+
+  offline_page_model->GetPageByOfflineId(
+      offline_id,
+      base::Bind(&FindPageWithOfflineIDDone,
+                 online_url,
+                 network_state,
+                 web_contents_getter,
+                 tab_id_getter,
+                 job));
+}
+
+void GetOfflineIdsForClientIdDone(
+    const GURL& online_url,
+    NetworkState network_state,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
+    OfflinePageRequestJob::Delegate::TabIdGetter tab_id_getter,
+    base::WeakPtr<OfflinePageRequestJob> job,
+    const MultipleOfflineIdResult& offline_ids) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (offline_ids.empty()) {
+    FailedToFindOfflinePage(job);
+    return;
+  }
+
+  DCHECK_EQ(1u, offline_ids.size());
+  FindPageWithOfflineID(online_url,
+                        offline_ids[0],
+                        network_state,
+                        web_contents_getter,
+                        tab_id_getter,
+                        job);
+}
+
+// Tries to find an offline page associated with |client_id|.
+void FindPageWithClientID(
+    const GURL& online_url,
+    const ClientId& client_id,
+    NetworkState network_state,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
+    OfflinePageRequestJob::Delegate::TabIdGetter tab_id_getter,
+    base::WeakPtr<OfflinePageRequestJob> job) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  OfflinePageModel* offline_page_model =
+      GetOfflinePageModel(web_contents_getter);
+  if (!offline_page_model) {
+    FailedToFindOfflinePage(job);
+    return;
+  }
+
+  offline_page_model->GetOfflineIdsForClientId(
+      client_id,
+      base::Bind(&GetOfflineIdsForClientIdDone,
+                 online_url,
+                 network_state,
+                 web_contents_getter,
+                 tab_id_getter,
+                 job));
+}
+
+// Tries to find the offline page to serve for |online_url|.
+void SelectPage(
+    const GURL& online_url,
+    const std::string& offline_header_string,
+    NetworkState network_state,
+    content::ResourceRequestInfo::WebContentsGetter web_contents_getter,
+    OfflinePageRequestJob::Delegate::TabIdGetter tab_id_getter,
+    base::WeakPtr<OfflinePageRequestJob> job) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  OfflinePageHeader offline_header(offline_header_string);
+
+  // If an id, either offline ID or client ID, is present in the offline header,

[Dmitry Titov, 2016/09/09 04:33:06]

This logic is too complicated. It's hard to recall it looking at headers for the
request w/o inspecting the code. The culprit is merging client_id.id and
offline_id in the header, and this logic later tries to figure out which id was
actually specified. Also there is namespace that adds confusion and potentially
will need a wildcard or list (when async_loading is added)

Instead, I'd propose to use explicit instructions in the custom header, as
following:
client_id=<guid> or
offline_id=12345

Note there is no need to pass namespace since those are both unique ids.

As you say, the header that has both or neither or has one but there is no page
wiht such id would fall back to online URL.

[jianli, 2016/09/10 00:56:04]

Per discussion, we only support passing offline id now.

+  // try to load that particular version.
+  if (!offline_header.id().empty()) {
+    // If namespace is not set in offline header, the id given in the offline
+    // header should be used as offline ID.
+    if (offline_header.name_space().empty()) {
+      // if the id string cannot be converted to int64 id, fall through to
+      // select page via online URL.
+      int64_t offline_id;
+      if (base::StringToInt64(offline_header.id(), &offline_id)) {
+        FindPageWithOfflineID(online_url, offline_id, network_state,
+                              web_contents_getter, tab_id_getter, job);
+        return;
+      }
+    } else {
+      // Otherwise, try to find the offline page based on the client ID.
+      ClientId client_id(offline_header.name_space(), offline_header.id());
+      FindPageWithClientID(online_url, client_id, network_state,
+                           web_contents_getter, tab_id_getter, job);
+      return;
+    }
+  }
+
+  SelectPageForOnlineURL(online_url, network_state, web_contents_getter,
+                         tab_id_getter, job);
+}
+
 }  // namespace
 
 // static
 void OfflinePageRequestJob::ReportAggregatedRequestResult(
     AggregatedRequestResult result) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.AggregatedRequestResult",
       static_cast<int>(result),
       static_cast<int>(AggregatedRequestResult::AGGREGATED_REQUEST_RESULT_MAX));
 }
 
@@ skipping 60 matching lines @@
                             weak_ptr_factory_.GetWeakPtr()));
 }
 
 void OfflinePageRequestJob::StartAsync() {
   NetworkState network_state = GetNetworkState(request());
   if (network_state == NetworkState::CONNECTED_NETWORK) {
     FallbackToDefault();
     return;
   }
 
+  std::string offline_header_string;
+  request()->extra_request_headers().GetHeader(kOfflinePageHeader,
+                                               &offline_header_string);
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&SelectOfflinePage,
+      base::Bind(&SelectPage,
                  request()->url(),
+                 offline_header_string,
                  network_state,
-                 profile_id_,
                  delegate_->GetWebContentsGetter(request()),
                  delegate_->GetTabIdGetter(),
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void OfflinePageRequestJob::Kill() {
   net::URLRequestJob::Kill();
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
@@ skipping 19 matching lines @@
   file_path_ = offline_file_path;
   URLRequestFileJob::Start();
 }
 
 void OfflinePageRequestJob::SetDelegateForTesting(
     std::unique_ptr<Delegate> delegate) {
   delegate_ = std::move(delegate);
 }
 
 }  // namespace offline_pages