Fix the logic in extensions GetCurrentWindow:

- We try to find an associated window for the calling extension page.
- If there is none (bg pages), fallback to the topmost browser window.
- If the extension is enabled in incognito, include incognito windows in the
search for "topmost".

This fixes a bug where clicking a browser action in an incognito window might open a tab in a normal window, which is confusing.

BUG=39113

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

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

9 files changed, 46 insertions, 39 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index dfb84ba..9fb9c9d 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -807,7 +807,8 @@ void AutomationProvider::GetBrowserWindow(int index, int* handle) {
 void AutomationProvider::FindNormalBrowserWindow(int* handle) {
   *handle = 0;
   Browser* browser = BrowserList::FindBrowserWithType(profile_,
-                                                      Browser::TYPE_NORMAL);
+                                                      Browser::TYPE_NORMAL,
+                                                      false);
   if (browser)
     *handle = browser_tracker_->Add(browser);
 }
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc
index a9aa9b6..5aada78 100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -370,7 +370,7 @@ void OpenAll(gfx::NativeWindow parent,
   NewBrowserPageNavigator navigator_impl(profile);
   if (!navigator) {
     Browser* browser =
-        BrowserList::FindBrowserWithType(profile, Browser::TYPE_NORMAL);
+        BrowserList::FindBrowserWithType(profile, Browser::TYPE_NORMAL, false);
     if (!browser || !browser->GetSelectedTabContents()) {
       navigator = &navigator_impl;
     } else {
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 2f80049..48cb020 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -355,8 +355,7 @@ void Browser::OpenWindowWithRestoredTabs(Profile* profile) {
 void Browser::OpenURLOffTheRecord(Profile* profile, const GURL& url) {
   Profile* off_the_record_profile = profile->GetOffTheRecordProfile();
   Browser* browser = BrowserList::FindBrowserWithType(
-      off_the_record_profile,
-      TYPE_NORMAL);
+      off_the_record_profile, TYPE_NORMAL, false);
   if (!browser)
     browser = Browser::Create(off_the_record_profile);
   // TODO(eroman): should we have referrer here?
@@ -3214,7 +3213,8 @@ bool Browser::CanCloseWithInProgressDownloads() {
 
 // static
 Browser* Browser::GetOrCreateTabbedBrowser(Profile* profile) {
-  Browser* browser = BrowserList::FindBrowserWithType(profile, TYPE_NORMAL);
+  Browser* browser = BrowserList::FindBrowserWithType(profile, TYPE_NORMAL,
+                                                      false);
   if (!browser)
     browser = Browser::Create(profile);
   return browser;
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index 1d97388..ec7865d1 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -81,12 +81,18 @@ 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.
+// 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) {
-  return (type == Browser::TYPE_ANY || browser->type() == type) &&
+                                  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;
 }
 
 // Finds a registered Browser object matching |profile| and |type|. This
@@ -94,11 +100,12 @@ bool BrowserMatchesProfileAndType(Browser* browser,
 // 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) {
+Browser* FindInLastActiveMatching(Profile* profile, Browser::Type type,
+                                  bool match_incognito) {
   for (BrowserList::list_type::const_reverse_iterator i =
        BrowserList::begin_last_active(); i != BrowserList::end_last_active();
        ++i) {
-    if (BrowserMatchesProfileAndType(*i, profile, type))
+    if (BrowserMatchesProfileAndType(*i, profile, type, match_incognito))
       return *i;
   }
   return NULL;
@@ -109,10 +116,11 @@ Browser* FindInLastActiveMatching(Profile* profile, Browser::Type type) {
 // 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) {
+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))
+    if (BrowserMatchesProfileAndType(*i, profile, type, match_incognito))
       return *i;
   }
   return NULL;
@@ -295,7 +303,7 @@ void BrowserList::WindowsSessionEnding() {
 bool BrowserList::HasBrowserWithProfile(Profile* profile) {
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
-    if (BrowserMatchesProfileAndType(*i, profile, Browser::TYPE_ANY))
+    if (BrowserMatchesProfileAndType(*i, profile, Browser::TYPE_ANY, false))
       return true;
   }
   return false;
@@ -327,21 +335,20 @@ 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);
+  return FindInLastActiveMatching(p, Browser::TYPE_ANY, false);
 }
 
 // static
-Browser* BrowserList::FindBrowserWithType(Profile* p, Browser::Type t) {
-  Browser* browser = FindInLastActiveMatching(p, t);
+Browser* BrowserList::FindBrowserWithType(Profile* p, Browser::Type t,
+                                          bool match_incognito) {
+  Browser* browser = FindInLastActiveMatching(p, t, match_incognito);
   // Fall back to a forward scan of all Browsers if no active one was found.
-  return browser ? browser : FindBrowserMatching(p, t);
+  return browser ? browser : FindBrowserMatching(p, t, match_incognito);
 }
 
 // static
 Browser* BrowserList::FindBrowserWithProfile(Profile* p) {
-  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);
+  return FindBrowserWithType(p, Browser::TYPE_ANY, false);
 }
 
 // static
@@ -359,7 +366,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))
+    if (BrowserMatchesProfileAndType(*i, p, type, false))
       ++result;
   }
   return result;
@@ -370,7 +377,7 @@ 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))
+    if (BrowserMatchesProfileAndType(*i, p, Browser::TYPE_ANY, false))
       result++;
   }
   return result;
