Fix warning prompting on closing a window that will cancel downloads.

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index be90b942982244963ae8490c68538d3bdb27e199..e6bc1b6a7aa971f047d35838d61e66b710f59160 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -17,8 +17,8 @@
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
-#include "base/string_util.h"
 #include "base/string_number_conversions.h"
+#include "base/string_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
@@ -58,6 +58,7 @@
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/printing/cloud_print/cloud_print_setup_flow.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sessions/restore_tab_helper.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sessions/session_service_factory.h"
@@ -1042,6 +1043,73 @@ void Browser::InProgressDownloadResponse(bool cancel_downloads) {
   ShowDownloadsTab();
 }
 
+bool Browser::OkToCloseWithInProgressDownloads(
+    DownloadClosePreventionType* type,
+    int* num_downloads_blocking) const {
+  DCHECK(type);
+  DCHECK(num_downloads_blocking);
+
+  if (is_attempting_to_close_browser_)
+    return true;
+
+  int total_download_count = 0;
+  int profile_download_count = 0;
+  CheckDownloadsInProgress(&total_download_count, &profile_download_count);
+  if (total_download_count == 0)
+    return true;                        // No downloads; can definitely close.
+
+  // Let's figure out if we are the last window for our profile, and

[sky, 2011/07/21 20:08:11]

window -> tabbed window

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

Done.

+  // the last window period.
+  // Note that we cannot just use BrowserList::GetBrowserCount as browser
+  // windows closing is delayed and the returned count might include windows
+  // that are being closed.
+  // The browser allowed to be closed only if:
+  // 1. It is a regular browser and there are no regular downloads present or
+  //    this is not the last regular browser window.
+  // 2. It is an incognito browser and there are no incognito downloads present
+  //    for this incognito profile or this is not the last incognito
+  //    browser window.
+  // Note the lack of parallelism: regular profiles are kept around until
+  // browser close, whereas incognito profiles are destroyed on last
+  // associated window close.
+  int profile_window_count = 0;
+  int total_window_count = 0;
+  for (BrowserList::const_iterator iter = BrowserList::begin();
+       iter != BrowserList::end(); ++iter) {
+    // Don't count this browser window or any other in the process of closing.
+    // Only consider tabbed browser windows, not popups.
+    Browser* const browser = *iter;
+    if (browser == this
+        || browser->is_attempting_to_close_browser_

[sky, 2011/07/21 20:08:11]

nit: || on previous line

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

Done.

+        || !browser->is_type_tabbed())

[sky, 2011/07/21 20:08:11]

How come non-tabbed windows don't count?

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

That's an excellent question and I don't know the answer; I was copying the code
and only modifying the parts I was clear on.  What we're doing here is trying to
figure out ahead of time what will result in profile destruction (which means
we're trying to keep separate code paths in sync, which is a losing battle, I
just didn't see another way to do it).  If the existence of a non-tabbed windows
won't keep the associated profile alive we should skip them, and if they do, we
shouldn't.  What are the contexts in which we use non-tabbed windows, do you
know?  Does that summary spark any sense of why we might treat them specially?

[sky, 2011/07/22 03:00:40]

Popups are the common case of a non-tabbed browser, and it keeps chrome running.
App windows are the other case, but as common.

[Randy Smith (Not in Mondays), 2011/07/22 20:41:30]

Ok, sounds like I shouldn't exclude non-tabbed stuff then.  Done.

+      continue;
+
+    if ((*iter)->profile() == profile())
+      profile_window_count++;
+    total_window_count++;
+  }
+
+  // If there aren't any other windows, we're at browser shutdown.
+  if (total_window_count == 0) {
+    *type = BROWSER_SHUTDOWN;

[sky, 2011/07/21 20:08:11]

Since this case just means there are no more tabbed browsers, BROWSER_SHUTDOWN
is misleading. How about something like DOWNLOAD_CLOSE_NO_MORE_TABBED_BROWSERS,
or something.

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

Profiles are only killed on browser shutdown, so if non-tabbed windows keep the
browser alive, the bug's in the code above, not here.

[sky, 2011/07/22 03:00:40]

non-tabbed windows definitely keep chrome running.

[Randy Smith (Not in Mondays), 2011/07/22 20:41:30]

Done.

+    *num_downloads_blocking = total_download_count;
+    return false;
+  }
+
+  // If there aren't any other windows on our profile, we're an incognito
+  // profile, and there are downloads associated with that profile,
+  // indicate that.
+  if (profile_window_count == 0 && profile_download_count > 0 &&
+      profile()->IsOffTheRecord()) {
+    *type = LAST_WINDOW_IN_INCOGNITO_PROFILE;

[sky, 2011/07/21 20:08:11]

LAST_WINDOW -> LAST_TABBED_BROWSER

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

Holding off on this until we resolve the tabbed window issue.

+    *num_downloads_blocking = profile_download_count;
+    return false;
+  }
+
+  // Those are the only conditions under which we will block shutdown.
+  return true;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Browser, TabStripModel pass-thrus:
 
@@ -4399,30 +4467,34 @@ void Browser::ClearUnloadState(TabContents* tab, bool process_now) {
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, In-progress download termination handling (private):
 
-void Browser::CheckDownloadsInProgress(bool* normal_downloads_are_present,
-                                       bool* incognito_downloads_are_present) {
-  *normal_downloads_are_present = false;
-  *incognito_downloads_are_present = false;
-
-  // If there are no download in-progress, our job is done.
-  DownloadManager* download_manager = NULL;
-  // But first we need to check for the existence of the download manager, as
-  // GetDownloadManager() will unnecessarily try to create one if it does not
-  // exist.
-  if (profile()->HasCreatedDownloadManager())
-    download_manager = profile()->GetDownloadManager();
-  if (profile()->IsOffTheRecord()) {
-    // Browser is incognito and so download_manager if present is for incognito
-    // downloads.
-    *incognito_downloads_are_present =
-        (download_manager && download_manager->in_progress_count() != 0);
-    // Check original profile.
-    if (profile()->GetOriginalProfile()->HasCreatedDownloadManager())
-      download_manager = profile()->GetOriginalProfile()->GetDownloadManager();
+void Browser::CheckDownloadsInProgress(int* total_download_count,

[sky, 2011/07/21 20:08:11]

This method feels like it should be two methods. One for total download count,
and one for profile download count.

Also, since the implementation is Profile specific and not Browser specific, it
should be some where else (Browser already has enough state). Maybe it should be
on Profile or a static method on DownloadManager that takes a Profile.

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

That makes sense, though I'm not sure where to put them--DownloadManager has the
same problem as Browser.  I've dumped them on Profile and ProfileManager, which
brings mirandac back in as a reviewer.  Miranda, could you take a look at the
new situation?

+                                       int* profile_download_count) const {
+  // Downloads on the current profile are easy.
+  if (profile()->HasCreatedDownloadManager()) {
+    *profile_download_count =
+        profile()->GetDownloadManager()->in_progress_count();
+  } else {
+    *profile_download_count = 0;
   }
 
-  *normal_downloads_are_present =
-      (download_manager && download_manager->in_progress_count() != 0);
+  // Total downloads are a bit harder.
+  std::vector<Profile*> profiles(
+      g_browser_process->profile_manager()->GetLoadedProfiles());
+  int count = 0;
+  std::vector<Profile*>::const_iterator profile_it =

[sky, 2011/07/21 20:08:11]

nit: move this into the for loop

[Randy Smith (Not in Mondays), 2011/07/21 21:36:31]

The loop now matches similar loops in profile_manager.cc which uses indices;
this is now moot.

+      profiles.begin();
+  for (; profile_it != profiles.end(); ++profile_it) {
+    Profile* p = *profile_it;
+    if (p->HasCreatedDownloadManager()) {
+      count += p->GetDownloadManager()->in_progress_count();
+    }
+    if (p->HasOffTheRecordProfile() &&
+        p->GetOffTheRecordProfile()->HasCreatedDownloadManager()) {
+      count += p->GetOffTheRecordProfile()
+          ->GetDownloadManager()->in_progress_count();
+    }
+  }
+  *total_download_count = count;
 }
 
 bool Browser::CanCloseWithInProgressDownloads() {
@@ -4434,61 +4506,14 @@ bool Browser::CanCloseWithInProgressDownloads() {
     // RESPONSE_RECEIVED case, the user decided to go along with the closing.
     return true;
   }
-  // Indicated that normal (non-incognito) downloads are pending.
-  bool normal_downloads_are_present = false;
-  bool incognito_downloads_are_present = false;
-  CheckDownloadsInProgress(&normal_downloads_are_present,
-                           &incognito_downloads_are_present);
-  if (!normal_downloads_are_present && !incognito_downloads_are_present)
-    return true;
-
-  if (is_attempting_to_close_browser_)
-    return true;
-
-  if ((!normal_downloads_are_present && !profile()->IsOffTheRecord()) ||
-      (!incognito_downloads_are_present && profile()->IsOffTheRecord()))
-    return true;
-
-  // Let's figure out if we are the last window for our profile.
-  // Note that we cannot just use BrowserList::GetBrowserCount as browser
-  // windows closing is delayed and the returned count might include windows
-  // that are being closed.
-  // The browser allowed to be closed only if:
-  // 1. It is a regular browser and there are no regular downloads present or
-  //    this is not the last regular browser window.
-  // 2. It is an incognito browser and there are no incognito downloads present
-  //    or this is not the last incognito browser window.
-  int count = 0;
-  for (BrowserList::const_iterator iter = BrowserList::begin();
-       iter != BrowserList::end(); ++iter) {
-    // Don't count this browser window or any other in the process of closing.
-    // Only consider tabbed browser windows, not popups.
-    Browser* const browser = *iter;
-    if (browser == this
-        || browser->is_attempting_to_close_browser_
-        || !browser->is_type_tabbed())
-      continue;
 
-    // Verify that this is not the last non-incognito or incognito browser,
-    // depending on the pending downloads.
-    if (normal_downloads_are_present && !profile()->IsOffTheRecord() &&
-        browser->profile()->IsOffTheRecord())
-      continue;
-    if (incognito_downloads_are_present && profile()->IsOffTheRecord() &&
-        !browser->profile()->IsOffTheRecord())
-      continue;
-
-    // We test the original profile, because an incognito browser window keeps
-    // the original profile alive (and its DownloadManager).
-    // We also need to test explicitly the profile directly so that 2 incognito
-    // profiles count as a match.
-    if ((*iter)->profile() == profile() ||
-        (*iter)->profile()->GetOriginalProfile() == profile())
-      count++;
-  }
-  if (count > 0)
+  DownloadClosePreventionType type;
+  int num_downloads_blocking;
+  if (OkToCloseWithInProgressDownloads(&type, &num_downloads_blocking))
     return true;
 
+  // Closing this window will kill some downloads; prompt to make sure
+  // that's ok.
   cancel_download_confirmation_state_ = WAITING_FOR_RESPONSE;
   window_->ConfirmBrowserCloseWithPendingDownloads();