Make devtools_window.cc multi-desktop aware.

chrome/browser/devtools/devtools_window.cc

Index: chrome/browser/devtools/devtools_window.cc
diff --git a/chrome/browser/devtools/devtools_window.cc b/chrome/browser/devtools/devtools_window.cc
index 48783235d1cfe257f720d7228b43419a22283e74..9c8abb67e17f5edae0e915f6bfb09fedabd6abe2 100644
--- a/chrome/browser/devtools/devtools_window.cc
+++ b/chrome/browser/devtools/devtools_window.cc
@@ -24,9 +24,10 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/browser/ui/browser_list_impl.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
@@ -415,34 +416,8 @@ bool DevToolsWindow::FindInspectedBrowserAndTabIndex(Browser** browser,
   if (!inspected_web_contents_)
     return false;
 
-  bool found = FindInspectedBrowserAndTabIndexFromBrowserList(
-      chrome::BrowserListImpl::GetInstance(chrome::HOST_DESKTOP_TYPE_NATIVE),
-      browser,
-      tab);
-  // On Windows 8 we can have the desktop environment and the ASH environment
-  // active concurrently. If we fail to find the inspected web contents in the
-  // native browser list, then we should look in the ASH browser list.
-#if defined(OS_WIN) && defined(USE_AURA)
-  if (!found) {
-    found = FindInspectedBrowserAndTabIndexFromBrowserList(
-        chrome::BrowserListImpl::GetInstance(chrome::HOST_DESKTOP_TYPE_ASH),
-        browser,
-        tab);
-  }
-#endif
-  return found;
-}
-
-bool DevToolsWindow::FindInspectedBrowserAndTabIndexFromBrowserList(
-    chrome::BrowserListImpl* browser_list,
-    Browser** browser,
-    int* tab) {
-  if (!inspected_web_contents_)
-    return false;
-
-  for (chrome::BrowserListImpl::const_iterator it = browser_list->begin();
-       it != browser_list->end(); ++it) {
-    int tab_index = (*it)->tab_strip_model()->GetIndexOfWebContents(
+  for (chrome::BrowserIterator it; !it.done(); it.Next()) {

[pfeldman, 2013/02/07 20:12:49]

You seem to be reverting https://codereview.chromium.org/11519012, is that
intentional?

[gab, 2013/02/07 20:31:45]

Yes, BrowserIterator is a new class which is multi-desktop aware.
BrowserList::const_iterator was only going over the native desktop.

Ananta originally hardcoded a few places to be aware of both list, but this was
getting messy so I built BrowserIterator which now implicitly goes over both
lists.

So, although this looks like a revert, it's subtly not :)!

You made me realize I had forgot the call in the header though (not sure why it
was ever exposed publicly since it was only used here...) -- removed.

+    int tab_index = it->tab_strip_model()->GetIndexOfWebContents(
         inspected_web_contents_);
     if (tab_index != TabStripModel::kNoTab) {
       *browser = *it;
@@ -832,8 +807,12 @@ void DevToolsWindow::OpenInNewTab(const std::string& url) {
   if (inspected_web_contents_) {
     inspected_web_contents_->OpenURL(params);
   } else {
-    for (BrowserList::const_iterator it = BrowserList::begin();
-         it != BrowserList::end(); ++it) {
+    chrome::HostDesktopType host_desktop_type =
+        browser_ ? browser_->host_desktop_type() : chrome::GetActiveDesktop();

[gab, 2013/02/07 18:01:36]

@pfeldman: Looks like you introduced OpenInNewTab in http://crrev.com/118666 a
year ago or so. Few questions for you:
1) Is OpenInNewTab() still used? Even if I remove all instances of it Chrome
still compiles, but I'm thinking perhaps some IPC calls from webkit depend on
it..?
Grep'ing suggests that this perhaps true, but "OpenInNewTab" is kind of a
generic name and unrelated messages could use the same name...

2) Is |browser_| guaranteed to be non-NULL here (i.e. from the code it seems
like it is if Show() is always called first... and many other methods make this
assumption and skip the NULL check it seems)?
If so, that would be preferable as chrome::GetActiveDesktop() is a very last
resort call for when we have absolutely no context...

[pfeldman, 2013/02/07 20:12:49]

It is used, see DevToolsHostMsg_OpenInNewTab issued in devtools_client.cc. It is
called from the WebDevToolsFrontendClient::openInNewTab override. That override
has default implementation and hence it compiles for you.
browser_ is a browser we create for showing undocked devtools window. So it is
NULL when you see devtools docked and non-NULL when it is undocked.

[gab, 2013/02/07 20:31:45]

I see so is it possible for both inspected_web_contents and browser_ to be
NULL?!

+    const chrome::BrowserListImpl* browser_list =
+        chrome::BrowserListImpl::GetInstance(host_desktop_type);
+    for (chrome::BrowserListImpl::const_iterator it = browser_list->begin();
+         it != browser_list->end(); ++it) {
       if ((*it)->type() == Browser::TYPE_TABBED) {
         (*it)->OpenURL(params);
         break;