Introduce content::PageState

Introduce content::PageState.

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.

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

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

[darin (slow to review), 2013-05-16 20:38:35]

Brett, I'm just looking for some initial design feedback from you.  You don't
need to do an exhaustive code review yet (unless you want to).  Thanks!!

[tfarina, 2013-05-16 22:16:48]

https://codereview.chromium.org/14985014/diff/7001/content/public/common/page...
File content/public/common/page_state.h (right):

https://codereview.chromium.org/14985014/diff/7001/content/public/common/page...
content/public/common/page_state.h:46: };
DISALLOW_COPY_AND_ASSIGN?

[darin (slow to review), 2013-05-17 00:26:16]

On 2013/05/16 22:16:48, tfarina wrote:
>
https://codereview.chromium.org/14985014/diff/7001/content/public/common/page...
> File content/public/common/page_state.h (right):
> 
>
https://codereview.chromium.org/14985014/diff/7001/content/public/common/page...
> content/public/common/page_state.h:46: };
> DISALLOW_COPY_AND_ASSIGN?

No, the idea is to treat PageState like std::string or GURL.  You should be able
to copy them around by value.  The default copy constructor and assignment
operator is perfect.

[brettw, 2013-05-17 04:57:10]

PageState seems fine.

https://codereview.chromium.org/14985014/diff/7001/content/public/renderer/hi...
File content/public/renderer/history_item_serialization.h (right):

