Fix: Prerendering was confusing SessionService to not save sessionStorage.

Fix: Prerendering was confusing SessionService to not save sessionStorage.

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

[marja, 2012-09-21 09:59:42]

Hi sky & michaeln, could you review this fix?

For the bug to occur, it's essential for the bug that
PrerenderContents::CreateWebContents uses WebContents::CreateWithSessionStorage.

The test now needs to create a TabContents. I saw a note that TabContents is
going away, and nobody should construct thm, but in order to test this, I need
to somehow create a TabContents, since TabStripModel::ReplaceTabAt wants a
TabContents. I hope this is okay.

[marja, 2012-09-21 10:00:47]

... and the needed OWNERS bits:

sky: pls review everything
michaeln: content/browser/dom_storage

[Avi (use Gerrit), 2012-09-21 12:51:00]

http://codereview.chromium.org/10969012/diff/7011/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

http://codereview.chromium.org/10969012/diff/7011/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore_browsertest.cc:873: TabStripModel*
tab_strip_model = browser()->tab_strip_model();
Can you note that you don't need any of the helpers that a TabContents provides,
but are creating a TabContents only because that's what the tab strip model
requires? I will need to know that when I hit this file in the TabContents work.

[marja, 2012-09-21 13:15:02]

http://codereview.chromium.org/10969012/diff/7011/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

http://codereview.chromium.org/10969012/diff/7011/chrome/browser/sessions/ses...
chrome/browser/sessions/session_restore_browsertest.cc:873: TabStripModel*
tab_strip_model = browser()->tab_strip_model();
On 2012/09/21 12:51:00, Avi wrote:
> Can you note that you don't need any of the helpers that a TabContents
provides,
> but are creating a TabContents only because that's what the tab strip model
> requires? I will need to know that when I hit this file in the TabContents
work.

Done.

[Avi (use Gerrit), 2012-09-21 14:05:54]

LGTM

Thank you; I look forward to working on removing TabContents from this code.

[sky, 2012-09-21 16:26:31]

http://codereview.chromium.org/10969012/diff/12004/chrome/browser/sessions/se...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/10969012/diff/12004/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1721: void
SessionService::OnBrowserAdded(Browser* browser) {
Can we move the implementation of observing BrowserList and TabStripModel into a
separate class that invokes newly exposed methods on SessionService? That way
SessionService doesn't have as many dependencies.

[marja, 2012-09-24 09:23:09]

Thanks for comments!

http://codereview.chromium.org/10969012/diff/12004/chrome/browser/sessions/se...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/10969012/diff/12004/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1721: void
SessionService::OnBrowserAdded(Browser* browser) {
On 2012/09/21 16:26:31, sky wrote:
> Can we move the implementation of observing BrowserList and TabStripModel into
a
> separate class that invokes newly exposed methods on SessionService? That way
> SessionService doesn't have as many dependencies.

Ok, it seems that Browser already has this logic to monitor the tab creation,
and it was also calling SessionService funcs to notify it e.g., about window
closing, so I think notifying SessionService about tab creation / closing fits
in, too. (See the latest patch set.) Wdyt?

[sky, 2012-09-24 14:20:00]

LGTM

[michaeln, 2012-09-24 18:49:37]

lgtm2

http://codereview.chromium.org/10969012/diff/12007/chrome/browser/sessions/se...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/10969012/diff/12007/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1744:
contents->GetController().GetDefaultSessionStorageNamespace();
tiny nit: indent 2 more spaces

http://codereview.chromium.org/10969012/diff/12007/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1747:
SessionTabHelper::FromWebContents(contents);
ditto

[marja, 2012-09-26 07:51:01]

Thanks for reviewing!

http://codereview.chromium.org/10969012/diff/12007/chrome/browser/sessions/se...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/10969012/diff/12007/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1744:
contents->GetController().GetDefaultSessionStorageNamespace();
On 2012/09/24 18:49:37, michaeln wrote:
> tiny nit: indent 2 more spaces

Done.

http://codereview.chromium.org/10969012/diff/12007/chrome/browser/sessions/se...
chrome/browser/sessions/session_service.cc:1747:
SessionTabHelper::FromWebContents(contents);
On 2012/09/24 18:49:37, michaeln wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-09-26 07:51:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/10969012/18001

[commit-bot: I haz the power, 2012-09-26 10:04:32]

Change committed as 158778