Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573673003:
Detect and fix overlapping displays (Closed)
DescriptionDetect and fix overlapping displays
Overlapping displays prevent us from correctly creating the mouse warp
regions. This CL adds code to detect and fix any overlap when the display
layout is applied.
This was initially a bug in the options UI that was recently fixed. The backend
still needs to prevent this buggy condition.
BUG=614660
TEST=ash_unittests --gtest_filter=DisplayManagerTest.NoOverlappedDisplays*
Review-Url: https://codereview.chromium.org/2573673003
Cr-Commit-Position: refs/heads/master@{#447402}
Committed: https://chromium.googlesource.com/chromium/src/+/6c80a875d8534f5f9d965a9808446655b5f4e0a2
Patch Set 1 : [WIP] Pending unit_tests #Patch Set 2 : [WIP] Don't shift primary display #Patch Set 3 : [WIP] Added Test #Patch Set 4 : [WIP] Fix tests and store resolved layout #Patch Set 5 : [WIP] Fixed a couple of bugs and added two more tests #Patch Set 6 : Fix compile on Windows #
Total comments: 14
Patch Set 7 : Rebase #Patch Set 8 : Oshima's comments #Patch Set 9 : No layout in unified mode #Patch Set 10 : Better algorithm, still need to update test comments #Patch Set 11 : Update tests, ignore invalid layouts. #Patch Set 12 : More accurate test numbers #
Total comments: 4
Patch Set 13 : Reparent once #
Total comments: 6
Patch Set 14 : Extract Reparent #
Total comments: 4
Patch Set 15 : 2 more tests. #Patch Set 16 : Nit #
Total comments: 15
Patch Set 17 : stevenjb's comments #
Total comments: 4
Patch Set 18 : Nits #Messages
Total messages: 98 (75 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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Detect and fix overlapping displays Overlapping displays prevent us from correctly creating the mouse warp regions. This CL adds code to detect and fix any overlap when the display layout is applied. This was initially a bug in the options UI that was recently fixed. The backend still needs to prevent this buggy condition. BUG=614660 ========== to ========== Detect and fix overlapping displays Overlapping displays prevent us from correctly creating the mouse warp regions. This CL adds code to detect and fix any overlap when the display layout is applied. This was initially a bug in the options UI that was recently fixed. The backend still needs to prevent this buggy condition. BUG=614660 TEST=ash_unittests --gtest_filter=DisplayManagerTest.NoOverlappedDisplays* ==========
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
afakhry@chromium.org changed reviewers: + oshima@chromium.org
Oshima-san, please review. Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/15 17:51:05, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Oshima-san, あけましておめでとございます！ Now that you are back, could you please take a look at this? Thanks!
On 2017/01/05 22:31:25, afakhry wrote: > On 2016/12/15 17:51:05, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > Oshima-san, あけましておめでとございます！ > > Now that you are back, could you please take a look at this? Thanks! Sorry for delay. I'll review this today.
https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:52: }; can you document how logic works? https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:59: const gfx::Rect& b_bounds = parent_display.bounds(); you can get intersects and check if the width is 1. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:103: if (intersection.IsEmpty()) can't you also skip if they're touching? https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:141: if (target_display_placement_itr == placement_list->end()) this should happen only for primary, right? https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:154: if (parent_display_itr == displays->end()) Can this happen? https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:165: target_display_placement_itr->parent_display_id = source_display->id(); this should not happen to the primary, otherwise it'll create circular dependency. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:216: placement_list, updated_displays); can't adjusting X will cause new overlap?
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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/2573673003/diff/100001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:52: }; On 2017/01/09 21:46:46, oshima wrote: > can you document how logic works? I added a comment above DeIntersectDisplays(), and more comments overall. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:59: const gfx::Rect& b_bounds = parent_display.bounds(); On 2017/01/09 21:46:46, oshima wrote: > you can get intersects and check if the width is 1. Unfortunately, that doesn't work. The intersection width is 0 rather than 1 if they are touching. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:103: if (intersection.IsEmpty()) On 2017/01/09 21:46:46, oshima wrote: > can't you also skip if they're touching? Yes, but I need the intersection Rect anyways below. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:141: if (target_display_placement_itr == placement_list->end()) On 2017/01/09 21:46:46, oshima wrote: > this should happen only for primary, right? As a matter of fact, this should never happen! We always start processing from the primary display, so the primary display is always the source, never the target display. I replaced this 'if' with a 'DCHECK'. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:154: if (parent_display_itr == displays->end()) On 2017/01/09 21:46:46, oshima wrote: > Can this happen? It shouldn't. Replaced with a DCHECK. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:165: target_display_placement_itr->parent_display_id = source_display->id(); On 2017/01/09 21:46:46, oshima wrote: > this should not happen to the primary, otherwise it'll create circular > dependency. The primary display is never the target display. So this should never happen. However, I added a check to make sure that we don't offset the target display if the source is a descendant of it. I also added a new test to make sure this case is handled correctly. https://codereview.chromium.org/2573673003/diff/100001/ui/display/display_lay... ui/display/display_layout.cc:216: placement_list, updated_displays); On 2017/01/09 21:46:46, oshima wrote: > can't adjusting X will cause new overlap? Yes, in a very rare case. To fix that, I modified it to run the algorithm as long as there's still an overlap for a maximum of 4 iterations. In the third iteration, I relax the requirement of offsetting only along the shorter overlap side, and just offset along the axis. The newly added test checks this special case.
friendly ping.
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: Try jobs failed on following builders: 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...
Patchset #11 (id:200001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 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...
Oshima-san, I implemented the new algorithm. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Oshima-san, friendly ping.
https://codereview.chromium.org/2573673003/diff/240001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/240001/ui/display/display_lay... ui/display/display_layout.cc:160: for (int i = 1; i < static_cast<int>(sorted_displays.size()); ++i) { nit: size_t https://codereview.chromium.org/2573673003/diff/240001/ui/display/display_lay... ui/display/display_layout.cc:211: target_display->UpdateWorkAreaFromInsets(insets); what if the target display overlaps with two displays? Shouldn't the following code be done after checking against all source displays?
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
https://codereview.chromium.org/2573673003/diff/240001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/240001/ui/display/display_lay... ui/display/display_layout.cc:160: for (int i = 1; i < static_cast<int>(sorted_displays.size()); ++i) { On 2017/01/30 18:06:13, oshima wrote: > nit: size_t Done. https://codereview.chromium.org/2573673003/diff/240001/ui/display/display_lay... ui/display/display_layout.cc:211: target_display->UpdateWorkAreaFromInsets(insets); On 2017/01/30 18:06:14, oshima wrote: > what if the target display overlaps with two displays? > > Shouldn't the following code be done after checking against all source displays? Yes. Done.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... ui/display/display_layout.cc:217: updated_displays->insert(target_display->id()); this can add the target display more than once if it overlaps more than one source displays. can you include this scenario in the unit test? https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... ui/display/display_layout.cc:268: } refactor "reparent" part to a method?
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...
https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... ui/display/display_layout.cc:160: for (size_t i = 1; i < sorted_displays.size(); ++i) { Actually I need to use int here. I need to be able to compare j >= 0 without compiler warnings, in other words, I need j to be signed. https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... ui/display/display_layout.cc:217: updated_displays->insert(target_display->id()); On 2017/01/30 20:30:43, oshima wrote: > this can add the target display more than once if it overlaps more than one > source displays. can you include this scenario in the unit test? This shouldn't be a problem since I already changed |updated_displays| from a vector to a set to avoid this case. https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... ui/display/display_layout.cc:268: } On 2017/01/30 20:30:43, oshima wrote: > refactor "reparent" part to a method? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/260001/ui/display/display_lay... ui/display/display_layout.cc:160: for (size_t i = 1; i < sorted_displays.size(); ++i) { ok. You can actually iterate back on sorted_displays, but I'll leave it to you. https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... File ash/display/display_manager_unittest.cc (right): https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... ash/display/display_manager_unittest.cc:744: } can you also add the scenario like this [1][2][3] [ p ] where [2] does not fit between [1] and [3] ? the lgtm https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... ui/display/display_layout.cc:191: placement.parent_display_id; nit: emplace would be better
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...
afakhry@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb@ for display_options_handler.cc. Thanks! https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... File ash/display/display_manager_unittest.cc (right): https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... ash/display/display_manager_unittest.cc:744: } On 2017/01/31 16:55:43, oshima wrote: > can you also add the scenario like this > > > [1][2][3] > [ p ] > > where [2] does not fit between [1] and [3] ? > > the lgtm Done. https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... ui/display/display_layout.cc:191: placement.parent_display_id; On 2017/01/31 16:55:43, oshima wrote: > nit: emplace would be better emplace() won't compile with -std=gnu++11 that we are using: ../../ui/display/display_layout.cc:191:31: error: no member named 'emplace' in 'std::__debug::map<long, long, std::less<long>, std::allocator<std::pair<const long, long> > >' display_to_parent_ids_map.emplace( ~~~~~~~~~~~~~~~~~~~~~~~~~ ^
On 2017/01/31 19:19:32, afakhry wrote: > +stevenjb@ for display_options_handler.cc. Thanks! > > https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... > File ash/display/display_manager_unittest.cc (right): > > https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... > ash/display/display_manager_unittest.cc:744: } > On 2017/01/31 16:55:43, oshima wrote: > > can you also add the scenario like this > > > > > > [1][2][3] > > [ p ] > > > > where [2] does not fit between [1] and [3] ? > > > > the lgtm > > Done. > > https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... > File ui/display/display_layout.cc (right): > > https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... > ui/display/display_layout.cc:191: placement.parent_display_id; > On 2017/01/31 16:55:43, oshima wrote: > > nit: emplace would be better > > emplace() won't compile with -std=gnu++11 that we are using: > > ../../ui/display/display_layout.cc:191:31: error: no member named 'emplace' in > 'std::__debug::map<long, long, std::less<long>, std::allocator<std::pair<const > long, long> > >' > display_to_parent_ids_map.emplace( > ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ then insert (make_pair) (sorry, I've mixed anroid c++ and chromium)
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...
On 2017/01/31 19:26:05, oshima wrote: > On 2017/01/31 19:19:32, afakhry wrote: > > +stevenjb@ for display_options_handler.cc. Thanks! > > > > > https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... > > File ash/display/display_manager_unittest.cc (right): > > > > > https://codereview.chromium.org/2573673003/diff/280001/ash/display/display_ma... > > ash/display/display_manager_unittest.cc:744: } > > On 2017/01/31 16:55:43, oshima wrote: > > > can you also add the scenario like this > > > > > > > > > [1][2][3] > > > [ p ] > > > > > > where [2] does not fit between [1] and [3] ? > > > > > > the lgtm > > > > Done. > > > > > https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... > > File ui/display/display_layout.cc (right): > > > > > https://codereview.chromium.org/2573673003/diff/280001/ui/display/display_lay... > > ui/display/display_layout.cc:191: placement.parent_display_id; > > On 2017/01/31 16:55:43, oshima wrote: > > > nit: emplace would be better > > > > emplace() won't compile with -std=gnu++11 that we are using: > > > > ../../ui/display/display_layout.cc:191:31: error: no member named 'emplace' in > > 'std::__debug::map<long, long, std::less<long>, std::allocator<std::pair<const > > long, long> > >' > > display_to_parent_ids_map.emplace( > > ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ > > then insert (make_pair) (sorry, I've mixed anroid c++ and chromium) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2573673003/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc (right): https://codereview.chromium.org/2573673003/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:364: display_manager->GetCurrentResolvedDisplayLayout().FindPlacementById( We need to make a similar change to display_info_provider_chromeos.cc https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:68: while (display_to_parent_ids_map.count(current_id)) { This is a little confusing. I believe this test is valid only for the |display_id|? I think we should add this test to line 63, and make this a while(1) loop. Not finding a display in the map would be an error. We may also want to set a max depth or check for loops (i.e. two non primary displays reference each other as parents). Also, we're effectively doing the lookup twice, once for count() and once for at(). https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:94: return rb == ry; Calculate just rb and ry inside the if, and rr and rx below. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:137: return; nit: Test this before calling instead. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:184: // Fixes any overlapping displays and repared displays if necessary. repairs https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:234: It would be nice to move some or all of the rest of this into one or more smaller functions. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:275: if (updated_displays) This appears to always be true?
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
https://codereview.chromium.org/2573673003/diff/310001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc (right): https://codereview.chromium.org/2573673003/diff/310001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:364: display_manager->GetCurrentResolvedDisplayLayout().FindPlacementById( On 2017/01/31 21:58:56, stevenjb wrote: > We need to make a similar change to display_info_provider_chromeos.cc Done. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:68: while (display_to_parent_ids_map.count(current_id)) { On 2017/01/31 21:58:56, stevenjb wrote: > This is a little confusing. I believe this test is valid only for the > |display_id|? I think we should add this test to line 63, and make this a > while(1) loop. Not finding a display in the map would be an error. We may also > want to set a max depth or check for loops (i.e. two non primary displays > reference each other as parents). > > Also, we're effectively doing the lookup twice, once for count() and once for > at(). I removed that check entirely. This error would be caught very early on by DisplayLayout::Validate(). Checking for cycles is a candidate to be added to DisplayLayout::Validate() which we already call before applying the layout. However, adding a max depth to prevent errors is a good idea ... Done. Instead of while(1), I wrote it as while (current_id != primary_id && depth < kMaxDepth) {...} https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:94: return rb == ry; On 2017/01/31 21:58:56, stevenjb wrote: > Calculate just rb and ry inside the if, and rr and rx below. Done. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:137: return; On 2017/01/31 21:58:56, stevenjb wrote: > nit: Test this before calling instead. Done. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:184: // Fixes any overlapping displays and repared displays if necessary. On 2017/01/31 21:58:56, stevenjb wrote: > repairs I think I meant to say, "reparents". https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:234: On 2017/01/31 21:58:56, stevenjb wrote: > It would be nice to move some or all of the rest of this into one or more > smaller functions. Done. https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:275: if (updated_displays) On 2017/01/31 21:58:56, stevenjb wrote: > This appears to always be true? Correct. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/310001/ui/display/display_lay... ui/display/display_layout.cc:184: // Fixes any overlapping displays and repared displays if necessary. On 2017/01/31 23:23:44, afakhry wrote: > On 2017/01/31 21:58:56, stevenjb wrote: > > repairs > > I think I meant to say, "reparents". That makes more sense :) https://codereview.chromium.org/2573673003/diff/330001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/330001/ui/display/display_lay... ui/display/display_layout.cc:186: // This function assumes both displays already intersects. intersect https://codereview.chromium.org/2573673003/diff/330001/ui/display/display_lay... ui/display/display_layout.cc:200: // This function assumes both displays already intersects. nit: You could probably share a single comment for bot functions.
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...
afakhry@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+Devlin for display_info_provider_chromeos.cc OWNERS. https://codereview.chromium.org/2573673003/diff/330001/ui/display/display_lay... File ui/display/display_layout.cc (right): https://codereview.chromium.org/2573673003/diff/330001/ui/display/display_lay... ui/display/display_layout.cc:186: // This function assumes both displays already intersects. On 2017/01/31 23:40:55, stevenjb wrote: > intersect Done. https://codereview.chromium.org/2573673003/diff/330001/ui/display/display_lay... ui/display/display_layout.cc:200: // This function assumes both displays already intersects. On 2017/01/31 23:40:55, stevenjb wrote: > nit: You could probably share a single comment for bot functions. Done.
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc lgtm
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
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2573673003/#ps350001 (title: "Nits")
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": 350001, "attempt_start_ts": 1485912660038520,
"parent_rev": "56361462e2cc147331618705e3d54dc69312ed51", "commit_rev":
"6c80a875d8534f5f9d965a9808446655b5f4e0a2"}
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1485912660038520,
"parent_rev": "56361462e2cc147331618705e3d54dc69312ed51", "commit_rev":
"6c80a875d8534f5f9d965a9808446655b5f4e0a2"}
Message was sent while issue was closed.
Description was changed from ========== Detect and fix overlapping displays Overlapping displays prevent us from correctly creating the mouse warp regions. This CL adds code to detect and fix any overlap when the display layout is applied. This was initially a bug in the options UI that was recently fixed. The backend still needs to prevent this buggy condition. BUG=614660 TEST=ash_unittests --gtest_filter=DisplayManagerTest.NoOverlappedDisplays* ========== to ========== Detect and fix overlapping displays Overlapping displays prevent us from correctly creating the mouse warp regions. This CL adds code to detect and fix any overlap when the display layout is applied. This was initially a bug in the options UI that was recently fixed. The backend still needs to prevent this buggy condition. BUG=614660 TEST=ash_unittests --gtest_filter=DisplayManagerTest.NoOverlappedDisplays* Review-Url: https://codereview.chromium.org/2573673003 Cr-Commit-Position: refs/heads/master@{#447402} Committed: https://chromium.googlesource.com/chromium/src/+/6c80a875d8534f5f9d965a980844... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:350001) as https://chromium.googlesource.com/chromium/src/+/6c80a875d8534f5f9d965a980844... |
