Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644733004:
Chrome OS: New window on teleported browser window should show on current desktop (Closed)
|
Created:
3 years, 11 months ago by Qiang(Joe) Xu Modified:
3 years, 10 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChrome OS: New window on teleported browser window should show on current desktop
Changes:
Regression comes from: https://codereview.chromium.org/2434463004, which is to separate ash/ from chrome/.
(1) set_show_for_user in AddBrowserWindow
(2) regression happens because of isProcessingUserEvent breaks. Found that we don't actually need this, so removing it.
(3) Fix test in MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest
BUG=608840
TEST=emulator test saw comment #8 bug fixed in 608840.
Patch Set 1 #Patch Set 2 : remove unused include #
Total comments: 1
Patch Set 3 : add test coverage #
Total comments: 8
Patch Set 4 : possible fix? #Patch Set 5 : delete isProcessingUserEvent #
Total comments: 4
Messages
Total messages: 37 (25 generated)
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop BUG=608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop BUG=608840 ==========
The CQ bit was checked by warx@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.
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop BUG=608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ==========
warx@chromium.org changed reviewers: + skuhne@chromium.org
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop Changes: (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ==========
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop Changes: (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop Changes: (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ==========
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop Changes: (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop Changes: NewWindow will first new a browser window and navigate to new tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ==========
Hi Stefan, please take a look first on my changes, if it looks good to you, I will add test coverage and add other reviewers. Thanks!
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should show on current active desktop Changes: NewWindow will first new a browser window and navigate to new tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should always show on current active desktop Changes: NewWindow will first new a browser window and navigate to new tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ==========
https://codereview.chromium.org/2644733004/diff/20001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (left): https://codereview.chromium.org/2644733004/diff/20001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:2835: CreateBrowserAndTabWithProfile(profile2, user2, "http://test2")); This is to create a visiting(teleported) browser2 on desktop 1 according to the change.
Patchset #3 (id:40001) has been deleted
warx@chromium.org changed reviewers: + oshima@chromium.org, sky@chromium.org, xdai@chromium.org
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should always show on current active desktop Changes: NewWindow will first new a browser window and navigate to new tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840 ========== to ========== ChromeOS: In multiprofile scenario, new browser window should always show on current active desktop Changes: NewWindow will first new a browser window and navigate to new empty tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers (3) add test coverage BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840, add test coverage MultiUserBrowserWindowChromeOSTest.NewWindowOnTeleportedBrowserWindow ==========
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should always show on current active desktop Changes: NewWindow will first new a browser window and navigate to new empty tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers (3) add test coverage BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840, add test coverage MultiUserBrowserWindowChromeOSTest.NewWindowOnTeleportedBrowserWindow ========== to ========== ChromeOS: In multiprofile scenario, new browser window should always show on current active desktop Changes: NewWindow will first new a browser window and navigate to new empty tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers (3) add test coverage BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840, and add test coverage: MultiUserBrowserWindowChromeOSTest.NewWindowOnTeleportedBrowserWindow ==========
Hi all, ptal, thanks!
https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/ash/m... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1564: const_cast<user_manager::User*>(user), profile); The const_casts here seem wrong. Doesn't SetUserToProfileMappingForTesting() take a const user? https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1576: new chrome::MultiUserWindowManagerChromeOS(test_account_id1_); Who owns this? https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1589: GetUserWindowManager()->SetAnimationSpeedForTest( Can this be done early on rather than for each switch? https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1684: std::unique_ptr<Browser> browser2( When you close the tab in the browser that should trigger deleting the window. So, I don't think you want a unique_ptr here. https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/ash/m... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos_unittest.cc:1686: ASSERT_TRUE(browser2); IMO this check isn't useful, CreateBrowserAndTab always returns a browser. https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator.cc:443: multi_user_util::MoveWindowToCurrentDesktop(new_window); This seems wrong to me. When we create the browser don't we supply context that corresponds to the active desktop so that it appears in the right place?
https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator.cc:443: multi_user_util::MoveWindowToCurrentDesktop(new_window); On 2017/01/20 01:03:08, sky wrote: > This seems wrong to me. When we create the browser don't we supply context that > corresponds to the active desktop so that it appears in the right place? I agree. warx@, please see my comment in the bug.
https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/2644733004/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator.cc:443: multi_user_util::MoveWindowToCurrentDesktop(new_window); If I remember it correctly there was the possibility that a chrome window on a desktop of a (could be hidden) user spawned another window and that should show up on the desktop on where the chrome window is. So e.g.: User clicks on query - which processes the result for a second in which the user switches to another desktop. Then the resulting window shows up - but it should be presented on the desktop of the originating browser - not the current one.
The CQ bit was checked by warx@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...
Description was changed from ========== ChromeOS: In multiprofile scenario, new browser window should always show on current active desktop Changes: NewWindow will first new a browser window and navigate to new empty tab. We can make sure that browser window shows on current active desktop during navigating. (1) Add possible MoveWindowToCurrentDesktop in chrome::Navigate, to fix ctrl+n new window in multiprofile scenario that new browser window should show on current active desktop. (2) Fix a unittest MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest.BrowserMenuGenerationTwoUsers (3) add test coverage BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840, and add test coverage: MultiUserBrowserWindowChromeOSTest.NewWindowOnTeleportedBrowserWindow ========== to ========== Chrome OS: New window on teleported browser window should show on current desktop Changes: Regression comes from: https://codereview.chromium.org/2434463004, which is to separate ash/ from chrome/. BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840. ==========
Hi all, It turns out after https://codereview.chromium.org/2434463004. New browser window is created at browser's profile's desktop. I would like to convey this patch first to see if this is a possible fix. I would also like to hear some thoughts as currently I still don't know the root cause of it. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/03 21:22:43, Qiang(Joe) Xu wrote: > Hi all, It turns out after https://codereview.chromium.org/2434463004. New > browser window is created at browser's profile's desktop. > > I would like to convey this patch first to see if this is a possible fix. > > I would also like to hear some thoughts as currently I still don't know the root > cause of it. > > Thanks!
On 2017/02/04 00:04:25, sky wrote: > On 2017/02/03 21:22:43, Qiang(Joe) Xu wrote: > > Hi all, It turns out after https://codereview.chromium.org/2434463004. New > > browser window is created at browser's profile's desktop. > > > > I would like to convey this patch first to see if this is a possible fix. > > > > I would also like to hear some thoughts as currently I still don't know the > root > > cause of it. > > > > Thanks! I still maintain you are working around the issue and not addressing the core problem. If you don't fix the root issue other problems are likely to surface.
Also, code in c/b/ui should not refer to platform specific ui code.
Description was changed from ========== Chrome OS: New window on teleported browser window should show on current desktop Changes: Regression comes from: https://codereview.chromium.org/2434463004, which is to separate ash/ from chrome/. BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840. ========== to ========== Chrome OS: New window on teleported browser window should show on current desktop Changes: Regression comes from: https://codereview.chromium.org/2434463004, which is to separate ash/ from chrome/. (1) set_show_for_user in AddBrowserWindow (2) regression happens because of isProcessingUserEvent breaks. Found that we don't actually need this, so removing it. (3) Fix test in MultiProfileMultiBrowserShelfLayoutChromeLauncherControllerImplTest BUG=608840 TEST=emulator test saw comment #8 bug fixed in 608840. ==========
Hi all, please take a look, thanks! Regression is because https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/multi_user/multi_u... returns false now. I chatted with Oshima, relying on isProcessingUserEvent seems not reliable. Besides, we can totally remove it now. Instead, gurantee it in AddBrowserWindow. https://bugs.chromium.org/p/chromium/issues/detail?id=309793 doesn't happen anymore. https://codereview.chromium.org/2644733004/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc (left): https://codereview.chromium.org/2644733004/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:2851: // users running browser list. If we create browser with profile 2, this should be teleported browser in desktop 1. Then SwitchActiveUser(account_id2), the app menu should be CheckAppMenu(laucher_controller_.get(), item_browser, 0, one_menu_item2); I don't think directly creating a teleported browser makes any sense. Since we can only do this once there is a teleported browser on current desktop. https://codereview.chromium.org/2644733004/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc (left): https://codereview.chromium.org/2644733004/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:188: previous_animation_type_); This trunk is just formatting. https://codereview.chromium.org/2644733004/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:323: window_to_entry_[window]->set_show_for_user(current_account_id_); we don't need this.. checked https://codereview.chromium.org/55303003, the bug doesn't happen any more.
The CQ bit was checked by warx@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/2644733004/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc (right): https://codereview.chromium.org/2644733004/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:598: window_to_entry_[native_window]->set_show_for_user(current_account_id_); Won't this always open the window in the current active? That may not be the right desktop if the window is opened after the desktop has switched for example. I think you need to tell MUWM on which account's desktop it should open in ChromeNewWindowClient.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
