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

Index: chrome/browser/ui/views/frame/system_menu_model_builder.cc
diff --git a/chrome/browser/ui/views/frame/system_menu_model_builder.cc b/chrome/browser/ui/views/frame/system_menu_model_builder.cc
index e877f978abed33e58fd8ba5a01dc8e57f4dec52a..86eb03fc9801e3c7b78938ba32750591456438d7 100644
--- a/chrome/browser/ui/views/frame/system_menu_model_builder.cc
+++ b/chrome/browser/ui/views/frame/system_menu_model_builder.cc
@@ -9,8 +9,10 @@
 #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"
@@ -25,6 +27,24 @@
 #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)
@@ -108,7 +128,12 @@ void SystemMenuModelBuilder::BuildSystemMenuForAppOrPopupWindow(
   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) {