Make AW state_serializer handle restoring also legacy format

android_webview/native/state_serializer_unittest.cc

Index: android_webview/native/state_serializer_unittest.cc
diff --git a/android_webview/native/state_serializer_unittest.cc b/android_webview/native/state_serializer_unittest.cc
index a9671c6717476a0887b5e019f6661df52df5b50c..6d6cad1426b7199a53bd6d064fac5df038cee53d 100644
--- a/android_webview/native/state_serializer_unittest.cc
+++ b/android_webview/native/state_serializer_unittest.cc
@@ -27,8 +27,8 @@ TEST(AndroidWebViewStateSerializerTest, TestHeaderSerialization) {
   EXPECT_TRUE(result);
 
   base::PickleIterator iterator(pickle);
-  result = internal::RestoreHeaderFromPickle(&iterator);
-  EXPECT_TRUE(result);
+  uint32_t version = internal::RestoreHeaderFromPickle(&iterator);
+  EXPECT_GT(version, 0U);
 }
 
 TEST(AndroidWebViewStateSerializerTest, TestNavigationEntrySerialization) {
@@ -98,6 +98,77 @@ TEST(AndroidWebViewStateSerializerTest, TestNavigationEntrySerialization) {
   EXPECT_EQ(http_status_code, copy->GetHttpStatusCode());
 }
 
+TEST(AndroidWebViewStateSerializerTest,
+     TestLegacyNavigationEntrySerialization) {
+  // This is required for NavigationEntry::Create.
+  content::ContentClient content_client;
+  content::SetContentClient(&content_client);
+  content::ContentBrowserClient browser_client;
+  content::SetBrowserClientForTesting(&browser_client);
+
+  scoped_ptr<content::NavigationEntry> entry(
+      content::NavigationEntry::Create());
+
+  const GURL url("http://url");
+  const GURL virtual_url("http://virtual_url");
+  content::Referrer referrer;
+  referrer.url = GURL("http://referrer_url");
+  referrer.policy = blink::WebReferrerPolicyOrigin;
+  const base::string16 title(base::UTF8ToUTF16("title"));
+  const content::PageState page_state =
+      content::PageState::CreateFromEncodedData("completely bogus state");
+  const bool has_post_data = true;
+  const GURL original_request_url("http://original_request_url");
+  const GURL base_url_for_data_url("http://base_url");
+  const string data_url_as_string("data:text/html;charset=utf-8;base64,");
+  const bool is_overriding_user_agent = true;
+  const base::Time timestamp = base::Time::FromInternalValue(12345);
+  const int http_status_code = 404;
+
+  entry->SetURL(url);
+  entry->SetVirtualURL(virtual_url);
+  entry->SetReferrer(referrer);
+  entry->SetTitle(title);
+  entry->SetPageState(page_state);
+  entry->SetHasPostData(has_post_data);
+  entry->SetOriginalRequestURL(original_request_url);
+  entry->SetBaseURLForDataURL(base_url_for_data_url);
+  {
+    scoped_refptr<base::RefCountedString> s = new base::RefCountedString();
+    s->data().assign(data_url_as_string);
+    entry->SetDataURLAsString(s);
+  }
+  entry->SetIsOverridingUserAgent(is_overriding_user_agent);
+  entry->SetTimestamp(timestamp);
+  entry->SetHttpStatusCode(http_status_code);

[mnaganov (inactive), 2016/02/10 16:37:45]

I would suggest extracting entry initialization code into a function to avoid
copy-pasting it.

[sbergner, 2016/02/11 07:23:28]

Acknowledged.

+
+  base::Pickle pickle;
+  bool result =
+      internal::WriteNavigationEntryToPickle(20130814, *entry, &pickle);
+  EXPECT_TRUE(result);
+
+  scoped_ptr<content::NavigationEntry> copy(content::NavigationEntry::Create());
+  base::PickleIterator iterator(pickle);
+  result = internal::RestoreNavigationEntryFromPickle(20130814, &iterator,

[mnaganov (inactive), 2016/02/10 16:37:45]

Shouldn't this actually be a test where we save a pickle in the old format and
then restore in the new format? We will not be saving using the old format in
the real life anymore.

[sbergner, 2016/02/11 07:23:28]

What this test aims to check is that we can still actually load the legacy
format as well. To do that I create a legacy pickle (with the
WriteNavigationEntryToPickle(20130814,...)) so when reading that pickle the
state_version of the pickle is the 20130814 version. I'm trying to mimic what
happens in reality when a state has been saved with the old WebView version
(pickle version=20130814) and then restored with this new version.

[mnaganov (inactive), 2016/02/11 17:21:48]

Right, I've got confused by this direct version specification in the call to
"Restore...". I have a suggestion how to make this more clear (and perhaps even
closer to the real case) -- when saving, write both the header and the contents
to the pickle, and then on restore use the public "RestoreFromPickle" to test
that it will read the header and restore the entry using the saved version.
WDYT?

[sbergner, 2016/02/12 07:53:31]

Hm, I do agree that actually trying to test the complete pickle restore (incl.
header) would be good. However I believe that means we have to do one of two
things:
1. Construct a WebContents object to feed into a new internal::WriteToPickle()
which also takes a state_version
2. Duplicate the data writing of selection index and entry count so we would
call:
   WriteHeaderToPickle(20130814, ..);
   pickle->WriteInt(1);  // entry count
   pickle->WriteInt(0);  // selected index
   WriteNavigationEntryToPickle(20130814, ..);

Regarding option 1, I am not very familiar with content::WebContents but initial
feeling was that it wasn't very easy to construct that object and get it to
contain the wanted navigation entries.

For option 2 I don't really like the fact that we have to rely on knowledge
about implementation details of how WriteToPickle works.

Do you see other alternatives or think any of the above is a good way to go?

[mnaganov (inactive), 2016/02/12 16:22:47]

Yeah, involving an instance of a real or mocked out WebContents would be too
heavy. I think the resulting set of tests is good enough. Thanks! 

+                                                      copy.get());
+  EXPECT_TRUE(result);
+
+  EXPECT_EQ(url, copy->GetURL());
+  EXPECT_EQ(virtual_url, copy->GetVirtualURL());
+  EXPECT_EQ(referrer.url, copy->GetReferrer().url);
+  EXPECT_EQ(referrer.policy, copy->GetReferrer().policy);
+  EXPECT_EQ(title, copy->GetTitle());
+  EXPECT_EQ(page_state, copy->GetPageState());
+  EXPECT_EQ(has_post_data, copy->GetHasPostData());
+  EXPECT_EQ(original_request_url, copy->GetOriginalRequestURL());
+  EXPECT_EQ(base_url_for_data_url, copy->GetBaseURLForDataURL());
+  // DataURL not supported by 20130814 format
+  EXPECT_FALSE(copy->GetDataURLAsString());
+  EXPECT_EQ(is_overriding_user_agent, copy->GetIsOverridingUserAgent());
+  EXPECT_EQ(timestamp, copy->GetTimestamp());
+  EXPECT_EQ(http_status_code, copy->GetHttpStatusCode());
+}
+

[mnaganov (inactive), 2016/02/11 17:21:48]

It also makes sense to add a test that uses a pickle with some non-supported
version in the header, and verify that we will reject it.

[sbergner, 2016/02/12 07:53:32]

Acknowledged.

 TEST(AndroidWebViewStateSerializerTest, TestEmptyDataURLSerialization) {
   // This is required for NavigationEntry::Create.
   content::ContentClient content_client;