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

content/browser/download/download_request_handle.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_HANDLE_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_HANDLE_H_
 #pragma once
 
 #include <string>
 
+#include "base/compiler_specific.h"
+
 class DownloadManager;
 class ResourceDispatcherHost;
 class TabContents;
 
 // A handle used by the download system for operations on the URLRequest
 // 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.
   // TODO(rdsmith): Ideally, actions would be forbidden rather than
   // no-ops, to confirm that no non-testing code actually uses
   // a null DownloadRequestHandle.  But for now, we need the no-op
   // behavior for unit tests.  Long-term, this should be fixed by
   // allowing mocking of ResourceDispatcherHost in unit tests.
   DownloadRequestHandle();
 
   // Note that |rdh| is required to be non-null.
   DownloadRequestHandle(ResourceDispatcherHost* rdh,
                         int child_id,
                         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.
   ResourceDispatcherHost* rdh_;
 
   // The ID of the child process that started the download.
   int child_id_;
 
   // The ID of the render view that started the download.
   int render_view_id_;
 
   // The ID associated with the request used for the download.
   int request_id_;
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_HANDLE_H_