Adds DisplayErrorDialog to show the error message of the failure of mirroring to the users.

Adds DisplayErrorDialog to show the error message of the failure of mirroring to the users.

This CL depends on http://codereview.chromium.org/10989084/.

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

[oshima, 2012-10-01 21:45:20]

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
File ash/display/display_error_dialog.cc (right):

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:34: DisplayController* display_controller =
what happens when I tried to mirror while this is open?

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:36: gfx::Display* secondary_display =
display_controller->GetSecondaryDisplay();
SceenAsh::GetSecondaryDisplay()

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:45: Shell::GetContainer(root_window,
kShellWindowId_AlwaysOnTopContainer),
can you use Widget::InitParams::keep_on_top?

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
File ash/display/display_error_dialog.h (right):

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.h:9: #include "ui/gfx/size.h"
you can use forward decl unless you embed it on the class.

[Jun Mukai, 2012-10-01 23:59:34]

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
File ash/display/display_error_dialog.cc (right):

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:34: DisplayController* display_controller =
On 2012/10/01 21:45:20, oshima wrote:
> what happens when I tried to mirror while this is open?

Good point... Added a global variable to make this singleton.

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:36: gfx::Display* secondary_display =
display_controller->GetSecondaryDisplay();
On 2012/10/01 21:45:20, oshima wrote:
> SceenAsh::GetSecondaryDisplay()

Done.

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:45: Shell::GetContainer(root_window,
kShellWindowId_AlwaysOnTopContainer),
On 2012/10/01 21:45:20, oshima wrote:
> can you use Widget::InitParams::keep_on_top?

Done.

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
File ash/display/display_error_dialog.h (right):

http://codereview.chromium.org/10986087/diff/1001/ash/display/display_error_d...
ash/display/display_error_dialog.h:9: #include "ui/gfx/size.h"
On 2012/10/01 21:45:20, oshima wrote:
> you can use forward decl unless you embed it on the class.

Done.

[oshima, 2012-10-02 18:18:42]

http://codereview.chromium.org/10986087/diff/8001/ash/display/display_error_d...
File ash/display/display_error_dialog.cc (right):

http://codereview.chromium.org/10986087/diff/8001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:37: return;
We probably should close this when external display is disconnected. I guess
it's moved to the primary now.
(please confirm with alex/glen)

http://codereview.chromium.org/10986087/diff/8001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:48: params.bounds =
gfx::Rect(secondary_display.bounds());
do you need gfx::Rect ?

[Jun Mukai, 2012-10-02 21:43:04]

http://codereview.chromium.org/10986087/diff/8001/ash/display/display_error_d...
File ash/display/display_error_dialog.cc (right):

http://codereview.chromium.org/10986087/diff/8001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:37: return;
On 2012/10/02 18:18:42, oshima wrote:
> We probably should close this when external display is disconnected. I guess
> it's moved to the primary now.
> (please confirm with alex/glen)

Changed the code to close the dialog, but I'm asking on crbug too just in case. 
Thanks.

http://codereview.chromium.org/10986087/diff/8001/ash/display/display_error_d...
ash/display/display_error_dialog.cc:48: params.bounds =
gfx::Rect(secondary_display.bounds());
On 2012/10/02 18:18:42, oshima wrote:
> do you need gfx::Rect ?
removed.  Thanks.

[Jun Mukai, 2012-10-04 18:20:08]

I changed the code a bit due to an offline discussion.  Now the error dialog
will be closed to any display configuration change.

I made this change because of the purpose of this dialog.  Certainly this is
currently only for the error message of mirroring, it would be used to show some
other errors around display configurations, thus it'd be nicer to close itself
upon the display configuration change (and reappear if it fails again).

oshima, could you take another look?

[oshima, 2012-10-04 20:28:14]

http://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_d...
File ash/display/display_error_dialog.cc (right):

http://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_d...
ash/display/display_error_dialog.cc:65: container->AddChildView(label_);
I think you should just add label_ to this.
GetContentsView is an API that widgets uses.

