On resume perform a delayed call to SetDisplayPower()

On resume perform a delayed call to SetDisplayPower()

Allow time for display events to settle before querying for new
displays.

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

[dnicoara, 2014-08-12 18:39:45]

PTAL

I've tried just reusing the state, but that didn't work well when a monitor is
disconnected while suspended, so I went with Oshima's suggestion.

I've tested this on a Pixel to make sure everything still works, though I don't
have the offending hardware to verify that it fixes the issue.

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.h:129: static const int
kSetDisplayPowerNoFlags;
If no one minds, I'd like to have a followup CL to make an enum for these
values.

[oshima, 2014-08-12 20:26:14]

dan is probably the best person to review this. this looks good to me though.

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.h:129: static const int
kSetDisplayPowerNoFlags;
On 2014/08/12 18:39:44, dnicoara wrote:
> If no one minds, I'd like to have a followup CL to make an enum for these
> values.

sgtm

[Daniel Erat, 2014-08-12 21:00:04]

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:93: // static
nit: don't need "static" comments here; it's obvious from
"DisplayConfigurator::"

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:513: configure_timer_->Reset();
is this broken? if configure_timer_ holds a SetDisplayPower() task, you're going
to run that instead of ConfigureDisplays() now.

i don't know the history of why this timer is in a scoped_ptr, but it doesn't
seem necessary. could you just change it to be a normal member, and then make
sure that we always call Start() when we want to reset the timer to ensure that
it's running with the correct task?

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:555:
base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
please add a new constant for this with a comment describing what it's used for.
(is 500 ms the right number?)

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator_unittest.cc:851:
test_api_.TriggerConfigureTimeout();
you should be testing the return value here to verify that the timer was
actually running. please also annotate this method with WARN_UNUSED_RESULT.

[oshima, 2014-08-12 21:05:18]

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:513: configure_timer_->Reset();
On 2014/08/12 21:00:04, Daniel Erat wrote:
> is this broken? if configure_timer_ holds a SetDisplayPower() task, you're
going
> to run that instead of ConfigureDisplays() now.

Reset uses the same task, so it will still call SetDisplayPower, i think.

> i don't know the history of why this timer is in a scoped_ptr, but it doesn't
> seem necessary. could you just change it to be a normal member, and then make
> sure that we always call Start() when we want to reset the timer to ensure
that
> it's running with the correct task?

[dnicoara, 2014-08-12 21:46:21]

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:93: // static
On 2014/08/12 21:00:03, Daniel Erat wrote:
> nit: don't need "static" comments here; it's obvious from
> "DisplayConfigurator::"

Done.

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:513: configure_timer_->Reset();
On 2014/08/12 21:05:17, oshima wrote:
> On 2014/08/12 21:00:04, Daniel Erat wrote:
> > is this broken? if configure_timer_ holds a SetDisplayPower() task, you're
> going
> > to run that instead of ConfigureDisplays() now.
> 
> Reset uses the same task, so it will still call SetDisplayPower, i think.
> 
> > i don't know the history of why this timer is in a scoped_ptr, but it
doesn't
> > seem necessary. could you just change it to be a normal member, and then
make
> > sure that we always call Start() when we want to reset the timer to ensure
> that
> > it's running with the correct task?
> 

Removed scoped_ptr.

Yes, resetting will just update the timer and reuse the same task. It is
intentional since SetDisplayPower() does what ConfigureDisplays() does and more.
I added a comment for clarity.

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:555:
base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
On 2014/08/12 21:00:03, Daniel Erat wrote:
> please add a new constant for this with a comment describing what it's used
for.
> (is 500 ms the right number?)

Done.

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator_unittest.cc (right):

https://codereview.chromium.org/463633005/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator_unittest.cc:851:
test_api_.TriggerConfigureTimeout();
On 2014/08/12 21:00:04, Daniel Erat wrote:
> you should be testing the return value here to verify that the timer was
> actually running. please also annotate this method with WARN_UNUSED_RESULT.

Done.

[Daniel Erat, 2014-08-12 22:00:49]

lgtm

https://codereview.chromium.org/463633005/diff/40001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/463633005/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:120: bool TriggerConfigureTimeout();
nit: please add WARN_UNUSED_RESULT from base/compiler_specific.h here

https://codereview.chromium.org/463633005/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:352: // Dispatch().
nit: is this mention of Dispatch() obsolete?

[dnicoara, 2014-08-13 13:28:50]

https://codereview.chromium.org/463633005/diff/40001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.h (right):

https://codereview.chromium.org/463633005/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:120: bool TriggerConfigureTimeout();
On 2014/08/12 22:00:49, Daniel Erat wrote:
> nit: please add WARN_UNUSED_RESULT from base/compiler_specific.h here

Done

Sorry I missed it.

https://codereview.chromium.org/463633005/diff/40001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.h:352: // Dispatch().
On 2014/08/12 22:00:49, Daniel Erat wrote:
> nit: is this mention of Dispatch() obsolete?

Updated comment.

[dnicoara, 2014-08-13 13:29:48]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-08-13 13:30:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/463633005/60001

[commit-bot: I haz the power, 2014-08-13 14:43:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-13 14:53:47]

Committed patchset #4 (60001) as 289292