Create the correct SiteInstance when restoring tabs that belong to apps.

Create the correct SiteInstance when restoring tabs that belong to apps.

BUG=89371
TEST=see bug


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

[Mihai Parparita -not on Chrome, 2011-07-19 21:23:48]

Not sure why the cl upload didn't have the description.

Charlie: could you review this?

John and Scott: do you have an opinion about where GetSiteInstance
belongs (it can't be next to TabContents or SiteInstance since it
depends on chrome/ stuff)?

Mihai

On Tue, Jul 19, 2011 at 5:21 PM,  <mihaip@chromium.org> wrote:
> http://codereview.chromium.org/7448012/
>

[Charlie Reis, 2011-07-19 21:38:28]

Looking good so far-- thanks.

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:112: RenderProcessHost* process
= render_view_host->process();
Is this change still necessary?

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_util.h (right):

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_util.h:23: // contents.
Can you keep something like the "or the SiteInstance for |source_contents| if it
represents the same website as |url|.  Returns NULL otherwise" part of the
comment?  I think it more clearly described the behavior of the method.

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:96: #include
"chrome/browser/ui/webui/chrome_web_ui_factory.h"
Is this needed?

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1157: GURL restore_url =
navigations.at(selected_navigation).virtual_url();
Sanity check: do we know this index is safe?

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1206: GURL restore_url =
navigations.at(selected_navigation).virtual_url();
Ditto.

[Mihai Parparita -not on Chrome, 2011-07-20 17:42:52]

On Tue, Jul 19, 2011 at 5:38 PM,  <creis@chromium.org> wrote:
http://codereview.chromium.org/7448012/diff/5001/chrome/browser/chrome_conten...
> chrome/browser/chrome_content_browser_client.cc:112: RenderProcessHost*
> process = render_view_host->process();
> Is this change still necessary?

Not necessary, but I happened to notice that "process" wasn't needed
until after a bunch of early returns, so I thought I would clean that
up while I was at it.

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/tab_util.h (right):
>
>
http://codereview.chromium.org/7448012/diff/5001/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_util.h:23: // contents.
> Can you keep something like the "or the SiteInstance for
> |source_contents| if it represents the same website as |url|.  Returns
> NULL otherwise" part of the comment?  I think it more clearly described
> the behavior of the method.

Done.

> http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
>
>
http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc...
> chrome/browser/ui/browser.cc:96: #include
> "chrome/browser/ui/webui/chrome_web_ui_factory.h"
> Is this needed?

No, that should have ended up in tab_util.cc. Removed.

http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc...
> chrome/browser/ui/browser.cc:1157: GURL restore_url =
> navigations.at(selected_navigation).virtual_url();
> Sanity check: do we know this index is safe?

Yes, RestoreTab in session_restore.cc does bounds checking before
passing off the parameters to AddRestoredTab.

>
http://codereview.chromium.org/7448012/diff/5001/chrome/browser/ui/browser.cc...
> chrome/browser/ui/browser.cc:1206: GURL restore_url =
> navigations.at(selected_navigation).virtual_url();
> Ditto.

Similarly, TabRestoreService::PopulateTab does bounds checking before
persisting the tab data that that is later passed off to
ReplaceRestoredTab.

Mihai

[Charlie Reis, 2011-07-20 20:41:22]

LGTM (with one style nit)

> Not necessary, but I happened to notice that "process" wasn't needed
> until after a bunch of early returns, so I thought I would clean that
> up while I was at it.

Sounds good.

> Yes, RestoreTab in session_restore.cc does bounds checking before
> passing off the parameters to AddRestoredTab.
> 
> Similarly, TabRestoreService::PopulateTab does bounds checking before
> persisting the tab data that that is later passed off to
> ReplaceRestoredTab.


Great, thanks for checking.

http://codereview.chromium.org/7448012/diff/10001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_util.h (right):

http://codereview.chromium.org/7448012/diff/10001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_util.h:25: TabContents* source_contents,
Profile* profile, const GURL& url);
I think the arguments are supposed to each be on their own line if they're not
all on the first line, right?

[Mihai Parparita -not on Chrome, 2011-07-20 20:50:22]

On Wed, Jul 20, 2011 at 4:41 PM,  <creis@chromium.org> wrote:
> I think the arguments are supposed to each be on their own line if
> they're not all on the first line, right?

Yep. Fixed.

Mihai

[sky, 2011-07-20 21:06:57]

LGTM

[commit-bot: I haz the power, 2011-07-21 04:48:06]

Change committed as 93337