Replace the DownloadFileManager with direct ownership

content/browser/download/download_file.h

Index: content/browser/download/download_file.h
diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h
index e707a26ca10607149db7f8e14d587451e6d20cb8..6ac8d19039c9353d5f30092cce47d674b2f4b410 100644
--- a/content/browser/download/download_file.h
+++ b/content/browser/download/download_file.h
@@ -11,7 +11,6 @@
 #include "base/callback_forward.h"
 #include "base/file_path.h"
 #include "content/common/content_export.h"
-#include "content/public/browser/download_id.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 
 namespace content {
@@ -24,6 +23,12 @@ class DownloadManager;
 // cancelled, the DownloadFile is destroyed.
 class CONTENT_EXPORT DownloadFile {
  public:
+  // Callback used with Initialize.  On a successful initialize, |reason| will
+  // be DOWNLOAD_INTERRUPT_REASON_NONE; on a failed rename, it will be

[benjhayden, 2012/07/25 15:19:16]

rename? I thought this was talking about Initialize?

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

Done.

+  // set to the reason for the failure.
+  typedef base::Callback<void(content::DownloadInterruptReason reason)>
+      InitializeCallback;
+
   // Callback used with Rename().  On a successful rename |reason| will be
   // DOWNLOAD_INTERRUPT_REASON_NONE and |path| the path the rename
   // was done to.  On a failed rename, |reason| will contain the
@@ -33,10 +38,9 @@ class CONTENT_EXPORT DownloadFile {
 
   virtual ~DownloadFile() {}
 
-  // If calculate_hash is true, sha256 hash will be calculated.
   // Returns DOWNLOAD_INTERRUPT_REASON_NONE on success, or a network
   // error code on failure.
-  virtual DownloadInterruptReason Initialize() = 0;
+  virtual void Initialize(const InitializeCallback& callback) = 0;
 
   // Rename the download file to |full_path|.  If that file exists and
   // |overwrite_existing_file| is false, |full_path| will be uniquified by
@@ -68,14 +72,11 @@ class CONTENT_EXPORT DownloadFile {
   // Returns the current (intermediate) state of the hash as a byte string.
   virtual std::string GetHashState() = 0;
 
-  // Cancels the download request associated with this file.
-  virtual void CancelDownloadRequest() = 0;
-
-  virtual int Id() const = 0;
-  virtual DownloadManager* GetDownloadManager() = 0;
-  virtual const DownloadId& GlobalId() const = 0;
+  // For testing.  Must be called on FILE thread.
+  static int GetNumberOfDownloadFiles();
 
-  virtual std::string DebugString() const = 0;
+ protected:
+  static int number_active_objects_;

[benjhayden, 2012/07/25 15:19:16]

Could this be done more cleanly using another interface?

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

I'm not quite sure what you have in mind, but I'm happy to consider cleaner ways
of accomplishing this :-J.  Say a bit more?

[benjhayden, 2012/08/01 18:00:18]

Methods on DownloadDestinationObserver like OnInitialized()/OnDestroyed(), or
use a MockDownloadFileFactory to monitor DownloadFile creation events?
If you don't like either of those, let's look at what you need this field for. I
presume you want to test that the right number of DownloadFiles were
created/destroyed, but that seems like an unusual assertion to want to make in
the first place. So what are you really trying to test? Maybe there's a more
direct way to test it using mocks.

I traced this to EnsureNoPendingDownloads in download_browsertest.cc, which I
see predates this CL. Is this probing for in-progress downloads that haven't
been persisted yet? Could you use OnDownloadCreated() for that instead? Perhaps
a job for another CL, perhaps not; I'd like to see where you plan on taking this
mechanism.

I only challenge this field because it seems like something you would challenge
if I wrote it, although perhaps not. It *looks* like a hack to me, which, of
course, says nothing about whether or not it actually is a hack, which is why I
ask what you're really trying to do. Does it look like a hack to you? If not,
why not?

[Randy Smith (Not in Mondays), 2012/08/03 17:32:44]

We talked about this some in person, and I suspect you got from my tone of voice
that I do indeed consider this a hack :-}.  And you're quite right, if you had
something like this, I'd challenge it (though I hope I'd be a bit less inclined
to challenge it if you were cleaning it up than if you created it in the first
place :-}).

Having said all that.  Hmmm.  The question the test that uses this is trying to
answer is "Did I do something stupid, way down there at the bottom of the stack,
during the course of  doing Everything Involved In a Download?"  So I do think
that we need to have this accessible from a browser test.  Your idea of using a
DownloadFileFactory to wrap DownloadFiles and count it that way is an
interesting one, though, and one I think I like better than what we're currently
doing.  The code would be more complex overall, but all the complexity would be
in test code and we could simply yank the static field and the "Ensure" method
out of the chrome pool.  Which I like.  

Having said *that*: Doing that change isn't really in line with what this CL is
doing.  Willing to let me do it in a separate CL?

 };

[benjhayden, 2012/07/25 15:19:16]

DISALLOW_COPY_AND_ASSIGN while you're here?

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

My inclination is to avoid DISALLOW_COPY_AND_ASSIGN for abstract base classes. 
They can't be copied anyway (because they can't exist, and the copy operation
involves constructing an object of the class), and specifying a copy constructor
(as DCAA does) means that there is no default constructor for the class, which
means you have to write one, which for a class with no data members and no
defined methods seems extreme.  For instance, DownloadItem doesn't have DCAA,
but DownloadItemImpl does.  DownloadFileImpl also has it.  How badly do you want
it here?  

 
 }  // namespace content