Support serving offline page by offline ID

chrome/browser/android/offline_pages/offline_page_request_job.cc

Index: chrome/browser/android/offline_pages/offline_page_request_job.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_request_job.cc b/chrome/browser/android/offline_pages/offline_page_request_job.cc
index 886ab76848337dbf2d8b5570c8de32ab997b43eb..30b4bbca19bf2d32649e0f916097db1d2b0f83c4 100644
--- a/chrome/browser/android/offline_pages/offline_page_request_job.cc
+++ b/chrome/browser/android/offline_pages/offline_page_request_job.cc
@@ -10,6 +10,7 @@
 #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"
@@ -33,9 +34,12 @@
 
 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 {
 
@@ -113,27 +117,54 @@ class DefaultDelegate : public OfflinePageRequestJob::Delegate {
   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) {
@@ -158,11 +189,12 @@ bool IsNetworkProhibitivelySlow(net::URLRequest* request) {
 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;
   }
 
@@ -172,10 +204,11 @@ NetworkState GetNetworkState(net::URLRequest* request) {
   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
@@ -241,6 +274,17 @@ void ReportRequestResult(
       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);
@@ -314,7 +358,7 @@ RequestResult AccessOfflineFile(
 }
 
 // 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,
@@ -334,7 +378,7 @@ void SelectPageForOnlineURLDone(
   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
@@ -344,46 +388,174 @@ void FailedToSelectOfflinePage(base::WeakPtr<OfflinePageRequestJob> job) {
 }
 
 // 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
@@ -464,13 +636,16 @@ void OfflinePageRequestJob::StartAsync() {
     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()));