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/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
+#include "base/string_util.h"
 #include "base/stringprintf.h"
-#include "base/string_util.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/autofill/personal_data_manager_factory.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"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/character_encoding.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 #include "chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h"
 #include "chrome/browser/debugger/devtools_toggle_action.h"
 #include "chrome/browser/debugger/devtools_window.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_started_animation.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/default_apps_trial.h"
 #include "chrome/browser/extensions/extension_browser_event_router.h"
 #include "chrome/browser/extensions/extension_disabled_infobar_delegate.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_helper.h"
@@ skipping 13 matching lines @@
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prerender/prerender_tab_helper.h"
 #include "chrome/browser/printing/background_printing_manager.h"
 #include "chrome/browser/printing/cloud_print/cloud_print_setup_flow.h"
 #include "chrome/browser/printing/print_preview_tab_controller.h"
 #include "chrome/browser/printing/print_view_manager.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 943 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();
 }
 
+Browser::DownloadClosePreventionType Browser::OkToCloseWithInProgressDownloads(
+    int* num_downloads_blocking) const {
+  DCHECK(num_downloads_blocking);
+

[achuithb, 2011/10/13 23:05:08]

I'd set *num_downloads_blocking to 0 here since an uninitialized value is and
could be passed in.

[Randy Smith (Not in Mondays), 2011/10/14 00:37:41]

Done.

+  if (is_attempting_to_close_browser_)
+    return DOWNLOAD_CLOSE_OK;
+
+  // If we're not running a full browser process with a profile manager
+  // (testing), it's ok to close the browser.
+  if (!g_browser_process->profile_manager())
+    return DOWNLOAD_CLOSE_OK;
+
+  int total_download_count = DownloadService::TotalDownloadCount();
+  if (total_download_count == 0)
+    return DOWNLOAD_CLOSE_OK;   // No downloads; can definitely close.
+

[achuithb, 2011/10/13 23:05:08]

Have you considered combining the above 3 conditions into 1 statement? Also,
total_download_count seems unnecessary to me.

[Randy Smith (Not in Mondays), 2011/10/14 00:37:41]

My preference is for it being the way it is now; I think it makes the logic
clearer.  (And I had enough headaches trying to get the logic clear in my head
for this routine, albeit for the stuff below, that I don't want to make it less
clear intentionally :-}).
Syntactically or semantically?  If syntactically, it's used below.  If
semantically...well, I need to check it at some point in the code.  I could
avoid the check if there are any windows around, but I can avoid the window
check if there aren't any downloads around.  It seems like six of one, half a
dozen of the other.  Do you see a reason to only computer the total download
count if we're at the last window, as opposed to doing it the other way?

[achuithb, 2011/10/14 19:22:10]

That's fine.
 
I was wrong - I didn't see it being used below. Sorry for the confusion.

+  // Let's figure out if we are the last window for our profile, and
+  // 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 both of the following are true:
+  // 1. There are no downloads present for any profile or it is not the
+  //    last Browser.
+  // 2. (Incognito Browser) There are no incognito downloads associated with
+  //    the Browser's profile or it is not the last Browser associated with
+  //    that profile.
+  // 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;

[achuithb, 2011/10/13 23:05:08]

Does it make sense to move lines 1072-1085 into a separate function? Maybe in
BrowserList? I feel like this function is rather long making it difficult to
follow.

[Randy Smith (Not in Mondays), 2011/10/14 00:37:41]

Huh.  I don't have that feeling about it.  The *logic* of the function is
convoluted, I agree, but I don't actually think subroutinizing it would help
(I'd love to come up with better comment phrasing, though).  But the function
doesn't seem that long to me--only 64 lines, 14 of which are a big old comment
in the middle.

I've played with the comments (eliminated the big one, actually); see if you're
more comfortable with the routine now.

[achuithb, 2011/10/14 19:22:10]

Yup, definitely better since it fits on my 15" screen now.

+  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.
+    Browser* const browser = *iter;
+    if (browser == this ||
+        browser->is_attempting_to_close_browser_)

[sky, 2011/10/13 20:23:43]

nit: move this up to the previous line.

[achuithb, 2011/10/13 23:05:08]

You lost the condition to guard against counting popups/non-tabbed browser
windows. Was that intentional?

[Randy Smith (Not in Mondays), 2011/10/14 00:37:41]

Done.

[Randy Smith (Not in Mondays), 2011/10/14 00:37:41]

Yes.  See earlier comment on this CL @ 
http://codereview.chromium.org/7466033/diff/4001/chrome/browser/ui/browser.cc...

+      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) {
+    *num_downloads_blocking = total_download_count;
+    return DOWNLOAD_CLOSE_BROWSER_SHUTDOWN;
+  }
+
+  // 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.
+  DownloadService* download_service =
+      DownloadServiceFactory::GetForProfile(profile());
+  if (profile_window_count == 0 && download_service->DownloadCount() > 0 &&
+      profile()->IsOffTheRecord()) {
+    *num_downloads_blocking = download_service->DownloadCount();
+    return DOWNLOAD_CLOSE_LAST_WINDOW_IN_INCOGNITO_PROFILE;
+  }
+
+  // Those are the only conditions under which we will block shutdown.
+  return DOWNLOAD_CLOSE_OK;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // 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 3795 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;
-  DownloadService* download_service =
-      DownloadServiceFactory::GetForProfile(profile());
-  // 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 (download_service->HasCreatedDownloadManager())
-    download_manager = download_service->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.
-    DownloadService* download_service = DownloadServiceFactory::GetForProfile(
-        profile()->GetOriginalProfile());
-    if (download_service->HasCreatedDownloadManager())
-      download_manager = download_service->GetDownloadManager();
-  }
-
-  *normal_downloads_are_present =
-      (download_manager && download_manager->in_progress_count() != 0);
-}
-
 bool Browser::CanCloseWithInProgressDownloads() {
   if (cancel_download_confirmation_state_ != NOT_PROMPTED) {

[achuithb, 2011/10/13 23:05:08]

I know this is not your code, but do you think you could change this to:

if (cancel_download_confirmation_state_ != NOT_PROMPTED)
  return cancel_download_confirmation_state_ != WAITING_FOR_RESPONSE;

It would make this more readable and concise.

[Randy Smith (Not in Mondays), 2011/10/14 00:37:41]

I agree with you, but I'm reluctant to make a change to code I don't own, when
also requested by a non-owner, in an unrelated CL.  Miranda, Scott: If either of
you want to bless this request, I'm happy to do it.

[Randy Smith (Not in Mondays), 2011/10/14 16:13:03]

Done.

     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)
+
+  int num_downloads_blocking;
+  if (DOWNLOAD_CLOSE_OK ==
+      OkToCloseWithInProgressDownloads(&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 328 matching lines @@
     window_->GetLocationBar()->ShowFirstRunBubble(bubble_type);
   } else if (is_type_tabbed()) {
     GlobalErrorService* service =
         GlobalErrorServiceFactory::GetForProfile(profile());
     GlobalError* error = service->GetFirstGlobalErrorWithBubbleView();
     if (error) {
       error->ShowBubbleView(this);
     }
   }
 }