Fixing problem with incomplete browser windows showing up after task manager was used

Fixing problem with a tabless browser windows showing up after a task manager was used

I traced it down to the creation of the task manager which grabs an existing (or new) browser. It requires the browser only for two things: determining the system it is running in and for giving the created web dialog a context (a window which is used (by Aura only) to position the window within the hierarchy.

As I can see it there are no other dependencies to the browser and it can be disposed of after the task manager was created (and there are no tabs in the tabbed browser).

BUG=176670
TEST=visual


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

[Mr4D (OOO till 08-26), 2013-03-19 21:55:41]

Please have a look!

[sky, 2013-03-19 23:27:26]

I get why you want to do this, but there is no point in creating a browser if
we're going to throw it out. The code to show the TaskManager should be changed
to deal with a NULL browser.

[Mr4D (OOO till 08-26), 2013-03-20 22:49:48]

Okay, ripped out the intermediate call chain, made it possible to accept NULL as
a browser and moved the task manager into browser dialogs.

Please have another look!

[sky, 2013-03-20 23:52:08]

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_browsertest_util.cc:98: void
TaskManagerBrowserTestUtil::ShowTaskManagerAndWaitForReady(
Nuke this since it's just a call through to ShowTaskManager.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:168:
chrome::OpenTaskManager(NULL, false);
NULL is ok here, but if the active window is a browser you should use it, right?

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:539: browser ?
browser->host_desktop_type() : chrome::HOST_DESKTOP_TYPE_ASH;
Document why browser == NULL is treated as ASH.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:558:
DialogDelegateView::CreateDialogWidget(instance_, window, NULL);
Can you verify this does the right thing if window is NULL. I have a feeling it
hits a DCHECK, but I could be wrong.

[Mr4D (OOO till 08-26), 2013-03-21 00:20:10]

Addressed (and fixed build problems).

Please have another look!

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_browsertest_util.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_browsertest_util.cc:98: void
TaskManagerBrowserTestUtil::ShowTaskManagerAndWaitForReady(
On 2013/03/20 23:52:08, sky wrote:
> Nuke this since it's just a call through to ShowTaskManager.

Done.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:168:
chrome::OpenTaskManager(NULL, false);
Well - I was under the impression that you wanted it this way, but - done.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:539: browser ?
browser->host_desktop_type() : chrome::HOST_DESKTOP_TYPE_ASH;
On 2013/03/20 23:52:08, sky wrote:
> Document why browser == NULL is treated as ASH.

Done.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:558:
DialogDelegateView::CreateDialogWidget(instance_, window, NULL);
I did already before checking in and it did not.

[sky, 2013-03-21 00:25:55]

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/ash/chro...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/ash/chro...
chrome/browser/ui/ash/chrome_shell_delegate.cc:168:
chrome::OpenTaskManager(NULL, false);
On 2013/03/21 00:20:10, Mr4D wrote:
> Well - I was under the impression that you wanted it this way, but - done.

What I said is that if the active window is a browser you should pass that in,
otherwise NULL.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:558:
DialogDelegateView::CreateDialogWidget(instance_, window, NULL);
On 2013/03/21 00:20:10, Mr4D wrote:
> I did already before checking in and it did not. 

So then don't you need to deal with it being null here? By null here I mean 559
returning null.

[Mr4D (OOO till 08-26), 2013-03-21 00:29:30]

Sorry for the misunderstanding.

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/task_manager_view.cc (right):

https://codereview.chromium.org/12712018/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/task_manager_view.cc:558:
DialogDelegateView::CreateDialogWidget(instance_, window, NULL);
You misunderstood - It did not cause any problems (hit DCHECK or anything). I
was intently passing in (instance_, NULL, NULL) and everything worked.

[sky, 2013-03-21 00:48:07]

LGTM

https://codereview.chromium.org/12712018/diff/11001/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12712018/diff/11001/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate.cc:171:
chrome::HOST_DESKTOP_TYPE_ASH);
This isn't quite what I think should happen either. But, it doesn't really
matter here so go back to NULL.

[Mr4D (OOO till 08-26), 2013-03-21 00:53:36]

Thanks!

https://codereview.chromium.org/12712018/diff/11001/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12712018/diff/11001/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate.cc:171:
chrome::HOST_DESKTOP_TYPE_ASH);
Okay - in that case I am not sure what you wanted. But Okay - passing NULL
again. Done!

[commit-bot: I haz the power, 2013-03-21 00:54:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/12712018/24001

[commit-bot: I haz the power, 2013-03-21 12:24:36]

Change committed as 189579

[sky, 2013-03-21 15:53:20]

https://codereview.chromium.org/12712018/diff/11001/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12712018/diff/11001/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate.cc:171:
chrome::HOST_DESKTOP_TYPE_ASH);
On 2013/03/21 00:53:36, Mr4D wrote:
> Okay - in that case I am not sure what you wanted. But Okay - passing NULL
> again. Done!

Here is what I meant:

aura::Window* active = ash::wm::GetActiveWindow();
Browser* active_browser = NULL;
while (active && !active_browser) {
  active_browser = GetBrowserViewForNativeWindow(active);
  active = active->parent();
}
chrome::OpenTaskManager(browser, false);

But TaskManagerView::Show doesn't really need a browser, it needs a context for
where to create the window and the host desktop type. So, providing a views
specific variant that takes this would be better.
ChromeShellDelegate::ShowTaskManager would become:

OpenViewsTaskManager(ash::Shell::GetActiveRootWindow(), HOST_DESKTOP_TYPE_ASH,
false);

And view's implementation of OpenTaskManager would call through to
OpenViewsTaskManager:

OpenViewsTaskManager(browser->window()->GetNativeWindow(),
browser->host_desktop_type(), show_background);

This is better in that it means it's clear the TaskManager code only needs a
gfx::NativeWindow and HostDesktopType, not a Browser which could have other
random dependencies.

But anyway, passing in NULL is fine here too.