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. I now also maintain a fallback to walk the entire registered list of browsers forward if the active scan fails. This is likely only in a testing environment where a Browser may never have been activated.

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

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

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

4 files changed, 65 insertions, 49 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.h b/chrome/browser/browser.h
index 2372f95..a7a3160 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -48,10 +48,11 @@ class Browser : public TabStripModelDelegate,
   // TODO(sky): move into a common place that is referenced by both ui_tests
   // and chrome.
   enum Type {
-    TYPE_NORMAL = 0,
-    TYPE_POPUP = 1,
-    TYPE_APP = 2,
+    TYPE_NORMAL = 1,
+    TYPE_POPUP = 2,
+    TYPE_APP = 4,
     TYPE_APP_POPUP = TYPE_APP | TYPE_POPUP,
+    TYPE_ANY = TYPE_NORMAL | TYPE_POPUP | TYPE_APP
   };
 
   // Possible elements of the Browser window.
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index aba80b7..dd7b9dc 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -79,6 +79,45 @@ class BrowserActivityObserver : public NotificationObserver {
 
 BrowserActivityObserver* activity_observer = NULL;
 
+// Returns true if the specified |browser| has a matching profile and type to
+// those specified. |type| can also be TYPE_ANY, which means only |profile|
+// must be matched.
+bool BrowserMatchesProfileAndType(Browser* browser,
+                                  Profile* profile,
+                                  Browser::Type type) {
+  return (type == Browser::TYPE_ANY || browser->type() == type) &&
+      browser->profile() == profile;
+}
+
+// Finds a registered Browser object matching |profile| and |type|. This
+// walks the list of Browsers that have ever been activated from most recently
+// activated to least. If a Browser has never been activated, such as in a test
+// scenario, this function will _not_ find it. Fall back to
+// FindBrowserMatching() in that case.
+Browser* FindInLastActiveMatching(Profile* profile, Browser::Type type) {
+  BrowserList::list_type::const_reverse_iterator browser =
+      BrowserList::begin_last_active();
+  for (; browser != BrowserList::end_last_active(); ++browser) {
+    if (BrowserMatchesProfileAndType(*browser, profile, type))
+      return *browser;
+  }
+  return NULL;
+}
+
+// Finds a registered Browser object matching |profile| and |type| even if that
+// Browser has never been activated. This is a forward walk, and intended as a
+// last ditch fallback mostly to handle tests run on machines where no window is
+// ever activated. The user experience if this function is relied on is not good
+// since matching browsers will be returned in registration (creation) order.
+Browser* FindBrowserMatching(Profile* profile, Browser::Type type) {
+  BrowserList::const_iterator browser = BrowserList::begin();
+  for (; browser != BrowserList::end(); ++browser) {
+    if (BrowserMatchesProfileAndType(*browser, profile, type))
+      return *browser;
+  }
+  return NULL;
+}
+
 }  // namespace
 
 BrowserList::list_type BrowserList::browsers_;
@@ -242,7 +281,7 @@ 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_ANY))
       return true;
   }
   return false;
@@ -270,48 +309,23 @@ Browser* BrowserList::GetLastActive() {
 
 // static
 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) {
-      return *browser;
-    }
-  }
-
-  return NULL;
+  // We are only interested in last active browsers, so we don't fall back to all
+  // browsers like FindBrowserWith* do.
+  return FindInLastActiveMatching(p, Browser::TYPE_ANY);
 }
 
 // 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;
-  }
-  return NULL;
+  Browser* browser = FindInLastActiveMatching(p, t);
+  // Fall back to a forward scan of all Browsers if no active one was found.
+  return browser ? browser : FindBrowserMatching(p, t);
 }
 
 // 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;
-  }
-  return NULL;
+  Browser* browser = FindInLastActiveMatching(p, Browser::TYPE_ANY);
+  // Fall back to a forward scan of all Browsers if no active one was found.
+  return browser ? browser : FindBrowserMatching(p, Browser::TYPE_ANY);
 }
 
 // static
@@ -329,8 +343,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))
+      ++result;
   }
   return result;
 }
@@ -340,7 +354,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_ANY))
       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