Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427843002:
Delay display configuration after waking up from suspend with multi displays (Closed)
DescriptionDelay display configuration after waking up from suspend with multi displays
Perfrom display configuration after a delay when waking up from suspend in multi
display mode, for all displays to have a chance to be detected and added, before
the first configuration.
BUG=614624
TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.*
TEST=connect an external monitor, have several windows open in both displays,
press Search+Shift+L to suspend the device, wait for a few seconds until the
device is fully suspeneded, wake up by pressing any key. The windows in both
displays should be exactly where you left them before suspend.
Committed: https://crrev.com/343040c3e082e53591a4d7d822cefc70ba900c82
Cr-Commit-Position: refs/heads/master@{#429070}
Patch Set 1 #
Total comments: 13
Patch Set 2 : On Resume + unit tests #Patch Set 3 : Working solution on stumpy #
Total comments: 18
Patch Set 4 : derat's comments #
Total comments: 6
Patch Set 5 : derat's comments #
Total comments: 4
Patch Set 6 : Painful Rebase #
Total comments: 9
Patch Set 7 : More comments #Patch Set 8 : Remove the callback #
Total comments: 18
Patch Set 9 : Comments and nits #
Total comments: 8
Patch Set 10 : Few more comments #
Total comments: 4
Patch Set 11 : Nits #
Messages
Total messages: 73 (33 generated)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
afakhry@chromium.org changed reviewers: + oshima@chromium.org
Oshima, please review. Thank you! https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator.cc (left): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:925: // operations. Outdated comment? Do we still need the below Reset()?
oshima@chromium.org changed reviewers: + derat@chromium.org
+derat@ https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator.cc (left): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:925: // operations. On 2016/10/17 22:35:28, afakhry wrote: > Outdated comment? Do we still need the below Reset()? +derat https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:40: // for external displays to be detected and added. please add reference to the bug
derat@chromium.org changed reviewers: + dbasehore@chromium.org, dnicoara@chromium.org, marcheu@chromium.org
adding others who should take a look (and who might be able to offer more-reliable suggestions than adding a delay). do we still eventually end up in the correct state? is the underlying issue that we're probing once on resume before the external display has powered on, switching to single-display mode and resizing windows, and then later seeing the external display and going back to the dual-display state? https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator_unittest.cc:300: bool configuration_done_ = false; please add a comment describing when this is true and when it's false. https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator_unittest.cc:1984: please add a new test that verifies that the code you added is getting called.
I'd wait for marcheu@'s feedback on the timeout. My immediate thought would be that the kernel would block on probing the hardware (rather than return incomplete results), but I'm not familiar enough with the kernel code. https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:870: if (power_state == chromeos::DISPLAY_POWER_ALL_ON && I don't think this is the correct place to start the timer since SetDisplayPowerInternal() is called in other circumstances as well. You want to set the timer in ResumeDisplays() to limit it to the resume case.
https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:870: if (power_state == chromeos::DISPLAY_POWER_ALL_ON && On 2016/10/19 14:42:41, dnicoara wrote: > I don't think this is the correct place to start the timer since > SetDisplayPowerInternal() is called in other circumstances as well. You want to > set the timer in ResumeDisplays() to limit it to the resume case. good catch; this seems wrong to me too. when powerd has turned the displays off due to the user being inactive but not yet suspended, and the user then hits a key, powerd makes a d-bus call that results in SetDisplayPower being called. we don't want to delay turning them on there. mind trying to add a test that would've caught this regression? https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator_unittest.cc:270: void WaitForConfiguration() { how about making this take an expected_duration parameter and check that the timer has the expected delay? it could e.g. return false if the delay is wrong, and the callers could check that.
Description was changed from ========== Delay display configuration after waking up from suspend with dual displays Perfrom display configuration after a delay when waking up from suspend in dual display mode, for all displays to have a chance to be detected and added, before the first configuration. BUG=614624 TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.* TEST=connect an external monitor, have several windows open in both displays, press Search+Shift+L to suspend the device, wait for a few seconds until the device is fully suspeneded, wake up by pressing any key. The windows in both displays should be exactly where you left them before suspend. ========== to ========== Delay display configuration after waking up from suspend with multi displays Perfrom display configuration after a delay when waking up from suspend in multi display mode, for all displays to have a chance to be detected and added, before the first configuration. BUG=614624 TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.* TEST=connect an external monitor, have several windows open in both displays, press Search+Shift+L to suspend the device, wait for a few seconds until the device is fully suspeneded, wake up by pressing any key. The windows in both displays should be exactly where you left them before suspend. ==========
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:40: // for external displays to be detected and added. On 2016/10/18 22:00:05, oshima wrote: > please add reference to the bug Done. https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:870: if (power_state == chromeos::DISPLAY_POWER_ALL_ON && On 2016/10/19 14:42:41, dnicoara wrote: > I don't think this is the correct place to start the timer since > SetDisplayPowerInternal() is called in other circumstances as well. You want to > set the timer in ResumeDisplays() to limit it to the resume case. Done. https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator.cc:870: if (power_state == chromeos::DISPLAY_POWER_ALL_ON && On 2016/10/19 16:57:56, Daniel Erat wrote: > On 2016/10/19 14:42:41, dnicoara wrote: > > I don't think this is the correct place to start the timer since > > SetDisplayPowerInternal() is called in other circumstances as well. You want > to > > set the timer in ResumeDisplays() to limit it to the resume case. > > good catch; this seems wrong to me too. when powerd has turned the displays off > due to the user being inactive but not yet suspended, and the user then hits a > key, powerd makes a d-bus call that results in SetDisplayPower being called. we > don't want to delay turning them on there. > > mind trying to add a test that would've caught this regression? Thanks for catching that. Done. I modified the unit tests to validate that the delay is kept at a minimum. https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator_unittest.cc:270: void WaitForConfiguration() { On 2016/10/19 16:57:56, Daniel Erat wrote: > how about making this take an expected_duration parameter and check that the > timer has the expected delay? it could e.g. return false if the delay is wrong, > and the callers could check that. I think the best approach is to encapsulate the waiting logic into its own object. Please take a look at the ConfigurationWaiter. https://codereview.chromium.org/2427843002/diff/1/ui/display/chromeos/display... ui/display/chromeos/display_configurator_unittest.cc:1984: On 2016/10/18 22:17:41, Daniel Erat wrote: > please add a new test that verifies that the code you added is getting called. Done.
https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:972: ResumeDisplaysInternal(base::Bind(&DoNothing)); this seems weird: a helper method that's only called in one place? why isn't the code directly in this method? https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:1170: // the display power on when it enables the backlight. did you mean to keep this comment above the SetDisplayPower call? it doesn't seem to make much sense here. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.h (right): https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:280: MultipleDisplayState display_state() const { return current_display_state_; } please move all of these back below the d'tor -- chrome style is usually to put inlined getters and setters above other methods. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:303: friend class test::DisplayConfiguratorTest; please avoid listing test classes as friends; it often results in brittle tests that need to be updated whenever the implementation is changed. if this is just for the two constants below, please make them be public static members instead. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:387: void ResumeDisplaysInternal(const ConfigurationCallback& callback); document this method (what it does and who calls it) https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:153: base::TimeDelta Wait() { add WARN_UNUSED_RESULT to make sure callers check the return value? https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:183: base::TimeTicks start_; please add comments describing what all of these members are for https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:275: static const int kMaxShortDelayMs = make both of these be base::TimeDeltas (possibly non-static, if you're worried about complex static types, although i don't think it really matters in a test) so you don't need to call base::TimeDelta::FromMilliseconds over and over later https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:746: EXPECT_LT(delay, base::TimeDelta::FromMilliseconds(kMaxShortDelayMs)); how about inlining all of these? EXPECT_LT(config_waiter.Wait(), kMaxShortDelay);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:972: ResumeDisplaysInternal(base::Bind(&DoNothing)); On 2016/10/21 21:09:44, Daniel Erat wrote: > this seems weird: a helper method that's only called in one place? why isn't the > code directly in this method? Unittests need to provide a callback, but clients of the DisplayConfigurator don't care whether configuration was done or not. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:1170: // the display power on when it enables the backlight. On 2016/10/21 21:09:44, Daniel Erat wrote: > did you mean to keep this comment above the SetDisplayPower call? it doesn't > seem to make much sense here. Yep, thanks for catching that. Done. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.h (right): https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:280: MultipleDisplayState display_state() const { return current_display_state_; } On 2016/10/21 21:09:45, Daniel Erat wrote: > please move all of these back below the d'tor -- chrome style is usually to put > inlined getters and setters above other methods. Really? I always thought it's the other way around! Apparently in our codebase we have a mix of both styles (for example: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_process.... and https://cs.chromium.org/chromium/src/chrome/browser/chromeos/file_manager/vol...) I moved them above as you suggested. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:303: friend class test::DisplayConfiguratorTest; On 2016/10/21 21:09:45, Daniel Erat wrote: > please avoid listing test classes as friends; it often results in brittle tests > that need to be updated whenever the implementation is changed. if this is just > for the two constants below, please make them be public static members instead. It was actually more needed for ResumeDisplaysInternal(), since unit tests need to provide a callback to know exactly when configuration was done. Here's what I did based on your suggestion: - Removed the friend. - Moved the static constants to public. - Made ResumeDisplaysInternal() --> ResumeDisplaysWithCallback() and moved it to public. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:387: void ResumeDisplaysInternal(const ConfigurationCallback& callback); On 2016/10/21 21:09:45, Daniel Erat wrote: > document this method (what it does and who calls it) Done. I made it public and moved it next to ResumeDisplays() and renamed it to ResumeDisplaysWithCallback(). https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:153: base::TimeDelta Wait() { On 2016/10/21 21:09:45, Daniel Erat wrote: > add WARN_UNUSED_RESULT to make sure callers check the return value? Done. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:183: base::TimeTicks start_; On 2016/10/21 21:09:45, Daniel Erat wrote: > please add comments describing what all of these members are for Done. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:275: static const int kMaxShortDelayMs = On 2016/10/21 21:09:45, Daniel Erat wrote: > make both of these be base::TimeDeltas (possibly non-static, if you're worried > about complex static types, although i don't think it really matters in a test) > so you don't need to call base::TimeDelta::FromMilliseconds over and over later Great suggestion. Actually, they can even be constexprs. I moved them out of class since we don't need the friendship to access the DisplayConfigurator constants. https://codereview.chromium.org/2427843002/diff/40001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:746: EXPECT_LT(delay, base::TimeDelta::FromMilliseconds(kMaxShortDelayMs)); On 2016/10/21 21:09:45, Daniel Erat wrote: > how about inlining all of these? > > EXPECT_LT(config_waiter.Wait(), kMaxShortDelay); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:977: if (!configure_display_ || display_externally_controlled_) should you be running the callback here? SuspendDisplays() runs its callback before returning in this situation. https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.h (right): https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:262: void ResumeDisplaysWithCallback(const ConfigurationCallback& callback); instead of having two methods, please just make ResumeDisplays take a callback and update the existing callers to pass base::DoNothing. also, please update the comments on SuspendDisplays and ResumeDisplays to mention that the callbacks can be executed either synchronously or asynchronously. https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:40: constexpr base::TimeDelta kMinLongDelayMs = do these tests actually wait this long for the code to run? it looks like they might. if so, please find a way to avoid doing this -- tests need to run quickly and shouldn't sleep. see the existing DisplayConfigurator::TestApi::TriggerConfigureTimeout() method used by other tests. to test the delay before you make the timer fire, add a new method to TestApi like: base::TimeDelta GetConfigureDelay() const { return configurator_->configure_timer_.IsRunning() ? configurator_->configure_timer_.GetCurrentDelay() : base::TimeDelta(); }
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:977: if (!configure_display_ || display_externally_controlled_) On 2016/10/24 16:31:04, Daniel Erat wrote: > should you be running the callback here? SuspendDisplays() runs its callback > before returning in this situation. Done. https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.h (right): https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.h:262: void ResumeDisplaysWithCallback(const ConfigurationCallback& callback); On 2016/10/24 16:31:04, Daniel Erat wrote: > instead of having two methods, please just make ResumeDisplays take a callback > and update the existing callers to pass base::DoNothing. > > also, please update the comments on SuspendDisplays and ResumeDisplays to > mention that the callbacks can be executed either synchronously or > asynchronously. Done. base::DoNothing() won't work though, it's void(void) and we need a void(bool). https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/60001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator_unittest.cc:40: constexpr base::TimeDelta kMinLongDelayMs = On 2016/10/24 16:31:04, Daniel Erat wrote: > do these tests actually wait this long for the code to run? it looks like they > might. if so, please find a way to avoid doing this -- tests need to run quickly > and shouldn't sleep. see the existing > DisplayConfigurator::TestApi::TriggerConfigureTimeout() method used by other > tests. to test the delay before you make the timer fire, add a new method to > TestApi like: > > base::TimeDelta GetConfigureDelay() const { > return configurator_->configure_timer_.IsRunning() ? > configurator_->configure_timer_.GetCurrentDelay() : > base::TimeDelta(); > } That's a great suggestion! The tests indeed used to wait for long durations, which wasn't pleasant and could introduce future flakiness. This simplifies the waiters logic a lot. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
marcheu@google.com changed reviewers: + marcheu@google.com
https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:997: } why not do this unconditionally? For example if you have a chromebox with 1 monitor, the postponed configuration would also be interesting.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator.cc:996: this, &DisplayConfigurator::ConfigureDisplays); this is part of why i'm confused (see question in test file). should the callback be queued in this case instead of passed to SetDisplayPower below? https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:37: enum CallbackResult { i don't understand what you're verifying by checking if the callback is run now. the production code just passes DoNothing to it, and i _think_ it's always run synchronously by tests. is it to check that SetDisplayPower is called synchronously? if so, is there a simpler way you can check that, e.g. by just checking the power state before calling Wait()? https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:166: // Simulates Waiting for the configuration to complete by manually triggering nit: s/Waiting/waiting/ https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:2054: EXPECT_GE(test_api_.GetConfigureDelay(), kMinLongDelayMs); you should use EXPECT_EQ rather than EXPECT_GE for all of these now, right? you should get back the configured delay instead of some arbitrary delta, i think.
https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:997: } On 2016/10/25 00:41:11, marcheu1 wrote: > why not do this unconditionally? For example if you have a chromebox with 1 > monitor, the postponed configuration would also be interesting. +derat@ What do you think of this suggestion? I think the long delay here isn't very useful when we have a single monitor. https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator.cc:996: this, &DisplayConfigurator::ConfigureDisplays); On 2016/10/25 16:05:32, Daniel Erat wrote: > this is part of why i'm confused (see question in test file). should the > callback be queued in this case instead of passed to SetDisplayPower below? (Also see my answer in the test file). This is a bit tricky! The way I understand it is that (for tests) the delayed configuration callback is only interesting when SetDisplayPower() is not a no-op (i.e. when there is a change). When SetDisplayPower() is not a no-op, it will enqueue the callback itself. See the following example: 1- We call ResumeDisplays() while in dual display mode, power is still off (no change from current/pending). Configuration is scheduled 2 seconds later. 1.1 - SetDisplayPower() is called. It's a no-op. callback is invoked immediately as success. We don't care about the delayed configuration (yet). 2- Now we simulate ChromeDisplayPowerServiceProviderDelegate::SetDisplayPower() turning the displays on by calling SetDisplayPower() manually with ALL_ON. 2.1 - This is not a no-op. Configuration timer is reset, callback is enqueued and will be invoked when the configuration is done later. Sorry if it's confusing. That's the best way I can explain it. https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:37: enum CallbackResult { On 2016/10/25 16:05:33, Daniel Erat wrote: > i don't understand what you're verifying by checking if the callback is run now. > the production code just passes DoNothing to it, and i _think_ it's always run > synchronously by tests. is it to check that SetDisplayPower is called > synchronously? if so, is there a simpler way you can check that, e.g. by just > checking the power state before calling Wait()? ResumeDisplays() can result in a delayed configuration in multiple tests for which the callback will be invoked asynchronously. We're not just trying to verify that SetDisplayPower() is called, but rather the whole operation completed as expected. So validate the correct delay AND the correct callback result (true/false). https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:166: // Simulates Waiting for the configuration to complete by manually triggering On 2016/10/25 16:05:32, Daniel Erat wrote: > nit: s/Waiting/waiting/ Done. https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:2054: EXPECT_GE(test_api_.GetConfigureDelay(), kMinLongDelayMs); On 2016/10/25 16:05:32, Daniel Erat wrote: > you should use EXPECT_EQ rather than EXPECT_GE for all of these now, right? you > should get back the configured delay instead of some arbitrary delta, i think. Done.
https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:37: enum CallbackResult { On 2016/10/25 20:14:11, afakhry wrote: > On 2016/10/25 16:05:33, Daniel Erat wrote: > > i don't understand what you're verifying by checking if the callback is run > now. > > the production code just passes DoNothing to it, and i _think_ it's always run > > synchronously by tests. is it to check that SetDisplayPower is called > > synchronously? if so, is there a simpler way you can check that, e.g. by just > > checking the power state before calling Wait()? > > ResumeDisplays() can result in a delayed configuration in multiple tests for > which the callback will be invoked asynchronously. We're not just trying to > verify that SetDisplayPower() is called, but rather the whole operation > completed as expected. > > So validate the correct delay AND the correct callback result (true/false). thanks for the detailed explanation! i understand why you're passing the callback to SetDisplayPower and verifying that it's called with the correct value. the thing that confuses me is just why the callback is also passed to ResumeDisplays. the real code in PowerEventObserver passes DoNothing to ResumeDisplays, so it's not clear to me what's being tested by passing a callback to ResumeDisplays in the tests. sorry if i'm being dense. :-)
https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:997: } On 2016/10/25 20:14:10, afakhry wrote: > On 2016/10/25 00:41:11, marcheu1 wrote: > > why not do this unconditionally? For example if you have a chromebox with 1 > > monitor, the postponed configuration would also be interesting. > > +derat@ What do you think of this suggestion? > I think the long delay here isn't very useful when we have a single monitor. it seems okay to me if it simplifies the code or makes it more robust. are there any situations where a chromebox resumes with a single external display and fails to find it, where we might benefit from scheduling this extra config?
On 2016/10/26 15:50:00, Daniel Erat wrote: > https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... > File ui/display/chromeos/display_configurator_unittest.cc (right): > > https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... > ui/display/chromeos/display_configurator_unittest.cc:37: enum CallbackResult { > On 2016/10/25 20:14:11, afakhry wrote: > > On 2016/10/25 16:05:33, Daniel Erat wrote: > > > i don't understand what you're verifying by checking if the callback is run > > now. > > > the production code just passes DoNothing to it, and i _think_ it's always > run > > > synchronously by tests. is it to check that SetDisplayPower is called > > > synchronously? if so, is there a simpler way you can check that, e.g. by > just > > > checking the power state before calling Wait()? > > > > ResumeDisplays() can result in a delayed configuration in multiple tests for > > which the callback will be invoked asynchronously. We're not just trying to > > verify that SetDisplayPower() is called, but rather the whole operation > > completed as expected. > > > > So validate the correct delay AND the correct callback result (true/false). > > thanks for the detailed explanation! > > i understand why you're passing the callback to SetDisplayPower and verifying > that it's called with the correct value. the thing that confuses me is just why > the callback is also passed to ResumeDisplays. the real code in > PowerEventObserver passes DoNothing to ResumeDisplays, so it's not clear to me > what's being tested by passing a callback to ResumeDisplays in the tests. > > sorry if i'm being dense. :-) Since ResumeDisplays() calls SetDisplayPower(), we're also interested in the result of that in tests, especially if it will lead to a delayed configuration, in which case we must verify the delay, and the result.
On 2016/10/27 15:51:39, afakhry wrote: > On 2016/10/26 15:50:00, Daniel Erat wrote: > > > https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... > > File ui/display/chromeos/display_configurator_unittest.cc (right): > > > > > https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... > > ui/display/chromeos/display_configurator_unittest.cc:37: enum CallbackResult { > > On 2016/10/25 20:14:11, afakhry wrote: > > > On 2016/10/25 16:05:33, Daniel Erat wrote: > > > > i don't understand what you're verifying by checking if the callback is > run > > > now. > > > > the production code just passes DoNothing to it, and i _think_ it's always > > run > > > > synchronously by tests. is it to check that SetDisplayPower is called > > > > synchronously? if so, is there a simpler way you can check that, e.g. by > > just > > > > checking the power state before calling Wait()? > > > > > > ResumeDisplays() can result in a delayed configuration in multiple tests for > > > which the callback will be invoked asynchronously. We're not just trying to > > > verify that SetDisplayPower() is called, but rather the whole operation > > > completed as expected. > > > > > > So validate the correct delay AND the correct callback result (true/false). > > > > thanks for the detailed explanation! > > > > i understand why you're passing the callback to SetDisplayPower and verifying > > that it's called with the correct value. the thing that confuses me is just > why > > the callback is also passed to ResumeDisplays. the real code in > > PowerEventObserver passes DoNothing to ResumeDisplays, so it's not clear to me > > what's being tested by passing a callback to ResumeDisplays in the tests. > > > > sorry if i'm being dense. :-) > > Since ResumeDisplays() calls SetDisplayPower(), we're also interested in the > result of that in tests, especially if it will lead to a delayed configuration, > in which case we must verify the delay, and the result. Let me discuss with Oshima today to see if that will be useful.
On 2016/10/27 15:51:39, afakhry wrote: > On 2016/10/26 15:50:00, Daniel Erat wrote: > > > https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... > > File ui/display/chromeos/display_configurator_unittest.cc (right): > > > > > https://codereview.chromium.org/2427843002/diff/100001/ui/display/chromeos/di... > > ui/display/chromeos/display_configurator_unittest.cc:37: enum CallbackResult { > > On 2016/10/25 20:14:11, afakhry wrote: > > > On 2016/10/25 16:05:33, Daniel Erat wrote: > > > > i don't understand what you're verifying by checking if the callback is > run > > > now. > > > > the production code just passes DoNothing to it, and i _think_ it's always > > run > > > > synchronously by tests. is it to check that SetDisplayPower is called > > > > synchronously? if so, is there a simpler way you can check that, e.g. by > > just > > > > checking the power state before calling Wait()? > > > > > > ResumeDisplays() can result in a delayed configuration in multiple tests for > > > which the callback will be invoked asynchronously. We're not just trying to > > > verify that SetDisplayPower() is called, but rather the whole operation > > > completed as expected. > > > > > > So validate the correct delay AND the correct callback result (true/false). > > > > thanks for the detailed explanation! > > > > i understand why you're passing the callback to SetDisplayPower and verifying > > that it's called with the correct value. the thing that confuses me is just > why > > the callback is also passed to ResumeDisplays. the real code in > > PowerEventObserver passes DoNothing to ResumeDisplays, so it's not clear to me > > what's being tested by passing a callback to ResumeDisplays in the tests. > > > > sorry if i'm being dense. :-) > > Since ResumeDisplays() calls SetDisplayPower(), we're also interested in the > result of that in tests, especially if it will lead to a delayed configuration, > in which case we must verify the delay, and the result. why run a callback and check its result, though? the thing that you're interested in is what DisplayConfigurator actually did to the displays, right? can't you already get that from ActionLogger?
https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... ui/display/chromeos/display_configurator.cc:997: } On 2016/10/26 15:51:55, Daniel Erat wrote: > On 2016/10/25 20:14:10, afakhry wrote: > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > why not do this unconditionally? For example if you have a chromebox with 1 > > > monitor, the postponed configuration would also be interesting. > > > > +derat@ What do you think of this suggestion? > > I think the long delay here isn't very useful when we have a single monitor. > > it seems okay to me if it simplifies the code or makes it more robust. are there > any situations where a chromebox resumes with a single external display and > fails to find it, where we might benefit from scheduling this extra config? I just discussed with Oshima. We think that the delayed configuration only makes sense when we have 2+ monitors. The issue we're trying to solve is preventing us from detecting a single display only first (wrongly) then later detect other displays (windows will move to the wrong display and stay there). But when we have a single monitor, there's no problem detecting only one first, and connecting others later if something was connected during suspend.
On 2016/10/27 17:37:13, afakhry wrote: > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > File ui/display/chromeos/display_configurator.cc (right): > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > ui/display/chromeos/display_configurator.cc:997: } > On 2016/10/26 15:51:55, Daniel Erat wrote: > > On 2016/10/25 20:14:10, afakhry wrote: > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > why not do this unconditionally? For example if you have a chromebox with > 1 > > > > monitor, the postponed configuration would also be interesting. > > > > > > +derat@ What do you think of this suggestion? > > > I think the long delay here isn't very useful when we have a single monitor. > > > > it seems okay to me if it simplifies the code or makes it more robust. are > there > > any situations where a chromebox resumes with a single external display and > > fails to find it, where we might benefit from scheduling this extra config? > > I just discussed with Oshima. We think that the delayed configuration only makes > sense when we have 2+ monitors. The issue we're trying to solve is preventing us > from detecting a single display only first (wrongly) then later detect other > displays (windows will move to the wrong display and stay there). But when we > have a single monitor, there's no problem detecting only one first, and > connecting others later if something was connected during suspend. thanks, that sounds fine to me. (i'm still a bit confused about the callback, in case you didn't see my previous comment.)
On 2016/10/27 22:40:44, Daniel Erat wrote: > On 2016/10/27 17:37:13, afakhry wrote: > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > File ui/display/chromeos/display_configurator.cc (right): > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > ui/display/chromeos/display_configurator.cc:997: } > > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > On 2016/10/25 20:14:10, afakhry wrote: > > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > > why not do this unconditionally? For example if you have a chromebox > with > > 1 > > > > > monitor, the postponed configuration would also be interesting. > > > > > > > > +derat@ What do you think of this suggestion? > > > > I think the long delay here isn't very useful when we have a single > monitor. > > > > > > it seems okay to me if it simplifies the code or makes it more robust. are > > there > > > any situations where a chromebox resumes with a single external display and > > > fails to find it, where we might benefit from scheduling this extra config? > > > > I just discussed with Oshima. We think that the delayed configuration only > makes > > sense when we have 2+ monitors. The issue we're trying to solve is preventing > us > > from detecting a single display only first (wrongly) then later detect other > > displays (windows will move to the wrong display and stay there). But when we > > have a single monitor, there's no problem detecting only one first, and > > connecting others later if something was connected during suspend. > > thanks, that sounds fine to me. > > (i'm still a bit confused about the callback, in case you didn't see my previous > comment.) Sorry. Yes, I saw your comment and I was planning to discuss with you, but I got stuck yesterday on a P0 bug. The thing is ConfigurationWaiter::Wait() when I first wrote it was supposed to block until actually the configuration callback is called, hence the need was for passing the callback to ResumeDisplays(). Now that we simulate the waiting by invoking the timer immediately if it's running, we still rely on |pending_configuration_| to distinguish between whether the timer was not running because the configuration was done and the callback was invoked asynchronously (i.e. before we called Wait()), or it wasn't running because it's an actual error.
On 2016/10/28 16:51:01, afakhry wrote: > On 2016/10/27 22:40:44, Daniel Erat wrote: > > On 2016/10/27 17:37:13, afakhry wrote: > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > File ui/display/chromeos/display_configurator.cc (right): > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > ui/display/chromeos/display_configurator.cc:997: } > > > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > > On 2016/10/25 20:14:10, afakhry wrote: > > > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > > > why not do this unconditionally? For example if you have a chromebox > > with > > > 1 > > > > > > monitor, the postponed configuration would also be interesting. > > > > > > > > > > +derat@ What do you think of this suggestion? > > > > > I think the long delay here isn't very useful when we have a single > > monitor. > > > > > > > > it seems okay to me if it simplifies the code or makes it more robust. are > > > there > > > > any situations where a chromebox resumes with a single external display > and > > > > fails to find it, where we might benefit from scheduling this extra > config? > > > > > > I just discussed with Oshima. We think that the delayed configuration only > > makes > > > sense when we have 2+ monitors. The issue we're trying to solve is > preventing > > us > > > from detecting a single display only first (wrongly) then later detect other > > > displays (windows will move to the wrong display and stay there). But when > we > > > have a single monitor, there's no problem detecting only one first, and > > > connecting others later if something was connected during suspend. > > > > thanks, that sounds fine to me. > > > > (i'm still a bit confused about the callback, in case you didn't see my > previous > > comment.) > > Sorry. Yes, I saw your comment and I was planning to discuss with you, but I got > stuck yesterday on a P0 bug. > > The thing is ConfigurationWaiter::Wait() when I first wrote it was supposed to > block until actually the configuration > callback is called, hence the need was for passing the callback to > ResumeDisplays(). > > Now that we simulate the waiting by invoking the timer immediately if it's > running, we still rely on |pending_configuration_| to distinguish between > whether the timer was not running because the configuration was done and the > callback was invoked asynchronously (i.e. before we called Wait()), or it wasn't > running because it's an actual error. i'm still confused. in cases when you expect the configuration to be done synchronously, why can't you just check the actions before triggering the timeout? more specifically, in cases where the timer shouldn't be started: configurator_.DoSomethingToTriggerChange(); EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); EXPECT_FALSE(test_api_.TriggerConfigureTimeout()); in cases where we do expect it to be started: configurator_.DoSomethingToTriggerChange(); EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); // if we also expect synchronous changes EXPECT_EQ(..., test_api_->GetConfigureDelay()); EXPECT_TRUE(test_api_.TriggerConfigureTimeout()); EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); EXPECT_FALSE(test_api_.TriggerConfigureTimeout()); if there's additionally async config that gets posted without a timeout, throw in another: base::RunLoop().RunUntilIdle(); EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); at the appropriate place. does that work?
On 2016/10/28 17:11:14, Daniel Erat wrote: > On 2016/10/28 16:51:01, afakhry wrote: > > On 2016/10/27 22:40:44, Daniel Erat wrote: > > > On 2016/10/27 17:37:13, afakhry wrote: > > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > > File ui/display/chromeos/display_configurator.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > > ui/display/chromeos/display_configurator.cc:997: } > > > > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > > > On 2016/10/25 20:14:10, afakhry wrote: > > > > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > > > > why not do this unconditionally? For example if you have a chromebox > > > with > > > > 1 > > > > > > > monitor, the postponed configuration would also be interesting. > > > > > > > > > > > > +derat@ What do you think of this suggestion? > > > > > > I think the long delay here isn't very useful when we have a single > > > monitor. > > > > > > > > > > it seems okay to me if it simplifies the code or makes it more robust. > are > > > > there > > > > > any situations where a chromebox resumes with a single external display > > and > > > > > fails to find it, where we might benefit from scheduling this extra > > config? > > > > > > > > I just discussed with Oshima. We think that the delayed configuration only > > > makes > > > > sense when we have 2+ monitors. The issue we're trying to solve is > > preventing > > > us > > > > from detecting a single display only first (wrongly) then later detect > other > > > > displays (windows will move to the wrong display and stay there). But when > > we > > > > have a single monitor, there's no problem detecting only one first, and > > > > connecting others later if something was connected during suspend. > > > > > > thanks, that sounds fine to me. > > > > > > (i'm still a bit confused about the callback, in case you didn't see my > > previous > > > comment.) > > > > Sorry. Yes, I saw your comment and I was planning to discuss with you, but I > got > > stuck yesterday on a P0 bug. > > > > The thing is ConfigurationWaiter::Wait() when I first wrote it was supposed to > > block until actually the configuration > > callback is called, hence the need was for passing the callback to > > ResumeDisplays(). > > > > Now that we simulate the waiting by invoking the timer immediately if it's > > running, we still rely on |pending_configuration_| to distinguish between > > whether the timer was not running because the configuration was done and the > > callback was invoked asynchronously (i.e. before we called Wait()), or it > wasn't > > running because it's an actual error. > > i'm still confused. in cases when you expect the configuration to be done > synchronously, why can't you just check the actions before triggering the > timeout? > > more specifically, in cases where the timer shouldn't be started: > > configurator_.DoSomethingToTriggerChange(); > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); > EXPECT_FALSE(test_api_.TriggerConfigureTimeout()); > > in cases where we do expect it to be started: > > configurator_.DoSomethingToTriggerChange(); > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); // if we also expect > synchronous changes > EXPECT_EQ(..., test_api_->GetConfigureDelay()); > EXPECT_TRUE(test_api_.TriggerConfigureTimeout()); > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); > EXPECT_FALSE(test_api_.TriggerConfigureTimeout()); > > if there's additionally async config that gets posted without a timeout, throw > in another: > > base::RunLoop().RunUntilIdle(); > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); > > at the appropriate place. > > does that work? Yes, it does work. But so we're both on the same page, what is the objection to encapsulating all this logic inside the Waiter with the help of the callback? Is it to make the tests more verbose about what to expect? Or is it to avoid having the public API of ResumeDisplays() having a callback?
On 2016/10/28 18:18:32, afakhry wrote: > On 2016/10/28 17:11:14, Daniel Erat wrote: > > On 2016/10/28 16:51:01, afakhry wrote: > > > On 2016/10/27 22:40:44, Daniel Erat wrote: > > > > On 2016/10/27 17:37:13, afakhry wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > > > File ui/display/chromeos/display_configurator.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > > > ui/display/chromeos/display_configurator.cc:997: } > > > > > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > > > > On 2016/10/25 20:14:10, afakhry wrote: > > > > > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > > > > > why not do this unconditionally? For example if you have a > chromebox > > > > with > > > > > 1 > > > > > > > > monitor, the postponed configuration would also be interesting. > > > > > > > > > > > > > > +derat@ What do you think of this suggestion? > > > > > > > I think the long delay here isn't very useful when we have a single > > > > monitor. > > > > > > > > > > > > it seems okay to me if it simplifies the code or makes it more robust. > > are > > > > > there > > > > > > any situations where a chromebox resumes with a single external > display > > > and > > > > > > fails to find it, where we might benefit from scheduling this extra > > > config? > > > > > > > > > > I just discussed with Oshima. We think that the delayed configuration > only > > > > makes > > > > > sense when we have 2+ monitors. The issue we're trying to solve is > > > preventing > > > > us > > > > > from detecting a single display only first (wrongly) then later detect > > other > > > > > displays (windows will move to the wrong display and stay there). But > when > > > we > > > > > have a single monitor, there's no problem detecting only one first, and > > > > > connecting others later if something was connected during suspend. > > > > > > > > thanks, that sounds fine to me. > > > > > > > > (i'm still a bit confused about the callback, in case you didn't see my > > > previous > > > > comment.) > > > > > > Sorry. Yes, I saw your comment and I was planning to discuss with you, but I > > got > > > stuck yesterday on a P0 bug. > > > > > > The thing is ConfigurationWaiter::Wait() when I first wrote it was supposed > to > > > block until actually the configuration > > > callback is called, hence the need was for passing the callback to > > > ResumeDisplays(). > > > > > > Now that we simulate the waiting by invoking the timer immediately if it's > > > running, we still rely on |pending_configuration_| to distinguish between > > > whether the timer was not running because the configuration was done and the > > > callback was invoked asynchronously (i.e. before we called Wait()), or it > > wasn't > > > running because it's an actual error. > > > > i'm still confused. in cases when you expect the configuration to be done > > synchronously, why can't you just check the actions before triggering the > > timeout? > > > > more specifically, in cases where the timer shouldn't be started: > > > > configurator_.DoSomethingToTriggerChange(); > > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); > > EXPECT_FALSE(test_api_.TriggerConfigureTimeout()); > > > > in cases where we do expect it to be started: > > > > configurator_.DoSomethingToTriggerChange(); > > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); // if we also expect > > synchronous changes > > EXPECT_EQ(..., test_api_->GetConfigureDelay()); > > EXPECT_TRUE(test_api_.TriggerConfigureTimeout()); > > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); > > EXPECT_FALSE(test_api_.TriggerConfigureTimeout()); > > > > if there's additionally async config that gets posted without a timeout, throw > > in another: > > > > base::RunLoop().RunUntilIdle(); > > EXPECT_EQ(JoinActions(...), log_->GetActionsAndClear()); > > > > at the appropriate place. > > > > does that work? > > Yes, it does work. But so we're both on the same page, what is the objection to > encapsulating all this logic inside the Waiter with the help of the callback? > Is it to make the tests more verbose about what to expect? Or is it to avoid > having the public API of ResumeDisplays() having a callback? my objection is around having the test assert that a callback that doesn't exist in production was called. it's asserting a side effect that exists solely for the sake of testing instead of asserting that the display was configured in the expected manner. if the code breaks such that it runs the callback but doesn't actually configure the display, or configures the display in the wrong way, the test will continue passing. it seems like we can avoid that by instead checking the config.
Patchset #8 (id:140001) has been deleted
Removed the callback and updated the tests.
https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator.cc:933: // the delayed configuration after waking up from suspend. i don't understand this condition or comment. the timer appears to only be started within this class with kConfigureDelayMs (500) or kResumeConfigureMultiDisplayDelayMs (2000). this condition starts the timer here with its current delay (which i think would always be 500), and then the 'else' block also starts the timer with 500. it seems like this if/else block can just be replaced with a single call to Start() using 500. or did you mean '>', so we'll preserve already-set longer delays? https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator.cc:990: // before configuration is performed. nit: also mention why this is done at the end of the comment: "... so we won't immediately resize the desktops and the windows on it to fit on a single display." https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:44: DisplayConfigurator::kConfigureDelayMs / 3); i don't understand why you need this now that you aren't actually waiting. if you're trying to detect the immediate-async-configuration case, shouldn't you just check equality with base::TimeDelta()? https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:48: constexpr base::TimeDelta kMinLongDelayMs = base::TimeDelta::FromMilliseconds( nit: rename to kLongDelayMs https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:167: // timer task to complete. this comment seems a bit inaccurate: it doesn't always trigger the configuration timer, right? if there's an async config scheduled, it returns base::TimeDelta(). i'm wondering if something like this would be better: // Simulates waiting for the next configuration. If an async task // is pending, runs it and returns base::TimeDelta(). Otherwise, // triggers the configuration timer and returns its delay. If the // timer wasn't running, returns base::TimeDelta::Max(). https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:181: const { nit: move this accessor and the one above it below the d'tor https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:206: bool pending_configuration_; this member seems redundant: can't you just check callback_result_ == CALLBACK_NOT_CALLED instead? https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:2089: // after a long delay. Afterwards,we should still expect to be in a dual nit: add space after ',' https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:2159: // Suspend. nit: insert blank line before this one
https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator.cc (right): https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator.cc:933: // the delayed configuration after waking up from suspend. On 2016/10/31 16:20:01, Daniel Erat wrote: > i don't understand this condition or comment. the timer appears to only be > started within this class with kConfigureDelayMs (500) or > kResumeConfigureMultiDisplayDelayMs (2000). this condition starts the timer here > with its current delay (which i think would always be 500), and then the 'else' > block also starts the timer with 500. it seems like this if/else block can just > be replaced with a single call to Start() using 500. > > or did you mean '>', so we'll preserve already-set longer delays? Your observation here is correct. We don't need this condition. No, I didn't want to preserve the longer delays, rather, I didn't want to make them even longer by resetting (e.g. when we have been waiting for 1.5s [out of 2s] and OnConfigurationChanged() event is received, I didn't want to reset the timer back to 2s [to avoid a long delay of 3.5s]). https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator.cc:990: // before configuration is performed. On 2016/10/31 16:20:02, Daniel Erat wrote: > nit: also mention why this is done at the end of the comment: "... so we won't > immediately resize the desktops and the windows on it to fit on a single > display." Done. https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:44: DisplayConfigurator::kConfigureDelayMs / 3); On 2016/10/31 16:20:02, Daniel Erat wrote: > i don't understand why you need this now that you aren't actually waiting. if > you're trying to detect the immediate-async-configuration case, shouldn't you > just check equality with base::TimeDelta()? Done. Renamed it to kNoDelay, and changed the tests to check for equality. https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:48: constexpr base::TimeDelta kMinLongDelayMs = base::TimeDelta::FromMilliseconds( On 2016/10/31 16:20:02, Daniel Erat wrote: > nit: rename to kLongDelayMs Done. https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:167: // timer task to complete. On 2016/10/31 16:20:02, Daniel Erat wrote: > this comment seems a bit inaccurate: it doesn't always trigger the configuration > timer, right? if there's an async config scheduled, it returns > base::TimeDelta(). i'm wondering if something like this would be better: > > // Simulates waiting for the next configuration. If an async task > // is pending, runs it and returns base::TimeDelta(). Otherwise, > // triggers the configuration timer and returns its delay. If the > // timer wasn't running, returns base::TimeDelta::Max(). Yes, that's a much more accurate comment! :) https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:181: const { On 2016/10/31 16:20:02, Daniel Erat wrote: > nit: move this accessor and the one above it below the d'tor Done. https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:206: bool pending_configuration_; On 2016/10/31 16:20:02, Daniel Erat wrote: > this member seems redundant: can't you just check callback_result_ == > CALLBACK_NOT_CALLED instead? Done. https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:2089: // after a long delay. Afterwards,we should still expect to be in a dual On 2016/10/31 16:20:02, Daniel Erat wrote: > nit: add space after ',' Done. https://codereview.chromium.org/2427843002/diff/160001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:2159: // Suspend. On 2016/10/31 16:20:02, Daniel Erat wrote: > nit: insert blank line before this one Done.
thanks! lgtm with a few comments https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:47: constexpr base::TimeDelta kLongDelayMs = base::TimeDelta::FromMilliseconds( nit: rename to 'kLongDelay' (sorry, i gave a bad suggestion last round :-P) https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:186: void OnConfigured(bool status) { is it possible to CHECK_EQ(callback_result_, CALLBACK_NOT_CALLED) here to make sure that the callback isn't invoked twice without Wait() being called in between? if that's not possible to do, feel free to ignore this. https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:192: // waiter's OnConfigured(). nit: maybe simplify to "Passed with configuration requests to run OnConfigured()." https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:718: config_waiter_.on_configuration_callback()); i'm not sure if it applies to this particular test, but if it's not expected for this (or some other configures) to happen synchronously, it might be good to add EXPECT_EQ(CALLBACK_NOT_CALLED, config_waiter_.callback()) checks before the calls to Wait(). that's more copy-and-paste though so feel free to ignore this if it doesn't seem like it's something we care about. :-)
https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:47: constexpr base::TimeDelta kLongDelayMs = base::TimeDelta::FromMilliseconds( On 2016/10/31 18:25:46, Daniel Erat wrote: > nit: rename to 'kLongDelay' (sorry, i gave a bad suggestion last round :-P) Done. https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:186: void OnConfigured(bool status) { On 2016/10/31 18:25:46, Daniel Erat wrote: > is it possible to CHECK_EQ(callback_result_, CALLBACK_NOT_CALLED) here to make > sure that the callback isn't invoked twice without Wait() being called in > between? if that's not possible to do, feel free to ignore this. Done. https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:192: // waiter's OnConfigured(). On 2016/10/31 18:25:46, Daniel Erat wrote: > nit: maybe simplify to "Passed with configuration requests to run > OnConfigured()." Done. https://codereview.chromium.org/2427843002/diff/180001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:718: config_waiter_.on_configuration_callback()); On 2016/10/31 18:25:46, Daniel Erat wrote: > i'm not sure if it applies to this particular test, but if it's not expected for > this (or some other configures) to happen synchronously, it might be good to add > EXPECT_EQ(CALLBACK_NOT_CALLED, config_waiter_.callback()) checks before the > calls to Wait(). that's more copy-and-paste though so feel free to ignore this > if it doesn't seem like it's something we care about. :-) We have a similar check in line 1780 of this file: EXPECT_EQ(CALLBACK_NOT_CALLED, config_waiter_.callback_result()); I added a few more where it made sense.
https://codereview.chromium.org/2427843002/diff/200001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/200001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:1014: EXPECT_EQ(CALLBACK_NOT_CALLED, config_waiter_.callback_result()); nit: it doesn't look like you passed the callback between the last call to Reset() and this point, so this is probably unnecessary. https://codereview.chromium.org/2427843002/diff/200001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:1810: base::RunLoop().RunUntilIdle(); what's the reason for calling RunUntilIdle() directly here instead of config_waiter_.Wait()?
On 2016/10/26 15:51:55, Daniel Erat wrote: > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > File ui/display/chromeos/display_configurator.cc (right): > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > ui/display/chromeos/display_configurator.cc:997: } > On 2016/10/25 20:14:10, afakhry wrote: > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > why not do this unconditionally? For example if you have a chromebox with 1 > > > monitor, the postponed configuration would also be interesting. > > > > +derat@ What do you think of this suggestion? > > I think the long delay here isn't very useful when we have a single monitor. > > it seems okay to me if it simplifies the code or makes it more robust. are there > any situations where a chromebox resumes with a single external display and > fails to find it, where we might benefit from scheduling this extra config? Yes, if you use only one monitor, on some chromeboxes, we will get a hotplug event on resume to put the monitor back. We would avoid an issue here if that event takes too long.
On 2016/10/26 15:51:55, Daniel Erat wrote: > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > File ui/display/chromeos/display_configurator.cc (right): > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > ui/display/chromeos/display_configurator.cc:997: } > On 2016/10/25 20:14:10, afakhry wrote: > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > why not do this unconditionally? For example if you have a chromebox with 1 > > > monitor, the postponed configuration would also be interesting. > > > > +derat@ What do you think of this suggestion? > > I think the long delay here isn't very useful when we have a single monitor. > > it seems okay to me if it simplifies the code or makes it more robust. are there > any situations where a chromebox resumes with a single external display and > fails to find it, where we might benefit from scheduling this extra config? Yes, if you use only one monitor, on some chromeboxes, we will get a hotplug event on resume to put the monitor back. We would avoid an issue here if that event takes too long.
On 2016/10/31 22:07:00, marcheu wrote: > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > File ui/display/chromeos/display_configurator.cc (right): > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > ui/display/chromeos/display_configurator.cc:997: } > > On 2016/10/25 20:14:10, afakhry wrote: > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > why not do this unconditionally? For example if you have a chromebox with > 1 > > > > monitor, the postponed configuration would also be interesting. > > > > > > +derat@ What do you think of this suggestion? > > > I think the long delay here isn't very useful when we have a single monitor. > > > > it seems okay to me if it simplifies the code or makes it more robust. are > there > > any situations where a chromebox resumes with a single external display and > > fails to find it, where we might benefit from scheduling this extra config? > > Yes, if you use only one monitor, on some chromeboxes, we will get a hotplug > event on resume to put the monitor back. We would avoid an issue here if that > event takes too long. I'm sorry, I don't understand what that issue we want to avoid is in the case of a single monitor. In 2+ displays, the issue is clear, we want to avoid moving windows around. But in a single display what's the bad side effect of detecting a single display too early?
https://codereview.chromium.org/2427843002/diff/200001/ui/display/chromeos/di... File ui/display/chromeos/display_configurator_unittest.cc (right): https://codereview.chromium.org/2427843002/diff/200001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:1014: EXPECT_EQ(CALLBACK_NOT_CALLED, config_waiter_.callback_result()); On 2016/10/31 21:27:54, Daniel Erat wrote: > nit: it doesn't look like you passed the callback between the last call to > Reset() and this point, so this is probably unnecessary. Done. https://codereview.chromium.org/2427843002/diff/200001/ui/display/chromeos/di... ui/display/chromeos/display_configurator_unittest.cc:1810: base::RunLoop().RunUntilIdle(); On 2016/10/31 21:27:54, Daniel Erat wrote: > what's the reason for calling RunUntilIdle() directly here instead of > config_waiter_.Wait()? We can still use Wait() with the same result. base::RunLoop().RunUntilIdle() seemed to clearly show that this is an async task that will invoke the OnConfigured() callback, and that the confgig timer is not involved. For consistency, I returned back to Wait().
On 2016/11/01 00:21:10, afakhry wrote: > On 2016/10/31 22:07:00, marcheu wrote: > > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > File ui/display/chromeos/display_configurator.cc (right): > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > ui/display/chromeos/display_configurator.cc:997: } > > > On 2016/10/25 20:14:10, afakhry wrote: > > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > > why not do this unconditionally? For example if you have a chromebox > with > > 1 > > > > > monitor, the postponed configuration would also be interesting. > > > > > > > > +derat@ What do you think of this suggestion? > > > > I think the long delay here isn't very useful when we have a single > monitor. > > > > > > it seems okay to me if it simplifies the code or makes it more robust. are > > there > > > any situations where a chromebox resumes with a single external display and > > > fails to find it, where we might benefit from scheduling this extra config? > > > > Yes, if you use only one monitor, on some chromeboxes, we will get a hotplug > > event on resume to put the monitor back. We would avoid an issue here if that > > event takes too long. > > I'm sorry, I don't understand what that issue we want to avoid is in the case of > a single monitor. In 2+ displays, the issue is clear, we want to avoid moving > windows around. But in a single display what's the bad side effect of detecting > a single display too early? you're going to detect the lack of display, turn the display off, then turn it back on. So resume time could look better in that case. But yes, it has nothing to do with moving windows.
lgtm
On 2016/11/01 02:55:38, marcheu1 wrote: > On 2016/11/01 00:21:10, afakhry wrote: > > On 2016/10/31 22:07:00, marcheu wrote: > > > On 2016/10/26 15:51:55, Daniel Erat wrote: > > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > > File ui/display/chromeos/display_configurator.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2427843002/diff/80001/ui/display/chromeos/dis... > > > > ui/display/chromeos/display_configurator.cc:997: } > > > > On 2016/10/25 20:14:10, afakhry wrote: > > > > > On 2016/10/25 00:41:11, marcheu1 wrote: > > > > > > why not do this unconditionally? For example if you have a chromebox > > with > > > 1 > > > > > > monitor, the postponed configuration would also be interesting. > > > > > > > > > > +derat@ What do you think of this suggestion? > > > > > I think the long delay here isn't very useful when we have a single > > monitor. > > > > > > > > it seems okay to me if it simplifies the code or makes it more robust. are > > > there > > > > any situations where a chromebox resumes with a single external display > and > > > > fails to find it, where we might benefit from scheduling this extra > config? > > > > > > Yes, if you use only one monitor, on some chromeboxes, we will get a hotplug > > > event on resume to put the monitor back. We would avoid an issue here if > that > > > event takes too long. > > > > I'm sorry, I don't understand what that issue we want to avoid is in the case > of > > a single monitor. In 2+ displays, the issue is clear, we want to avoid moving > > windows around. But in a single display what's the bad side effect of > detecting > > a single display too early? > > you're going to detect the lack of display, turn the display off, then turn it > back on. So resume time could look better in that case. But yes, it has nothing > to do with moving windows. Chrome/Ash ignores zero display state, so this should be fine.
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Delay display configuration after waking up from suspend with multi displays Perfrom display configuration after a delay when waking up from suspend in multi display mode, for all displays to have a chance to be detected and added, before the first configuration. BUG=614624 TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.* TEST=connect an external monitor, have several windows open in both displays, press Search+Shift+L to suspend the device, wait for a few seconds until the device is fully suspeneded, wake up by pressing any key. The windows in both displays should be exactly where you left them before suspend. ========== to ========== Delay display configuration after waking up from suspend with multi displays Perfrom display configuration after a delay when waking up from suspend in multi display mode, for all displays to have a chance to be detected and added, before the first configuration. BUG=614624 TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.* TEST=connect an external monitor, have several windows open in both displays, press Search+Shift+L to suspend the device, wait for a few seconds until the device is fully suspeneded, wake up by pressing any key. The windows in both displays should be exactly where you left them before suspend. ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Delay display configuration after waking up from suspend with multi displays Perfrom display configuration after a delay when waking up from suspend in multi display mode, for all displays to have a chance to be detected and added, before the first configuration. BUG=614624 TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.* TEST=connect an external monitor, have several windows open in both displays, press Search+Shift+L to suspend the device, wait for a few seconds until the device is fully suspeneded, wake up by pressing any key. The windows in both displays should be exactly where you left them before suspend. ========== to ========== Delay display configuration after waking up from suspend with multi displays Perfrom display configuration after a delay when waking up from suspend in multi display mode, for all displays to have a chance to be detected and added, before the first configuration. BUG=614624 TEST=display_unittests --gtest_filter=DisplayConfiguratorTest.* TEST=connect an external monitor, have several windows open in both displays, press Search+Shift+L to suspend the device, wait for a few seconds until the device is fully suspeneded, wake up by pressing any key. The windows in both displays should be exactly where you left them before suspend. Committed: https://crrev.com/343040c3e082e53591a4d7d822cefc70ba900c82 Cr-Commit-Position: refs/heads/master@{#429070} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/343040c3e082e53591a4d7d822cefc70ba900c82 Cr-Commit-Position: refs/heads/master@{#429070} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
