Fixed issue with DownloadTestObserver.

chrome/browser/download/download_test_observer.cc

Index: chrome/browser/download/download_test_observer.cc
diff --git a/chrome/browser/download/download_test_observer.cc b/chrome/browser/download/download_test_observer.cc
index 0cbcd0cea8b94433513d612add42506e05ecc87b..600ef5c36796fd6f2af959958b8773eb222a5909 100644
--- a/chrome/browser/download/download_test_observer.cc
+++ b/chrome/browser/download/download_test_observer.cc
@@ -19,9 +19,9 @@ using content::DownloadManager;
 // These functions take scoped_refptr's to DownloadManager because they
 // are posted to message queues, and hence may execute arbitrarily after
 // their actual posting.  Once posted, there is no connection between
-// these routines and the DownloadTestObserver class from which they came,
-// so the DownloadTestObserver's reference to the DownloadManager cannot
-// be counted on to keep the DownloadManager around.
+// these routines and the DownloadTestObserver class from which
+// they came, so the DownloadTestObserver's reference to the
+// DownloadManager cannot be counted on to keep the DownloadManager around.
 
 // Fake user click on "Accept".
 void AcceptDangerousDownload(scoped_refptr<DownloadManager> download_manager,
@@ -42,21 +42,15 @@ void DenyDangerousDownload(scoped_refptr<DownloadManager> download_manager,
 DownloadTestObserver::DownloadTestObserver(
     DownloadManager* download_manager,
     size_t wait_count,
-    DownloadItem::DownloadState download_finished_state,
     bool finish_on_select_file,
     DangerousDownloadAction dangerous_download_action)
     : download_manager_(download_manager),
       wait_count_(wait_count),
       finished_downloads_at_construction_(0),
       waiting_(false),
-      download_finished_state_(download_finished_state),
       finish_on_select_file_(finish_on_select_file),
       select_file_dialog_seen_(false),
       dangerous_download_action_(dangerous_download_action) {
-  download_manager_->AddObserver(this);  // Will call initial ModelChanged().
-  finished_downloads_at_construction_ = finished_downloads_.size();
-  EXPECT_NE(DownloadItem::REMOVING, download_finished_state)
-      << "Waiting for REMOVING is not supported.  Try COMPLETE.";
 }
 
 DownloadTestObserver::~DownloadTestObserver() {
@@ -129,9 +123,8 @@ void DownloadTestObserver::OnDownloadUpdated(DownloadItem* download) {
     }
   }
 
-  if (download->GetState() == download_finished_state_) {
+  if (IsDownloadInFinalState(download))
     DownloadInFinalState(download);
-  }
 }
 
 void DownloadTestObserver::ModelChanged(DownloadManager* manager) {
@@ -182,15 +175,28 @@ size_t DownloadTestObserver::NumDangerousDownloadsSeen() const {
   return dangerous_downloads_seen_.size();
 }
 
+size_t DownloadTestObserver::NumDownloadsSeenInState(
+    content::DownloadItem::DownloadState state) const {
+  StateMap::const_iterator it = states_observed_.find(state);
+
+  if (it == states_observed_.end())
+    return 0;
+
+  return it->second;
+}
+
 void DownloadTestObserver::DownloadInFinalState(DownloadItem* download) {
   if (finished_downloads_.find(download) != finished_downloads_.end()) {
-    // We've already seen terminal state on this download.
+    // We've already seen the final state on this download.
     return;
   }
 
   // Record the transition.
   finished_downloads_.insert(download);
 
+  // Record the state.
+  states_observed_[download->GetState()]++;  // Initializes to 0 the first time.
+
   SignalIfFinished();
 }
 
@@ -199,6 +205,50 @@ void DownloadTestObserver::SignalIfFinished() {
     MessageLoopForUI::current()->Quit();
 }
 
+DownloadTestObserverTerminal::DownloadTestObserverTerminal(
+    content::DownloadManager* download_manager,
+    size_t wait_count,
+    bool finish_on_select_file,
+    DangerousDownloadAction dangerous_download_action)
+        : DownloadTestObserver(download_manager,
+                               wait_count,
+                               finish_on_select_file,
+                               dangerous_download_action) {
+  download_manager_->AddObserver(this);  // Will call initial ModelChanged().
+  finished_downloads_at_construction_ = finished_downloads_.size();
+  states_observed_.clear();

[Randy Smith (Not in Mondays), 2012/03/08 18:25:31]

Completely up to you, but why duplicate this code in the derived classes rather
than in the base class constructor?

[ahendrickson, 2012/03/08 20:56:34]

This one is a bit tricky:  In the base class constructor, you can't override the
functions using the virtual function table (it hasn't been set up yet), so it
would be calling its own IsDownloadInFinalState() instead of the derived class'.

[Randy Smith (Not in Mondays), 2012/03/09 18:39:38]

Ah, good catch.  Yikes.  Would you comment that?  

[ahendrickson, 2012/03/09 20:07:23]

I *had* comments to this effect, but they seem to have disappeared.

Done.

+}
+
+DownloadTestObserverTerminal::~DownloadTestObserverTerminal() {
+}
+
+
+bool DownloadTestObserverTerminal::IsDownloadInFinalState(
+    content::DownloadItem* download) {
+  return (download->GetState() != DownloadItem::IN_PROGRESS);
+}
+
+DownloadTestObserverInProgress::DownloadTestObserverInProgress(
+    content::DownloadManager* download_manager,
+    size_t wait_count)
+        : DownloadTestObserver(download_manager,
+                               wait_count,
+                               true,
+                               ON_DANGEROUS_DOWNLOAD_FAIL) {

[Randy Smith (Not in Mondays), 2012/03/08 18:25:31]

Why FAIL rather than IGNORE?  Dangerous downloads won't interfere with
transitioning to IN_PROGRESS, so the class is more flexible without failing (I
think?).

[ahendrickson, 2012/03/08 20:56:34]

I don't think a download class can be declared dangerous before it goes into
IN_PROGRESS, so I don't think it matters what I put here.

There isn't an _IGNORE, but I could use _ACCEPT if you'd prefer.

[Randy Smith (Not in Mondays), 2012/03/09 18:39:38]

My model of the transitions say that it can happen simultaneously (the first
time we see IN_PROGRESS currently is at history addition, which is after several
different types of danger evaluation) and FAIL will result in a failure call in
OnDownloadUpdated.  Accept's not ideal either, but I don't think it matters too
much.  But whatever you do, please comment the header file with details about
what behavior will occur.

[ahendrickson, 2012/03/09 20:07:23]

Your model is correct.

The header file has the comments for this class.

+  download_manager_->AddObserver(this);  // Will call initial ModelChanged().
+  finished_downloads_at_construction_ = finished_downloads_.size();
+  states_observed_.clear();
+}
+
+DownloadTestObserverInProgress::~DownloadTestObserverInProgress() {
+}
+
+
+bool DownloadTestObserverInProgress::IsDownloadInFinalState(
+    content::DownloadItem* download) {
+  return (download->GetState() == DownloadItem::IN_PROGRESS);
+}
+
 DownloadTestFlushObserver::DownloadTestFlushObserver(
     DownloadManager* download_manager)
     : download_manager_(download_manager),