Update: I removed TYPE_ANY to see if I could fool the builders into letting this pass without crashing in browser_tests. It makes the code uglier, but I want to debug and figure out what's going on.

Rework the way the FindBrowserWithProfile/Type methods work.

We now always walk the last active list backwards rather than consulting the last active then walking the registered browser list forwards.

This ensures that when the last active browser is a popup or app frame the last active TYPE_NORMAL browser is located when opening a new tab.

http://crbug.com/17498
TEST=Open an app frame. Open a browser window (Ctrl+N) and load a page. Minimize it. Open another browser window and minimize it. Activate the app frame. Press Ctrl+T. The second browser window should be restored and have a new tab added to it rather than the first.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30531
Review URL: http://codereview.chromium.org/330013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@30659 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 34 insertions, 37 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 295beff..60d08c8 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1616,7 +1616,8 @@ void Browser::DuplicateContentsAt(int index) {
   } else {
     Browser* browser = NULL;
     if (type_ & TYPE_APP) {
-      browser = Browser::CreateForApp(app_name_, profile_, type_ & TYPE_POPUP);
+      browser = Browser::CreateForApp(app_name_, profile_,
+                                      !!(type_ & TYPE_POPUP));
     } else if (type_ == TYPE_POPUP) {
       browser = Browser::CreateForPopup(profile_);
     }
@@ -1973,7 +1974,7 @@ void Browser::DetachContents(TabContents* source) {
 
 bool Browser::IsPopup(TabContents* source) {
   // A non-tabbed BROWSER is an unconstrained popup.
-  return (type() & TYPE_POPUP);
+  return !!(type() & TYPE_POPUP);
 }
 
 void Browser::ToolbarSizeChanged(TabContents* source, bool is_animating) {
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index aba80b7..c5ef49c 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -79,6 +79,16 @@ class BrowserActivityObserver : public NotificationObserver {
 
 BrowserActivityObserver* activity_observer = NULL;
 
+// Returns true if the specified |browser| has a matching profile and type to
+// those specified. If |any_type| is true, only |profile| must be matched.
+bool BrowserMatchesProfileAndType(Browser* browser,
+                                  Profile* profile,
+                                  Browser::Type type,
+                                  bool any_type) {
+  return (any_type || browser->type() == type) &&
+      browser->profile() == profile;
+}
+
 }  // namespace
 
 BrowserList::list_type BrowserList::browsers_;
@@ -242,8 +252,10 @@ void BrowserList::WindowsSessionEnding() {
 bool BrowserList::HasBrowserWithProfile(Profile* profile) {
   BrowserList::const_iterator iter;
   for (size_t i = 0; i < browsers_.size(); ++i) {
-    if (browsers_[i]->profile() == profile)
+    if (BrowserMatchesProfileAndType(browsers_[i], profile,
+                                     Browser::TYPE_NORMAL, true)) {
       return true;
+    }
   }
   return false;
 }
@@ -272,44 +284,28 @@ Browser* BrowserList::GetLastActive() {
 Browser* BrowserList::GetLastActiveWithProfile(Profile* p) {
   list_type::reverse_iterator browser = last_active_browsers_.rbegin();
   for (; browser != last_active_browsers_.rend(); ++browser) {
-    if ((*browser)->profile() == p) {
+    if (BrowserMatchesProfileAndType(*browser, p, Browser::TYPE_NORMAL, true))
       return *browser;
-    }
   }
-
   return NULL;
 }
 
 // static
 Browser* BrowserList::FindBrowserWithType(Profile* p, Browser::Type t) {
-  Browser* last_active = GetLastActive();
-  if (last_active && last_active->profile() == p && last_active->type() == t)
-    return last_active;
-
-  BrowserList::const_iterator i;
-  for (i = BrowserList::begin(); i != BrowserList::end(); ++i) {
-    if (*i == last_active)
-      continue;
-
-    if ((*i)->profile() == p && (*i)->type() == t)
-      return *i;
+  list_type::reverse_iterator browser = last_active_browsers_.rbegin();
+  for (; browser != last_active_browsers_.rend(); ++browser) {
+    if (BrowserMatchesProfileAndType(*browser, p, t, false))
+      return *browser;
   }
   return NULL;
 }
 
 // static
 Browser* BrowserList::FindBrowserWithProfile(Profile* p) {
-  Browser* last_active = GetLastActive();
-  if (last_active && last_active->profile() == p)
-    return last_active;
-
-  BrowserList::const_iterator i;
-  for (i = BrowserList::begin(); i != BrowserList::end(); ++i) {
-    if (*i == last_active)
-      continue;
-
-    if ((*i)->profile() == p)
-      return *i;
+  list_type::reverse_iterator browser = last_active_browsers_.rbegin();
+  for (; browser != last_active_browsers_.rend(); ++browser) {
+    if (BrowserMatchesProfileAndType(*browser, p, Browser::TYPE_NORMAL, true))
+      return *browser;
   }
   return NULL;
 }
@@ -329,8 +325,8 @@ size_t BrowserList::GetBrowserCountForType(Profile* p, Browser::Type type) {
   BrowserList::const_iterator i;
   size_t result = 0;
   for (i = BrowserList::begin(); i != BrowserList::end(); ++i) {
-    if ((*i)->profile() == p && (*i)->type() == type)
-      result++;
+    if (BrowserMatchesProfileAndType(*i, p, type, false))
+      ++result;
   }
   return result;
 }
@@ -340,7 +336,7 @@ size_t BrowserList::GetBrowserCount(Profile* p) {
   BrowserList::const_iterator i;
   size_t result = 0;
   for (i = BrowserList::begin(); i != BrowserList::end(); ++i) {
-    if ((*i)->profile() == p)
+    if (BrowserMatchesProfileAndType(*i, p, Browser::TYPE_NORMAL, true))
       result++;
   }
   return result;
diff --git a/chrome/browser/browser_list.h b/chrome/browser/browser_list.h
index b510b8c..45a0246 100644
--- a/chrome/browser/browser_list.h
+++ b/chrome/browser/browser_list.h
@@ -58,14 +58,14 @@ class BrowserList {
   // open browser owned by |profile| is returned. If none exist, returns NULL.
   static Browser* GetLastActiveWithProfile(Profile *profile);
 
-  // Find an existing browser window with the provided type. If the last active
-  // has the right type, it is returned. Otherwise, the next available browser
-  // is returned. Returns NULL if no such browser currently exists.
+  // Find an existing browser window with the provided type. Searches in the
+  // order of last activation. Only browsers that have been active can be
+  // returned. Returns NULL if no such browser currently exists.
   static Browser* FindBrowserWithType(Profile* p, Browser::Type t);
 
-  // Find an existing browser window with the provided profile. If the last
-  // active has the right profile, it is returned.  Returns NULL if no such
-  // browser currently exists.
+  // Find an existing browser window with the provided profile. Searches in the
+  // order of last activation. Only browsers that have been active can be
+  // returned. Returns NULL if no such browser currently exists.
   static Browser* FindBrowserWithProfile(Profile* p);
 
   // Find an existing browser with the provided ID. Returns NULL if no such