Fix the crash of DisplayErrorDialog.

Fix the crash of DisplayErrorDialog.

The reported crash happens due to the lack of context. Also I noticed
that it'll crash if the dialog happens to appear in single-display
environment. Added the fix for this too.

Also added test cases to ensure the code doesn't crash.

BUG=174462
TEST=ash_unittests passes


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

[James Cook, 2013-02-05 23:59:52]

One question about widget cleanup.

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog_unittest.cc:21:
EXPECT_TRUE(dialog->GetWidget()->IsVisible());
Does this need to manually close the widget at the end of the test? 
dialog->GetWidget()->CloseNow()?

I'm not sure if AshTestBase cleans these up automatically.

[oshima, 2013-02-06 00:17:28]

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
File ash/display/display_error_dialog.cc (right):

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.cc:105: DisplayErrorDialog*
DisplayErrorDialog::GetInstanceForTest() {
DCHECK(g_instance);

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog_unittest.cc:21:
EXPECT_TRUE(dialog->GetWidget()->IsVisible());
On 2013/02/05 23:59:52, James Cook (Chromium) wrote:
> Does this need to manually close the widget at the end of the test? 
> dialog->GetWidget()->CloseNow()?
> 
> I'm not sure if AshTestBase cleans these up automatically.

If the widget's ownership is NATIVE_WIDGET_OWNS_WIDGET (which is normal),
it will. All windows are deleted upon shutdown (on both test and non test
environment), which will delete all widgets with this ownership.

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog_unittest.cc:30: EXPECT_EQ(dialog,
DisplayErrorDialog::GetInstanceForTest());
can you test if the dialog is on the right root window?

[Jun Mukai, 2013-02-06 00:37:18]

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
File ash/display/display_error_dialog.cc (right):

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.cc:105: DisplayErrorDialog*
DisplayErrorDialog::GetInstanceForTest() {
On 2013/02/06 00:17:28, oshima wrote:
> DCHECK(g_instance);

g_instance can be NULL in case the dialog is gone (like DisplayDisconnected test
case).

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12209026/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog_unittest.cc:30: EXPECT_EQ(dialog,
DisplayErrorDialog::GetInstanceForTest());
On 2013/02/06 00:17:28, oshima wrote:
> can you test if the dialog is on the right root window?

Done.

[oshima, 2013-02-06 00:41:08]

lgtm with nit

https://codereview.chromium.org/12209026/diff/1005/ash/display/display_error_...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12209026/diff/1005/ash/display/display_error_...
ash/display/display_error_dialog_unittest.cc:22:
ScreenAsh::GetSecondaryDisplay().id());
you can use ash::Shell::GetAllRootWindows()[1]

[Jun Mukai, 2013-02-06 00:43:59]

https://codereview.chromium.org/12209026/diff/1005/ash/display/display_error_...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12209026/diff/1005/ash/display/display_error_...
ash/display/display_error_dialog_unittest.cc:22:
ScreenAsh::GetSecondaryDisplay().id());
On 2013/02/06 00:41:08, oshima wrote:
> you can use ash::Shell::GetAllRootWindows()[1] 
> 

Done.

[Jun Mukai, 2013-02-06 00:54:16]

jamescook, please give me an approval for ash/ash.gyp

[James Cook, 2013-02-06 00:59:45]

lgtm

[commit-bot: I haz the power, 2013-02-06 01:15:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/12209026/2003

[commit-bot: I haz the power, 2013-02-06 03:21:50]

Change committed as 180887