Use a DownloadRequestHandle pointer in construction to allow mocking for tests.

content/browser/download/download_request_handle.h

Index: content/browser/download/download_request_handle.h
diff --git a/content/browser/download/download_request_handle.h b/content/browser/download/download_request_handle.h
index c65ebe67aae663bf1449d5483dca24e47525df56..0affa4279449ca63364770dd731c5f404d3288d4 100644
--- a/content/browser/download/download_request_handle.h
+++ b/content/browser/download/download_request_handle.h
@@ -8,6 +8,8 @@
 
 #include <string>
 
+#include "base/compiler_specific.h"
+
 class DownloadManager;
 class ResourceDispatcherHost;
 class TabContents;
@@ -16,7 +18,29 @@ class TabContents;
 // or objects conditional on it (e.g. TabContents).
 // This class needs to be copyable, so we can pass it across threads and not
 // worry about lifetime or const-ness.
-class DownloadRequestHandle {
+//
+// DownloadRequestHandleInterface is defined for mocking purposes.
+class DownloadRequestHandleInterface {

[cbentzel, 2011/10/27 17:20:39]

I haven't seen the "Interface" suffix used too heavily in the chromium code
base, but it is not disallowed.

Usually I've seen the abstract base class called DownloadRequestHandle, followed
by DownloadRequestHandleImpl for the concrete implementation.

IMO this is the way to go - you want to encourage people to use the interfaces
as much as possible, and having a shorter name is just one way to do it.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...

[Randy Smith (Not in Mondays), 2011/10/27 18:21:22]

Well, that's actually precisely why I *didn't* go with "" vs. "Impl" (I did
consider it).  Everywhere except for inside of DownloadFile and DownloadItem,
DownloadRequestHandle is the primary object, and it's passed around by value. 
So I think this is tied to the basic design choice for this CL (pass around by
value except for construction of DownloadItem/DownloadFile), and I'd rather not
change the naming unless we change that choice.

[cbentzel, 2011/10/27 18:45:34]

Maybe it's me, but it doesn't look like the current DownloadRequestHandle is
used in too many places.

http://codesearch.google.com/codesearch#search&q=DownloadRequestHandle&exact_...

Perhaps this is because codesearch hasn't caught up to the DownloadCreateInfo
split. 

Why wouldn't you want to use the interface, except in DownloadResourceHandle
where it's created? Lifetime issues?

+ public:
+  virtual ~DownloadRequestHandleInterface() {}
+
+  // These functions must be called on the UI thread.
+  virtual TabContents* GetTabContents() const = 0;
+  virtual DownloadManager* GetDownloadManager() const = 0;
+
+  // Pause or resume the matching URL request.

[cbentzel, 2011/10/27 17:20:39]

Do these also need to be called on the UI thread?

[Randy Smith (Not in Mondays), 2011/10/27 18:21:22]

Nope; they're ok to call anywhere.

+  virtual void PauseRequest() const = 0;
+  virtual void ResumeRequest() const = 0;
+
+  // Cancel the request.
+  virtual void CancelRequest() const = 0;
+
+  // Describe the object.
+  virtual std::string DebugString() const = 0;
+};
+
+
+class DownloadRequestHandle : public DownloadRequestHandleInterface {

[cbentzel, 2011/10/27 17:20:39]

Do you want to move this into a separate header? Probably not a big deal since
this forward declares RDH, but it does have some details not needed by consumers
of the interface.

[Randy Smith (Not in Mondays), 2011/10/27 18:21:22]

I'm neutral on it--happy to move it if you want, but didn't think it was worth
it originally.  Let me know.

[cbentzel, 2011/10/27 18:45:34]

Not too concerned. I'll revisit after you add the mocks.

  public:
   // Create a null DownloadRequestHandle: getters will return null, and
   // all actions are no-ops.
@@ -33,18 +57,13 @@ class DownloadRequestHandle {
                         int render_view_id,
                         int request_id);
 
-  // These functions must be called on the UI thread.
-  TabContents* GetTabContents() const;
-  DownloadManager* GetDownloadManager() const;
-
-  // Pause or resume the matching URL request.
-  void PauseRequest() const;
-  void ResumeRequest() const;
-
-  // Cancel the request
-  void CancelRequest() const;
-
-  std::string DebugString() const;
+  // Implement DownloadRequestHandleInterface interface.
+  virtual TabContents* GetTabContents() const OVERRIDE;

[cbentzel, 2011/10/27 17:20:39]

The OVERRIDE isn't completely needed here because of the =0's in the base class,
but are probably good practice to keep in.

[Randy Smith (Not in Mondays), 2011/10/27 18:21:22]

Done.

+  virtual DownloadManager* GetDownloadManager() const OVERRIDE;
+  virtual void PauseRequest() const OVERRIDE;
+  virtual void ResumeRequest() const OVERRIDE;
+  virtual void CancelRequest() const OVERRIDE;
+  virtual std::string DebugString() const OVERRIDE;
 
  private:
   // The resource dispatcher host.