View-source URLs should use a separate SiteInstance in all cases.

View-source URLs should use a separate SiteInstance in all cases.

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

[nasko, 2012-08-16 22:13:57]

Charlie,
Can you review the fix for the SiteInstance being reused when navigating to
view-source from about:blank?
Thanks,
Nasko

[Charlie Reis, 2012-08-16 22:28:09]

Thanks, seems about right.

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:998: // we
create a new SiteInstance.
This comment isn't quite right.  This should only be true when navigating to
view-source.  Otherwise we should re-use the about:blank page's SiteInstance.

nit: with a

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1007: //
RenderViewHost.
This sort of confuses the issue.  By putting any navigation here, we could be
affecting what happens in the view-source case below.  (Imagine if we only
re-used the about:blank SiteInstance if no other navigations had taken place. 
Then this test would miss the bug.)

I don't think we need this block.

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1014: //
Now navigate to the view-source URL and ensure we got different
nit: got a

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/web_co...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/web_co...
content/browser/web_contents/render_view_host_manager.cc:506: return
SiteInstance::CreateForURL(browser_context, dest_url);
Please move this check between the HasWrongProcessForURL check and the session
restore check.  (It's more related to the HasWrongProcessForURL case.)

Also, please point to http://crbug.com/123007 in the comment.

[nasko, 2012-08-16 22:59:23]

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:998: // we
create a new SiteInstance.
On 2012/08/16 22:28:09, creis wrote:
> This comment isn't quite right.  This should only be true when navigating to
> view-source.  Otherwise we should re-use the about:blank page's SiteInstance.
> 
> nit: with a

Done.

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1007: //
RenderViewHost.
On 2012/08/16 22:28:09, creis wrote:
> This sort of confuses the issue.  By putting any navigation here, we could be
> affecting what happens in the view-source case below.  (Imagine if we only
> re-used the about:blank SiteInstance if no other navigations had taken place. 
> Then this test would miss the bug.)
> 
> I don't think we need this block.

Done.

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/render...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1014: //
Now navigate to the view-source URL and ensure we got different
On 2012/08/16 22:28:09, creis wrote:
> nit: got a

Done.

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/web_co...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10825402/diff/1/content/browser/web_co...
content/browser/web_contents/render_view_host_manager.cc:506: return
SiteInstance::CreateForURL(browser_context, dest_url);
On 2012/08/16 22:28:09, creis wrote:
> Please move this check between the HasWrongProcessForURL check and the session
> restore check.  (It's more related to the HasWrongProcessForURL case.)
> 
> Also, please point to http://crbug.com/123007 in the comment.

Done.

[Charlie Reis, 2012-08-16 23:25:07]

LGTM

[commit-bot: I haz the power, 2012-08-17 15:36:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10825402/9001

[commit-bot: I haz the power, 2012-08-17 18:47:59]

Change committed as 152126