Android WebView save/restoreState and backForardList Part 1

android_webview/native/state_serializer.cc

Index: android_webview/native/state_serializer.cc
diff --git a/android_webview/native/state_serializer.cc b/android_webview/native/state_serializer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b6c2518e3a50f7d9142de8c393ac83e881e621f3
--- /dev/null
+++ b/android_webview/native/state_serializer.cc
@@ -0,0 +1,223 @@
+// Copyright (c) 2012 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 "android_webview/native/state_serializer.h"
+
+#include <string>
+#include <vector>

[boliu, 2012/11/19 19:57:31]

Not needed, will remove before cq/submit

[boliu, 2012/11/20 22:18:16]

Done.

+
+#include "base/memory/scoped_vector.h"
+#include "base/pickle.h"
+#include "base/time.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/web_contents.h"
+
+using std::string;
+using std::vector;
+
+namespace android_webview {
+
+namespace {
+// Sanity check value that we are restoring from a valid pickle.
+const uint32 AW_STATE_VERSION = 20121116;

[mkosiba (inactive), 2012/11/20 17:40:57]

1) I don't think this is a good idea. The next person to touch the file will
probably forget to update the value so if you really want to have a 'value in
the header' you should use a magic value instead of a date

2) if the app decides to hand us garbage we should simply not crash and not
assume that if the header is valid then the data must be OK

[boliu, 2012/11/20 19:35:38]

I see the point that next person might miss this, but how does a magic value
(which I assume is constant) help with this?

Anyway, might be better to use the sdk version here instead?
There is a lot of defensive code to make sure this does not happen. It should
just return false. I have a java test to test an invalid bundle passed, but
testing here would pretty much be testing Pickle, which (I assume) already has
great test coverage.

[joth, 2012/11/20 19:51:02]

A major motivator for a version number in general is that should a future
version ever need to be able to import old versions, this enables it. While we
don't need that specific feature here, I've previously found it's a very
convenient pattern to use and benefit increases if used consistency.
A comment at the end of the WriteToPickle method reminding to rev. the version
if fields are added or removed wouldn't hurt.

[boliu, 2012/11/20 22:18:16]

Added comment.

