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

Commit

Permalink
Revert of Force calling PostDisplayConfigurationChange() even if eary…
Browse files Browse the repository at this point in the history
…ling 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}
  • Loading branch information
engedy authored and Commit bot committed Sep 12, 2014
1 parent c328fa4 commit 7f7b757
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 18 deletions.
3 changes: 1 addition & 2 deletions ash/display/display_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,7 @@ TEST_F(DisplayControllerTest, BoundsUpdated) {

// No change
UpdateDisplay("400x500*2,300x300");
// We still call into Pre/PostDisplayConfigurationChange().
EXPECT_EQ(1, observer.CountAndReset());
EXPECT_EQ(0, observer.CountAndReset());
EXPECT_EQ(0, observer.GetFocusChangedCountAndReset());
EXPECT_EQ(0, observer.GetActivationChangedCountAndReset());

Expand Down
23 changes: 7 additions & 16 deletions ash/display/display_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,13 @@ void DisplayManager::UpdateDisplays(
scoped_ptr<NonDesktopDisplayUpdater> non_desktop_display_updater(
new NonDesktopDisplayUpdater(this, delegate_));

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

// Do not update |displays_| if there's nothing to be updated. Without this,
// it will not update the display layout, which causes the bug
// http://crbug.com/155948.
if (display_changes.empty() && added_display_indices.empty() &&
removed_displays.empty()) {
// When changing from software mirroring mode to sinlge display mode, it
// is possible there is no need to update |displays_| and we early out
// here. 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.
if (delegate_)
delegate_->PostDisplayConfigurationChange();
return;
}

size_t updated_index;
if (UpdateSecondaryDisplayBoundsForLayout(&new_displays, &updated_index) &&
std::find(added_display_indices.begin(),
Expand Down

0 comments on commit 7f7b757

Please sign in to comment.