Copy max_restored_page_id_ when needed.

Copy max_restored_page_id_ when needed.

BUG=366597
R=nasko@chromium.org,jcivelli@chromium.org

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

[nasko, 2014-04-24 15:55:17]

creis@ is a better reviewer for this area and he is working on future changes to
session history.

[Charlie Reis, 2014-04-24 18:02:02]

Thanks for catching this!  I appreciate the bug with repro steps and the test. 
It's a subtle area, so those help a lot.

LGTM with two nits and a question.

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1307:
max_restored_page_id_ = source->max_restored_page_id_;
nit: Let's put this down near the CopyMaxPageIDsFrom call a few lines below,
since they're semantically related.

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl_unittest.cc:4177:
TEST_F(NavigationControllerTest, CopyRestoredStateAndNavigate) {
nit: Can you move this up near the other block of CopyStateFrom tests, around
line 3750?

https://codereview.chromium.org/257463004/diff/1/content/test/test_web_conten...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/test/test_web_conten...
content/test/test_web_contents.cc:103:
UpdateMaxPageIDIfNecessary(render_view_host);
Looks like we override this method to cut out the view-related parts.  Would it
make sense to include the GetMaxPageIDForSiteInstance call as well, and pass the
resulting max_page_id to CreateRenderView below?

[Alexander Semashko, 2014-04-24 21:40:36]

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1307:
max_restored_page_id_ = source->max_restored_page_id_;
On 2014/04/24 18:02:02, Charlie Reis wrote:
> nit: Let's put this down near the CopyMaxPageIDsFrom call a few lines below,
> since they're semantically related.

Done.

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl_unittest.cc:4177:
TEST_F(NavigationControllerTest, CopyRestoredStateAndNavigate) {
On 2014/04/24 18:02:02, Charlie Reis wrote:
> nit: Can you move this up near the other block of CopyStateFrom tests, around
> line 3750?

Done.

https://codereview.chromium.org/257463004/diff/1/content/test/test_web_conten...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/test/test_web_conten...
content/test/test_web_contents.cc:103:
UpdateMaxPageIDIfNecessary(render_view_host);
On 2014/04/24 18:02:02, Charlie Reis wrote:
> Looks like we override this method to cut out the view-related parts.  Would
it
> make sense to include the GetMaxPageIDForSiteInstance call as well, and pass
the
> resulting max_page_id to CreateRenderView below?

Currently TestRenderViewHost::CreateRenderView does not use max_page_id, so
this change alone won't have any effect. It must be accompanied by changes to
other test classes, but I don't know what exactly should be done here and what
are the possible uses of this.

[Charlie Reis, 2014-04-24 21:48:04]

LGTM

https://codereview.chromium.org/257463004/diff/1/content/test/test_web_conten...
File content/test/test_web_contents.cc (right):

https://codereview.chromium.org/257463004/diff/1/content/test/test_web_conten...
content/test/test_web_contents.cc:103:
UpdateMaxPageIDIfNecessary(render_view_host);
On 2014/04/24 21:40:37, ahest wrote:
> On 2014/04/24 18:02:02, Charlie Reis wrote:
> > Looks like we override this method to cut out the view-related parts.  Would
> it
> > make sense to include the GetMaxPageIDForSiteInstance call as well, and pass
> the
> > resulting max_page_id to CreateRenderView below?
> 
> Currently TestRenderViewHost::CreateRenderView does not use max_page_id, so
> this change alone won't have any effect. It must be accompanied by changes to
> other test classes, but I don't know what exactly should be done here and what
> are the possible uses of this.

Ok, let's leave it as is until a test needs it.

[Alexander Semashko, 2014-04-25 15:27:16]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2014-04-25 22:17:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahest@yandex-team.ru/257463004/20001

[Alexander Semashko, 2014-04-26 00:03:13]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2014-04-26 00:05:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahest@yandex-team.ru/257463004/30001

[commit-bot: I haz the power, 2014-04-26 10:59:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 10:59:40]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Alexander Semashko, 2014-04-28 12:33:56]

The CQ bit was checked by ahest@yandex-team.ru

[commit-bot: I haz the power, 2014-04-28 12:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahest@yandex-team.ru/257463004/30001

[commit-bot: I haz the power, 2014-04-28 14:01:47]

Change committed as 266552