+}  // namespace
+
+bool WriteToPickle(const content::WebContents& web_contents,
+                   Pickle* pickle) {
+  DCHECK(pickle);
+
+  if (!WriteHeaderToPickle(pickle))
+    return false;
+
+  const content::NavigationController& controller =
+      web_contents.GetController();
+  const int entry_count = controller.GetEntryCount();
+  const int selected_entry = controller.GetCurrentEntryIndex();
+  DCHECK(entry_count >= 0);
+  DCHECK(selected_entry >= -1);  // -1 is valid
+  DCHECK(selected_entry < entry_count);
+
+  if (!pickle->WriteInt(entry_count))
+    return false;
+
+  if (!pickle->WriteInt(selected_entry))
+    return false;
+
+  for (int i = 0; i < entry_count; ++i) {
+    if (!WriteNavigationEntryToPickle(*controller.GetEntryAtIndex(i), pickle))
+      return false;
+  }
+
+  return true;
+}
+
+bool RestoreFromPickle(PickleIterator* iterator,
+                       content::WebContents* web_contents) {
+  DCHECK(iterator);
+  DCHECK(web_contents);
+
+  if (!RestoreHeaderFromPickle(iterator))
+    return false;
+
+  int entry_count = -1;
+  int selected_entry = -2;  // -1 is a valid value
+
+  if (!iterator->ReadInt(&entry_count))
+    return false;
+
+  if (!iterator->ReadInt(&selected_entry))
+    return false;
+
+  if (entry_count < 0)
+    return false;
+  if (selected_entry < -1)
+    return false;
+  if (selected_entry >= entry_count)
+    return false;
+
+  ScopedVector<content::NavigationEntry> restored_entries;
+  for (int i = 0; i < entry_count; ++i) {
+    restored_entries.push_back(content::NavigationEntry::Create());
+    if (!RestoreNavigationEntryFromPickle(iterator, restored_entries[i]))
+      return false;
+  }
+
+  // |web_contents| takes ownership of these entries after this call.
+  web_contents->GetController().Restore(
+      selected_entry,
+      content::NavigationController::RESTORE_LAST_SESSION_EXITED_CLEANLY,
+      &restored_entries.get());
+  DCHECK_EQ(0u, restored_entries.size());
+
+  return true;
+}
+
+bool WriteHeaderToPickle(Pickle* pickle) {
+  return pickle->WriteUInt32(AW_STATE_VERSION);
+}
+
+bool RestoreHeaderFromPickle(PickleIterator* iterator) {
+  uint32 state_version = -1;
+  if (!iterator->ReadUInt32(&state_version))
+    return false;
+
+  if (AW_STATE_VERSION != state_version)
+    return false;
+
+  return true;
+}
+
+bool WriteNavigationEntryToPickle(const content::NavigationEntry& entry,
+                                  Pickle* pickle) {
+  if (!pickle->WriteString(entry.GetURL().spec()))
+    return false;
+
+  if (!pickle->WriteString(entry.GetVirtualURL().spec()))
+    return false;
+
+  const content::Referrer& referrer = entry.GetReferrer();
+  if (!pickle->WriteString(referrer.url.spec()))
+    return false;
+  if (!pickle->WriteInt(static_cast<int>(referrer.policy)))
+    return false;
+
+  if (!pickle->WriteString16(entry.GetTitle()))
+    return false;
+
+  if (!pickle->WriteString(entry.GetContentState()))
+    return false;
+
+  if (!pickle->WriteBool(static_cast<int>(entry.GetHasPostData())))
+    return false;
+
+  if (!pickle->WriteString(entry.GetOriginalRequestURL().spec()))
+    return false;
+
+  if (!pickle->WriteBool(static_cast<int>(entry.GetIsOverridingUserAgent())))
+    return false;
+
+  if (!pickle->WriteInt64(entry.GetTimestamp().ToInternalValue()))
+    return false;
+
+  return true;
+}
+
+bool RestoreNavigationEntryFromPickle(PickleIterator* iterator,
+                                      content::NavigationEntry* entry) {

[mkosiba (inactive), 2012/11/20 17:40:57]

I bet we already have code for serializing NavigationEntries in chrome/ any
reason we can't share that?

[boliu, 2012/11/20 19:35:38]

Yes we do. It's TabNavigation in chrome/browser/sessions/session_types.[h|cc].

Reason for not using it is:
* TabNavigation is tightly integrated with the rest of chrome session restore
and sync code
* android webview *may* have different requirements for fields to store since we
are the only ones using things like base_url_for_data_url
* TabNavigation does unnecessary copying of data, which in webview case, is
undesired since I assume save/restore is called on android all the time
* and of course easier to write my own thing.

I think I'll put the first three points in the issue description :)

[joth, 2012/11/20 19:51:02]

May even be worth a block comment at top of this file.

[boliu, 2012/11/20 22:18:16]

Done.

+  {
+    string url;
+    if (!iterator->ReadString(&url))
+      return false;
+    entry->SetURL(GURL(url));
+  }
+
+  {
+    string virtual_url;
+    if (!iterator->ReadString(&virtual_url))
+      return false;
+    entry->SetVirtualURL(GURL(virtual_url));
+  }
+
+  {
+    content::Referrer referrer;
+    string referrer_url;
+    int policy;
+
+    if (!iterator->ReadString(&referrer_url))
+      return false;
+    if (!iterator->ReadInt(&policy))
+      return false;
+
+    referrer.url = GURL(referrer_url);
+    referrer.policy = static_cast<WebKit::WebReferrerPolicy>(policy);
+    entry->SetReferrer(referrer);
+  }
+
+  {
+    string16 title;
+    if (!iterator->ReadString16(&title))
+      return false;
+    entry->SetTitle(title);
+  }
+
+  {
+    string content_state;
+    if (!iterator->ReadString(&content_state))
+      return false;
+    entry->SetContentState(content_state);
+  }
+
+  {
+    bool has_post_data;
+    if (!iterator->ReadBool(&has_post_data))
+      return false;
+    entry->SetHasPostData(has_post_data);
+  }
+
+  {
+    string original_request_url;
+    if (!iterator->ReadString(&original_request_url))
+      return false;
+    entry->SetOriginalRequestURL(GURL(original_request_url));
+  }
+
+  {
+    bool is_overriding_user_agent;
+    if (!iterator->ReadBool(&is_overriding_user_agent))
+      return false;
+    entry->SetIsOverridingUserAgent(is_overriding_user_agent);
+  }
+
+  {
+    int64 timestamp;
+    if (!iterator->ReadInt64(&timestamp))
+      return false;
+    entry->SetTimestamp(base::Time::FromInternalValue(timestamp));

[joth, 2012/11/20 19:51:02]

wild idea: as we don't need to support reading these across versions, we could
define a IPC struct for them and have that do the heavy work serializing /
deserializing? 

Another option would be to define a protobuf for it; not sure how much protobuf
code we're currently using though and if we want to pull that in just for this.

[boliu, 2012/11/20 21:29:38]

Ignoring the fact that this is totally abusing IPC code...

If we define a new ipc struct, then I assume we'd need an extra copy into the
struct.
If we define an ipc trait, does that block others from ever serializing
NavigationEntry in another form?

Anyway, yeah, this one is a bit too wild I think :)
I've never used profobufs before but I've heard good things about it. I don't
see a lot of dependencies on third_party/protobuf from chromium code though. Do
you think it's worth taking time investigating this?

+  }
+
+  return true;
+}
+
+}  // namespace android_webview