Introduce content::PageState (again).

Introduce content::PageState (again).

This is a concrete class wrapping a string that contains the
data of a serialized WebKit::WebHistoryItem class. Previously,
we've just passed around these as strings, giving them names
like "state", "content_state" or "history_state". It has been
hard to identify all of the places in the code where these
strings get passed around. A concrete class should make usage
more apparent. Plus, instead of manipulating the strings using
methods from webkit/glue/glue_serialize.h, we can just declare
methods on the PageState class. This makes the code much cleaner.

This first pass just implements PageState in terms of glue_serialize.
It also adds content/public/renderer/history_item_serialization.h
as the home for PageState to WebKit::WebHistoryItem conversion,
which should ideally only be usable from the renderer process.
(This bit is a step toward resolving bug 237243.)

page_state.h declares operator==() to support DCHECK_EQ, which
seems consistent with the idea of PageState being a replacement
for std::string. I didn't want to litter tests with calls to
PageState::ToEncodedData(). That would get cumbersome.

Originally reviewed at:
https://codereview.chromium.org/14985014

The only difference is that page_state.cc is now split into two
pieces: page_state.cc and page_state_webkit.cc.  The second holds
the definition of all methods that depend on webkit/glue.  That way
code like Chrome Frame and the iOS port of Chromium can use PageState
without pulling in a dependency on webkit/glue at link time.

BUG=240426
R=brettw@chromium.org, grt@chromium.org, joth@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202678

[grt (UTC plus 2), 2013-05-28 20:01:22]

lgtm regarding the size of npchrome_frame.dll.

[brettw, 2013-05-28 21:08:10]

lgtm

[joth, 2013-05-28 22:04:34]

https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
android_webview/native/state_serializer.cc:150: if
(!pickle->WriteString(entry.GetPageState().ToEncodedData()))
does the alter the format it is written out to the pickle as? If so, pleae could
you bump AW_STATE_VERSION => 20130528?

[darin (slow to review), 2013-05-28 22:47:01]

On 2013/05/28 22:04:34, joth wrote:
>
https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
> File android_webview/native/state_serializer.cc (right):
> 
>
https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
> android_webview/native/state_serializer.cc:150: if
> (!pickle->WriteString(entry.GetPageState().ToEncodedData()))
> does the alter the format it is written out to the pickle as? If so, pleae
could
> you bump AW_STATE_VERSION => 20130528?

No, there is no format change.

[joth, 2013-05-28 22:48:45]

aw/ lgtm

Thanks darin!

[Tom Sepez, 2013-05-28 22:48:49]

LGTM messages.

[darin (slow to review), 2013-05-28 22:52:10]

Committed patchset #4 manually as r202678 (presubmit successful).

[joth, 2013-05-28 22:59:44]

https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
File android_webview/native/state_serializer.cc (right):

https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
android_webview/native/state_serializer.cc:216:
content::PageState::CreateFromEncodedData(content_state));
latent question.... what if |content_state| is bogus and so the constructed
PageState::IsValid() will return false?
- I think we should detect it and bail out with a 'return false' here?
- That would mean the //DHCECK(IsValid()) you have commented out in the
PageState c'tor could never be enforced - least we make an untestable code path
here.
- Or we could have CreateFromPossiblyInvalidEndedData() factory method? hmm.

(Receiving bogus data here is plausible, if highly unlikely, as the state is
handed over to embedding app to persist and return to us.)

[darin (slow to review), 2013-05-28 23:04:13]

On 2013/05/28 22:59:44, joth wrote:
>
https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
> File android_webview/native/state_serializer.cc (right):
> 
>
https://codereview.chromium.org/16162003/diff/13001/android_webview/native/st...
> android_webview/native/state_serializer.cc:216:
> content::PageState::CreateFromEncodedData(content_state));
> latent question.... what if |content_state| is bogus and so the constructed
> PageState::IsValid() will return false?
> - I think we should detect it and bail out with a 'return false' here?
> - That would mean the //DHCECK(IsValid()) you have commented out in the
> PageState c'tor could never be enforced - least we make an untestable code
path
> here.
> - Or we could have CreateFromPossiblyInvalidEndedData() factory method? hmm.
> 
> (Receiving bogus data here is plausible, if highly unlikely, as the state is
> handed over to embedding app to persist and return to us.)

Please tell me you aren't serious about this data being handed over to
third-party apps!!!

I'm planning on deleting support for old formats after a couple revisions of
Chrome.

I think we need to talk.  This data format was never intended as a binary
contract between Chrome and third parties.  You've got me super worried now!!!

[joth, 2013-05-28 23:24:17]

to summarize offline chat: this data is only handed over as an opaque blob
and the contract is it's for temporary (volatile) use, NOT for restoring
across device reboot (i.e. never across a chromium update).
My point was more, there exists the potential for some pathologically
broken app to corrupt that data, so we take basic steps to detect & reject
such error.




