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

chrome/browser/ui/browser_close_browsertest.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 "base/logging.h"
+#include "base/stringprintf.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/download/download_item.h"
+#include "chrome/browser/download/download_test_observer.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+#include "content/browser/net/url_request_slow_download_job.h"
+#include "content/browser/tab_contents/tab_contents.h"
+#include "content/common/page_transition_types.h"
+#include "content/common/url_constants.h"
+
+class BrowserCloseTest : public InProcessBrowserTest {
+ protected:
+  Profile* CreateNewProfile() {

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

Why is this here?

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

See my answer to the question about the disabled test, below.

+    NOTREACHED();
+    return NULL;
+  }
+
+  void CreateStalledDownload(Browser* browser, int num_downloads) {

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

Please add descriptions for the methods that aren't obvious, especially ones
that have out params.

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

Done.

+    GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl);
+
+    if (num_downloads == 0)
+      return;
+
+    // Setup an observer waiting for the given number of downloads
+    // to get to IN_PROGRESS.
+    DownloadManager* download_manager =
+        browser->profile()->GetDownloadManager();
+    scoped_ptr<DownloadTestObserver> observer(
+        new DownloadTestObserver(
+            download_manager, num_downloads,
+            DownloadItem::IN_PROGRESS,
+            true,                           // Bail on select file
+            DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
+
+    // Set of that number of downloads.
+    while (num_downloads--)
+      ui_test_utils::NavigateToURLWithDisposition(
+          browser, url, NEW_BACKGROUND_TAB,
+          ui_test_utils::BROWSER_TEST_NONE);
+
+    // Wait for them.
+    observer->WaitForFinished();
+  }
+
+  void CompleteAllDownloads() {
+    GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl);
+    ui_test_utils::NavigateToURL(browser(), finish_url);
+
+    // Go through and, for every single profile, wait until there are
+    // no active downloads on that download manager.
+    std::vector<Profile*> profiles(
+        g_browser_process->profile_manager()->GetLoadedProfiles());
+    for (std::vector<Profile*>::const_iterator pit = profiles.begin();
+         pit != profiles.end(); ++pit) {
+      if ((*pit)->HasCreatedDownloadManager()) {
+        DownloadManager *mgr = (*pit)->GetDownloadManager();
+        scoped_refptr<DownloadTestFlushObserver> observer(
+            new DownloadTestFlushObserver(mgr));
+        observer->WaitForFlush();
+      }
+    }
+  }
+
+  Browser* CreateBrowserOnProfile(Profile* profile) {
+    Browser* new_browser = Browser::Create(profile);
+    new_browser->AddSelectedTabWithURL(GURL(chrome::kAboutBlankURL),
+                                       PageTransition::START_PAGE);
+    ui_test_utils::WaitForNavigation(
+        &new_browser->GetSelectedTabContents()->controller());
+    new_browser->window()->Show();
+    return new_browser;
+  }
+
+  // Assumes a single browser on the profile passed in in *base_browser.
+  bool AdjustBrowsersOnProfile(Browser** base_browser, int num_windows) {
+    Browser::DownloadClosePreventionType type;
+    int num_downloads_blocking;
+    if (num_windows == 0) {
+      if (!(*base_browser)->OkToCloseWithInProgressDownloads(
+              &type, &num_downloads_blocking))
+        return false;
+      (*base_browser)->window()->Close();
+      *base_browser = 0;
+      return true;
+    }
+
+    // num_windows > 0
+    Profile* profile((*base_browser)->profile());
+    for (int w = 1; w < num_windows; w++) {
+      CreateBrowserOnProfile(profile);
+    }
+    return true;
+  }
+
+};
+
+struct DownloadsCloseCheckCase {
+  std::string DebugString() const;
+
+  // Input
+  struct {
+    struct {
+      int windows;
+      int downloads;
+    } regular;
+    struct {
+      int windows;
+      int downloads;
+    } incognito;
+  } profile_a;
+
+  struct {
+    struct {
+      int windows;
+      int downloads;
+    } regular;
+    struct {
+      int windows;
+      int downloads;
+    } incognito;
+  } profile_b;
+
+  // We always probe a window in profile A.
+  enum { REGULAR = 0, INCOGNITO = 1 } window_to_probe;

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

enums should be first.

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

This line declares a member variable window_to_probe using an anonymous enum;
it's not really an enum declaration.  If I move the enum first, I have to name
it and create two lines where there was one before.  If that's what you want,
I'll do it, but I want to just confirm that?

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

Fair enough. Leave it.

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

Done.

+
+  // Output
+  bool warn;  // Controlling; if we don't warn, the rest aren't checked.
+
+  Browser::DownloadClosePreventionType type;
+  int num_blocking;
+};
+
+std::string DownloadsCloseCheckCase::DebugString() const {
+  std::string result;
+  result += "Case: {";
+  if (profile_a.regular.windows || profile_a.regular.downloads)
+    result += base::StringPrintf("Regular profile A: (%d w, %d d), ",
+                                 profile_a.regular.windows,
+                                 profile_a.regular.downloads);
+  if (profile_a.incognito.windows || profile_a.incognito.downloads)
+    result += base::StringPrintf("Incognito profile A: (%d w, %d d), ",
+                                 profile_a.incognito.windows,
+                                 profile_a.incognito.downloads);
+  if (profile_b.regular.windows || profile_b.regular.downloads)
+    result += base::StringPrintf("Regular profile B: (%d w, %d d), ",
+                                 profile_b.regular.windows,
+                                 profile_b.regular.downloads);
+  if (profile_b.incognito.windows || profile_b.incognito.downloads)
+    result += base::StringPrintf("Incognito profile B: (%d w, %d d), ",
+                                 profile_b.incognito.windows,
+                                 profile_b.incognito.downloads);
+  result += (window_to_probe == REGULAR ? "Probe regular" :
+             window_to_probe == INCOGNITO ? "Probe incognito" :
+             "Probe unknown");
+  result += "} -> ";
+  if (!warn) {
+    result += "No warning";
+  } else {
+    result += base::StringPrintf(
+        "%s (%d downloads) warning",
+        (type == Browser::BROWSER_SHUTDOWN ? "Browser shutdown" :
+         type == Browser::LAST_WINDOW_IN_INCOGNITO_PROFILE ? "Incognito close" :
+         "Unknown"), num_blocking);
+  }
+  return result;
+}
+
+static const DownloadsCloseCheckCase download_close_check_cases[] = {
+  // Top level nesting is {profile_a, profile_b}
+  // Second level nesting is {regular, incognito
+  // Third level (inner) nesting is {windows, downloads}
+
+  // Last window (incognito) triggers browser close warning.
+  {{{0, 0}, {1, 1}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::INCOGNITO,
+   true, Browser::BROWSER_SHUTDOWN, 1},
+
+  // Last incognito window triggers incognito close warning.
+  {{{1, 0}, {1, 1}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::INCOGNITO,
+   true, Browser::LAST_WINDOW_IN_INCOGNITO_PROFILE, 1},
+
+  // Last incognito window with no downloads triggers no warning.
+  {{{0, 0}, {1, 0}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::INCOGNITO,
+   1},
+
+  // Last incognito window with window+download on another incognito profile
+  // triggers no warning.
+  {{{0, 0}, {1, 0}}, {{0, 0}, {1, 1}}, DownloadsCloseCheckCase::INCOGNITO,
+   1},
+
+  // Non-last incognito window triggers no warning.
+  {{{0, 0}, {2, 1}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::INCOGNITO,
+   1},
+
+  // Non-last regular window triggers no warning.
+  {{{2, 1}, {0, 0}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   1},
+
+  // Last regular window triggers browser close.
+  {{{1, 1}, {0, 0}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   true, Browser::BROWSER_SHUTDOWN, 1},
+
+  // Last regular window triggers browser close for download on different
+  // profile.
+  {{{1, 0}, {0, 0}}, {{0, 1}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   true, Browser::BROWSER_SHUTDOWN, 1},
+
+  // Last regular window triggers no warning if incognito
+  // active (http://crbug.com/61257).
+  {{{1, 0}, {1, 1}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   1},
+
+  // Last regular window triggers no warning if other profile window active.
+  {{{1, 1}, {0, 0}}, {{1, 0}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   1},
+
+  // Last regular window triggers no warning if other incognito window
+  // active.
+  {{{1, 0}, {0, 0}}, {{0, 0}, {1, 1}}, DownloadsCloseCheckCase::REGULAR,
+   1},
+
+  // Last regular window triggers no warning if incognito active.
+  {{{1, 1}, {1, 0}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   1},
+
+  // Test plural for regular.
+  {{{1, 2}, {0, 0}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::REGULAR,
+   true, Browser::BROWSER_SHUTDOWN, 2},
+
+  // Test plural for incognito.
+  {{{1, 0}, {1, 2}}, {{0, 0}, {0, 0}}, DownloadsCloseCheckCase::INCOGNITO,
+   true, Browser::LAST_WINDOW_IN_INCOGNITO_PROFILE, 2},
+};
+
+IN_PROC_BROWSER_TEST_F(BrowserCloseTest, DISABLED_DownloadsCloseCheck) {

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

How come you're checking in a test that's disabled?

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

So if you'd like me to make that a separate CL, I'm willing; this is just trying
to write the automated testing for a change at the same time as I write the
change.  The code to support that test (the code to create a new profile for
testing in multi-profile context, which would result in filling in
CreateNewProfile() above) isn't written yet, and I'm worried about getting my
change into MS-14 if I wait on it, so I just barreled ahead doing as much as I
could.  

Shall I split this portion of the change out?  If so, is it ok if I leave the
refactor of DownloadTestObserver in?  That's potentially useful for other
people, and it was finicky enough to do that I don't look forward to reverting
it and doing it again later.  Without this test, it doesn't strictly speaking
have much to do with this CL, but it doesn't change functionality either--it's
just a refactor.

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

My vote goes for separate cl with test in it.

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

The postponement of multi-profile has made committing this CL less urgent; I'll
try to get the auto-tests running before commit, and fall back to a split if I
need to commit without them.

+  Profile* profile_a = browser()->profile();
+  Profile* profile_b = CreateNewProfile();
+
+  for (size_t i = 0; i < arraysize(download_close_check_cases); ++i) {
+    const DownloadsCloseCheckCase& check_case(download_close_check_cases[i]);
+
+    // Loop invariant; so that we don't actually try and close the browser,
+    // we always enter the loop with a single browser window open on the
+    // main profile.  That means we need to exit the loop the same way.
+    // So we setup everything except for the profile_a regular, and then
+    // flip the bit on the main window.
+    // Note that this means that browser() is unreliable in the context
+    // of this function; we'll be killing that main window and recreating
+    // it fairly frequently.
+
+    ASSERT_EQ(1u, BrowserList::size());
+    Browser* entry_browser = *BrowserList::begin();
+    ASSERT_EQ(profile_a, entry_browser->profile());
+    int total_downloads = 0;
+    int profile_downloads = 0;
+    entry_browser->CheckDownloadsInProgress(
+        &total_downloads, &profile_downloads);
+    ASSERT_EQ(0, total_downloads);
+
+    Profile* profile_a_incognito = profile_a->GetOffTheRecordProfile();
+    Profile* profile_b_incognito = profile_b->GetOffTheRecordProfile();
+
+    // For simplicty of coding, we create a window on each profile so that
+    // we can easily create downloads, then we destroy or create windows
+    // as necessary.
+    Browser* browser_a_regular(CreateBrowserOnProfile(profile_a));
+    Browser* browser_a_incognito(CreateBrowserOnProfile(profile_a_incognito));
+    Browser* browser_b_regular(CreateBrowserOnProfile(profile_b));
+    Browser* browser_b_incognito(CreateBrowserOnProfile(profile_b_incognito));
+
+    // Kill our entry browser.
+    entry_browser->window()->Close();
+    entry_browser = NULL;
+
+    // Create all downloads needed.
+    CreateStalledDownload(
+        browser_a_regular, check_case.profile_a.regular.downloads);
+    CreateStalledDownload(
+        browser_a_incognito, check_case.profile_a.incognito.downloads);
+    CreateStalledDownload(
+        browser_b_regular, check_case.profile_b.regular.downloads);
+    CreateStalledDownload(
+        browser_b_incognito, check_case.profile_b.incognito.downloads);
+
+    // Adjust the windows
+    ASSERT_TRUE(AdjustBrowsersOnProfile(
+        &browser_a_regular, check_case.profile_a.regular.windows));
+    ASSERT_TRUE(AdjustBrowsersOnProfile(
+        &browser_a_incognito, check_case.profile_a.incognito.windows));
+    ASSERT_TRUE(AdjustBrowsersOnProfile(
+        &browser_b_regular, check_case.profile_b.regular.windows));
+    ASSERT_TRUE(AdjustBrowsersOnProfile(
+        &browser_b_incognito, check_case.profile_b.incognito.windows));
+
+    // All that work, for this one little test.
+    ASSERT_TRUE((check_case.window_to_probe ==
+                 DownloadsCloseCheckCase::REGULAR) ||
+                (check_case.window_to_probe ==
+                 DownloadsCloseCheckCase::INCOGNITO));
+
+    Browser::DownloadClosePreventionType type;
+    int num_downloads_blocking;
+    bool result =
+        (check_case.window_to_probe == DownloadsCloseCheckCase::REGULAR ?
+         browser_a_regular :
+         browser_a_incognito)->OkToCloseWithInProgressDownloads(
+             &type, &num_downloads_blocking);
+    EXPECT_EQ(check_case.warn, result) << check_case.DebugString();
+    if (!result) {
+      EXPECT_EQ(check_case.type, type) << check_case.DebugString();
+      EXPECT_EQ(check_case.num_blocking, num_downloads_blocking)
+          << check_case.DebugString();
+    }
+
+    // Release and all the downloads
+    CompleteAllDownloads();
+
+    // Create a new main window and kill everything else.
+    entry_browser = CreateBrowserOnProfile(profile_a);
+    if (browser_a_regular)
+      browser_a_regular->window()->Close();
+    if (browser_a_incognito)
+      browser_a_incognito->window()->Close();
+    if (browser_b_regular)
+      browser_b_regular->window()->Close();
+    if (browser_b_incognito)
+      browser_b_incognito->window()->Close();
+  }
+}