Fixed issue with DownloadTestObserver.

chrome/browser/download/download_test_observer.h

Index: chrome/browser/download/download_test_observer.h
diff --git a/chrome/browser/download/download_test_observer.h b/chrome/browser/download/download_test_observer.h
index 3404f425e3c9d6fc57beb11ee7ca6e49536a251b..3095f40b6e1662d8f571ae852a1292568482bdb9 100644
--- a/chrome/browser/download/download_test_observer.h
+++ b/chrome/browser/download/download_test_observer.h
@@ -13,20 +13,20 @@
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 
-// Construction of this class defines a system state, based on some number
-// of downloads being seen in a particular state + other events that
-// may occur in the download system.  That state will be recorded if it
-// occurs at any point after construction.  When that state occurs, the class
-// is considered finished.  Callers may either probe for the finished state, or
-// wait on it.
+// Detects changes to the downloads after construction.
+// Finishes when one of the following happens:
+//   - A specified number of downloads change to a terminal state (defined
+//     as anything but IN_PROGRESS).
+//   - Specific events, such as a select file dialog.
+// Callers may either probe for the finished state, or wait on it.
 //
 // TODO(rdsmith): Detect manager going down, remove pointer to
 // DownloadManager, transition to finished.  (For right now we
 // just use a scoped_refptr<> to keep it around, but that may cause
 // timeouts on waiting if a DownloadManager::Shutdown() occurs which
 // cancels our in-progress downloads.)
-class DownloadTestObserver : public content::DownloadManager::Observer,
-                             public content::DownloadItem::Observer {
+class DownloadTestObserverTerminal : public content::DownloadManager::Observer,
+                                     public content::DownloadItem::Observer {
  public:
   // Action an observer should take if a dangerous download is encountered.
   enum DangerousDownloadAction {
@@ -36,26 +36,23 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   };
 
   // Create an object that will be considered finished when |wait_count|
-  // download items have entered state |download_finished_state|.
+  // download items have entered a terminal state (either CANCELLED or
+  // another state that you can't normally transition out of).
   // If |finish_on_select_file| is true, the object will also be
   // considered finished if the DownloadManager raises a
   // SelectFileDialogDisplayed() notification.
-
-  // TODO(rdsmith): Consider rewriting the interface to take a list of events
-  // to treat as completion events.
-  DownloadTestObserver(
+  DownloadTestObserverTerminal(
       content::DownloadManager* download_manager,
       size_t wait_count,
-      content::DownloadItem::DownloadState download_finished_state,
       bool finish_on_select_file,
       DangerousDownloadAction dangerous_download_action);
 
-  virtual ~DownloadTestObserver();
+  virtual ~DownloadTestObserverTerminal();
 
   // State accessors.
   bool select_file_dialog_seen() const { return select_file_dialog_seen_; }
 
-  // Wait for whatever state was specified in the constructor.
+  // Wait for the requested number of downloads to enter a terminal state.
   void WaitForFinished();
 
   // Return true if everything's happened that we're configured for.
@@ -73,6 +70,10 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
 
   size_t NumDangerousDownloadsSeen() const;
 
+ protected:
+  // Called to see if a download item is in a final state.
+  virtual bool IsDownloadInFinalState(content::DownloadItem* download);
+
  private:
   typedef std::set<content::DownloadItem*> DownloadSet;
 
@@ -114,11 +115,8 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   // all downloads completing.
   bool waiting_;
 
-  // The state on which to consider the DownloadItem finished.
-  content::DownloadItem::DownloadState download_finished_state_;
-
-  // True if we should transition the DownloadTestObserver to finished if
-  // the select file dialog comes up.
+  // True if we should transition the DownloadTestObserverTerminal to
+  // finished if the select file dialog comes up.
   bool finish_on_select_file_;
 
   // True if we've seen the select file dialog.
@@ -130,7 +128,29 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   // Holds the download ids which were dangerous.
   std::set<int32> dangerous_downloads_seen_;
 
-  DISALLOW_COPY_AND_ASSIGN(DownloadTestObserver);
+  DISALLOW_COPY_AND_ASSIGN(DownloadTestObserverTerminal);
+};
+
+// Detects changes to the downloads after construction.
+// Finishes when one of the following happens:
+//   - A specified number of downloads change to the IN_PROGRESS state.
+// Callers may either probe for the finished state, or wait on it.
+//
+class DownloadTestObserverInProgress : public DownloadTestObserverTerminal {

[Randy Smith (Not in Mondays), 2012/03/07 21:39:16]

This is test code, so I won't insist on a change, but the right thing to do here
is to have a separate class that has all the shared functionality for
*InProgress and *Terminal and have them both inherit from it.  If anyone were
ever to take a *InProgress and cast it to a *Terminal they'd get a really bad
surprise (which is what I meant by there not being an ISA relationship between
the two classes). 

It's possible that you could get the proper behavior (ban the ISA relationship)
by making the inheritance private (you'd probably need to write some wrapper
functions).  I don't know if that would work, but if it did, you'd make me
noticeably more comfortable about this implementation (it wouldn't be right, but
at least it wouldn't be broken anymore :-}).  If that doesn't work, please put
in a note saying "Inheritance purely for implementation convenience; there is no
ISA relationship between these classes and they should not be casted to each
other."

[ahendrickson, 2012/03/08 17:36:07]

Done.

+ public:
+  // Create an object that will be considered finished when |wait_count|
+  // download items have entered state |IN_PROGRESS|.
+
+  DownloadTestObserverInProgress(
+      content::DownloadManager* download_manager,
+      size_t wait_count);
+
+  virtual ~DownloadTestObserverInProgress();
+
+ private:
+  virtual bool IsDownloadInFinalState(content::DownloadItem* download) OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadTestObserverInProgress);
 };
 
 // The WaitForFlush() method on this class returns after: