DownloadManagerDelegate::ShouldCompleteDownload(callback)

content/public/browser/download_manager_delegate.h

 // Copyright (c) 2012 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_PUBLIC_BROWSER_DOWNLOAD_MANAGER_DELEGATE_H_
 #define CONTENT_PUBLIC_BROWSER_DOWNLOAD_MANAGER_DELEGATE_H_
 #pragma once
 
 #include "base/basictypes.h"
 #include "base/callback.h"
@@ skipping 40 matching lines @@
 
   // Called when the download system wants to alert a WebContents that a
   // download has started, but the TabConetnts has gone away. This lets an
   // delegate return an alternative WebContents. The delegate can return NULL.
   virtual WebContents* GetAlternativeWebContentsToNotifyForDownload();
 
   // Tests if a file type should be opened automatically.
   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
   // DownloadItem::DelayedDownloadOpened when it's done with the item,
   // and is responsible for opening it.  This function is called
   // after the final rename, but before the download state is set to COMPLETED.
   virtual bool ShouldOpenDownload(DownloadItem* item);
 
   // Returns true if we need to generate a binary hash for downloads.
   virtual bool GenerateFileHash();
@@ skipping 37 matching lines @@
   virtual void ChooseSavePath(WebContents* web_contents,
                               const FilePath& suggested_path,
                               const FilePath::StringType& default_extension,
                               bool can_save_as_complete,
                               SaveFilePathPickedCallback callback) {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_DOWNLOAD_MANAGER_DELEGATE_H_