Make devtools_window.cc multi-desktop aware.

chrome/browser/devtools/devtools_window.cc

 // Copyright (c) 2012 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 <algorithm>
 
 #include "base/command_line.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/string_number_conversions.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/extensions/api/debugger/debugger_api.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/file_select_helper.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #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"
 #include "chrome/common/extensions/manifest_url_handler.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_client_host.h"
@@ skipping 368 matching lines @@
   browser_->tab_strip_model()->AddWebContents(
       web_contents_, -1, content::PAGE_TRANSITION_AUTO_TOPLEVEL,
       TabStripModel::ADD_ACTIVE);
 }
 
 bool DevToolsWindow::FindInspectedBrowserAndTabIndex(Browser** browser,
                                                      int* tab) {
   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;
       *tab = tab_index;
       return true;
     }
   }
   return false;
 }
 
@@ skipping 369 matching lines @@
 
 void DevToolsWindow::OpenInNewTab(const std::string& url) {
   OpenURLParams params(GURL(url),
                        content::Referrer(),
                        NEW_FOREGROUND_TAB,
                        content::PAGE_TRANSITION_LINK,
                        false /* is_renderer_initiated */);
   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;
       }
     }
   }
 }
 
 void DevToolsWindow::SaveToFile(const std::string& url,
                                 const std::string& content,
@@ skipping 147 matching lines @@
 
 // static
 DevToolsDockSide DevToolsWindow::SideFromString(
     const std::string& dock_side) {
   if (dock_side == kDockSideRight)
     return DEVTOOLS_DOCK_SIDE_RIGHT;
   if (dock_side == kDockSideBottom)
     return DEVTOOLS_DOCK_SIDE_BOTTOM;
   return DEVTOOLS_DOCK_SIDE_UNDOCKED;
 }