OOPIF: Support session restore by combining/splitting frame PageStates.

OOPIF: Support session restore by combining/splitting frame PageStates.

In OOPIF-enabled modes, each FrameNavigationEntry has a per-frame
PageState object.  The states for each frame in the tree need to be
combined into a single PageState for GetPageState, and a single
PageState needs to be split into individual FrameNavigationEntries for
SetPageState.

Because GetPageState is const and returns a const ref, we need to
cache the combined PageState each time it could change.

BUG=545219
TEST=Close and restore a tab or session with OOPIFs.

Committed: https://crrev.com/7115a704adb9c7109f61d34d6b50937dd8c60bc6
Cr-Commit-Position: refs/heads/master@{#363025}

[Charlie Reis, 2015-12-02 18:14:17]

creis@chromium.org changed reviewers:
+ avi@chromium.org

[Charlie Reis, 2015-12-02 18:14:18]

Avi, can I have you start to take a look?  There are still 3 session restore
browser_tests I need to fix (likely because they're passing a bogus PageState)
and I'd like to add one more content_browsertest, but otherwise it's working as
expected.

The basic premise here is to (1) build up the full PageState from each FNE's
single-frame PageState to support NE::GetPageState, and (2) break apart a full
PageState into single-frame PageStates for each FNE to support NE::SetPageState.

I tried going a step further and using ExplodedFrameState on
FrameNavigationEntry (rather than a single-frame PageState), which avoids some
of the redundant encode/decode work we're doing, but that got big and tricky in
a hurry.  (See https://codereview.chromium.org/1415603018/.)  I still think
that's a promising direction to head with a few modifications, but it's riskier
than we need for getting session restore working in the short term.  This
simpler CL is just a step in that direction.

[Avi (use Gerrit), 2015-12-02 19:17:43]

I'm almost sold, but I'd much rather change the API than work around it.

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:28: static int
GetUniqueIDInConstructor() {
Now that this is in an anonymous namespace, you can drop the "static" here.

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:206: // be able to return a
const ref from GetPageState.
I would much rather change the API. PageState isn't that big, and if we can
return a copy by value rather than const ref, then we can avoid the error-prone
scheme of having to call UpdatePageState after updating the state of any given
frame and simplify this a lot.

[Charlie Reis, 2015-12-02 19:48:18]

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:206: // be able to return a
const ref from GetPageState.
On 2015/12/02 19:17:43, Avi wrote:
> I would much rather change the API. PageState isn't that big, and if we can
> return a copy by value rather than const ref, then we can avoid the
error-prone
> scheme of having to call UpdatePageState after updating the state of any given
> frame and simplify this a lot.

I can give it another try in a preliminary CL, though I remember it quickly got
unwieldy the last time I attempted.

Needing to call UpdatePageState alongside FNE::set_page_state is definitely
unfortunate, but I did kind of like having a separate NavEntry::page_state_ from
FNE::page_state_, since that frees us up to move FNE to a FrameState object
instead (while still supporting default Chrome).

[Charlie Reis, 2015-12-02 20:41:49]

Patchset #3 (id:40001) has been deleted

[Charlie Reis, 2015-12-02 20:43:03]

Patchset #3 (id:60001) has been deleted

[Charlie Reis, 2015-12-02 20:44:14]

Ah, that is nicer-- thanks!  I'll try to get the remaining tests fixed/added
this afternoon.

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:28: static int
GetUniqueIDInConstructor() {
On 2015/12/02 19:17:43, Avi wrote:
> Now that this is in an anonymous namespace, you can drop the "static" here.

Done.

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:206: // be able to return a
const ref from GetPageState.
On 2015/12/02 19:48:18, Charlie Reis wrote:
> On 2015/12/02 19:17:43, Avi wrote:
> > I would much rather change the API. PageState isn't that big, and if we can
> > return a copy by value rather than const ref, then we can avoid the
> error-prone
> > scheme of having to call UpdatePageState after updating the state of any
given
> > frame and simplify this a lot.
> 
> I can give it another try in a preliminary CL, though I remember it quickly
got
> unwieldy the last time I attempted.
> 
> Needing to call UpdatePageState alongside FNE::set_page_state is definitely
> unfortunate, but I did kind of like having a separate NavEntry::page_state_
from
> FNE::page_state_, since that frees us up to move FNE to a FrameState object
> instead (while still supporting default Chrome).

Ah, I was mistaken!  The last time, I tried to remove the const first, hoping to
preserve the const ref (by keeping a cached copy).  Removing the const was the
viral change.

Dropping the const ref is much easier (and leads to a simpler CL).  We can do a
separate PageState for NavigationEntryImpl later when we start to move towards
FrameState.  Thanks for the suggestion!

[Avi (use Gerrit), 2015-12-02 21:15:45]

lgtm

So much nicer.

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1496483002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:206: // be able to return a
const ref from GetPageState.
On 2015/12/02 20:44:14, Charlie Reis wrote:
> On 2015/12/02 19:48:18, Charlie Reis wrote:
> > On 2015/12/02 19:17:43, Avi wrote:
> > > I would much rather change the API. PageState isn't that big, and if we
can
> > > return a copy by value rather than const ref, then we can avoid the
> > error-prone
> > > scheme of having to call UpdatePageState after updating the state of any
> given
> > > frame and simplify this a lot.
> > 
> > I can give it another try in a preliminary CL, though I remember it quickly
> got
> > unwieldy the last time I attempted.
> > 
> > Needing to call UpdatePageState alongside FNE::set_page_state is definitely
> > unfortunate, but I did kind of like having a separate NavEntry::page_state_
> from
> > FNE::page_state_, since that frees us up to move FNE to a FrameState object
> > instead (while still supporting default Chrome).
> 
> Ah, I was mistaken!  The last time, I tried to remove the const first, hoping
to
> preserve the const ref (by keeping a cached copy).  Removing the const was the
> viral change.
> 
> Dropping the const ref is much easier (and leads to a simpler CL).  We can do
a
> separate PageState for NavigationEntryImpl later when we start to move towards
> FrameState.  Thanks for the suggestion!

You're welcome.

Returning a const object by value is weird anyway.

[Charlie Reis, 2015-12-03 05:52:59]

Patchset #6 (id:140001) has been deleted

[Charlie Reis, 2015-12-03 05:53:07]

Patchset #5 (id:120001) has been deleted

[Charlie Reis, 2015-12-03 06:38:25]

Avi, can you review the diff between patch sets 3 and 5?  I added a new test and
fixed the remaining failing tests.  Feel free to CQ if you're still happy.

(The existing tests were failing because they use bogus PageStates.  We can
handle those the same way as before by just storing them on the main frame FNE
anytime we notice that there aren't children involved.)

[Avi (use Gerrit), 2015-12-03 16:08:40]

On 2015/12/03 06:38:25, Charlie Reis wrote:
> Avi, can you review the diff between patch sets 3 and 5?  I added a new test
and
> fixed the remaining failing tests.  Feel free to CQ if you're still happy.
> 
> (The existing tests were failing because they use bogus PageStates.  We can
> handle those the same way as before by just storing them on the main frame FNE
> anytime we notice that there aren't children involved.)

I'm only vaguely happy about this. Your code makes sure to round-trip bogus
data; can we fix the tests to not use that?

[Charlie Reis, 2015-12-03 17:05:13]

On 2015/12/03 16:08:40, Avi wrote:
> On 2015/12/03 06:38:25, Charlie Reis wrote:
> > Avi, can you review the diff between patch sets 3 and 5?  I added a new test
> and
> > fixed the remaining failing tests.  Feel free to CQ if you're still happy.
> > 
> > (The existing tests were failing because they use bogus PageStates.  We can
> > handle those the same way as before by just storing them on the main frame
FNE
> > anytime we notice that there aren't children involved.)
> 
> I'm only vaguely happy about this. Your code makes sure to round-trip bogus
> data; can we fix the tests to not use that?

I think we'll want to do that as part of the larger FrameState refactor.  I'd
like this CL to be more about making OOPIF modes consume and produce the same
PageState output as default Chrome-- making NavEntry stricter about what it
accepts will make this a bit riskier.  (This current change also has the
advantage that it supports empty PageStates (i.e., !IsValid), which is used in
some cases today.)

I definitely want to update the tests in another step, though, since it's weird
to use bogus data.  Sound reasonable?

[Avi (use Gerrit), 2015-12-03 18:46:00]

On 2015/12/03 17:05:13, Charlie Reis wrote:
> On 2015/12/03 16:08:40, Avi wrote:
> > On 2015/12/03 06:38:25, Charlie Reis wrote:
> > > Avi, can you review the diff between patch sets 3 and 5?  I added a new
test
> > and
> > > fixed the remaining failing tests.  Feel free to CQ if you're still happy.
> > > 
> > > (The existing tests were failing because they use bogus PageStates.  We
can
> > > handle those the same way as before by just storing them on the main frame
> FNE
> > > anytime we notice that there aren't children involved.)
> > 
> > I'm only vaguely happy about this. Your code makes sure to round-trip bogus
> > data; can we fix the tests to not use that?
> 
> I think we'll want to do that as part of the larger FrameState refactor.  I'd
> like this CL to be more about making OOPIF modes consume and produce the same
> PageState output as default Chrome-- making NavEntry stricter about what it
> accepts will make this a bit riskier.  (This current change also has the
> advantage that it supports empty PageStates (i.e., !IsValid), which is used in
> some cases today.)
> 
> I definitely want to update the tests in another step, though, since it's
weird
> to use bogus data.  Sound reasonable?

OK, that sounds like a good plan. LGTM

[Charlie Reis, 2015-12-03 18:48:55]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-12-03 18:50:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496483002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496483002/160001

[commit-bot: I haz the power, 2015-12-03 18:56:04]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-12-03 18:57:11]

Description was changed from

==========
OOPIF: Support session restore by combining/splitting frame PageStates.

In OOPIF-enabled modes, each FrameNavigationEntry has a per-frame
PageState object.  The states for each frame in the tree need to be
combined into a single PageState for GetPageState, and a single
PageState needs to be split into individual FrameNavigationEntries for
SetPageState.

Because GetPageState is const and returns a const ref, we need to
cache the combined PageState each time it could change.

BUG=545219
TEST=Close and restore a tab or session with OOPIFs.
==========

to

==========
OOPIF: Support session restore by combining/splitting frame PageStates.

In OOPIF-enabled modes, each FrameNavigationEntry has a per-frame
PageState object.  The states for each frame in the tree need to be
combined into a single PageState for GetPageState, and a single
PageState needs to be split into individual FrameNavigationEntries for
SetPageState.

Because GetPageState is const and returns a const ref, we need to
cache the combined PageState each time it could change.

BUG=545219
TEST=Close and restore a tab or session with OOPIFs.

Committed: https://crrev.com/7115a704adb9c7109f61d34d6b50937dd8c60bc6
Cr-Commit-Position: refs/heads/master@{#363025}
==========

[commit-bot: I haz the power, 2015-12-03 18:57:12]

Patchset 5 (id:??) landed as
https://crrev.com/7115a704adb9c7109f61d34d6b50937dd8c60bc6
Cr-Commit-Position: refs/heads/master@{#363025}