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..9cafbd570982ef0e3df530325566e24337180ac7 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
+//     in derived classes).
+//   - 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,17 +36,13 @@ 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.
   // 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(
       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 +51,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 the requested number of downloads to enter a terminal state.
   void WaitForFinished();
 
   // Return true if everything's happened that we're configured for.
@@ -73,16 +69,17 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
 
   size_t NumDangerousDownloadsSeen() const;
 
- private:
+  size_t NumDownloadsSeenInState(
+      content::DownloadItem::DownloadState state) const;

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

Up to you, but I'd be more inclined to leave this functionality out of the class
and let the user probe for it separately (maybe by a separate routine on
DownloadTest).

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

It's here specifically for IN_PROGRESS tests, which could transition out of that
into another state before checking in the tests.

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

Ah, makes sense.  Ok.

+
+ protected:
   typedef std::set<content::DownloadItem*> DownloadSet;
 
-  // Called when we know that a download item is in a final state.
-  // Note that this is not the same as it first transitioning in to the
-  // final state; multiple notifications may occur once the item is in
-  // that state.  So we keep our own track of transitions into final.
-  void DownloadInFinalState(content::DownloadItem* download);
+  // Maps states to the number of times they have been encountered
+  typedef std::map<content::DownloadItem::DownloadState, size_t> StateMap;
 
-  void SignalIfFinished();
+  // Called to see if a download item is in a final state.
+  virtual bool IsDownloadInFinalState(content::DownloadItem* download) = 0;
 
   // The observed download manager.
   scoped_refptr<content::DownloadManager> download_manager_;

[benjhayden, 2012/03/08 20:30:02]

Would it be too much trouble to keep member fields private and provide protected
accessors as needed? 'Yes' is an acceptable answer, although 'not this CL' would
be preferable. It's just a readability issue.

[ahendrickson, 2012/03/08 21:28:55]

My preference would be to punt this to another CL, but if people feel strongly
enough about it I'll do it here.

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

For this kind of issue, it's useful to look at whether there's something about
the design that's problematic (i.e. assume it's a code smell--where's the
cheese?).  In this case, it looks to me as if the reason all of these member
variables are accessed by the derived classes is because they're accessed in the
derived class constructors, code that is duplicated between derived class
constructors (another code smell, already noted).  You pointed out that we
needed that to be executed after the base class constructor was executed so that
the virtual function table for the base class would be initialized (good catch,
and nice bullet dodge).  Well, is there any way we can execute that common code
in the base class after base class construction?  (I.e. I'm suggesting making an
Init() base class method that's called from the derived class constructor.)

To answer your basic issue above, I dislike letting real design/encapsulation
issues in before landing.  If it was just readability I'd be ok with a new CL,
but based on my analysis above, it's not.  Let me know if you disagree.

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

OK, done.

@@ -92,13 +89,11 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   // reaches wait_count_, we're done.
   DownloadSet finished_downloads_;
 
-  // The set of DownloadItem's we are currently observing.  Generally there
-  // won't be any overlap with the above; once we see the final state
-  // on a DownloadItem, we'll stop observing it.
-  DownloadSet downloads_observed_;
-
-  // The number of downloads to wait on completing.
-  size_t wait_count_;
+  // The map of states to the number of times they have been observed since
+  // we started looking.
+  // Recorded at the time downloads_observed_ is recorded, but cleared in the
+  // constructor to exclude pre-existing states.
+  StateMap states_observed_;
 
   // The number of downloads entered in final state in initial
   // ModelChanged().  We use |finished_downloads_| to track the incoming
@@ -110,13 +105,27 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   // to finished.
   int finished_downloads_at_construction_;
 
+ private:
+  // Called when we know that a download item is in a final state.
+  // Note that this is not the same as it first transitioning in to the
+  // final state; multiple notifications may occur once the item is in
+  // that state.  So we keep our own track of transitions into final.
+  void DownloadInFinalState(content::DownloadItem* download);
+
+  void SignalIfFinished();
+
+  // The set of DownloadItem's we are currently observing.  Generally there
+  // won't be any overlap with the above; once we see the final state
+  // on a DownloadItem, we'll stop observing it.
+  DownloadSet downloads_observed_;
+
+  // The number of downloads to wait on completing.
+  size_t wait_count_;
+
   // Whether an internal message loop has been started and must be quit upon
   // 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 +142,49 @@ class DownloadTestObserver : public content::DownloadManager::Observer,
   DISALLOW_COPY_AND_ASSIGN(DownloadTestObserver);
 };
 
+class DownloadTestObserverTerminal : public DownloadTestObserver {
+ public:
+  // Create an object that will be considered finished when |wait_count|
+  // download items have entered a terminal state (any but IN_PROGRESS).
+  // If |finish_on_select_file| is true, the object will also be
+  // considered finished if the DownloadManager raises a
+  // SelectFileDialogDisplayed() notification.
+  DownloadTestObserverTerminal(
+      content::DownloadManager* download_manager,
+      size_t wait_count,
+      bool finish_on_select_file,
+      DangerousDownloadAction dangerous_download_action);
+
+  virtual ~DownloadTestObserverTerminal();
+
+ private:
+  virtual bool IsDownloadInFinalState(content::DownloadItem* download) OVERRIDE;
+
+  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.

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

You haven't specified the behavior on a dangerous download (which I'm suggesting
changing elsewhere) or on a select file prompt.  The select file prompt behavior
I'm feeling iffy about--if the select file dialog is popped up, that will block
the download visibly transitioning to the IN_PROGRESS state, and if the select
file dialog is nulled, a "finish_on_select_file" may result in an exit before
the downloads really get to IN_PROGRESS.  So you might want to actually give the
user control over select file dialog behavior?  (I think I said something
different than this earlier (and my apologies for the mixed message).  That
means that one of the two times I'm confused--I obviously believe it's the
previous one, but you might want to check the code.  I think that we don't see
IN_PROGRESS downloads until after entry into history, which is delayed on the
select file dialog--that's the important set of events to figure out.)

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

I think we're in trouble if the select file dialog appears, no matter what.  The
tests can specify to automatically accept the default and avoid them.

I changed it to ignoring the select file dialog for IN_PROGRESS tests.

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

Confused, and I'd like to be unconfused before we land this.  I think we're in
the same position with IN_PROGRESS that we've been previously, and that we want
the full select functionality.  Specifically:

* If the test is testing to see if the select file dialog appears and wants to
exit when it gets that dialog, ignoring won't be right (we may not have any
IN_PROGRESS tests in this space, so this may be a comment about having options
for future tests).

* If the test is expecting the select file dialog and wants to blow past it,
it'll null the dialog out.  However, this will still fire the select file dialog
notification (done from a lower level), so *not* ignoring it won't be right for
these tests.

So I think we need to port over the control logic from the old code.  Do you see
something I'm missing?

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

OK, added the parameter to DownloadObserverTestInProgress, and updated the
comments.

+// Callers may either probe for the finished state, or wait on it.
+//

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

nit: Could you nuke the blank comment line?  I think that's not the usual
custom.

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

Done.

+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();
+
+ private:
+  virtual bool IsDownloadInFinalState(content::DownloadItem* download) OVERRIDE;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadTestObserverInProgress);
+};
+
 // The WaitForFlush() method on this class returns after:
 //      * There are no IN_PROGRESS download items remaining on the
 //        DownloadManager.