Persist offline page info in a navigation entry if needed

components/sessions/content/content_serialized_navigation_builder.cc

Index: components/sessions/content/content_serialized_navigation_builder.cc
diff --git a/components/sessions/content/content_serialized_navigation_builder.cc b/components/sessions/content/content_serialized_navigation_builder.cc
index 59df5cc2d016730fa5a64ff1c9875b12ad3cd696..7c85d1690342dd11d23b3a1c306b8b4069e045aa 100644
--- a/components/sessions/content/content_serialized_navigation_builder.cc
+++ b/components/sessions/content/content_serialized_navigation_builder.cc
@@ -4,6 +4,8 @@
 
 #include "components/sessions/content/content_serialized_navigation_builder.h"
 
+#include "base/strings/string_tokenizer.h"
+#include "base/strings/string_util.h"
 #include "components/sessions/content/content_record_password_state.h"
 #include "components/sessions/core/serialized_navigation_entry.h"
 #include "content/public/browser/browser_context.h"
@@ -15,6 +17,34 @@
 
 namespace sessions {
 
+namespace {
+
+const char kOfflinePageHeader[] = "X-Chrome-offline";

[nasko, 2016/09/09 21:42:48]

Doesn't the full header value include ":" as well? I don't see it accounted for
anywhere.

[jianli, 2016/09/09 22:32:48]

Done.

+
+// Returns the offline page info that needs to be persisted, based on extra
+// headers.
+// TODO(jianli): Move this logic to components/offline_pages/request.

[nasko, 2016/09/09 21:42:48]

Is this TODO supposed to be resolved in later patchset or entirely different CL?
I'd suggest the former.

[jianli, 2016/09/09 22:32:48]

It will be in a different CL since doing it now in this patch will involve
restructure components/offline_pages directory which might make it hard to merge
to M54.

+std::string GetOfflinePageInfo(const content::NavigationEntry& entry) {
+  std::string extra_headers = entry.GetExtraHeaders();
+  if (extra_headers.empty())
+    return std::string();
+  // The offline header will be the only extra header if it is present.

[nasko, 2016/09/09 21:42:48]

There is no code that enforces that. Wouldn't adding another header in front of
the X-Chrome-offline one totally break your feature silently?

[jianli, 2016/09/09 22:32:48]

Most of time extra request header will not be sent. It will only be sent in a
few limited scenarios, like offline pages or data saver mode. It is not likely
that another header is also added together with offline header. Per discussion
with creis, we feel that limiting the extra header to only single offline header
will make the code more secure.

+  if (!base::StartsWith(extra_headers,
+                        kOfflinePageHeader,
+                        base::CompareCase::INSENSITIVE_ASCII)) {
+    return std::string();
+  }
+  std::string value = extra_headers.substr(arraysize(kOfflinePageHeader) - 1);

[nasko, 2016/09/09 21:42:48]

Doesn't arraysize require including base/macros.h?

[jianli, 2016/09/09 22:32:48]

Done.

+  base::StringTokenizer tokenizer(value, ", ");
+  while (tokenizer.GetNext()) {
+    if (tokenizer.token() == "persist=1")
+      return value;
+  }
+  return std::string();
+}
+
+}  // namespace
+
 // static
 SerializedNavigationEntry
 ContentSerializedNavigationBuilder::FromNavigationEntry(
@@ -41,6 +71,7 @@ ContentSerializedNavigationBuilder::FromNavigationEntry(
   navigation.http_status_code_ = entry.GetHttpStatusCode();
   navigation.redirect_chain_ = entry.GetRedirectChain();
   navigation.password_state_ = GetPasswordStateFromNavigation(entry);
+  navigation.offline_page_info_ = GetOfflinePageInfo(entry);
 
   return navigation;
 }
@@ -53,6 +84,11 @@ ContentSerializedNavigationBuilder::ToNavigationEntry(
     content::BrowserContext* browser_context) {
   blink::WebReferrerPolicy policy =
       static_cast<blink::WebReferrerPolicy>(navigation->referrer_policy_);
+  // The extra headers, except the one used for offline page support, are not
+  // sync'ed across sessions.
+  std::string extra_headers;
+  if (!navigation->offline_page_info_.empty())
+    extra_headers = kOfflinePageHeader + navigation->offline_page_info_;

[nasko, 2016/09/09 21:42:48]

Shouldn't header name and value be separated by ":"?

[jianli, 2016/09/09 22:32:48]

Done.

   std::unique_ptr<content::NavigationEntry> entry(
       content::NavigationController::CreateNavigationEntry(
           navigation->virtual_url_,
@@ -62,8 +98,7 @@ ContentSerializedNavigationBuilder::ToNavigationEntry(
           // Use a transition type of reload so that we don't incorrectly
           // increase the typed count.
           ui::PAGE_TRANSITION_RELOAD, false,
-          // The extra headers are not sync'ed across sessions.
-          std::string(), browser_context));
+          extra_headers, browser_context));
 
   entry->SetTitle(navigation->title_);
   entry->SetPageState(content::PageState::CreateFromEncodedData(