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

chrome/browser/ui/browser.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/browser.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #endif  // OS_WIN
 
 #include <algorithm>
 #include <string>
 
 #include "base/base_paths.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
+#include "base/string_number_conversions.h"
 #include "base/string_util.h"
-#include "base/string_number_conversions.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/autofill/autofill_manager.h"
 #include "chrome/browser/background/background_contents_service.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 19 matching lines @@
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/instant/instant_controller.h"
 #include "chrome/browser/instant/instant_unload_handler.h"
 #include "chrome/browser/net/browser_url_util.h"
 #include "chrome/browser/net/url_fixer_upper.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/platform_util.h"
 #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"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/sync_ui_util.h"
 #include "chrome/browser/tab_closeable_state_watcher.h"
 #include "chrome/browser/tab_contents/background_contents.h"
@@ skipping 964 matching lines @@
 
   // Sets the confirmation state to NOT_PROMPTED so that if the user tries to
   // close again we'll show the warning again.
   cancel_download_confirmation_state_ = NOT_PROMPTED;
 
   // Show the download page so the user can figure-out what downloads are still
   // in-progress.
   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:
 
 int Browser::tab_count() const {
   return tab_handler_->GetTabStripModel()->count();
 }
 
 int Browser::active_index() const {
   return tab_handler_->GetTabStripModel()->active_index();
 }
@@ skipping 3337 matching lines @@
       MessageLoop::current()->PostTask(
           FROM_HERE,
           method_factory_.NewRunnableMethod(&Browser::ProcessPendingTabs));
     }
   }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // 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() {
   if (cancel_download_confirmation_state_ != NOT_PROMPTED) {
     if (cancel_download_confirmation_state_ == WAITING_FOR_RESPONSE) {
       // We need to hear from the user before we can close.
       return false;
     }
     // 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)
+
+  DownloadClosePreventionType type;
+  int num_downloads_blocking;
+  if (OkToCloseWithInProgressDownloads(&type, &num_downloads_blocking))
     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)
-    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();
 
   // Return false so the browser does not close.  We'll close if the user
   // confirms in the dialog.
   return false;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, Assorted utility functions (private):
@@ skipping 281 matching lines @@
   window_->BookmarkBarStateChanged(animate_type);
 }
 
 void Browser::ShowSyncSetup() {
   ProfileSyncService* service = profile()->GetProfileSyncService();
   if (service->HasSyncSetupCompleted())
     ShowOptionsTab(chrome::kSyncSetupSubPage);
   else
     profile()->GetProfileSyncService()->ShowLoginDialog();
 }