Pull back windows to the original user desktop when clicking on the correspond window list bubble i…

chrome/browser/ui/views/frame/system_menu_model_builder.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/frame/system_menu_model_builder.h"
 
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/host_desktop.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/wrench_menu_model.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/models/simple_menu_model.h"
 
 #if defined(OS_CHROMEOS)
 #include "ash/session/session_state_delegate.h"
 #include "ash/shell.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "components/user_manager/user_info.h"
 #include "ui/base/l10n/l10n_util.h"
 #endif
 
+namespace {
+
+// Check if the current window is chrome settings app or popup window.

[Peter Kasting, 2015/05/15 08:05:49]

Nit: How about:

Given a |browser| that's an app or popup window, checks if it's hosting the
settings page.

[xdai1, 2015/05/15 18:12:48]

Done.

+bool IsChromeSettingsAppOrPopupWindow(Browser* browser) {
+  if (browser) {

[Peter Kasting, 2015/05/15 08:05:49]

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().

[xdai1, 2015/05/15 18:12:48]

Done.

+    TabStripModel* tab_strip = browser->tab_strip_model();
+    DCHECK_EQ(1, tab_strip->count());
+    const GURL gurl = tab_strip->GetWebContentsAt(0)->GetURL();

[Peter Kasting, 2015/05/15 08:05:49]

Nit: Use constructor-style init for class objects like GURL

[xdai1, 2015/05/15 18:12:48]

Done.

+    if (gurl.SchemeIs(content::kChromeUIScheme) && gurl.has_host() &&

[Peter Kasting, 2015/05/15 08:05:49]

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.

[xdai1, 2015/05/15 18:12:48]

Done.

+        gurl.host().find(chrome::kChromeUISettingsHost) != std::string::npos) {
+      return true;
+    }
+  }
+  return false;
+}
+
+}  // namespace
+
 SystemMenuModelBuilder::SystemMenuModelBuilder(
     ui::AcceleratorProvider* provider,
     Browser* browser)
     : menu_delegate_(provider, browser) {
 }
 
 SystemMenuModelBuilder::~SystemMenuModelBuilder() {
 }
 
 void SystemMenuModelBuilder::Init() {
@@ skipping 63 matching lines @@
                                 encoding_menu_contents_.get());
   if (browser()->is_app() && chrome::CanOpenTaskManager()) {
     model->AddSeparator(ui::NORMAL_SEPARATOR);
     model->AddItemWithStringId(IDC_TASK_MANAGER, IDS_TASK_MANAGER);
   }
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   model->AddSeparator(ui::NORMAL_SEPARATOR);
   model->AddItemWithStringId(IDC_CLOSE_WINDOW, IDS_CLOSE);
 #endif
 
-  AppendTeleportMenu(model);
+  // We don't append teleport menu for Chrome Settings window. The Settings
+  // window on Chrome OS is currently unique. It's a browser window with an
+  // app-like frame and don't have a user icon badged when teleporting. To avoid
+  // confusion and edge cases, we disable teleporting for Settings window.

[Peter Kasting, 2015/05/15 08:05:49]

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.

[xdai1, 2015/05/15 18:12:48]

Done.

+  if (!IsChromeSettingsAppOrPopupWindow(browser()))
+    AppendTeleportMenu(model);
 }
 
 void SystemMenuModelBuilder::AddFrameToggleItems(ui::SimpleMenuModel* model) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDebugEnableFrameToggle)) {
     model->AddSeparator(ui::NORMAL_SEPARATOR);
     model->AddItem(IDC_DEBUG_FRAME_TOGGLE,
                    base::ASCIIToUTF16("Toggle Frame Type"));
   }
 }
@@ skipping 33 matching lines @@
     const user_manager::UserInfo* user_info = delegate->GetUserInfo(user_index);
     model->AddItem(
         user_index == 1 ? IDC_VISIT_DESKTOP_OF_LRU_USER_2
                         : IDC_VISIT_DESKTOP_OF_LRU_USER_3,
         l10n_util::GetStringFUTF16(IDS_VISIT_DESKTOP_OF_LRU_USER,
                                    user_info->GetDisplayName(),
                                    base::ASCIIToUTF16(user_info->GetEmail())));
   }
 #endif
 }