Android WebView save/restoreState and backForardList Part 1

Android WebView save/restoreState and backForardList Part 1

Design for save/restoreState is do all serialization in native and pass
an opaque byte array to java to put into the bundle.

This patch contains the native code to (de)serialize a WebContents with
some unit test coverage.

Reasons for not re-using TabNavigation under chrome/:
* Android WebView has different requirements for fields to store since
we are the only ones using values like base_url_for_data_url.
* TabNavigation does unnecessary copying of data, which in Android
WebView case, is undesired since save/restore is called in Android
very frequently.
* TabNavigation is tightly integrated with the rest of chrome session
restore and sync code, and has other purpose in addition to serializing
NavigationEntry.

Optimization-wise, there will be an unnecessary copy when converting
native to java byte array. Only way to avoid it is to know the
allocation size before pickling starts. Maybe consider pre-computing the
size (brittle), or use chrome's design of capping the size of the data
and throwing away old entries and "less important" information. Only
consider this if speed becomes a problem.

BUG=
Android WebView only change. Ran through trybots.
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169024

[boliu, 2012-11-18 21:22:45]

Decided to break this into parts. What I had on Friday is here:
http://codereview.chromium.org/11348113/

Joth: review
Ben: Why does base_url_for_data_url not need to be persisted (I think you know
this area?)
Martin: FYI, new unit tests :)

[boliu, 2012-11-19 19:57:30]

Ping!

http://codereview.chromium.org/11420056/diff/5001/android_webview/native/stat...
File android_webview/native/state_serializer.cc (right):

http://codereview.chromium.org/11420056/diff/5001/android_webview/native/stat...
android_webview/native/state_serializer.cc:8: #include <vector>
Not needed, will remove before cq/submit

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

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
File android_webview/native/state_serializer.cc (right):

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
android_webview/native/state_serializer.cc:24: const uint32 AW_STATE_VERSION =
20121116;
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

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
android_webview/native/state_serializer.cc:148: content::NavigationEntry* entry)
{
I bet we already have code for serializing NavigationEntries in chrome/ any
reason we can't share that?

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
File android_webview/native/state_serializer.h (right):

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
android_webview/native/state_serializer.h:37: // side of tests.
normally you would stick'em in an internals namespace

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

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.cc:24: const uint32 AW_STATE_VERSION =
20121116;
On 2012/11/20 17:40:57, Martin Kosiba wrote:
> 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
> 
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?

> 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

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.

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.cc:148: content::NavigationEntry* entry)
{
On 2012/11/20 17:40:57, Martin Kosiba wrote:
> I bet we already have code for serializing NavigationEntries in chrome/ any
> reason we can't share that?

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]

(not read in detail yet)

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
File android_webview/native/state_serializer.cc (right):

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
android_webview/native/state_serializer.cc:24: const uint32 AW_STATE_VERSION =
20121116;
On 2012/11/20 19:35:38, boliu wrote:
> On 2012/11/20 17:40:57, Martin Kosiba wrote:
> > 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
> > 
> 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?
> 
> > 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
> 
> 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.

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.

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
android_webview/native/state_serializer.cc:148: content::NavigationEntry* entry)
{
On 2012/11/20 19:35:38, boliu wrote:
> On 2012/11/20 17:40:57, Martin Kosiba wrote:
> > I bet we already have code for serializing NavigationEntries in chrome/ any
> > reason we can't share that?
> 
> 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 :)

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

https://chromiumcodereview.appspot.com/11420056/diff/5001/android_webview/nat...
android_webview/native/state_serializer.cc:217:
entry->SetTimestamp(base::Time::FromInternalValue(timestamp));
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]

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.cc:217:
entry->SetTimestamp(base::Time::FromInternalValue(timestamp));
On 2012/11/20 19:51:02, joth wrote:
> 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?

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 :)

> 
> 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.

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?

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

PTAL

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.cc:8: #include <vector>
On 2012/11/19 19:57:31, boliu wrote:
> Not needed, will remove before cq/submit

Done.

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.cc:24: const uint32 AW_STATE_VERSION =
20121116;
Added comment.

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.cc:148: content::NavigationEntry* entry)
{
On 2012/11/20 19:51:02, joth wrote:
> On 2012/11/20 19:35:38, boliu wrote:
> > On 2012/11/20 17:40:57, Martin Kosiba wrote:
> > > I bet we already have code for serializing NavigationEntries in chrome/
any
> > > reason we can't share that?
> > 
> > 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 :)
> 
> May even be worth a block comment at top of this file.

Done.

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
File android_webview/native/state_serializer.h (right):

https://codereview.chromium.org/11420056/diff/5001/android_webview/native/sta...
android_webview/native/state_serializer.h:37: // side of tests.
On 2012/11/20 17:40:57, Martin Kosiba wrote:
> normally you would stick'em in an internals namespace

Done.

[joth, 2012-11-20 23:06:48]

lgtm (assuming martin had no other comments)

https://codereview.chromium.org/11420056/diff/6004/android_webview/native/sta...
File android_webview/native/state_serializer.h (right):

https://codereview.chromium.org/11420056/diff/6004/android_webview/native/sta...
android_webview/native/state_serializer.h:23: // success. Note that no promise
is made that the output variable is not
nit: Functions to write and restore...

(i read this as comment for the function only, then wondered why you're
repeating yourself about output)


nit: output variable => output parameter.

[boliu, 2012-11-20 23:21:12]

Martin: I put NOTRY=true. Assuming you don't have any more problems with this,
please cq this while I sleep :)

Joth: In response to your IPC comment in mail: Another point I forgot to mention
is IPC is too rigid that it only serializes a struct. The current chrome
implementation caps the overall serialization size to 63kB (I think?) and start
throwing away "less important" variables if going over. We may or may not go
this way (I'd much rather throw away entire old history entries), but I'm saying
IPC serialization won't allow us to do this.

https://codereview.chromium.org/11420056/diff/6004/android_webview/native/sta...
File android_webview/native/state_serializer.h (right):

https://codereview.chromium.org/11420056/diff/6004/android_webview/native/sta...
android_webview/native/state_serializer.h:23: // success. Note that no promise
is made that the output variable is not
On 2012/11/20 23:06:48, joth wrote:
> nit: Functions to write and restore...
> 
> (i read this as comment for the function only, then wondered why you're
> repeating yourself about output)
> 
> 
> nit: output variable => output parameter.

This is not true anymore since NavigationController::Restore as written cannot
fail, so if RestoreFromPickle returns false, then web_contents is indeed not
modified. Removed this comment.

[mkosiba (inactive), 2012-11-21 12:17:03]

lgtm

[commit-bot: I haz the power, 2012-11-21 12:17:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/11420056/10002

[commit-bot: I haz the power, 2012-11-21 12:34:11]

Change committed as 169024