Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503623002:
Support creation of toplevel mus::Windows on separate displays (Closed)
DescriptionSupport creation of toplevel mus::Windows on separate displays
. Mus window should be shown in a correct display.
. Add unit test.
BUG=614070
Committed: https://crrev.com/1e73070ad937c592052cf5dac97e88b686dbc598
Cr-Commit-Position: refs/heads/master@{#434324}
Patch Set 1 #Patch Set 2 : Initalize container id property in wm_test_base. #Patch Set 3 : Fix include orders. #Patch Set 4 : Fix container root lookup. Don't need to initialize container id for unit test. #
Total comments: 2
Patch Set 5 : remove log. #Patch Set 6 : Add logs to debug #Patch Set 7 : Update patch to correct screen bounds, instead of using target root for selecting display. #Patch Set 8 : Clean up/format code. #
Total comments: 6
Patch Set 9 : Convert display point to screen point in root_window_controller.cc instead. Format cleanup. #Patch Set 10 : Remove logs. #Patch Set 11 : Clean up/reformat code. Remove pointer. #Patch Set 12 : Add const to reference. #Patch Set 13 : Add blank line back. #
Total comments: 18
Patch Set 14 : Remove unnecessary subclass. Remove reference pointer to screen bounds. Add unit test to existing w… #Patch Set 15 : Add blank line back. #Patch Set 16 : Add blank line back. Remove duplicate function. #
Total comments: 2
Patch Set 17 : Remove GetPrimary/SecondaryRootWindowController methods. #
Total comments: 15
Patch Set 18 : Change display_id default value to kInvalidDisplayID. Rename unit test. Cleanup/format code. #
Total comments: 4
Patch Set 19 : Edit comments/names. #
Total comments: 3
Patch Set 20 : Don't use mus kInitialDisplayId_Property if the display id is negative. #
Total comments: 2
Patch Set 21 : Allow display_id be negative. #Patch Set 22 : Rebase and change constant kInvalidDisplayId namespace. #
Total comments: 7
Patch Set 23 : Cleanup/Format code/name. #Patch Set 24 : Keep default value for display_id in .h file instead of .cc. #Patch Set 25 : remove default display_id value in .cc #
Total comments: 7
Patch Set 26 : Move unit test to root_window_controller_unittest.cc and remove changes in window_manager_unittest.… #Patch Set 27 : Reduce code size to convert rect from window to screen coordinate. #
Total comments: 3
Messages
Total messages: 143 (103 generated)
The CQ bit was checked by thanhph@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...
mfomitchev@chromium.org changed reviewers: + mfomitchev@chromium.org
https://codereview.chromium.org/2503623002/diff/60001/ash/common/wm/container... File ash/common/wm/container_finder.cc (right): https://codereview.chromium.org/2503623002/diff/60001/ash/common/wm/container... ash/common/wm/container_finder.cc:79: target_root = context->GetRootWindow(); This makes sense to me, but the original way seems like it should work too. Perhaps we need to fix FindContainerRoot?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2503623002/diff/60001/ash/common/wm/container... File ash/common/wm/container_finder.cc (right): https://codereview.chromium.org/2503623002/diff/60001/ash/common/wm/container... ash/common/wm/container_finder.cc:79: target_root = context->GetRootWindow(); On 2016/11/15 20:29:34, mfomitchev wrote: > This makes sense to me, but the original way seems like it should work too. > Perhaps we need to fix FindContainerRoot? If the bounds is correct, then this function FindContainerRoot is probably doing a right thing. I think I passed in the incorrect bounds during my test case setup or this needs to be fixed in here https://cs.chromium.org/chromium/src/ash/mus/root_window_controller.cc?type=c... This current patch, however, uses the context to find root window so the incorrect bounds don't affect the root window selection (unit test correctly passed with this fix).
The CQ bit was checked by thanhph@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...
thanhph@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in Hi Scott, I see your name in the suggested owner. Please review my patch. Thanks! Thanh.
https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... File ash/mus/bridge/wm_root_window_controller_mus.h (right): https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... ash/mus/bridge/wm_root_window_controller_mus.h:43: gfx::Rect* ConvertWindowToScreen(const WmWindowMus* source, Doesn't seem like there's any reason to create a method for this here. Can we just do the calculations in NewTopLevelWindow? Also, perhaps it would be more convenient to use WmWindow::ConvertPointToScreen rather than WmRootWindowControllerMus? Also, we are leaking the rect - it is created on the heap and never deleted. Just use an object, not an object pointer. https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... File ash/mus/bridge/wm_window_mus.cc (left): https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... ash/mus/bridge/wm_window_mus.cc:136: Doesn't seem like there's particular reason to remove this line (especially considering this is the only change to the file). https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: // TODO(sky): window->bounds() isn't quite right. Remove this now?
On 2016/11/17 17:14:01, mfomitchev wrote: > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... > File ash/mus/bridge/wm_root_window_controller_mus.h (right): > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... > ash/mus/bridge/wm_root_window_controller_mus.h:43: gfx::Rect* > ConvertWindowToScreen(const WmWindowMus* source, > Doesn't seem like there's any reason to create a method for this here. Can we > just do the calculations in NewTopLevelWindow? Also, perhaps it would be more > convenient to use WmWindow::ConvertPointToScreen rather than > WmRootWindowControllerMus? > > Also, we are leaking the rect - it is created on the heap and never deleted. > Just use an object, not an object pointer. > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... > File ash/mus/bridge/wm_window_mus.cc (left): > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... > ash/mus/bridge/wm_window_mus.cc:136: > Doesn't seem like there's particular reason to remove this line (especially > considering this is the only change to the file). > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... > File ash/mus/root_window_controller.cc (right): > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... > ash/mus/root_window_controller.cc:105: // TODO(sky): window->bounds() isn't > quite right. > Remove this now? Also the title of the CL is a bit misleading - we are not just creating a unit test, we are also fixing an issue.
Description was changed from ========== Add unit test for "support creation of toplevel mus::Windows on separate displays" BUG=614070 ========== to ========== Add unit test for "support creation of toplevel mus::Windows on separate displays" BUG=614070 ==========
https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... File ash/mus/bridge/wm_root_window_controller_mus.h (right): https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... ash/mus/bridge/wm_root_window_controller_mus.h:43: gfx::Rect* ConvertWindowToScreen(const WmWindowMus* source, On 2016/11/17 17:14:01, mfomitchev wrote: > Doesn't seem like there's any reason to create a method for this here. Can we > just do the calculations in NewTopLevelWindow? Also, perhaps it would be more > convenient to use WmWindow::ConvertPointToScreen rather than > WmRootWindowControllerMus? Putting this function here could be good for future reuse. I'll move it back to root_window_controller.cc since there's no other use of it now. I tried this WmWindow::ConvertPointToScreen the whole day yesterday but didn't work. The problem was somehow WmWindow wasn't initialized and as a result, I kept getting ASAN:DEADLYSIGNAL. > > Also, we are leaking the rect - it is created on the heap and never deleted. > Just use an object, not an object pointer. Thanks. I'll try this now. https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... File ash/mus/bridge/wm_window_mus.cc (left): https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... ash/mus/bridge/wm_window_mus.cc:136: On 2016/11/17 17:14:01, mfomitchev wrote: > Doesn't seem like there's particular reason to remove this line (especially > considering this is the only change to the file). Done. https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: // TODO(sky): window->bounds() isn't quite right. On 2016/11/17 17:14:01, mfomitchev wrote: > Remove this now? Done.
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 thanhph@chromium.org to run a CQ dry run
On 2016/11/17 19:08:30, thanhph wrote: > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... > File ash/mus/bridge/wm_root_window_controller_mus.h (right): > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_root... > ash/mus/bridge/wm_root_window_controller_mus.h:43: gfx::Rect* > ConvertWindowToScreen(const WmWindowMus* source, > On 2016/11/17 17:14:01, mfomitchev wrote: > > Doesn't seem like there's any reason to create a method for this here. Can we > > just do the calculations in NewTopLevelWindow? Also, perhaps it would be more > > convenient to use WmWindow::ConvertPointToScreen rather than > > WmRootWindowControllerMus? > > Putting this function here could be good for future reuse. I'll move it back to > root_window_controller.cc since there's no other use of it now. I tried this > WmWindow::ConvertPointToScreen the whole day yesterday but didn't work. The > problem was somehow WmWindow wasn't initialized and as a result, I kept getting > ASAN:DEADLYSIGNAL. > > > > > Also, we are leaking the rect - it is created on the heap and never deleted. > > Just use an object, not an object pointer. > > Thanks. I'll try this now. > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... > File ash/mus/bridge/wm_window_mus.cc (left): > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/bridge/wm_wind... > ash/mus/bridge/wm_window_mus.cc:136: > On 2016/11/17 17:14:01, mfomitchev wrote: > > Doesn't seem like there's particular reason to remove this line (especially > > considering this is the only change to the file). > > Done. > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... > File ash/mus/root_window_controller.cc (right): > > https://codereview.chromium.org/2503623002/diff/100002/ash/mus/root_window_co... > ash/mus/root_window_controller.cc:105: // TODO(sky): window->bounds() isn't > quite right. > On 2016/11/17 17:14:01, mfomitchev wrote: > > Remove this now? > > Done. Mikhail: I just added a new cl, please review. Thanks!
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 thanhph@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 thanhph@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_ozone_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 thanhph@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 thanhph@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 thanhph@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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by thanhph@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.
Also: Please update the CL description to reflect the title change and capitalize the first letter of the title/ https://codereview.chromium.org/2503623002/diff/230001/ash/mus/BUILD.gn File ash/mus/BUILD.gn (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/BUILD.gn#newco... ash/mus/BUILD.gn:248: "window_manager_ash_unittest.cc", Why not put the test into window_manager_unittest? All of these test are in ash/mus/, so _ash_ in the file name seems redundant. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point& screen_point_top_right = Nit: Any particular reason you are using const references here? const makes some sense, but I am not sure the reference does.. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:113: const gfx::Rect& screen_bound = The variable name is a little confusing - sounds like the bounds of the screen. Maybe call the variable "bounds_in_screen"? Then for consistency we can rename the other two "top_right_in_screen", "bottom_left_in_screen". https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:107: return roots.size() < 2 ? nullptr : roots[1]; The current code doesn't seem to account for the possibility that this can return nullptr. GetSecondaryDisplay() never returns null. It might make sense to design this method similarly? https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:151: ui::Window* WmTestBase::CreateFullscreenTestWindow(RootWindowController* root) { Avoid duplicate code between this and the above method. E.g. call this from the above method. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:159: // properties[ui::mojom::WindowManager::kInitialContainerId_Property] = remove https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (left): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:74: DOn't remove this blank line. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/window_manager... File ash/mus/window_manager_ash_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/window_manager... ash/mus/window_manager_ash_unittest.cc:11: class WindowManagerAshTest : public mus::WmTestBase { No reason to create a new class if you are not overriding any behavior. YOu can do using WindowManagerAshTes = mus::WmTestBase; if you wish. https://codereview.chromium.org/2503623002/diff/230001/services/ui/public/cpp... File services/ui/public/cpp/window_tree_client.cc (left): https://codereview.chromium.org/2503623002/diff/230001/services/ui/public/cpp... services/ui/public/cpp/window_tree_client.cc:8: don't remove this blank line
https://codereview.chromium.org/2503623002/diff/230001/ash/mus/BUILD.gn File ash/mus/BUILD.gn (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/BUILD.gn#newco... ash/mus/BUILD.gn:248: "window_manager_ash_unittest.cc", On 2016/11/18 17:36:24, mfomitchev wrote: > Why not put the test into window_manager_unittest? All of these test are in > ash/mus/, so _ash_ in the file name seems redundant. Done. Thanks! https://codereview.chromium.org/2503623002/diff/230001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point& screen_point_top_right = On 2016/11/18 17:36:24, mfomitchev wrote: > Nit: Any particular reason you are using const references here? const makes some > sense, but I am not sure the reference does.. Done. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:113: const gfx::Rect& screen_bound = On 2016/11/18 17:36:24, mfomitchev wrote: > The variable name is a little confusing - sounds like the bounds of the screen. > Maybe call the variable "bounds_in_screen"? Then for consistency we can rename > the other two "top_right_in_screen", "bottom_left_in_screen". Done. Thanks! https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:107: return roots.size() < 2 ? nullptr : roots[1]; On 2016/11/18 17:36:24, mfomitchev wrote: > The current code doesn't seem to account for the possibility that this can > return nullptr. GetSecondaryDisplay() never returns null. It might make sense > to design this method similarly? I get this idea from the function ui::Window* WmTestBase::GetSecondaryRootWindow() (just right above), and somehow it returns nullptr as well. I'm not sure why, we might just leave nullptr here as well. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:151: ui::Window* WmTestBase::CreateFullscreenTestWindow(RootWindowController* root) { On 2016/11/18 17:36:24, mfomitchev wrote: > Avoid duplicate code between this and the above method. E.g. call this from the > above method. I merged the codes in 2 functions. Thanks! https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:159: // properties[ui::mojom::WindowManager::kInitialContainerId_Property] = On 2016/11/18 17:36:24, mfomitchev wrote: > remove Done. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (left): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:74: On 2016/11/18 17:36:24, mfomitchev wrote: > DOn't remove this blank line. Done. https://codereview.chromium.org/2503623002/diff/230001/ash/mus/window_manager... File ash/mus/window_manager_ash_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/230001/ash/mus/window_manager... ash/mus/window_manager_ash_unittest.cc:11: class WindowManagerAshTest : public mus::WmTestBase { On 2016/11/18 17:36:24, mfomitchev wrote: > No reason to create a new class if you are not overriding any behavior. YOu can > do > using WindowManagerAshTes = mus::WmTestBase; > if you wish. Cool, thanks..I updated the code to this! https://codereview.chromium.org/2503623002/diff/230001/services/ui/public/cpp... File services/ui/public/cpp/window_tree_client.cc (left): https://codereview.chromium.org/2503623002/diff/230001/services/ui/public/cpp... services/ui/public/cpp/window_tree_client.cc:8: On 2016/11/18 17:36:24, mfomitchev wrote: > don't remove this blank line Done.
The CQ bit was checked by thanhph@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/2503623002/diff/290001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/290001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:150: window = root->window_manager()->NewTopLevelWindow(&properties); I think window manager is the same for all root windows? If so we don't actually need RootWindowController here as an arg to this function. What we need is the display id. So maybe this function could take display id (or at least Display) instead of RWC pointer. And then we can also get rid of the new GetPrimary/SecondaryRootWindowController methods. (You could still keep CreateFullscreenTestWindow() with no args if you wish, it should just call CreateFullscreenTestWindow(<id of the first display>) so that there's no code duplication.
https://codereview.chromium.org/2503623002/diff/290001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/290001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:150: window = root->window_manager()->NewTopLevelWindow(&properties); On 2016/11/18 20:35:58, mfomitchev wrote: > I think window manager is the same for all root windows? If so we don't actually > need RootWindowController here as an arg to this function. What we need is the > display id. So maybe this function could take display id (or at least Display) > instead of RWC pointer. And then we can also get rid of the new > GetPrimary/SecondaryRootWindowController methods. > > (You could still keep CreateFullscreenTestWindow() with no args if you wish, it > should just call CreateFullscreenTestWindow(<id of the first display>) so that > there's no code duplication. Great stuff here Mikhail, thanks! The WM is the same for all root windows. I have refactored/reduce the code to your suggestion. Thanks!
Repeat: Please update the CL description to reflect the title change and capitalize the first letter of the title. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:132: if (display_id) Need braces around multiline statement. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:134: mojo::ConvertTo<std::vector<uint8_t>>(display_id); Is 0 display id guaranteed to be the same as having no display id? Because as is if I call CreateFullscreenTestWindow(0), it won't set kInitialDisplayId_Property prop. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:62: ui::Window* CreateFullscreenTestWindow(int64_t display_id); Please put the default value (int64_t display_id = 0) in the declaration as it is essential. Also, please document the method, particularly how display_id is used. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:84: using WindowManagerAshTest = mus::WmTestBase; WindowManagerDisplayTest? https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:86: TEST_F(WindowManagerAshTest, isWidgetShownInCorrectDisplay) { Capitalize first letter. Widget -> Window. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:93: ui::Window* ui_primary_window = window_primary_display, window_secondary_display. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:101: EXPECT_NE(ui_primary_window->display_id(), ui_secondary_window->display_id()); A better test would be to actually ensure the two display ids are the ones we asked for, not just that they aren't equal to each other.
Description was changed from ========== Add unit test for "support creation of toplevel mus::Windows on separate displays" BUG=614070 ========== to ========== . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ==========
https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:132: if (display_id) On 2016/11/18 21:23:40, mfomitchev wrote: > Need braces around multiline statement. Done. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:134: mojo::ConvertTo<std::vector<uint8_t>>(display_id); On 2016/11/18 21:23:40, mfomitchev wrote: > Is 0 display id guaranteed to be the same as having no display id? Because as is > if I call CreateFullscreenTestWindow(0), it won't set kInitialDisplayId_Property > prop. Done. I change from 0 to display::Display::kInvalidDisplayID instead. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:62: ui::Window* CreateFullscreenTestWindow(int64_t display_id); On 2016/11/18 21:23:40, mfomitchev wrote: > Please put the default value (int64_t display_id = 0) in the declaration as it > is essential. Also, please document the method, particularly how display_id is > used. Done. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:84: using WindowManagerAshTest = mus::WmTestBase; On 2016/11/18 21:23:40, mfomitchev wrote: > WindowManagerDisplayTest? Thanks! I changed it WindowManagerDualDisplayTest. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:86: TEST_F(WindowManagerAshTest, isWidgetShownInCorrectDisplay) { On 2016/11/18 21:23:40, mfomitchev wrote: > Capitalize first letter. Widget -> Window. Done. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:93: ui::Window* ui_primary_window = On 2016/11/18 21:23:40, mfomitchev wrote: > window_primary_display, window_secondary_display. Done. https://codereview.chromium.org/2503623002/diff/310001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:101: EXPECT_NE(ui_primary_window->display_id(), ui_secondary_window->display_id()); On 2016/11/18 21:23:40, mfomitchev wrote: > A better test would be to actually ensure the two display ids are the ones we > asked for, not just that they aren't equal to each other. Done. Thanks!
The CQ bit was checked by thanhph@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.
CL description should contain the CL title as it's first line. (Look at all the other CLs, use the same format). https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/310001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:134: mojo::ConvertTo<std::vector<uint8_t>>(display_id); On 2016/11/21 14:37:53, thanhph wrote: > On 2016/11/18 21:23:40, mfomitchev wrote: > > Is 0 display id guaranteed to be the same as having no display id? Because as > is > > if I call CreateFullscreenTestWindow(0), it won't set > kInitialDisplayId_Property > > prop. > > Done. I change from 0 to display::Display::kInvalidDisplayID instead. Wait, this is different. Now we can't call this method w/o giving it a display id - it won't work. https://codereview.chromium.org/2503623002/diff/330001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (right): https://codereview.chromium.org/2503623002/diff/330001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:63: // Creates a full screen visbiel top level window window By default, - visible - missing period - don't explain the default value of display_id - that's clear from the signature. Say how it's used instead (the window should be displayed on the display with that id). https://codereview.chromium.org/2503623002/diff/330001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/330001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:84: using WindowManagerDualDisplayTest = mus::WmTestBase; "Dual" is too specific IMHO. It's specific to the one test case we added. Makes it so it would be hard to add more test cases w/o changing the name.
Description was changed from ========== . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ========== to ========== Support creation of toplevel mus::Windows on separate displays BUG=614070 ==========
Description was changed from ========== Support creation of toplevel mus::Windows on separate displays BUG=614070 ========== to ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ==========
I define default display_id in the header file wm_test_base.h as below so calling the function CreateFullscreenTestWindow without providing argument should still be ok. ui::Window* CreateFullscreenTestWindow(int64_t display_id = display::Display::kInvalidDisplayID); https://codereview.chromium.org/2503623002/diff/330001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (right): https://codereview.chromium.org/2503623002/diff/330001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:63: // Creates a full screen visbiel top level window window By default, On 2016/11/22 22:53:16, mfomitchev wrote: > - visible > - missing period > - don't explain the default value of display_id - that's clear from the > signature. Say how it's used instead (the window should be displayed on the > display with that id). Done, thanks! https://codereview.chromium.org/2503623002/diff/330001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/330001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:84: using WindowManagerDualDisplayTest = mus::WmTestBase; On 2016/11/22 22:53:16, mfomitchev wrote: > "Dual" is too specific IMHO. It's specific to the one test case we added. Makes > it so it would be hard to add more test cases w/o changing the name. I agree. I change it to DisplayWindowInSpecifiedDisplayTest. It's probably a bit clearer.
The CQ bit was checked by thanhph@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/2503623002/diff/350001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/350001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:132: mojo::ConvertTo<std::vector<uint8_t>>(display_id); For the default value kInvalidDisplayId, do you really want to insert the property with a value of -1? Or do you just want to not insert the property in that case?
https://codereview.chromium.org/2503623002/diff/350001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/350001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:132: mojo::ConvertTo<std::vector<uint8_t>>(display_id); On 2016/11/23 00:28:00, kylechar wrote: > For the default value kInvalidDisplayId, do you really want to insert the > property with a value of -1? Or do you just want to not insert the property in > that case? Good question. I found a use case of kInvalidDisplayId here https://cs.chromium.org/chromium/src/ash/mus/property_util.cc?sq=package:chro... which I interpret as we shouldn't add the property with a value of -1 since the function GetInitialDisplayId will return -1 in this case. Since uint8_t is used, I also assume display_id must be non negative, otherwise default display is used to show window. If not correct, let me know. Thanks!
The CQ bit was checked by thanhph@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_...)
The CQ bit was checked by thanhph@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_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2503623002/diff/350001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/350001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:132: mojo::ConvertTo<std::vector<uint8_t>>(display_id); On 2016/11/23 01:05:02, thanhph wrote: > On 2016/11/23 00:28:00, kylechar wrote: > > For the default value kInvalidDisplayId, do you really want to insert the > > property with a value of -1? Or do you just want to not insert the property in > > that case? > > Good question. I found a use case of kInvalidDisplayId here > https://cs.chromium.org/chromium/src/ash/mus/property_util.cc?sq=package:chro... > > which I interpret as we shouldn't add the property with a value of -1 since the > function GetInitialDisplayId will return -1 in this case. Since uint8_t is used, > I also assume display_id must be non negative, otherwise default display is used > to show window. If not correct, let me know. Thanks! So the value kInvalidDisplayId is appropriate to use to represent the concept of "no display specified", since it's a reserved value that will never correspond to a display. That part is fine. I meant that if no display is specified then maybe it's better to not add that property to the properties map, since it's meaningless. The value needs to be serialized, deserialized, converted back to int64_t and then dropped (it's a lot of work for nothing). The last part about the value of |display_id| having to be non-negative is wrong. |display_id| is int64_t or 8 bytes. It's being converted to a std::vector<uint8_t>, not uint8_t, and I'd imagine you'll find that the vector length is 8. All properties are just encoded into a vector of bytes (which are represented by uint8_t). https://codereview.chromium.org/2503623002/diff/370001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/370001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:131: if (display_id > -1) { if (display_id != display::kInvalidDisplayId) { ... }
The CQ bit was checked by thanhph@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...
Thanks for the clarification Kyle, maybe that's why the bots were unhappy. https://codereview.chromium.org/2503623002/diff/370001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/370001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:131: if (display_id > -1) { On 2016/11/23 14:14:20, kylechar wrote: > if (display_id != display::kInvalidDisplayId) { ... } Done.
The CQ bit was unchecked by thanhph@chromium.org
The CQ bit was checked by thanhph@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.
Also: Add a blank line between the title and the body in the description. I.e. Support creation of toplevel mus::Windows on separate displays. - Mus window should be shown in a correct display. - Add unit test. https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:125: ui::Window* WmTestBase::CreateFullscreenTestWindow(int64_t display_id) { Use full signature (with default value) here. https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:64: // shown on the display with that id. Nit: Creates a visibile fullscreen top level window on the display corresponding to the display_id provided. https://codereview.chromium.org/2503623002/diff/410001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:84: using DisplayWindowInSpecifiedDisplayTest = mus::WmTestBase; I think this is still too specific. This name corresponds to a set of test cases, and you are trying to make it specific to one test case. "IsWindowShownInCorrectDisplay" (the name in TEST_F below) is the name specific to the test case. This one on the other hand should be more generic - something that should be able to accommodate more tests in the future. E.g. WindowManagerlDisplayTest.
https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:125: ui::Window* WmTestBase::CreateFullscreenTestWindow(int64_t display_id) { On 2016/11/23 17:57:59, mfomitchev wrote: > Use full signature (with default value) here. Done. I can't put default values in both .cc and .h so I move the default value to file .cc. https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.h (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.h:64: // shown on the display with that id. On 2016/11/23 17:58:00, mfomitchev wrote: > Nit: > Creates a visibile fullscreen top level window on the display corresponding to > the display_id provided. Done, thanks! https://codereview.chromium.org/2503623002/diff/410001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:84: using DisplayWindowInSpecifiedDisplayTest = mus::WmTestBase; On 2016/11/23 17:58:00, mfomitchev wrote: > I think this is still too specific. This name corresponds to a set of test > cases, and you are trying to make it specific to one test case. > "IsWindowShownInCorrectDisplay" (the name in TEST_F below) is the name specific > to the test case. This one on the other hand should be more generic - something > that should be able to accommodate more tests in the future. E.g. > WindowManagerDisplayTest. I misinterpreted your last comment. Done, thanks!
The CQ bit was checked by thanhph@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...
Repeat: Add a blank line between the title and the body in the description. I.e. Support creation of toplevel mus::Windows on separate displays. - Mus window should be shown in a correct display. - Add unit test. https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... File ash/mus/test/wm_test_base.cc (right): https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... ash/mus/test/wm_test_base.cc:125: ui::Window* WmTestBase::CreateFullscreenTestWindow(int64_t display_id) { On 2016/11/23 18:42:19, thanhph wrote: > On 2016/11/23 17:57:59, mfomitchev wrote: > > Use full signature (with default value) here. > > Done. I can't put default values in both .cc and .h so I move the default value > to file .cc. Oh, okay, sorry - didn't realize this. Please keep them in the .h then.
Description was changed from ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ========== to ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ==========
On 2016/11/23 19:05:15, mfomitchev wrote: > Repeat: > Add a blank line between the title and the body in the description. I.e. > > Support creation of toplevel mus::Windows on separate displays. > > - Mus window should be shown in a correct display. > - Add unit test. Done, thanks! > > https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... > File ash/mus/test/wm_test_base.cc (right): > > https://codereview.chromium.org/2503623002/diff/410001/ash/mus/test/wm_test_b... > ash/mus/test/wm_test_base.cc:125: ui::Window* > WmTestBase::CreateFullscreenTestWindow(int64_t display_id) { > On 2016/11/23 18:42:19, thanhph wrote: > > On 2016/11/23 17:57:59, mfomitchev wrote: > > > Use full signature (with default value) here. > > > > Done. I can't put default values in both .cc and .h so I move the default > value > > to file .cc. > > Oh, okay, sorry - didn't realize this. Please keep them in the .h then. Ok, done!
The CQ bit was checked by thanhph@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
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 thanhph@chromium.org to run a CQ dry run
The CQ bit was unchecked by thanhph@chromium.org
The CQ bit was checked by thanhph@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/2503623002/diff/470001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point top_right_in_screen = wm_window has a GetBoundsInScreen. Can't you use it? https://codereview.chromium.org/2503623002/diff/470001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:86: TEST_F(WindowManagerDisplayTest, IsWindowShownInCorrectDisplay) { This is a test of functionality in RootWindowController, right? I would expect this test to be named RootWindowControllerTest and be in root_window_controller_unittest.cc
https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point top_right_in_screen = On 2016/11/23 20:48:19, sky wrote: > wm_window has a GetBoundsInScreen. Can't you use it? I can't. I tried to use it as below but got ASAN:DEADLYSIGNAL error because window_ wasn't initialized. Const gfx::Rect bounds_in_screen = WmWindowMus::Get(window)->GetBoundsInScreen(); Hence, I use wm_root_window_controller_ instead. https://codereview.chromium.org/2503623002/diff/470001/ash/mus/window_manager... File ash/mus/window_manager_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/window_manager... ash/mus/window_manager_unittest.cc:86: TEST_F(WindowManagerDisplayTest, IsWindowShownInCorrectDisplay) { On 2016/11/23 20:48:19, sky wrote: > This is a test of functionality in RootWindowController, right? I would expect > this test to be named RootWindowControllerTest and be in > root_window_controller_unittest.cc I used the name RootWindowControllerWmTest since RootWindowControllerTest is already used. There were some changes in WindowManager but not anymore. Thanks!
https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point top_right_in_screen = On 2016/11/23 21:31:42, thanhph wrote: > On 2016/11/23 20:48:19, sky wrote: > > wm_window has a GetBoundsInScreen. Can't you use it? > > I can't. I tried to use it as below but got ASAN:DEADLYSIGNAL error because > window_ wasn't initialized. > > Const gfx::Rect bounds_in_screen = > WmWindowMus::Get(window)->GetBoundsInScreen(); > > Hence, I use wm_root_window_controller_ instead. This is likely failing because window doesn't have a parent yet. I think you want: gfx::Point origin = wm_root_window_controller_->ConvertPointToScreen(WmWindowMus::Get(root_), gfx::Point()); gfx::Rect bounds_in_screen(origin, bounds.size());
https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point top_right_in_screen = On 2016/11/23 21:45:36, sky wrote: > On 2016/11/23 21:31:42, thanhph wrote: > > On 2016/11/23 20:48:19, sky wrote: > > > wm_window has a GetBoundsInScreen. Can't you use it? > > > > I can't. I tried to use it as below but got ASAN:DEADLYSIGNAL error because > > window_ wasn't initialized. > > > > Const gfx::Rect bounds_in_screen = > > WmWindowMus::Get(window)->GetBoundsInScreen(); > > > > Hence, I use wm_root_window_controller_ instead. > > This is likely failing because window doesn't have a parent yet. I think you > want: > > gfx::Point origin = > wm_root_window_controller_->ConvertPointToScreen(WmWindowMus::Get(root_), > gfx::Point()); > gfx::Rect bounds_in_screen(origin, bounds.size()); Scott, this will not work if there is a transform on the window or one of its parents, right? (bounds won't be the same)
https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/470001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:105: const gfx::Point top_right_in_screen = On 2016/11/23 21:45:36, sky wrote: > On 2016/11/23 21:31:42, thanhph wrote: > > On 2016/11/23 20:48:19, sky wrote: > > > wm_window has a GetBoundsInScreen. Can't you use it? > > > > I can't. I tried to use it as below but got ASAN:DEADLYSIGNAL error because > > window_ wasn't initialized. > > > > Const gfx::Rect bounds_in_screen = > > WmWindowMus::Get(window)->GetBoundsInScreen(); > > > > Hence, I use wm_root_window_controller_ instead. > > This is likely failing because window doesn't have a parent yet. I think you > want: > > gfx::Point origin = > wm_root_window_controller_->ConvertPointToScreen(WmWindowMus::Get(root_), > gfx::Point()); > gfx::Rect bounds_in_screen(origin, bounds.size()); Done, thanks! This is shorter.
The CQ bit was checked by thanhph@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/2503623002/diff/510001/ash/mus/root_window_co... File ash/mus/root_window_controller.cc (right): https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... ash/mus/root_window_controller.cc:107: gfx::Rect bounds_in_screen(origin, window->bounds().size()); What if one of the window's parents is transformed? In that case window->bounds().size() will not be equal to the visible size of the window on the screen, no?
On 2016/11/23 23:02:13, mfomitchev wrote: > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > File ash/mus/root_window_controller.cc (right): > > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > ash/mus/root_window_controller.cc:107: gfx::Rect bounds_in_screen(origin, > window->bounds().size()); > What if one of the window's parents is transformed? This particular code branch is trying to determine the parent. See line 117 where it adds the window to the value returned from wm::GetDefaultParent. So, there is no parent at this time. > In that case > window->bounds().size() will not be equal to the visible size of the window on > the screen, no?
LGTM
Patchset #29 (id:550001) has been deleted
On 2016/11/23 23:07:28, sky wrote: > On 2016/11/23 23:02:13, mfomitchev wrote: > > > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > > File ash/mus/root_window_controller.cc (right): > > > > > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > > ash/mus/root_window_controller.cc:107: gfx::Rect bounds_in_screen(origin, > > window->bounds().size()); > > What if one of the window's parents is transformed? > > This particular code branch is trying to determine the parent. See line 117 > where it adds the window to the value returned from wm::GetDefaultParent. So, > there is no parent at this time. > > > In that case > > window->bounds().size() will not be equal to the visible size of the window on > > the screen, no? Ah right, of course, thanks!
Patchset #28 (id:530001) has been deleted
The CQ bit was checked by thanhph@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 thanhph@chromium.org
The CQ bit was checked by thanhph@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: Exceeded global retry quota
The CQ bit was checked by thanhph@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 thanhph@chromium.org
The CQ bit was checked by thanhph@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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by thanhph@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 thanhph@chromium.org
The CQ bit was checked by thanhph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org Link to the patchset: https://codereview.chromium.org/2503623002/#ps510001 (title: "Reduce code size to convert rect from window to screen coordinate.")
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_TIMED_OUT, no build URL)
The CQ bit was checked by thanhph@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 thanhph@chromium.org
The CQ bit was checked by thanhph@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": 510001, "attempt_start_ts": 1479998973017420,
"parent_rev": "a290e8ce04f110f566d3b94afb84b05739f2f022", "commit_rev":
"358116d260bb07361f391be23fa0dfa853904394"}
Message was sent while issue was closed.
Description was changed from ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ========== to ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:510001)
Message was sent while issue was closed.
Description was changed from ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 ========== to ========== Support creation of toplevel mus::Windows on separate displays . Mus window should be shown in a correct display. . Add unit test. BUG=614070 Committed: https://crrev.com/1e73070ad937c592052cf5dac97e88b686dbc598 Cr-Commit-Position: refs/heads/master@{#434324} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/1e73070ad937c592052cf5dac97e88b686dbc598 Cr-Commit-Position: refs/heads/master@{#434324}
Message was sent while issue was closed.
I'm in the process of converting ash/mus to use aura and had to update this test. I noticed a couple of things. Don't change these as I'm going to change it shortly. I just wanted to mention it for future tests you write. https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... File ash/mus/root_window_controller_unittest.cc (right): https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... ash/mus/root_window_controller_unittest.cc:38: DCHECK(window_primary_display); For tests prefer ASSERT over DCHECK. The reason is in a test you want other tests to be run if one fails. A DCHECK will cause a crash and no other tests to run. https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... ash/mus/root_window_controller_unittest.cc:41: EXPECT_EQ(window_primary_display->display_id(), GetPrimaryDisplay().id()); In chrome code we use the format of expected, actual. So, the args here should be reversed.
Message was sent while issue was closed.
On 2016/11/29 17:43:13, sky wrote: > I'm in the process of converting ash/mus to use aura and had to update this > test. I noticed a couple of things. Don't change these as I'm going to change it > shortly. I just wanted to mention it for future tests you write. > > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > File ash/mus/root_window_controller_unittest.cc (right): > > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > ash/mus/root_window_controller_unittest.cc:38: DCHECK(window_primary_display); > For tests prefer ASSERT over DCHECK. The reason is in a test you want other > tests to be run if one fails. A DCHECK will cause a crash and no other tests to > run. > > https://codereview.chromium.org/2503623002/diff/510001/ash/mus/root_window_co... > ash/mus/root_window_controller_unittest.cc:41: > EXPECT_EQ(window_primary_display->display_id(), GetPrimaryDisplay().id()); > In chrome code we use the format of expected, actual. So, the args here should > be reversed. Great, thanks Scott! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
