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..97d55778b3204c7e9aa155a4f7953999d208e828 100644
--- a/chrome/browser/download/download_test_observer.h
+++ b/chrome/browser/download/download_test_observer.h
@@ -13,12 +13,12 @@
 #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 either CANCELLED, or one which you can't switch out of).

[Randy Smith (Not in Mondays), 2012/03/06 21:49:58]

I'd just list them here; INTERRUPTED can be switched out of once you do the
resumption, and CANCELLED can't be until then.

[ahendrickson, 2012/03/07 15:38:58]

I've changed it to read "anything but IN_PROGRESS".

[CANCELLED can be a state on the way to REMOVING.]

+//   - 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
@@ -36,7 +36,8 @@ 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.
@@ -46,7 +47,6 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   DownloadTestObserver(
       content::DownloadManager* download_manager,
       size_t wait_count,
-      content::DownloadItem::DownloadState download_finished_state,
       bool finish_on_select_file,
       DangerousDownloadAction dangerous_download_action);
 
@@ -55,7 +55,7 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   // State accessors.
   bool select_file_dialog_seen() const { return select_file_dialog_seen_; }
 
-  // Wait for whatever state was specified in the constructor.
+  // Wait for a terminal state, for the number of downloads requested.

[Randy Smith (Not in Mondays), 2012/03/06 21:49:58]

nit, suggestion: Maybe better worded as "Wait for the number of downloads
requested to enter a terminal state"?

[ahendrickson, 2012/03/07 15:38:58]

Done.

   void WaitForFinished();
 
   // Return true if everything's happened that we're configured for.
@@ -73,7 +73,7 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
 
   size_t NumDangerousDownloadsSeen() const;
 
- private:
+ protected:

[Randy Smith (Not in Mondays), 2012/03/06 21:49:58]

So this gives me a bit of an interface minimality allergic reaction; you're
opening up the entire implementation of this class to anything that inherits
from it.  Can you figure out what the common functionality needed is and have
that be an implementation class that you either inherit from (privately) or
delegate to?

(Another way to think of it: conceptually, DownloadTestObserver() is a class
that blocks until some number of Downloads enter a terminal state. 
DownloadTestObserverInProgress block until some number of downloads enter a
*non* terminal state.  So there is *not* an ISA relationship between the two
classes, which makes this type of change purely about implementation convenience
at the cost of interface cleanliness.  Which, as you might imagine, doesn't make
me happy :-J.)

[ahendrickson, 2012/03/07 15:38:58]

Done.

   typedef std::set<content::DownloadItem*> DownloadSet;
 
   // Called when we know that a download item is in a final state.
@@ -82,6 +82,9 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   // that state.  So we keep our own track of transitions into final.
   void DownloadInFinalState(content::DownloadItem* download);
 
+  // Called to see if a download item is in a final state.
+  virtual bool IsDownloadInFinalState(content::DownloadItem* download);
+
   void SignalIfFinished();
 
   // The observed download manager.
@@ -114,9 +117,6 @@ 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.
   bool finish_on_select_file_;
@@ -133,6 +133,28 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   DISALLOW_COPY_AND_ASSIGN(DownloadTestObserver);
 };
 
+// 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 DownloadTestObserver {
+ 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();
+
+ protected:

[Randy Smith (Not in Mondays), 2012/03/06 21:49:58]

FYI: This can be private and everything will work well.

[ahendrickson, 2012/03/07 15:38:58]

Done.

+  virtual bool IsDownloadInFinalState(content::DownloadItem* download);
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadTestObserverInProgress);
+};
+
 // The WaitForFlush() method on this class returns after:
 //      * There are no IN_PROGRESS download items remaining on the
 //        DownloadManager.