New issue
Advanced search Search tips

Issue 831379 link

Starred by 1 user
Status: Fixed
Owner:
steve...@chromium.org
Closed: Apr 2018
Cc:
derat@chromium.org
xiy...@chromium.org
sammiequon@chromium.org
afakhry@chromium.org
jonr...@chromium.org
jamescook@chromium.org
osh...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 678949


Sign in to add a comment

Move DisplayConfigurationObserver from chrome to ash

Project Member Reported by steve...@chromium.org, Apr 10 2018 
Separating this out from  issue 678949 

When attempting to move DisplayConfigurationObserver to src/ash in https://chromium-review.googlesource.com/c/chromium/src/+/1003379, several unit tests break.

The root cause for the breakage is this logic in DisplayConfigurationObserver::OnTabletModeStarted():

  display_manager->SetMirrorMode(display::MirrorMode::kNormal, base::nullopt);
  display_manager->layout_store()->set_forced_mirror_mode(true);

This results in a single mirrored display instead of 2 displays, breaking test assumptions.

We can either:
a) Allow tests to configure Shell to not initialize DisplayConfigurationObserver
b) Modify the test expectations

a) is simpler but b) seems more robust.

Thoughts?

Comment 1 by steve...@chromium.org, Apr 10 2018

Cc: derat@chromium.org xiy...@chromium.org afakhry@chromium.org

Comment 2 by steve...@chromium.org, Apr 10 2018 

Test failures:
2 tests failed:
    OverviewButtonTrayTest.SecondaryTrayCreatedVisible (../../ash/system/overview/overview_button_tray_unittest.cc:203)
    TabletModeControllerTest.DisplayDisconnectionDuringOverview (../../ash/wm/tablet_mode/tablet_mode_controller_unittest.cc:502)
4 tests crashed:
    OverviewButtonTrayTest.DisplaysOnBothDisplays (../../ash/system/overview/overview_button_tray_unittest.cc:191)
    SplitViewWindowSelectorTest.SplitViewDragIndicatorsWidgetReparenting (../../ash/wm/overview/window_selector_unittest.cc:3601)
    TabletModeControllerTest.NoTabletModeWithDisabledInternalDisplay (../../ash/wm/tablet_mode/tablet_mode_controller_unittest.cc:520)
    TrayRotationLockTest.CreateSecondaryTrayView (../../ash/system/rotation/tray_rotation_lock_unittest.cc:140)

Comment 3 by steve...@chromium.org, Apr 10 2018

Cc: sammiequon@chromium.org

Comment 4 by jonr...@chromium.org, Apr 11 2018 

This is also complicated by issue 733092 where they are discussion how to move forward on tablet mode + docking.

In the interim I would support b) updating the test expectations. It's more robust, and only leaves us testing the actual supported behaviour

Comment 5 by osh...@chromium.org, Apr 12 2018 

yes b) is right thing in short term.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 13 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c

commit 9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri Apr 13 23:45:55 2018

Move DisplayConfigurationObserver and display_prefs_unittest to src/ash

This CL:
* Fixes a potential bug in DisplayConfigurationObserver where displays
  were removed from an observer method, impacting other observers.
* Fixes a handful of tests that break when DisplayConfigurationObserver
  triggers mirror mode when switching to tablet mode.

Bug:  831379 
Change-Id: I3606d31d3c9b258864064f74170107df8278b0ad
Reviewed-on: https://chromium-review.googlesource.com/1006226
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550814}
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/BUILD.gn
[add] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/display/display_configuration_observer.cc
[add] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/display/display_configuration_observer.h
[rename] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/display/display_prefs_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/shell.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/shell.h
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/system/rotation/tray_rotation_lock_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/wm/tablet_mode/tablet_mode_controller_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/chromeos/BUILD.gn
[delete] https://crrev.com/59b0df2d8080a3b42b12a34ca385ec3a92368e88/chrome/browser/chromeos/display/display_configuration_observer.cc
[delete] https://crrev.com/59b0df2d8080a3b42b12a34ca385ec3a92368e88/chrome/browser/chromeos/display/display_configuration_observer.h
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/ui/ash/chrome_shell_delegate.h

Comment 7 by afakhry@chromium.org, Apr 14 2018

Summary: Move DisplayConfigurationObserver from chrome to ash (was: Mive DisplayConfigurationObserver from chrome to ash)
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c

commit 9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Fri Apr 13 23:45:55 2018

Move DisplayConfigurationObserver and display_prefs_unittest to src/ash

This CL:
* Fixes a potential bug in DisplayConfigurationObserver where displays
  were removed from an observer method, impacting other observers.
* Fixes a handful of tests that break when DisplayConfigurationObserver
  triggers mirror mode when switching to tablet mode.

Bug:  831379 
Change-Id: I3606d31d3c9b258864064f74170107df8278b0ad
Reviewed-on: https://chromium-review.googlesource.com/1006226
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550814}
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/BUILD.gn
[add] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/display/display_configuration_observer.cc
[add] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/display/display_configuration_observer.h
[rename] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/display/display_prefs_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/shell.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/shell.h
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/system/rotation/tray_rotation_lock_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/wm/overview/window_selector_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/ash/wm/tablet_mode/tablet_mode_controller_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/chromeos/BUILD.gn
[delete] https://crrev.com/59b0df2d8080a3b42b12a34ca385ec3a92368e88/chrome/browser/chromeos/display/display_configuration_observer.cc
[delete] https://crrev.com/59b0df2d8080a3b42b12a34ca385ec3a92368e88/chrome/browser/chromeos/display/display_configuration_observer.h
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/extensions/system_display/display_info_provider_chromeos_unittest.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/9b21fa3b3022cf5e4bd0317438a9cfd77b186c5c/chrome/browser/ui/ash/chrome_shell_delegate.h

Comment 9 by steve...@chromium.org, Apr 23 2018

Status: Fixed (was: Started)
►

Sign in to add a comment