Don't allow the task manager to share a process with anything else.

Don't allow the task manager to share a process with anything else.

BUG=103605
TEST=Open more tabs than process limit, then preferences, then task manager.  Killing preferences shouldn't kill task manager.

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

[awong, 2011-11-15 20:23:43]

LGTM w/ nit

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:401: // Don't allow the Task
Manager to share a process with anything else.
Can we say "anything else, including other Task Managers"?

[Charlie Reis, 2011-11-15 20:28:38]

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:401: // Don't allow the Task
Manager to share a process with anything else.
On 2011/11/15 20:23:43, awong wrote:
> Can we say "anything else, including other Task Managers"?

If we actually want to enforce that, we would also need to change how the
TaskManager's SiteInstance is obtained.  As it turns out, opening a second Task
Manager in a tab will sometimes share with the first one because they use the
same SiteInstance.  I don't think that's a big deal.

[awong, 2011-11-15 20:31:38]

LGTM++

On 2011/11/15 20:28:38, creis wrote:
>
http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
> chrome/browser/chrome_content_browser_client.cc:401: // Don't allow the Task
> Manager to share a process with anything else.
> On 2011/11/15 20:23:43, awong wrote:
> > Can we say "anything else, including other Task Managers"?
> 
> If we actually want to enforce that, we would also need to change how the
> TaskManager's SiteInstance is obtained.  As it turns out, opening a second
Task
> Manager in a tab will sometimes share with the first one because they use the
> same SiteInstance.  I don't think that's a big deal.

Okay.

[Aaron Boodman, 2011-11-15 21:08:37]

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:406: if (site_url ==
GURL(chrome::kChromeUITaskManagerURL))
Nit: This kind of thing always makes me nervous. If anyone ever changes the task
manager to accept querystring arguments, or fragments, it will stop working.

Consider instead doing something like:

if (site_url.GetOrigin() == GURL(...).GetOrigin())

... but I would understand if you said that was overkill.

[Charlie Reis, 2011-11-15 21:29:48]

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/8520019/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:406: if (site_url ==
GURL(chrome::kChromeUITaskManagerURL))
On 2011/11/15 21:08:37, Aaron Boodman wrote:
> Nit: This kind of thing always makes me nervous. If anyone ever changes the
task
> manager to accept querystring arguments, or fragments, it will stop working.
> 
> Consider instead doing something like:
> 
> if (site_url.GetOrigin() == GURL(...).GetOrigin())
> 
> ... but I would understand if you said that was overkill.

Yeah, I agree that would be more robust.  Fixed.

[Charlie Reis, 2011-11-16 17:46:43]

Ping: Aaron, can you take another look?

[Aaron Boodman, 2011-11-16 17:55:05]

lgtm