Creates notifications for display resolution change.

Creates notifications for display resolution change.

The notification notifies the change of the resolution, and
contains "Revert" button in case the new resolution doesn't work well.

Also it creates timeout in case the change operation is potentially
dangerous, which means changing the resolution of the only one display.

BUG=230733, 266097
R=oshima@chromium.org, derat@chromium.org
TEST=new test cases in ash_unittests

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

[Daniel Erat, 2013-08-09 15:50:24]

all nits except for one question in resolution_notification_controller.cc

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller.cc (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.cc:230: AcceptResolutionChange();
can you explain this part? why does the change get accepted here?

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:24: // also manages the timeout
if the resolution change is potentially dangerous.
nit: "potentially dangerous" seems like too-strong wording. it would be better
to say something like "if the new resolution is unusable".

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:34: // change in case something
goes wrong. The notification timeouts if the
nit: s/timeouts if/times out in case/

("timeout" is a noun; "time out" is a verb)

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:35: // display resolution
change operation is potentially dangerous, i.e. there's
same here -- "potentially dangerous" makes it sound like choosing the wrong
resolution could damage hardware

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:38: // operations if something
goes wrong.
nit: s/operations/changes/

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:46: // the notification
timeouts.
nit: s/timeouts/times out/

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:75: // The remaining timeout in
seconds. 0 if the change does not timeout.
nit: s/does not timeout/does not time out/

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:83: ResolutionChangeInfo(int64
display_id,
nit: move c'tor and d'tor to top of members

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:88: };
nit: DISALLOW_COPY_AND_ASSIGN() (Timer isn't copyable)

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:106: static const int
kTimeoutInSec;
nit: static const members should be near the top of the private section
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller_unittest.cc (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller_unittest.cc:45: // so here
invokes UpdateDisplay() to emit that event explicitly.
nit: s/so here invokes/so invoke/

[Jun Mukai, 2013-08-09 16:49:16]

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller.cc (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.cc:230: AcceptResolutionChange();
On 2013/08/09 15:50:24, Daniel Erat wrote:
> can you explain this part? why does the change get accepted here?

What I concerned is the following situation:
1. connect external display, change its resolution (no timeout)
2. used for hours or so, but do not dismiss the notification at all, hidden in
the message center
3. and then disconnect the display
4. a few days later, connect the same display again

In that case, the user uses the display happily, so it should not revert the
change at disconnect.


But I changed my mind:
- because this notification has SystemPriority, it keeps appearing as far as
possible, until the user takes an action (close/click/click buttons)
- modifies 'Click' handler to accept. When the user clicks over the
notification, it means accept
- so, it should revert the change here, it means the disconnection happens
without any user's action

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:24: // also manages the timeout
if the resolution change is potentially dangerous.
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: "potentially dangerous" seems like too-strong wording. it would be better
> to say something like "if the new resolution is unusable".

done, but I think 'in case' would be better than 'if'. correct me if it's not.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:34: // change in case something
goes wrong. The notification timeouts if the
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: s/timeouts if/times out in case/
> 
> ("timeout" is a noun; "time out" is a verb)

Done.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:35: // display resolution
change operation is potentially dangerous, i.e. there's
On 2013/08/09 15:50:24, Daniel Erat wrote:
> same here -- "potentially dangerous" makes it sound like choosing the wrong
> resolution could damage hardware

just removed that sentence and write the condition directly.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:38: // operations if something
goes wrong.
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: s/operations/changes/

Done.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:46: // the notification
timeouts.
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: s/timeouts/times out/

Done.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:75: // The remaining timeout in
seconds. 0 if the change does not timeout.
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: s/does not timeout/does not time out/

Done.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:83: ResolutionChangeInfo(int64
display_id,
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: move c'tor and d'tor to top of members

Done.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:88: };
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: DISALLOW_COPY_AND_ASSIGN() (Timer isn't copyable)

Done.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:106: static const int
kTimeoutInSec;
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: static const members should be near the top of the private section
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...)

moved.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller_unittest.cc (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller_unittest.cc:45: // so here
invokes UpdateDisplay() to emit that event explicitly.
On 2013/08/09 15:50:24, Daniel Erat wrote:
> nit: s/so here invokes/so invoke/

Done.

[Daniel Erat, 2013-08-09 16:55:46]

LGTM (but see my comment -- might make more sense to switch back to the way it
was before)

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller.cc (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.cc:230: AcceptResolutionChange();
On 2013/08/09 16:49:16, Jun Mukai wrote:
> On 2013/08/09 15:50:24, Daniel Erat wrote:
> > can you explain this part? why does the change get accepted here?
> 
> What I concerned is the following situation:
> 1. connect external display, change its resolution (no timeout)
> 2. used for hours or so, but do not dismiss the notification at all, hidden in
> the message center
> 3. and then disconnect the display
> 4. a few days later, connect the same display again
> 
> In that case, the user uses the display happily, so it should not revert the
> change at disconnect.
> 
> 
> But I changed my mind:
> - because this notification has SystemPriority, it keeps appearing as far as
> possible, until the user takes an action (close/click/click buttons)
> - modifies 'Click' handler to accept. When the user clicks over the
> notification, it means accept
> - so, it should revert the change here, it means the disconnection happens
> without any user's action

ah, i didn't realize that this was for the multiple-display, timeout-is-unneeded
case. it might be worth adding a comment explaining this, but i think that the
logic that you had here (accepting the change even if the user didn't manually
dismiss the notification) was okay.

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/22703004/diff/2001/ash/display/resolution_not...
ash/display/resolution_notification_controller.h:24: // also manages the timeout
if the resolution change is potentially dangerous.
On 2013/08/09 16:49:16, Jun Mukai wrote:
> On 2013/08/09 15:50:24, Daniel Erat wrote:
> > nit: "potentially dangerous" seems like too-strong wording. it would be
better
> > to say something like "if the new resolution is unusable".
> 
> done, but I think 'in case' would be better than 'if'. correct me if it's not.

"in case" seems more precise to me too :-)

[oshima, 2013-08-09 17:42:56]

https://codereview.chromium.org/22703004/diff/10001/ash/display/resolution_no...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/22703004/diff/10001/ash/display/resolution_no...
ash/display/resolution_notification_controller.h:61: struct ResolutionChangeInfo
{
can you move the definition to .cc because this is private?

https://codereview.chromium.org/22703004/diff/10001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/22703004/diff/10001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:349:
GetDisplayManager()->GetDisplayInfo(display_id);
Looks like if I set the resolution while timeout dialog is shown,
it will use the value of the new resolution, instead of
the very first time that was working?

[Jun Mukai, 2013-08-09 21:04:57]

https://codereview.chromium.org/22703004/diff/10001/ash/display/resolution_no...
File ash/display/resolution_notification_controller.h (right):

https://codereview.chromium.org/22703004/diff/10001/ash/display/resolution_no...
ash/display/resolution_notification_controller.h:61: struct ResolutionChangeInfo
{
On 2013/08/09 17:42:56, oshima wrote:
> can you move the definition to .cc because this is private?

Done.

https://codereview.chromium.org/22703004/diff/10001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/22703004/diff/10001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:349:
GetDisplayManager()->GetDisplayInfo(display_id);
On 2013/08/09 17:42:56, oshima wrote:
> Looks like if I set the resolution while timeout dialog is shown,
> it will use the value of the new resolution, instead of
> the very first time that was working?

nice catch!
Added the handling of that case in ResolutionNotificationController since it
knows the original resolution.

[oshima_google, 2013-08-09 21:10:52]

lgtm

[commit-bot: I haz the power, 2013-08-09 21:16:22]

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

[commit-bot: I haz the power, 2013-08-09 22:29:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-10 01:43:51]

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

[commit-bot: I haz the power, 2013-08-10 07:00:05]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 16:15:44]

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

[commit-bot: I haz the power, 2013-08-10 18:13:48]

Change committed as 216881