On 28 May 2013 16:04, <darin@chromium.org> wrote:

> On 2013/05/28 22:59:44, joth wrote:
>
> https://codereview.chromium.**org/16162003/diff/13001/**
>
android_webview/native/state_**serializer.cc<https://codereview.chromium.org/16162003/diff/13001/android_webview/native/state_serializer.cc>
>
>> File android_webview/native/state_**serializer.cc (right):
>>
>
>
> https://codereview.chromium.**org/16162003/diff/13001/**
>
android_webview/native/state_**serializer.cc#newcode216<https://codereview.chromium.org/16162003/diff/13001/android_webview/native/state_serializer.cc#newcode216>
>
>> android_webview/native/state_**serializer.cc:216:
>> content::PageState::**CreateFromEncodedData(content_**state));
>> latent question.... what if |content_state| is bogus and so the
>> constructed
>> PageState::IsValid() will return false?
>> - I think we should detect it and bail out with a 'return false' here?
>> - That would mean the //DHCECK(IsValid()) you have commented out in the
>> PageState c'tor could never be enforced - least we make an untestable code
>>
> path
>
>> here.
>> - Or we could have CreateFromPossiblyInvalidEnded**Data() factory
>> method? hmm.
>>
>
>  (Receiving bogus data here is plausible, if highly unlikely, as the state
>> is
>> handed over to embedding app to persist and return to us.)
>>
>
> Please tell me you aren't serious about this data being handed over to
> third-party apps!!!
>
> I'm planning on deleting support for old formats after a couple revisions
> of
> Chrome.
>
> I think we need to talk.  This data format was never intended as a binary
> contract between Chrome and third parties.  You've got me super worried
> now!!!
>
>
https://codereview.chromium.**org/16162003/<https://codereview.chromium.org/1...
>

[joth, 2013-11-26 00:46:08]

(question on historic CL)

https://codereview.chromium.org/16162003/diff/13001/content/public/browser/na...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/16162003/diff/13001/content/public/browser/na...
content/public/browser/navigation_entry.h:79: // XXX
Just to check, was this XXX indicating something important? (I propose  to
remove it in https://codereview.chromium.org/84703003 else...)

[darin (slow to review), 2013-11-26 05:36:41]

https://codereview.chromium.org/16162003/diff/13001/content/public/browser/na...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/16162003/diff/13001/content/public/browser/na...
content/public/browser/navigation_entry.h:79: // XXX
On 2013/11/26 00:46:09, joth wrote:
> Just to check, was this XXX indicating something important? (I propose  to
> remove it in https://codereview.chromium.org/84703003 else...)

Whoops, that was not supposed to be committed. It was a reminder to me to revise
this comment block. Where it says "content state", it should instead say "page
state." I'll write a patch for that.

[darin (slow to review), 2013-11-26 05:47:19]

Here: https://codereview.chromium.org/87343002/


On Mon, Nov 25, 2013 at 9:36 PM, <darin@chromium.org> wrote:

>
> https://codereview.chromium.org/16162003/diff/13001/
> content/public/browser/navigation_entry.h
> File content/public/browser/navigation_entry.h (right):
>
> https://codereview.chromium.org/16162003/diff/13001/
> content/public/browser/navigation_entry.h#newcode79
> content/public/browser/navigation_entry.h:79: // XXX
> On 2013/11/26 00:46:09, joth wrote:
>
>> Just to check, was this XXX indicating something important? (I propose
>>
>  to
>
>> remove it in https://codereview.chromium.org/84703003 else...)
>>
>
> Whoops, that was not supposed to be committed. It was a reminder to me
> to revise this comment block. Where it says "content state", it should
> instead say "page state." I'll write a patch for that.
>
> https://codereview.chromium.org/16162003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joth, 2013-11-26 19:03:25]

Got it. Thanks!



On 25 November 2013 21:47, Darin Fisher <darin@chromium.org> wrote:

> Here: https://codereview.chromium.org/87343002/
>
>
> On Mon, Nov 25, 2013 at 9:36 PM, <darin@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/16162003/diff/13001/
>> content/public/browser/navigation_entry.h
>> File content/public/browser/navigation_entry.h (right):
>>
>> https://codereview.chromium.org/16162003/diff/13001/
>> content/public/browser/navigation_entry.h#newcode79
>> content/public/browser/navigation_entry.h:79: // XXX
>> On 2013/11/26 00:46:09, joth wrote:
>>
>>> Just to check, was this XXX indicating something important? (I propose
>>>
>>  to
>>
>>> remove it in https://codereview.chromium.org/84703003 else...)
>>>
>>
>> Whoops, that was not supposed to be committed. It was a reminder to me
>> to revise this comment block. Where it says "content state", it should
>> instead say "page state." I'll write a patch for that.
>>
>> https://codereview.chromium.org/16162003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.