Provides more types of errors for output status.

Provides more types of errors for display status.

Note that the text message itself is a placeholder. It has to be replaced
by real sentences.

BUG=176011
TEST=ash_unittests passes


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

[oshima, 2013-02-14 00:01:50]

https://codereview.chromium.org/12212171/diff/1/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/12212171/diff/1/ash/ash_strings.grd#newcode572
ash/ash_strings.grd:572: Can't create extended desktop. No output mode found.
Could not find a valid resolution for monitor <monitor name>

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

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.cc:72: message_id =
IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
Can you document when this can happen?

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.cc:136: if (dialog_ && dialog_->GetWidget() ==
widget) {
can dialog_->GetWidget() != widget happen? do you want to add DCHECK?

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

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.h:72: DisplayErrorDialog* dialog() const {
return dialog_; }
no const if it returns non-const internal state.

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

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog_unittest.cc:25: }
It's probably better to keep the order of setup/teardown in TearDown() ?

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:814: if (new_output_count !=
connected_output_count_) {
Not your fault, but this is wrong. This can cause the error
if you swap the display during the suspend...

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:823: Observer, observers_,
OnDisplayModeChangeFailed(new_state));
I think we need to move this out from xgrab section,
but let me fix the above issue first.

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.h:63: virtual void
OnDisplayModeChangeFailed(OutputState new_state) {}
failed_new_state would be better.

[Jun Mukai, 2013-02-14 00:54:03]

https://codereview.chromium.org/12212171/diff/1/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/12212171/diff/1/ash/ash_strings.grd#newcode572
ash/ash_strings.grd:572: Can't create extended desktop. No output mode found.
On 2013/02/14 00:01:50, oshima wrote:
> Could not find a valid resolution for monitor <monitor name>

Usually <monitor name> would be unavailable when this type of error happens. I
suspect that the monitor is even invisible at gfx level.

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

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.cc:72: message_id =
IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
On 2013/02/14 00:01:50, oshima wrote:
> Can you document when this can happen?

Done.

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.cc:136: if (dialog_ && dialog_->GetWidget() ==
widget) {
On 2013/02/14 00:01:50, oshima wrote:
> can dialog_->GetWidget() != widget happen? do you want to add DCHECK?

Done.

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

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog.h:72: DisplayErrorDialog* dialog() const {
return dialog_; }
On 2013/02/14 00:01:50, oshima wrote:
> no const if it returns non-const internal state.

made it return to const pointer.

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

https://codereview.chromium.org/12212171/diff/1/ash/display/display_error_dia...
ash/display/display_error_dialog_unittest.cc:25: }
On 2013/02/14 00:01:50, oshima wrote:
> It's probably better to keep the order of setup/teardown in TearDown() ?

Done.

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:814: if (new_output_count !=
connected_output_count_) {
On 2013/02/14 00:01:50, oshima wrote:
> Not your fault, but this is wrong. This can cause the error
> if you swap the display during the suspend...

okay, so I'll wait.

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.cc:823: Observer, observers_,
OnDisplayModeChangeFailed(new_state));
On 2013/02/14 00:01:50, oshima wrote:
> I think we need to move this out from xgrab section,
> but let me fix the above issue first.

I have little ideas what you're talking. Can I leave this as is for now?

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
File chromeos/display/output_configurator.h (right):

https://codereview.chromium.org/12212171/diff/1/chromeos/display/output_confi...
chromeos/display/output_configurator.h:63: virtual void
OnDisplayModeChangeFailed(OutputState new_state) {}
On 2013/02/14 00:01:50, oshima wrote:
> failed_new_state would be better.

Done.

[oshima, 2013-02-26 18:27:22]

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

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
ash/display/display_error_dialog.cc:76: message_id =
IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
log error with the new_state as it shouldn't happen.

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
ash/display/display_error_dialog.cc:124:
DisplayErrorObserver::~DisplayErrorObserver() {
DCHECK(!dialog_)

to make sure the dialog gets closed before the observer is
removed.

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
File ash/display/display_error_dialog.h (right):

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
ash/display/display_error_dialog.h:43: DisplayErrorDialog(chromeos::OutputState
failed_new_state);
explicit

https://codereview.chromium.org/12212171/diff/8001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/12212171/diff/8001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:819: if (changed)
you need {} for this case.

[Jun Mukai, 2013-02-28 22:57:36]

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

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
ash/display/display_error_dialog.cc:76: message_id =
IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
On 2013/02/26 18:27:22, oshima wrote:
> log error with the new_state as it shouldn't happen.

Done.

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
ash/display/display_error_dialog.cc:124:
DisplayErrorObserver::~DisplayErrorObserver() {
On 2013/02/26 18:27:22, oshima wrote:
> DCHECK(!dialog_)
> 
> to make sure the dialog gets closed before the observer is
> removed.

Done.

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
File ash/display/display_error_dialog.h (right):

https://codereview.chromium.org/12212171/diff/8001/ash/display/display_error_...
ash/display/display_error_dialog.h:43: DisplayErrorDialog(chromeos::OutputState
failed_new_state);
On 2013/02/26 18:27:22, oshima wrote:
> explicit

Done.

https://codereview.chromium.org/12212171/diff/8001/chromeos/display/output_co...
File chromeos/display/output_configurator.cc (right):

https://codereview.chromium.org/12212171/diff/8001/chromeos/display/output_co...
chromeos/display/output_configurator.cc:819: if (changed)
On 2013/02/26 18:27:22, oshima wrote:
> you need {} for this case.

Done.

[oshima, 2013-02-28 23:00:36]

lgtm with nit

https://codereview.chromium.org/12212171/diff/13001/ash/display/display_error...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12212171/diff/13001/ash/display/display_error...
ash/display/display_error_dialog_unittest.cc:47: };
DISALLOW_COPY_AND_ASSIGN

[Jun Mukai, 2013-02-28 23:03:12]

https://codereview.chromium.org/12212171/diff/13001/ash/display/display_error...
File ash/display/display_error_dialog_unittest.cc (right):

https://codereview.chromium.org/12212171/diff/13001/ash/display/display_error...
ash/display/display_error_dialog_unittest.cc:47: };
On 2013/02/28 23:00:36, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Jun Mukai, 2013-02-28 23:04:32]

