Skip to content
This repository was archived by the owner on Apr 3, 2020. It is now read-only.

Commit 7f7b757

Browse files
engedyCommit bot
engedy
authored and
Commit bot
committed
Revert of Force calling PostDisplayConfigurationChange() even if earyling out in UpdateDisplays (patchset #4 id:60001 of https://codereview.chromium.org/559213002/)
Reason for revert: I am reverting this as it resulted in test VirtualKeyboardUsabilityExperimentTest.VirtualKeyboardWindowTest consistently crashing on Linux ChromiumOS Tests, see: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/31731 http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/builds/2914 Stack trace: Objects involved in the operation: sequence "this" @ 0x0x3e568fbdcc90 { } Received signal 6 #0 0x7fcb9c8936fe base::debug::StackTrace::StackTrace() #1 0x7fcb9c893230 base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7fcb8c474cb0 \u003Cunknown> #3 0x7fcb8bbc4425 gsignal #4 0x7fcb8bbc7b8b abort #5 0x7fcb8c1c75ad __gnu_debug::_Error_formatter::_M_error() #6 0x7fcb9a306513 std::__debug::vector\u003C>::operator[]() #7 0x7fcb9a2fbba9 ash::DisplayManager::GetCurrentDisplayIdPair() #8 0x7fcb9a2d96cf ash::DisplayController::PostDisplayConfigurationChange() #9 0x7fcb9a2d98bc ash::DisplayController::PostDisplayConfigurationChange() #10 0x7fcb9a2fd22a ash::DisplayManager::UpdateDisplays() #11 0x7fcb9a2fdbe8 ash::DisplayManager::SetDisplayRotation() #12 0x7fcb9a33425b ash::VirtualKeyboardWindowController::FlipDisplay() #13 0x7fcb9a3340d7 ash::VirtualKeyboardWindowController::UpdateWindow() #14 0x7fcb9a2d92df ash::DisplayController::CreateOrUpdateNonDesktopDisplay() #15 0x7fcb9a2d937f ash::DisplayController::CreateOrUpdateNonDesktopDisplay() #16 0x7fcb9a300ea9 ash::(anonymous namespace)::NonDesktopDisplayUpdater::~NonDesktopDisplayUpdater() #17 0x7fcb9a300df1 ash::DisplayManager::CreateMirrorWindowIfAny() Original issue's description: > Force calling PostDisplayConfigurationChange() even if earyling out in UpdateDisplays > > When changing from software mirroring mode to sinlge display mode, it > is possible there is no need to update |displays_| and we early out > UpdateDisplays(). But we still want to run the PostDisplayConfigurationChange() > cause there are some clients need to act on this, e.g. > TouchTransformerController needs to adjust the TouchTransformer when > switching from dual displays to single display. > > BUG=chrome-os-partner:31868 > TEST=tested on Big, after existing software mirroring mode, the touch location > transformation is still correct. > > Committed: https://crrev.com/4802a8552a40e1f80606ca7171dc2f79930e7fb3 > Cr-Commit-Position: refs/heads/master@{#294481} [email protected],[email protected] NOTREECHECKS=true NOTRY=true BUG=chrome-os-partner:31868 Review URL: https://codereview.chromium.org/569553002 Cr-Commit-Position: refs/heads/master@{#294553}
1 parent c328fa4 commit 7f7b757

File tree

2 files changed

+8
-18
lines changed

2 files changed

+8
-18
lines changed

ash/display/display_controller_unittest.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -655,8 +655,7 @@ TEST_F(DisplayControllerTest, BoundsUpdated) {
655655

656656
// No change
657657
UpdateDisplay("400x500*2,300x300");
658-
// We still call into Pre/PostDisplayConfigurationChange().
659-
EXPECT_EQ(1, observer.CountAndReset());
658+
EXPECT_EQ(0, observer.CountAndReset());
660659
EXPECT_EQ(0, observer.GetFocusChangedCountAndReset());
661660
EXPECT_EQ(0, observer.GetActivationChangedCountAndReset());
662661

ash/display/display_manager.cc

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,13 @@ void DisplayManager::UpdateDisplays(
884884
scoped_ptr<NonDesktopDisplayUpdater> non_desktop_display_updater(
885885
new NonDesktopDisplayUpdater(this, delegate_));
886886

887+
// Do not update |displays_| if there's nothing to be updated. Without this,
888+
// it will not update the display layout, which causes the bug
889+
// http://crbug.com/155948.
890+
if (display_changes.empty() && added_display_indices.empty() &&
891+
removed_displays.empty()) {
892+
return;
893+
}
887894
// Clear focus if the display has been removed, but don't clear focus if
888895
// the destkop has been moved from one display to another
889896
// (mirror -> docked, docked -> single internal).
@@ -893,22 +900,6 @@ void DisplayManager::UpdateDisplays(
893900
if (delegate_)
894901
delegate_->PreDisplayConfigurationChange(clear_focus);
895902

896-
// Do not update |displays_| if there's nothing to be updated. Without this,
897-
// it will not update the display layout, which causes the bug
898-
// http://crbug.com/155948.
899-
if (display_changes.empty() && added_display_indices.empty() &&
900-
removed_displays.empty()) {
901-
// When changing from software mirroring mode to sinlge display mode, it
902-
// is possible there is no need to update |displays_| and we early out
903-
// here. But we still want to run the PostDisplayConfigurationChange()
904-
// cause there are some clients need to act on this, e.g.
905-
// TouchTransformerController needs to adjust the TouchTransformer when
906-
// switching from dual displays to single display.
907-
if (delegate_)
908-
delegate_->PostDisplayConfigurationChange();
909-
return;
910-
}
911-
912903
size_t updated_index;
913904
if (UpdateSecondaryDisplayBoundsForLayout(&new_displays, &updated_index) &&
914905
std::find(added_display_indices.begin(),

0 commit comments

Comments
 (0)