DownloadManagerDelegate::ShouldCompleteDownload(callback)

content/public/browser/download_manager_delegate.h

Index: content/public/browser/download_manager_delegate.h
diff --git a/content/public/browser/download_manager_delegate.h b/content/public/browser/download_manager_delegate.h
index 6cc557c6de7a88e5cdbb41f3d5fa28243a551653..f491d1e6e346f27aa7772a941d982533b24aea88 100644
--- a/content/public/browser/download_manager_delegate.h
+++ b/content/public/browser/download_manager_delegate.h
@@ -58,13 +58,15 @@ class CONTENT_EXPORT DownloadManagerDelegate {
   virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path);
 
   // Allows the delegate to override completion of the download.  If this
-  // function returns false, the download completion is delayed and the
-  // delegate is responsible for making sure that
-  // DownloadItem::MaybeCompleteDownload is called at some point in the
-  // future.  Note that at that point this function will be called again,
-  // and is responsible for returning true when it really is ok for the
-  // download to complete.
-  virtual bool ShouldCompleteDownload(DownloadItem* item);
+  // function returns false, the download completion is delayed and the delegate
+  // is responsible for making sure that |complete_callback| is run at some
+  // point in the future.  Note that at that point this function will be called
+  // again, and is responsible for returning true when it really is ok for the
+  // download to complete. If this method returns true, then |complete_callback|
+  // will not be run.

[Randy Smith (Not in Mondays), 2012/05/02 18:14:07]

This doesn't spec the behavior if this function is called multiple times, and it
should.  Consider the case where safe browsing blocks a download, then allows it
to complete, then gdrive blocks it; you'll get a callback after SB is done, but
ShouldCompleteDownload() will once again return false.  It's also worthwhile to
indicate whether or not it's ok to call before the callback's been run.

[benjhayden, 2012/05/03 19:21:14]

PTAL
Feel free to suggest alternate text.

Also, I implemented the gateway/trampoline in CDMD in case both OS_CHROMEOS and
SAFE_BROWSING_ENABLED.

[Randy Smith (Not in Mondays), 2012/05/03 19:50:46]

I was thinking about asking you to do that, but I think that it was actually ok
the way it was.  The issue is around how the consumer is expected to determine
if they should complete the download.  If we take the return value as
authoritative and the callback as a hint, the old code was ok--if both SB and
CrOS wanted to block the download, there would be an intermediate callback, and
then ShouldCompleteDownload would be called and it would still return false. 
Under the hood, the callback would be handed to CrOS instead of SB, but that's
ok.  

Given that's pretty much how the calling code (at least in DownloadManagerImpl)
wants to use ShouldCompleteDownload() and that the interface is designed to
assist that code, I think we're ok with the original code, and it's simpler. 
Let me know if you disagree--there may be something about the interface
subtleties I'm missing.

[benjhayden, 2012/05/04 17:02:54]

That's how DM wants to use it now, but that's not how SavePackage wants to use
it. It would be simpler if SavePackage could pass &SP::Finish to CDMD instead of
writing a new MaybeFinish() method, just to support MHTML+gdata. MaybeFinish()
would look a little like the conditional in SavePackage::OnDownloadUpdated()
that you love so much. :-)

+  virtual bool ShouldCompleteDownload(
+      DownloadItem* item,
+      const base::Closure& complete_callback);
 
   // Allows the delegate to override opening the download. If this function
   // returns false, the delegate needs to call