Added callback to DownloadUrl() so we can find download failures.

content/public/browser/download_manager.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.
 //
 // The DownloadManager object manages the process of downloading, including
 // updates to the history system and providing the information for displaying
 // the downloads view in the Destinations tab. There is one DownloadManager per
 // active browser context in Chrome.
 //
 // Download observers:
@@ skipping 14 matching lines @@
 // all persisted downloads.
 
 #ifndef CONTENT_PUBLIC_BROWSER_DOWNLOAD_MANAGER_H_
 #define CONTENT_PUBLIC_BROWSER_DOWNLOAD_MANAGER_H_
 #pragma once
 
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/message_loop_helpers.h"
 #include "base/time.h"
 #include "content/browser/download/interrupt_reasons.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/net_log.h"
 #include "net/base/net_errors.h"
@@ skipping 10 matching lines @@
 namespace content {
 class BrowserContext;
 class DownloadManagerDelegate;
 class DownloadQuery;
 class WebContents;
 
 // Browser's download manager: manages all downloads and destination view.
 class CONTENT_EXPORT DownloadManager
     : public base::RefCountedThreadSafe<DownloadManager> {
  public:
+  // NOTE: It is OK for the DownloadId to be invalid if there is an error.

[Randy Smith (Not in Mondays), 2012/03/07 21:16:57]

wording nit: "Is it OK" sounds like telling the consumer what they can and can't
do.  This is telling them what they should expect, so I think something like "If
there is an error, the DownloadId will be invalid." would be better.  

[ahendrickson, 2012/03/08 21:33:07]

Done.

+  typedef base::Callback<void(DownloadId, net::Error)> OnStartedCallback;
+
   virtual ~DownloadManager() {}
 
   static DownloadManager* Create(
       DownloadManagerDelegate* delegate,
       net::NetLog* net_log);
 
   // Shutdown the download manager. Must be called before destruction.
   virtual void Shutdown() = 0;
 
   // Interface to implement for observers that wish to be informed of changes
@@ skipping 94 matching lines @@
   // Downloads the content at |url|. |referrer| and |referrer_encoding| are the
   // referrer for the download, and may be empty. If |prefer_cache| is true,
   // then if the response to |url| is in the HTTP cache it will be used without
   // revalidation. If |post_id| is non-negative, then it identifies the post
   // transaction used to originally retrieve the |url| resource - it also
   // requires |prefer_cache| to be |true| since re-post'ing is not done.
   // |save_info| specifies where the downloaded file should be
   // saved, and whether the user should be prompted about the download.
   // |web_contents| is the web page that the download is done in context of,
   // and must be non-NULL.
+  // |callback| will be called when the download starts, or if an error
+  // occurs.

[Randy Smith (Not in Mondays), 2012/03/07 21:16:57]

This sentence is actually underspecified, as the download can start and an error
occur later.  I'm inclined to say that once we know the DownloadItem is being
created, we can call the callback with the DownloadId and let any other errors
be the DownloadItem::Observer interface's problem.  If you agree, this
documentation should be made more specific (and you should make sure that's the
functionality that's actually supported).

[jstritar, 2012/03/07 21:34:39]

What if we define a Delegate that has separate methods, where we guarantee that
one of them will be called?

class Delegate {
  public:
    void OnDownloadStarted(DownloadId id) = 0;
    void OnDownloadNotStarted(net::Error error) = 0;
};

Once OnDownloadStarted is invoked, the client can use DownloadItem::Observer.

[ahendrickson, 2012/03/08 21:33:07]

Done.

[Randy Smith (Not in Mondays), 2012/03/09 19:14:02]

typo: "prevents ad download".

[ahendrickson, 2012/03/09 20:50:08]

Done.

   virtual void DownloadUrl(const GURL& url,
                            const GURL& referrer,
                            const std::string& referrer_encoding,
                            bool prefer_cache,
                            int64 post_id,
                            const DownloadSaveInfo& save_info,
-                           content::WebContents* web_contents) = 0;
+                           content::WebContents* web_contents,
+                           const OnStartedCallback& callback) = 0;
 
   // Allow objects to observe the download creation process.
   virtual void AddObserver(Observer* observer) = 0;
 
   // Remove a download observer from ourself.
   virtual void RemoveObserver(Observer* observer) = 0;
 
   // Called by the embedder, after creating the download manager, to let it know
   // about downloads from previous runs of the browser.
   virtual void OnPersistentStoreQueryComplete(
@@ skipping 84 matching lines @@
   friend class base::RefCountedThreadSafe<
       DownloadManager, content::BrowserThread::DeleteOnUIThread>;
   friend struct content::BrowserThread::DeleteOnThread<
       content::BrowserThread::UI>;
   friend class base::DeleteHelper<DownloadManager>;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_DOWNLOAD_MANAGER_H_