diff --git a/chrome/browser/browser_list.h b/chrome/browser/browser_list.h
index 8dbe0bd..9ab6e22 100644
--- a/chrome/browser/browser_list.h
+++ b/chrome/browser/browser_list.h
@@ -65,8 +65,11 @@ class BrowserList {
 
   // 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);
+  // returned. If |match_incognito| is true, will match a browser with either
+  // a regular or incognito profile that matches the given one. Returns NULL if
+  // no such browser currently exists.
+  static Browser* FindBrowserWithType(Profile* p, Browser::Type t,
+                                      bool match_incognito);
 
   // Find an existing browser window with the provided profile. Searches in the
   // order of last activation. Only browsers that have been active can be
diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index 76a2300..03889ee 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -347,17 +347,9 @@ Browser* ExtensionFunctionDispatcher::GetCurrentBrowser(
   if (!include_incognito)
     profile = profile->GetOriginalProfile();
 
-  browser = BrowserList::GetLastActiveWithProfile(profile);
+  browser = BrowserList::FindBrowserWithType(profile, Browser::TYPE_ANY,
+                                             include_incognito);
 
-  // It's possible for a browser to exist, but to have never been active.
-  // This can happen if you launch the browser on a machine without an active
-  // desktop (a headless buildbot) or if you quickly give another app focus
-  // at launch time.  This is easy to do with browser_tests.
-  if (!browser)
-    browser = BrowserList::FindBrowserWithProfile(profile);
-
-  // TODO(erikkay): can this still return NULL?  Is Rafael's comment still
-  // valid here?
   // NOTE(rafaelw): This can return NULL in some circumstances. In particular,
   // a toolstrip or background_page onload chrome.tabs api call can make it
   // into here before the browser is sufficiently initialized to return here.
diff --git a/chrome/browser/extensions/extension_incognito_apitest.cc b/chrome/browser/extensions/extension_incognito_apitest.cc
index 64e3ed7..604dd90 100644
--- a/chrome/browser/extensions/extension_incognito_apitest.cc
+++ b/chrome/browser/extensions/extension_incognito_apitest.cc
@@ -29,7 +29,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, IncognitoNoScript) {
   ui_test_utils::OpenURLOffTheRecord(browser()->profile(),
       GURL("http://www.example.com:1337/files/extensions/test_file.html"));
   Browser* otr_browser = BrowserList::FindBrowserWithType(
-      browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL);
+      browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL,
+      false);
   TabContents* tab = otr_browser->GetSelectedTabContents();
 
   // Verify the script didn't run.
@@ -64,7 +65,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, IncognitoYesScript) {
   ui_test_utils::OpenURLOffTheRecord(browser()->profile(),
       GURL("http://www.example.com:1337/files/extensions/test_file.html"));
   Browser* otr_browser = BrowserList::FindBrowserWithType(
-      browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL);
+      browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL,
+      false);
   TabContents* tab = otr_browser->GetSelectedTabContents();
 
   // Verify the script ran.
@@ -125,7 +127,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, IncognitoPopup) {
   ui_test_utils::OpenURLOffTheRecord(browser()->profile(),
       GURL("http://www.example.com:1337/files/extensions/test_file.html"));
   Browser* incognito_browser = BrowserList::FindBrowserWithType(
-      browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL);
+      browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL,
+      false);
 
   // Simulate the incognito's browser action being clicked.
   BrowserActionTestUtil(incognito_browser).Press(0);
diff --git a/chrome/browser/geolocation/geolocation_browsertest.cc b/chrome/browser/geolocation/geolocation_browsertest.cc
index c891db1..0ecaa93 100644
--- a/chrome/browser/geolocation/geolocation_browsertest.cc
+++ b/chrome/browser/geolocation/geolocation_browsertest.cc
@@ -208,7 +208,8 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
     if (options == INITIALIZATION_OFFTHERECORD) {
       ui_test_utils::OpenURLOffTheRecord(browser()->profile(), current_url_);
       current_browser_ = BrowserList::FindBrowserWithType(
-          browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL);
+          browser()->profile()->GetOffTheRecordProfile(), Browser::TYPE_NORMAL,
+          false);
     } else if (options == INITIALIZATION_NEWTAB) {
       current_browser_ = browser();
       current_browser_->NewTab();
diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc
index 781c918..4dd4859 100644
--- a/chrome/test/ui_test_utils.cc
+++ b/chrome/test/ui_test_utils.cc
@@ -457,7 +457,7 @@ Browser* WaitForNewBrowser() {
 void OpenURLOffTheRecord(Profile* profile, const GURL& url) {
   Browser::OpenURLOffTheRecord(profile, url);
   Browser* browser = BrowserList::FindBrowserWithType(
-      profile->GetOffTheRecordProfile(), Browser::TYPE_NORMAL);
+      profile->GetOffTheRecordProfile(), Browser::TYPE_NORMAL, false);
   WaitForNavigations(&browser->GetSelectedTabContents()->controller(), 1);
 }