https://codereview.chromium.org/14985014/diff/7001/content/public/renderer/hi...
content/public/renderer/history_item_serialization.h:19: CONTENT_EXPORT
PageState HistoryItemToPageState(
Does this need to be in the public API? I was not expecting this.

[darin (slow to review), 2013-05-23 06:46:51]

On 2013/05/17 04:57:10, brettw wrote:
> PageState seems fine.
> 
>
https://codereview.chromium.org/14985014/diff/7001/content/public/renderer/hi...
> File content/public/renderer/history_item_serialization.h (right):
> 
>
https://codereview.chromium.org/14985014/diff/7001/content/public/renderer/hi...
> content/public/renderer/history_item_serialization.h:19: CONTENT_EXPORT
> PageState HistoryItemToPageState(
> Does this need to be in the public API? I was not expecting this.

Yes, unfortunately.  It is there to support
content/shell/renderer/webkit_test_runner.cc

[darin (slow to review), 2013-05-23 19:40:57]

This is ready for a real code review.  Thanks in advance!

[brettw, 2013-05-23 23:38:39]

LGTM, surprisingly simple once you get into it.

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
File content/public/common/page_state.h (right):

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
content/public/common/page_state.h:43: bool Equals(const PageState& page_state)
const;
I guess I don't feel that strongly, but since we're supporting operator== on
this anyway, I'd just remove this function and make == and != class members

https://codereview.chromium.org/14985014/diff/50001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/14985014/diff/50001/content/renderer/render_v...
content/renderer/render_view_impl.cc:1688:
item.setURLString(request.url().spec().utf16());
ugh, hopefully our refactoring can remove the need for spec().utf16()

[darin (slow to review), 2013-05-24 06:34:58]

On 2013/05/23 23:38:39, brettw wrote:
> LGTM, surprisingly simple once you get into it.
> 
>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> File content/public/common/page_state.h (right):
> 
>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> content/public/common/page_state.h:43: bool Equals(const PageState&
page_state)
> const;
> I guess I don't feel that strongly, but since we're supporting operator== on
> this anyway, I'd just remove this function and make == and != class members

I have a habit of always defining operator== as a non-member function so that
type coercion works equally on both the LHS as well as the RHS.


>
https://codereview.chromium.org/14985014/diff/50001/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/14985014/diff/50001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:1688:
> item.setURLString(request.url().spec().utf16());
> ugh, hopefully our refactoring can remove the need for spec().utf16()

Yes.  I should check with abarth.  There may already be a better way here.

[darin (slow to review), 2013-05-24 07:30:42]

+tsepez for IPC review

Tom, with this change the "content state" strings get replaced by a PageState
class.  You'll want to track PageState sent over IPC just as you would track
FilePath since PageState contains some FilePaths.

[Tom Sepez, 2013-05-24 17:46:48]

Darin, this is a good step to making this clearer. LGTM with comments.

https://codereview.chromium.org/14985014/diff/50001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/14985014/diff/50001/chrome/browser/ui/browser...
chrome/browser/ui/browser_commands.cc:1022:
active_entry->SetPageState(page_state.RemoveScrollOffset());
Do we similarly want to strip out the referenced files here?  I don't think the
view-source version of the page needs thee to be blessed.

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
File content/public/common/page_state.cc (right):

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
content/public/common/page_state.cc:27: PageState PageState::CreateForTesting(
Can this move to the _unittest file? Maybe as a testing subclass of PageState?

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
File content/public/common/page_state.h (right):

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
content/public/common/page_state.h:27: // designed as an archival format.  The
renderer process can decode the data
Is this true if the browser is going to crack this open and bless
ReferencedFiles?

https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
content/public/common/page_state.h:53: std::string data_;
I had imagined that in addition to the string, there would be a vector of file
paths here, and that the constructor(s) would take a string (for everything
else) + file path vector.  The then browser wouldn't hate to deal with the
opaque string at all, but would have access to the file paths.
That makes it harder to perserve state for session restore, since the the
browser now needs to wrap the opaque string + file vector in a format for
serialization (a double wrapping as it were). Is it worth it from a layering
perspective to keep the opaque parts opaque?

[Tom Sepez, 2013-05-24 17:47:41]

s/hate/have/ in page_state.h comment :).

[darin (slow to review), 2013-05-24 21:02:26]

On 2013/05/24 17:46:48, Tom Sepez wrote:
> Darin, this is a good step to making this clearer. LGTM with comments.
> 
>
https://codereview.chromium.org/14985014/diff/50001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/14985014/diff/50001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_commands.cc:1022:
> active_entry->SetPageState(page_state.RemoveScrollOffset());
> Do we similarly want to strip out the referenced files here?  I don't think
the
> view-source version of the page needs thee to be blessed.

That's a great idea, but I think it is a separate task.  I'd rather keep this
CL focused on just the PageState API addition.  No functional changes yet!


>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> File content/public/common/page_state.cc (right):
> 
>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> content/public/common/page_state.cc:27: PageState PageState::CreateForTesting(
> Can this move to the _unittest file? Maybe as a testing subclass of PageState?

It is needed by more than one unittest file.  What are your concerns with it
being on PageState?  Are you just worried about binary bloat?


>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> File content/public/common/page_state.h (right):
> 
>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> content/public/common/page_state.h:27: // designed as an archival format.  The
> renderer process can decode the data
> Is this true if the browser is going to crack this open and bless
> ReferencedFiles?
> 
>
https://codereview.chromium.org/14985014/diff/50001/content/public/common/pag...
> content/public/common/page_state.h:53: std::string data_;
> I had imagined that in addition to the string, there would be a vector of file
> paths here, and that the constructor(s) would take a string (for everything
> else) + file path vector.  The then browser wouldn't hate to deal with the
> opaque string at all, but would have access to the file paths.
> That makes it harder to perserve state for session restore, since the the
> browser now needs to wrap the opaque string + file vector in a format for
> serialization (a double wrapping as it were). Is it worth it from a layering
> perspective to keep the opaque parts opaque?

Indeed, something like that would be great.  To keep this CL under control, I
decided
to put off such restructuring.  I want to first get the PageState API in place,
and
then I will figure out how best to avoid the Blink API dependency from browser
code.
I have been considering turning PageState into a class that has the following
fields:

  data_
  data_containing_passwords_
  scroll_offset_
  referenced_files_

Then the various mutation functions would be trivial to implement.

One step at a time.  There's a lot of moving pieces with the current CL alone!

[Tom Sepez, 2013-05-24 21:24:22]

> being on PageState?  Are you just worried about binary bloat?
Yep, just wanted to avoid shipping those bits if possible.

LGTM nonetheless.  Thanks.

[darin (slow to review), 2013-05-24 22:01:23]

Committed patchset #13 manually as r202188 (presubmit successful).