Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1109393011:
Pull back windows to the original user desktop when clicking on the correspond window list bubble i… (Closed)
DescriptionPull back windows to the original user desktop when clicking on the correspond shelf icon or window list bubble item.
Also prevent teleport the Settings window between users.
BUG=478360
Committed: https://crrev.com/e7d888132fc2fd641622d86b2b4c92fa4d01b124
Cr-Commit-Position: refs/heads/master@{#330137}
Patch Set 1 #Patch Set 2 : Fix the failed tests. #Patch Set 3 : #
Total comments: 3
Patch Set 4 : Don't append teleport window context menu item for delicated settings window. #
Total comments: 7
Patch Set 5 : Use existing multi_user_util::MoveWindowToCurrentDesktop() instead. #Patch Set 6 : Change the function's name to IsChromeSettingsAppOrPopupWindow(). #
Total comments: 2
Patch Set 7 : Address pkasting@'s comments. #
Total comments: 10
Patch Set 8 : Address pkasting@'s comment. #
Messages
Total messages: 26 (8 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
xdai@chromium.org changed reviewers: + skuhne@chromium.org
Stefan, could you help to take a look at this CL please? I also hope you could give me some advice on what we should do when user try to teleport a window that contains a chrome://settings page or subpages. Just fail silently might not be an option. Thanks very much!
Let's see what the issue updates say https://codereview.chromium.org/1109393011/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.cc (right): https://codereview.chromium.org/1109393011/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.cc:62: chrome::MultiUserWindowManager::GetInstance(); You could avoid using the user manager by checking if the window is on the owned user's desktop: if (!window_manager->IsWindowOnDesktopOfUser(window, window_manager->GetWindowOwner(window)) { or alternatively if (window_manager->GetUserPresentingWindow(window) != window_manager->GetWindowOwner(window))) { https://codereview.chromium.org/1109393011/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc (right): https://codereview.chromium.org/1109393011/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/multi_user/multi_user_window_manager_chromeos.cc:346: // desktop. Why? This does not make sense. I do not see why we should not be able to move such a window to another desktop. Note also: If we would really would do this... ... we would need to intercept navigation operations in the browser and automatically pull windows back to the user's desktop which would then be totally confusing. ... we would need to disallow teleporting of such windows in the first place by removing the menu entry from the context menu, since otherwise the user would try to move the window and it would not move.
Thanks for the review, please take another look! https://codereview.chromium.org/1109393011/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.cc (right): https://codereview.chromium.org/1109393011/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.cc:62: chrome::MultiUserWindowManager::GetInstance(); On 2015/04/30 21:51:44, Mr4D wrote: > You could avoid using the user manager by checking if the window is on the owned > user's desktop: > > if (!window_manager->IsWindowOnDesktopOfUser(window, > window_manager->GetWindowOwner(window)) { > > or alternatively > > if (window_manager->GetUserPresentingWindow(window) != > window_manager->GetWindowOwner(window))) { Done.
Please have another look! https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.cc (right): https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.cc:53: chrome::MultiUserWindowManager::MULTI_PROFILE_MODE_SEPARATED) { Since this is the same code as in the other cc module you might want create a new multi_user_util function for it. With that you would then even able to get rid of the #if defined ... here. https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.cc:58: window, window_manager->GetWindowOwner(window))) same here https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.cc (right): https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_tab.cc:59: window, window_manager->GetWindowOwner(window))) You need to add {} around this since it's more then one line. https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:33: // subpages. If not mistaken, we stated in the issue that we only want to suppress the settings APP - NOT browser individual pages. As such - you should see if the window is an application and if it is the settings app - not if one of the tabs contains a settings app.
Thanks for the review, please take another look! https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.cc (right): https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_app_menu_item_browser.cc:53: chrome::MultiUserWindowManager::MULTI_PROFILE_MODE_SEPARATED) { On 2015/05/03 19:53:44, Mr4D wrote: > Since this is the same code as in the other cc module you might want create a > new multi_user_util function for it. With that you would then even able to get > rid of the #if defined ... here. Found an existing function... Used it instead. https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:33: // subpages. On 2015/05/03 19:53:45, Mr4D wrote: > If not mistaken, we stated in the issue that we only want to suppress the > settings APP - NOT browser individual pages. > > As such - you should see if the window is an application and if it is the > settings app - not if one of the tabs contains a settings app. > This function is only called by SystemMenuModelBuilder::BuildSystemMenuForAppOrPopupWindow(), which means it is a dedicated setting window or application.
Sorry - but it still needs some changes. https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:33: // subpages. Right - but it has an impact on tabbed browser windows. Imagine you (user) can teleport one tabbed browser window but not another. How should the user know why that is? Isn't that confusing? Looking at the issue, we agreed that we would only block *settings applications* from getting teleported. Tabbed browser windows however would be able to get teleported. Your code hinders tabbed browser windows to get teleported - which is not correct.
Patchset #6 (id:160001) has been deleted
On 2015/05/04 17:07:41, Mr4D wrote: > Sorry - but it still needs some changes. > > https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... > File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): > > https://codereview.chromium.org/1109393011/diff/120001/chrome/browser/ui/view... > chrome/browser/ui/views/frame/system_menu_model_builder.cc:33: // subpages. > Right - but it has an impact on tabbed browser windows. > > Imagine you (user) can teleport one tabbed browser window but not another. How > should the user know why that is? Isn't that confusing? > > Looking at the issue, we agreed that we would only block *settings applications* > from getting teleported. Tabbed browser windows however would be able to get > teleported. > > Your code hinders tabbed browser windows to get teleported - which is not > correct. Addressed your comment. Please take another look, thanks!
lgtm! Thanks for doing this!
xdai@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@, could you help to review the following file please: chrome/browser/ui/views/frame/system_menu_model_builder.cc Thanks!
I also am not convinced this behavior makes sense; commented on the bug. https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:37: base::ASCIIToUTF16(chrome::kChromeUISettingsHost); Don't do things this way. Instead, get the URL below as a GURL (not a string) and check its scheme() and host() against the constants here.
On 2015/05/06 10:49:50, Peter Kasting wrote: > I also am not convinced this behavior makes sense; commented on the bug. > > https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... > File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): > > https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/frame/system_menu_model_builder.cc:37: > base::ASCIIToUTF16(chrome::kChromeUISettingsHost); > Don't do things this way. > > Instead, get the URL below as a GURL (not a string) and check its scheme() and > host() against the constants here. pkasting@, could you help to take another look at the CL please? As discussed in the buganizer, the setting window is unique and don't have a badge on it, thus should not be allowed to teleport to another user's desktop. Thanks for your review.
My previous review comment (about how to deal with GURLs) stands; here's another nit as well. https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:135: // We don't append teleport menu for Chrome Settings window. Nit: Expand this comment to explain why it doesn't make sense to do this for this window.
On 2015/05/12 20:18:46, Peter Kasting wrote: > My previous review comment (about how to deal with GURLs) stands; here's another > nit as well. > > https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... > File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): > > https://codereview.chromium.org/1109393011/diff/180001/chrome/browser/ui/view... > chrome/browser/ui/views/frame/system_menu_model_builder.cc:135: // We don't > append teleport menu for Chrome Settings window. > Nit: Expand this comment to explain why it doesn't make sense to do this for > this window. Sorry, I overlooked the previous comment. Please take another look, thanks!
LGTM https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:32: // Check if the current window is chrome settings app or popup window. Nit: How about: Given a |browser| that's an app or popup window, checks if it's hosting the settings page. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:34: if (browser) { Nit: Simpler: if (!browser) return false; ... return gurl.SchemeIs(content::kChromeUIScheme) && ...; But actually, you can omit the first conditional entirely or change to a DCHECK(), since the caller unconditionally derefs browser(). https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:37: const GURL gurl = tab_strip->GetWebContentsAt(0)->GetURL(); Nit: Use constructor-style init for class objects like GURL https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:38: if (gurl.SchemeIs(content::kChromeUIScheme) && gurl.has_host() && Nit: No need to check has_host(); if there were no host, host() would return std::string() and the find() call below would return npos. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:134: // confusion and edge cases, we disable teleporting for Settings window. Nit: How about: Avoid appending the teleport menu for the settings window. This window's presentation is unique: it's a normal browser window with an app-like frame, which doesn't have a user icon badge. Thus if teleported it's not clear what user it applies to. Rather than bother to implement badging just for this rare case, simply prevent the user from teleporting the window.
Thanks for review. Comments addressed. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/system_menu_model_builder.cc (right): https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:32: // Check if the current window is chrome settings app or popup window. On 2015/05/15 08:05:49, Peter Kasting wrote: > Nit: How about: > > Given a |browser| that's an app or popup window, checks if it's hosting the > settings page. Done. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:34: if (browser) { On 2015/05/15 08:05:49, Peter Kasting wrote: > Nit: Simpler: > > if (!browser) > return false; > ... > return gurl.SchemeIs(content::kChromeUIScheme) && ...; > > But actually, you can omit the first conditional entirely or change to a > DCHECK(), since the caller unconditionally derefs browser(). Done. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:37: const GURL gurl = tab_strip->GetWebContentsAt(0)->GetURL(); On 2015/05/15 08:05:49, Peter Kasting wrote: > Nit: Use constructor-style init for class objects like GURL Done. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:38: if (gurl.SchemeIs(content::kChromeUIScheme) && gurl.has_host() && On 2015/05/15 08:05:49, Peter Kasting wrote: > Nit: No need to check has_host(); if there were no host, host() would return > std::string() and the find() call below would return npos. Done. https://codereview.chromium.org/1109393011/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/frame/system_menu_model_builder.cc:134: // confusion and edge cases, we disable teleporting for Settings window. On 2015/05/15 08:05:49, Peter Kasting wrote: > Nit: How about: > > Avoid appending the teleport menu for the settings window. This window's > presentation is unique: it's a normal browser window with an app-like frame, > which doesn't have a user icon badge. Thus if teleported it's not clear what > user it applies to. Rather than bother to implement badging just for this rare > case, simply prevent the user from teleporting the window. Done.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1109393011/#ps220001 (title: "Address pkasting@'s comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1109393011/220001
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e7d888132fc2fd641622d86b2b4c92fa4d01b124 Cr-Commit-Position: refs/heads/master@{#330137} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