jamescook, can you review this?

[James Cook, 2013-03-01 00:20:17]

LGTM with nits

Please update the issue description to indicate that this is related to monitor
configuration. I didn't know what "output status" meant without looking at the
code.

https://codereview.chromium.org/12212171/diff/13003/ash/display/display_error...
File ash/display/display_error_dialog.cc (left):

https://codereview.chromium.org/12212171/diff/13003/ash/display/display_error...
ash/display/display_error_dialog.cc:30: DisplayErrorDialog* g_instance = NULL;
Thank you for eliminating globals.

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

https://codereview.chromium.org/12212171/diff/13003/ash/display/display_error...
ash/display/display_error_dialog.cc:76: message_id =
IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
nit: add "break;" at end

https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc#newcode310
ash/shell.cc:310: #if defined(OS_CHROMEOS)
It seems like this entire block should go above instance_ = NULL (in case one of
these observers needs to access Shell during cleanup), or instance_ = NULL
should go at the top of this function.  Don't change it for this patch, but it's
something for you and Oshima to think about.

[oshima, 2013-03-01 00:43:07]

https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc#newcode310
ash/shell.cc:310: #if defined(OS_CHROMEOS)
On 2013/03/01 00:20:17, James Cook (Chromium) wrote:
> It seems like this entire block should go above instance_ = NULL (in case one
of
> these observers needs to access Shell during cleanup), or instance_ = NULL
> should go at the top of this function.  Don't change it for this patch, but
it's
> something for you and Oshima to think about.

I made the change in https://codereview.chromium.org/12367007/
so that observer won't be called during shutdown. I do agree
that it still makes sense to move it before = NULL though, 
just in case.

[Jun Mukai, 2013-03-01 01:32:54]

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

https://codereview.chromium.org/12212171/diff/13003/ash/display/display_error...
ash/display/display_error_dialog.cc:76: message_id =
IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
On 2013/03/01 00:20:17, James Cook (Chromium) wrote:
> nit: add "break;" at end

Done.

https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc#newcode310
ash/shell.cc:310: #if defined(OS_CHROMEOS)
On 2013/03/01 00:20:17, James Cook (Chromium) wrote:
> It seems like this entire block should go above instance_ = NULL (in case one
of
> these observers needs to access Shell during cleanup), or instance_ = NULL
> should go at the top of this function.  Don't change it for this patch, but
it's
> something for you and Oshima to think about.

Done.

[oshima, 2013-03-01 01:34:28]

On 2013/03/01 01:32:54, Jun Mukai wrote:
>
https://codereview.chromium.org/12212171/diff/13003/ash/display/display_error...
> File ash/display/display_error_dialog.cc (right):
> 
>
https://codereview.chromium.org/12212171/diff/13003/ash/display/display_error...
> ash/display/display_error_dialog.cc:76: message_id =
> IDS_ASH_DISPLAY_FAILURE_UNKNOWN;
> On 2013/03/01 00:20:17, James Cook (Chromium) wrote:
> > nit: add "break;" at end
> 
> Done.
> 
> https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/12212171/diff/13003/ash/shell.cc#newcode310
> ash/shell.cc:310: #if defined(OS_CHROMEOS)
> On 2013/03/01 00:20:17, James Cook (Chromium) wrote:
> > It seems like this entire block should go above instance_ = NULL (in case
one
> of
> > these observers needs to access Shell during cleanup), or instance_ = NULL
> > should go at the top of this function.  Don't change it for this patch, but
> it's
> > something for you and Oshima to think about.
> 
> Done.

you need to sync, just so you know.

[commit-bot: I haz the power, 2013-03-01 01:38:45]

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

[commit-bot: I haz the power, 2013-03-01 01:51:20]

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

[commit-bot: I haz the power, 2013-03-01 04:06:55]

Change committed as 185456