Second attempt at fix for Browser::AddTabWithURL. My first fix fixed

the crash, but I don't think it was really what you were after. I
believe this is it.

I've changed various places in Browser to use CanSupportWindowFeature
rather than SupportsWindowFeature. This gives us the old behavior while
allowing app windows to have tabs, which I believe is what you wanted.

BUG=42914
TEST=see bug

Review URL: http://codereview.chromium.org/1932005

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

1 files changed, 80 insertions, 49 deletions

diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index ae1eb148..1ec797c 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -79,48 +79,58 @@ 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. If |match_incognito| is true, we check both the incognito
-// and regular versions of the profile.
-bool BrowserMatchesProfileAndType(Browser* browser,
-                                  Profile* profile,
-                                  Browser::Type type,
-                                  bool match_incognito) {
-  bool profile_match = match_incognito ?
-      browser->profile()->GetOriginalProfile() ==
-          profile->GetOriginalProfile() :
-      browser->profile() == profile;
-  return (type == Browser::TYPE_ANY || browser->type() == type) &&
-      profile_match;
-}
+// Type used to indicate only the type should be matched.
+const int kMatchNothing                 = 0;
+
+// See BrowserMatches for details.
+const int kMatchOriginalProfile         = 1 << 0;
+
+// See BrowserMatches for details.
+const int kMatchCanSupportWindowFeature = 1 << 1;
+
+// Returns true if the specified |browser| matches the specified arguments.
+// |match_types| is a bitmask dictating what parameters to match:
+// . If it contains kMatchOriginalProfile then the original profile of the
+//   browser must match |profile->GetOriginalProfile()|. This is used to match
+//   incognito windows.
+// . If it contains kMatchCanSupportWindowFeature
+//   |CanSupportWindowFeature(window_feature)| must return true.
+bool BrowserMatches(Browser* browser,
+                    Profile* profile,
+                    Browser::Type type,
+                    Browser::WindowFeature window_feature,
+                    uint32 match_types) {
+  if (match_types & kMatchCanSupportWindowFeature &&
+      !browser->CanSupportWindowFeature(window_feature)) {
+    return false;
+  }
 
-// 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,
-                                  bool match_incognito) {
-  for (BrowserList::BrowserVector::const_reverse_iterator i =
-       BrowserList::begin_last_active(); i != BrowserList::end_last_active();
-       ++i) {
-    if (BrowserMatchesProfileAndType(*i, profile, type, match_incognito))
-      return *i;
+  if (match_types & kMatchOriginalProfile) {
+    if (browser->profile()->GetOriginalProfile() !=
+        profile->GetOriginalProfile())
+      return false;
+  } else if (browser->profile() != profile) {
+    return false;
   }
-  return NULL;
+
+  if (type != Browser::TYPE_ANY && browser->type() != type)
+    return false;
+
+  return true;
 }
 
-// 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,
-                             bool match_incognito) {
-  for (BrowserList::const_iterator i = BrowserList::begin();
-       i != BrowserList::end(); ++i) {
-    if (BrowserMatchesProfileAndType(*i, profile, type, match_incognito))
+// Returns the first browser in the specified iterator that returns true from
+// |BrowserMatches|, or null if no browsers match the arguments. See
+// |BrowserMatches| for details on the arguments.
+template <class T>
+Browser* FindBrowserMatching(const T& begin,
+                             const T& end,
+                             Profile* profile,
+                             Browser::Type type,
+                             Browser::WindowFeature window_feature,
+                             uint32 match_types) {
+  for (T i = begin; i != end; ++i) {
+    if (BrowserMatches(*i, profile, type, window_feature, match_types))
       return *i;
   }
   return NULL;
@@ -291,12 +301,11 @@ void BrowserList::WindowsSessionEnding() {
 
 // static
 bool BrowserList::HasBrowserWithProfile(Profile* profile) {
-  for (BrowserList::const_iterator i = BrowserList::begin();
-       i != BrowserList::end(); ++i) {
-    if (BrowserMatchesProfileAndType(*i, profile, Browser::TYPE_ANY, false))
-      return true;
-  }
-  return false;
+  return FindBrowserMatching(BrowserList::begin(),
+                             BrowserList::end(),
+                             profile, Browser::TYPE_ANY,
+                             Browser::FEATURE_NONE,
+                             kMatchNothing) != NULL;
 }
 
 // static
@@ -329,15 +338,35 @@ Browser* BrowserList::GetLastActive() {
 Browser* BrowserList::GetLastActiveWithProfile(Profile* p) {
   // 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, false);
+  return FindBrowserMatching(
+      BrowserList::begin_last_active(), BrowserList::end_last_active(), p,
+      Browser::TYPE_ANY, Browser::FEATURE_NONE, kMatchNothing);
 }
 
 // static
 Browser* BrowserList::FindBrowserWithType(Profile* p, Browser::Type t,
                                           bool match_incognito) {
-  Browser* browser = FindInLastActiveMatching(p, t, match_incognito);
+  uint32 match_types = match_incognito ? kMatchOriginalProfile : kMatchNothing;
+  Browser* browser = FindBrowserMatching(
+      BrowserList::begin_last_active(), BrowserList::end_last_active(),
+      p, t, Browser::FEATURE_NONE, match_types);
   // Fall back to a forward scan of all Browsers if no active one was found.
-  return browser ? browser : FindBrowserMatching(p, t, match_incognito);
+  return browser ? browser :
+      FindBrowserMatching(BrowserList::begin(), BrowserList::end(), p, t,
+                          Browser::FEATURE_NONE, match_types);
+}
+
+// static
+Browser* BrowserList::FindBrowserWithFeature(Profile* p,
+                                             Browser::WindowFeature feature) {
+  Browser* browser = FindBrowserMatching(
+      BrowserList::begin_last_active(), BrowserList::end_last_active(),
+      p, Browser::TYPE_ANY, Browser::FEATURE_NONE, kMatchNothing);
+  // Fall back to a forward scan of all Browsers if no active one was found.
+  return browser ? browser :
+      FindBrowserMatching(BrowserList::begin(), BrowserList::end(), p,
+                          Browser::TYPE_ANY, Browser::FEATURE_NONE,
+                          kMatchNothing);
 }
 
 // static
@@ -360,7 +389,7 @@ size_t BrowserList::GetBrowserCountForType(Profile* p, Browser::Type type) {
   size_t result = 0;
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
-    if (BrowserMatchesProfileAndType(*i, p, type, false))
+    if (BrowserMatches(*i, p, type, Browser::FEATURE_NONE, kMatchNothing))
       ++result;
   }
   return result;
@@ -371,8 +400,10 @@ size_t BrowserList::GetBrowserCount(Profile* p) {
   size_t result = 0;
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
-    if (BrowserMatchesProfileAndType(*i, p, Browser::TYPE_ANY, false))
+    if (BrowserMatches(*i, p, Browser::TYPE_ANY, Browser::FEATURE_NONE,
+                       kMatchNothing)) {
       result++;
+    }
   }
   return result;
 }