http://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_d...
ash/display/display_error_dialog.cc:110: void
DisplayErrorDialog::OnDisplayRemoved(const gfx::Display& old_display) {
I believe you don't need these three (OnDisplayBoundsChanged/Added/Removed)
because OnDisplayConfigurationChanging will always called before these methods.
Didn't it work without these?

[Jun Mukai, 2012-10-04 20:39:49]

https://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_...
File ash/display/display_error_dialog.cc (right):

https://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_...
ash/display/display_error_dialog.cc:65: container->AddChildView(label_);
On 2012/10/04 20:28:14, oshima wrote:
> I think you should just add label_ to this.
> GetContentsView is an API that widgets uses.

Done.

https://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_...
ash/display/display_error_dialog.cc:110: void
DisplayErrorDialog::OnDisplayRemoved(const gfx::Display& old_display) {
On 2012/10/04 20:28:14, oshima wrote:
> I believe you don't need these three (OnDisplayBoundsChanged/Added/Removed)
> because OnDisplayConfigurationChanging will always called before these
methods.
> Didn't it work without these?

You're right... removed.

[oshima, 2012-10-04 22:45:49]

lgtm

On Thu, Oct 4, 2012 at 1:39 PM, <mukai@chromium.org> wrote:

>
> https://codereview.chromium.**org/10986087/diff/3006/ash/**
>
display/display_error_dialog.**cc<https://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_dialog.cc>
> File ash/display/display_error_**dialog.cc (right):
>
> https://codereview.chromium.**org/10986087/diff/3006/ash/**
>
display/display_error_dialog.**cc#newcode65<https://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_dialog.cc#newcode65>
> ash/display/display_error_**dialog.cc:65: container->AddChildView(label_**
> );
>
> On 2012/10/04 20:28:14, oshima wrote:
>
>> I think you should just add label_ to this.
>> GetContentsView is an API that widgets uses.
>>
>
> Done.
>
> https://codereview.chromium.**org/10986087/diff/3006/ash/**
>
display/display_error_dialog.**cc#newcode110<https://codereview.chromium.org/10986087/diff/3006/ash/display/display_error_dialog.cc#newcode110>
>
> ash/display/display_error_**dialog.cc:110: void
> DisplayErrorDialog::**OnDisplayRemoved(const gfx::Display& old_display) {
> On 2012/10/04 20:28:14, oshima wrote:
>
>> I believe you don't need these three
>>
> (OnDisplayBoundsChanged/Added/**Removed)
>
>> because OnDisplayConfigurationChanging will always called before these
>>
> methods.
>
>> Didn't it work without these?
>>
>
> You're right... removed.
>
>
https://codereview.chromium.**org/10986087/<https://codereview.chromium.org/1...
>

[Jun Mukai, 2012-10-04 22:51:42]

sky, please check as the OWNER...

[sky, 2012-10-05 15:26:15]

LGTM

https://codereview.chromium.org/10986087/diff/21001/ash/display/display_error...
File ash/display/display_error_dialog.cc (right):

https://codereview.chromium.org/10986087/diff/21001/ash/display/display_error...
ash/display/display_error_dialog.cc:38:
g_instance->GetWidget()->GetNativeWindow()->Focus();
g_instance->GetWidget()->Activate();

https://codereview.chromium.org/10986087/diff/21001/ash/display/display_error...
File ash/display/display_error_dialog.h (right):

https://codereview.chromium.org/10986087/diff/21001/ash/display/display_error...
ash/display/display_error_dialog.h:28: // Dialog to show error messages when it
fails to change the display
Dialog used to show an error message when unable to change display configuration
to mirroring.

https://codereview.chromium.org/10986087/diff/21001/ash/display/display_error...
ash/display/display_error_dialog.h:33: // Shows the dialog to show errors.
Shows the error dialog.

https://codereview.chromium.org/10986087/diff/21001/ash/display/display_error...
ash/display/display_error_dialog.h:54:
DISALLOW_COPY_AND_ASSIGN(DisplayErrorDialog);
nit: newline between 53/54.

[Jun Mukai, 2012-10-05 20:30:50]

http://codereview.chromium.org/10986087/diff/21001/ash/display/display_error_...
File ash/display/display_error_dialog.cc (right):

http://codereview.chromium.org/10986087/diff/21001/ash/display/display_error_...
ash/display/display_error_dialog.cc:38:
g_instance->GetWidget()->GetNativeWindow()->Focus();
On 2012/10/05 15:26:15, sky wrote:
> g_instance->GetWidget()->Activate();

Done.

http://codereview.chromium.org/10986087/diff/21001/ash/display/display_error_...
File ash/display/display_error_dialog.h (right):

http://codereview.chromium.org/10986087/diff/21001/ash/display/display_error_...
ash/display/display_error_dialog.h:28: // Dialog to show error messages when it
fails to change the display
On 2012/10/05 15:26:15, sky wrote:
> Dialog used to show an error message when unable to change display
configuration
> to mirroring.

Done.

http://codereview.chromium.org/10986087/diff/21001/ash/display/display_error_...
ash/display/display_error_dialog.h:33: // Shows the dialog to show errors.
On 2012/10/05 15:26:15, sky wrote:
> Shows the error dialog.

Done.

http://codereview.chromium.org/10986087/diff/21001/ash/display/display_error_...
ash/display/display_error_dialog.h:54:
DISALLOW_COPY_AND_ASSIGN(DisplayErrorDialog);
On 2012/10/05 15:26:15, sky wrote:
> nit: newline between 53/54.

Done.

[commit-bot: I haz the power, 2012-10-05 20:31:38]

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

[commit-bot: I haz the power, 2012-10-05 23:55:34]

Change committed as 160521