Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885153004:
React to primary display change instead of removal (Closed)
|
Created:
3 years, 7 months ago by Felix Ekblom Modified:
3 years, 7 months ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid changing primary display in DisplayObserver call
BUG=668449, 722564
Review-Url: https://codereview.chromium.org/2885153004
Cr-Commit-Position: refs/heads/master@{#474196}
Committed: https://chromium.googlesource.com/chromium/src/+/491c6b2fc5e9d5fd2b7eada34026ff977797b493
Patch Set 1 : React to primary display change instead of removal #Patch Set 2 : Avoid primary display changes in DisplayObserver calls #Patch Set 3 : Workaround DisplayConfigurationController display switch limiter #
Total comments: 5
Patch Set 4 : Remove empty OnDisplayRemvoed + remvoe unnecessary "requested" state #Patch Set 5 : Add browser test to verify that the change caught the issue #
Total comments: 2
Patch Set 6 : Always attempt exit, simplifying tear down code originating from OobeBaseTest #
Total comments: 2
Patch Set 7 : Remove the probably unnecessary safe guards for test shutdown #
Total comments: 9
Patch Set 8 : Minor fixes and documentation changes #
Messages
Total messages: 44 (24 generated)
Description was changed from ========== React to primary display change instead of removal This ensures that the search for a new primary touch display is only started when the current primary display is disconnected instead of when any display is disconnected. BUG=668449 ========== to ========== Avoid changing primary display in DisplayObserver call BUG=668449 ==========
felixe@chromium.org changed reviewers: + alemate@chromium.org, jdufault@chromium.org, oshima@chromium.org
Follow up review addressing some intermittent problems. Patch Set 1 fixes an issue in which a CHECK would trigger sometimes in WindowTreeHostManager::GetPrimaryDisplayId(). Patch Set 2 ensures that we don't change primary display in the middle of a DisplayObserver notification. Patch Set 3 works around an issue in which the animation limiter in DisplayConfigurationController would block the primary display switch.
The CQ bit was checked by felixe@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/2885153004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:984: const display::Display& old_display) {} you can remove this https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:35: if (display_switch_requested_) Instead, can you check if the weak_ptr_factory_ has a weak ptr? https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:84: EXPECT_EQ(ids[0], GetPrimaryDisplay()); can you add another test case where the primary display is removed? First swap the primary display, then UpdateDisplay() with one display.
https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:84: EXPECT_EQ(ids[0], GetPrimaryDisplay()); On 2017/05/17 16:56:18, oshima wrote: > can you add another test case where the primary display is removed? > First swap the primary display, then UpdateDisplay() with one display. I'm not sure I understand what this would test. In the test environment there's no connection between DisplayObservers and OobeDisplayChooser as that's performed by other classes in the real setup. OobeDisplayChooser will not react to the removal of a display. Is it possible to describe the state (DisplayManager, WindowTreeHostManager, etc) to use for such a test statically? Or do you want me to introduce a DisplayObserver and mimic the interaction taking place in the real setup?
On 2017/05/18 11:12:04, felixe1 wrote: > Or do you want me to introduce a > DisplayObserver and mimic the interaction taking place in the real setup? I've experimented some now with more dynamic unit tests as well as browser tests (based on OobeBaseTest) but I've not been able to trigger the original bug in any automated test environment. The conditions needed to trigger the bug is not there in the test setup. While I do think that it would be interesting to find out why our test environments does not show the same behavior as the real setup I would prefer if we separated such an investigation task from this CL, both so that we can land this code but also so that someone with more domain knowledge in the area can look at the investigation in isolation from this task.
https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc (right): https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:84: EXPECT_EQ(ids[0], GetPrimaryDisplay()); On 2017/05/18 11:12:04, felixe1 wrote: > On 2017/05/17 16:56:18, oshima wrote: > > can you add another test case where the primary display is removed? > > First swap the primary display, then UpdateDisplay() with one display. > > I'm not sure I understand what this would test. > > In the test environment there's no connection between DisplayObservers and > OobeDisplayChooser as that's performed by other classes in the real setup. > OobeDisplayChooser will not react to the removal of a display. > > Is it possible to describe the state (DisplayManager, WindowTreeHostManager, > etc) to use for such a test statically? Or do you want me to introduce a > DisplayObserver and mimic the interaction taking place in the real setup? I've added a test now that trigger the bad behavior on ToT but passes with the code in this CL. It had to be a browser test and not a unit test because for some reason the display removal in the unit tests does not trigger the bad state.
The CQ bit was checked by felixe@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2885153004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc (right): https://codereview.chromium.org/2885153004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc:35: if (LoginDisplayHost::default_host()) { Shouldn't this be consistent every test run?
https://codereview.chromium.org/2885153004/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc (right): https://codereview.chromium.org/2885153004/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc:35: if (LoginDisplayHost::default_host()) { On 2017/05/23 01:44:31, jdufault wrote: > Shouldn't this be consistent every test run? You're right, this extra complexity originating from OobeBaseTest should not be needed in my simple one case test. I've addressed it now.
On 2017/05/22 14:40:53, felixe1 wrote: > https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc > (right): > > https://codereview.chromium.org/2885153004/diff/40001/chrome/browser/ui/webui... > chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_unittest.cc:84: > EXPECT_EQ(ids[0], GetPrimaryDisplay()); > On 2017/05/18 11:12:04, felixe1 wrote: > > On 2017/05/17 16:56:18, oshima wrote: > > > can you add another test case where the primary display is removed? > > > First swap the primary display, then UpdateDisplay() with one display. > > > > I'm not sure I understand what this would test. > > > > In the test environment there's no connection between DisplayObservers and > > OobeDisplayChooser as that's performed by other classes in the real setup. > > OobeDisplayChooser will not react to the removal of a display. > > > > Is it possible to describe the state (DisplayManager, WindowTreeHostManager, > > etc) to use for such a test statically? Or do you want me to introduce a > > DisplayObserver and mimic the interaction taking place in the real setup? > > I've added a test now that trigger the bad behavior on ToT but passes with the > code in this CL. It had to be a browser test and not a unit test because for > some reason the display removal in the unit tests does not trigger the bad > state. OobeTestBase extends InProcessBrowserTest, so it has to be browser test, or the result will be undefined. Technically, you should be able to reproduce the issue in unit test but this is fine too.
https://codereview.chromium.org/2885153004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc (right): https://codereview.chromium.org/2885153004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc:35: FROM_HERE, base::Bind(&chrome::AttemptExit)); Do you know why you need this?
https://codereview.chromium.org/2885153004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc (right): https://codereview.chromium.org/2885153004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc:35: FROM_HERE, base::Bind(&chrome::AttemptExit)); On 2017/05/23 14:45:39, oshima (OOO May 22-24) wrote: > Do you know why you need this? No, and I don't strictly know _if_ I need it. It looked like something that could make the tests behave more reliably so I copied it from the source of this test class, OobeBaseTest, where it has existed from the start (https://codereview.chromium.org/99863007). My test still behave (and exit) nicely on a few try runs so I'll go ahead and remove it.
The CQ bit was checked by felixe@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...
lgtm
Description was changed from ========== Avoid changing primary display in DisplayObserver call BUG=668449 ========== to ========== Avoid changing primary display in DisplayObserver call BUG=668449,722564 ==========
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 felixe@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
felixe@chromium.org changed reviewers: + achuith@chromium.org
lgtm https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:986: display::Display primary_display = nit also make this const while you're here https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:989: !(changed_metrics & DISPLAY_METRIC_BOUNDS)) { Maybe cache this value in a const bool? https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:35: if (weak_ptr_factory_.HasWeakPtrs()) What's going on here? https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h:19: // Must be called on the BrowserThread::UI thread. I think a comment describing what this function does is more useful? https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc:41: int64_t GetPrimaryDisplay() { GetPrimaryDisplayId?
lgtm
https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/login/ui/login_display_host_impl.cc:986: display::Display primary_display = On 2017/05/23 20:14:22, achuithb wrote: > nit also make this const while you're here Done. https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.cc:35: if (weak_ptr_factory_.HasWeakPtrs()) On 2017/05/23 20:14:22, achuithb wrote: > What's going on here? Addressed in the form of a code comment. https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser.h:19: // Must be called on the BrowserThread::UI thread. On 2017/05/23 20:14:22, achuithb wrote: > I think a comment describing what this function does is more useful? Done. https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc (right): https://codereview.chromium.org/2885153004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/login/oobe_display_chooser_browsertest.cc:41: int64_t GetPrimaryDisplay() { On 2017/05/23 20:14:22, achuithb wrote: > GetPrimaryDisplayId? Done.
The CQ bit was checked by felixe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2885153004/#ps140001 (title: "Minor fixes and documentation changes")
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
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...)
The CQ bit was checked by felixe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495604849215180,
"parent_rev": "83ecfce112c3154e0c89225e2738033bbc90d8c0", "commit_rev":
"491c6b2fc5e9d5fd2b7eada34026ff977797b493"}
Message was sent while issue was closed.
Description was changed from ========== Avoid changing primary display in DisplayObserver call BUG=668449,722564 ========== to ========== Avoid changing primary display in DisplayObserver call BUG=668449,722564 Review-Url: https://codereview.chromium.org/2885153004 Cr-Commit-Position: refs/heads/master@{#474196} Committed: https://chromium.googlesource.com/chromium/src/+/491c6b2fc5e9d5fd2b7eada34026... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/491c6b2fc5e9d5fd2b7eada34026... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
