On resume perform a delayed call to SetDisplayPower()

ui/display/chromeos/display_configurator.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/display/chromeos/display_configurator.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 71 matching lines @@
     if (display_power)
       (*display_power)[i] = on;
     if (on)
       num_on_displays++;
   }
   return num_on_displays;
 }
 
 }  // namespace
 
+
+// static

[Daniel Erat, 2014/08/12 21:00:03]

nit: don't need "static" comments here; it's obvious from
"DisplayConfigurator::"

[dnicoara, 2014/08/12 21:46:20]

Done.

+const int DisplayConfigurator::kSetDisplayPowerNoFlags = 0;
+// static
+const int DisplayConfigurator::kSetDisplayPowerForceProbe = 1 << 0;
+// static
+const int
+DisplayConfigurator::kSetDisplayPowerOnlyIfSingleInternalDisplay = 1 << 1;
+
 DisplayConfigurator::DisplayState::DisplayState()
     : display(NULL),
       touch_device_id(0),
       selected_mode(NULL),
       mirror_mode(NULL) {}
 
 bool DisplayConfigurator::TestApi::TriggerConfigureTimeout() {
   if (configurator_->configure_timer_.get() &&
       configurator_->configure_timer_->IsRunning()) {
-    configurator_->configure_timer_.reset();
-    configurator_->ConfigureDisplays();
+    configurator_->configure_timer_->user_task().Run();
     return true;
   } else {
     return false;
   }
 }
 
 // static
 const DisplayMode* DisplayConfigurator::FindDisplayModeMatchingSize(
     const DisplaySnapshot& display,
     const gfx::Size& size) {
@@ skipping 311 matching lines @@
   return false;
 }
 
 void DisplayConfigurator::PrepareForExit() {
   configure_display_ = false;
 }
 
 bool DisplayConfigurator::SetDisplayPower(
     chromeos::DisplayPowerState power_state,
     int flags) {
+  configure_timer_.reset();
+
   if (!configure_display_)
     return false;
 
   VLOG(1) << "SetDisplayPower: power_state="
           << DisplayPowerStateToString(power_state) << " flags=" << flags
           << ", configure timer="
           << ((configure_timer_.get() && configure_timer_->IsRunning()) ?
                   "Running" : "Stopped");
   if (power_state == power_state_ && !(flags & kSetDisplayPowerForceProbe))
     return true;
@@ skipping 49 matching lines @@
   native_display_delegate_->UngrabServer();
 
   NotifyObservers(success, new_state);
   return success;
 }
 
 void DisplayConfigurator::OnConfigurationChanged() {
   // Configure displays with |kConfigureDelayMs| delay,
   // so that time-consuming ConfigureDisplays() won't be called multiple times.
   if (configure_timer_.get()) {
     configure_timer_->Reset();

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

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?

[oshima, 2014/08/12 21:05:17]

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

[dnicoara, 2014/08/12 21:46:20]

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.

   } else {
     configure_timer_.reset(new base::OneShotTimer<DisplayConfigurator>());
     configure_timer_->Start(
         FROM_HERE,
         base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
         this,
         &DisplayConfigurator::ConfigureDisplays);
   }
 }
 
@@ skipping 18 matching lines @@
     // We need to make sure that the monitor configuration we just did actually
     // completes before we return, because otherwise the X message could be
     // racing with the HandleSuspendReadiness message.
     native_display_delegate_->SyncWithServer();
   }
 }
 
 void DisplayConfigurator::ResumeDisplays() {
   // Force probing to ensure that we pick up any changes that were made
   // while the system was suspended.
-  SetDisplayPower(power_state_, kSetDisplayPowerForceProbe);
+  configure_timer_.reset(new base::OneShotTimer<DisplayConfigurator>());
+  configure_timer_->Start(
+      FROM_HERE,
+      base::TimeDelta::FromMilliseconds(kConfigureDelayMs),

[Daniel Erat, 2014/08/12 21:00:03]

please add a new constant for this with a comment describing what it's used for.
(is 500 ms the right number?)

[dnicoara, 2014/08/12 21:46:20]

Done.

+      base::Bind(base::IgnoreResult(&DisplayConfigurator::SetDisplayPower),
+                 base::Unretained(this),
+                 power_state_,
+                 kSetDisplayPowerForceProbe));
 }
 
 void DisplayConfigurator::UpdateCachedDisplays() {
   std::vector<DisplaySnapshot*> snapshots =
       native_display_delegate_->GetDisplays();
 
   cached_displays_.clear();
   for (size_t i = 0; i < snapshots.size(); ++i) {
     DisplayState display_state;
     display_state.display = snapshots[i];
@@ skipping 387 matching lines @@
         return state_controller_->GetStateForDisplayIds(display_ids);
       }
     }
     default:
       NOTREACHED();
   }
   return MULTIPLE_DISPLAY_STATE_INVALID;
 }
 
 }  // namespace ui