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

chrome/browser/download/download_test_observer.h

Index: chrome/browser/download/download_test_observer.h
diff --git a/chrome/browser/download/download_test_observer.h b/chrome/browser/download/download_test_observer.h
index ffa4d8fcc6ad7e7f23b267da71641c3346cfc210..afd9b466f273c616dc530d4b1bfe37e11c6909d9 100644
--- a/chrome/browser/download/download_test_observer.h
+++ b/chrome/browser/download/download_test_observer.h
@@ -7,11 +7,13 @@
 #pragma once
 
 #include <set>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
+#include "net/base/net_errors.h"
 
 // Construction of this class defines a system state, based on some number
 // of downloads being seen in a particular state + other events that
@@ -198,4 +200,63 @@ class DownloadTestFlushObserver
   DISALLOW_COPY_AND_ASSIGN(DownloadTestFlushObserver);
 };
 
+// Waits for a callback indicating that the DownloadItem is about to be created,
+// or that an error occurred and it won't be created.

[Randy Smith (Not in Mondays), 2012/03/01 21:17:24]

Hmmm.  Good point that we now have a new need for a waiter that we didn't before
because there's another type of error.  But is there a simpler way to handle
this problem?  Specifically, would the use cases you envision for this be as
well handled by a test wrapper function that called DM::DownloadUrl with a local
callback and then called the UI message loop?  The callback would be a static
function in the file which would quit the message loop when the callback
occurred (after storing the information passed back via the callback).  I think
it would be much less code, and simpler to understand as well.

[ahendrickson, 2012/03/02 17:34:39]

I'll simplify it to only using one download, but I think it's pretty easy to
understand as it is, and encapsulates the behavior nicely.

+class DownloadTestItemCreationObserver

[benjhayden, 2012/03/01 19:48:53]

This doesn't appear to be used in this CL. Could it wait for the CL where it
will be used?

If I can see how it will be used, then I could see why it needs to be RCTS and
its own class instead of a static function/callback.

[ahendrickson, 2012/03/01 21:15:01]

It was split off from CL 9426029, which uses it.

Now using the test in download_browsertest.cc.

+    : public base::RefCountedThreadSafe<DownloadTestItemCreationObserver> {
+ public:
+  struct CreationInfo {
+    CreationInfo()
+        : download_id(content::DownloadId::Invalid()),
+          error(net::OK) {
+    }
+
+    content::DownloadId download_id;
+    net::Error error;
+  };
+
+  explicit DownloadTestItemCreationObserver(size_t count);
+
+  void WaitForDownloadItemCreation();
+
+  void DownloadItemCreationCallback(content::DownloadId download_id,
+                                    net::Error error);
+
+  content::DownloadId download_id(size_t index) const {
+    return creation_info_[index].download_id;
+  }
+  net::Error error(size_t index) const { return creation_info_[index].error; }
+  bool created(size_t index) const {
+    return (called_back_count_ > index) &&
+           (creation_info_[index].error == net::OK);
+  }
+  size_t num_created() const { return called_back_count_; }

[benjhayden, 2012/03/01 19:48:53]

This method's idea of creation is different from the previous method's idea.
Could that be a problem? Maybe just comment?

[ahendrickson, 2012/03/01 21:15:01]

Renamed.

+
+  const content::DownloadManager::OnStartedCallback& callback() {
+    return callback_;

[benjhayden, 2012/03/01 19:48:53]

Caching the callback seems unnecessary. It isn't expensive to re-bind every
time. 

[ahendrickson, 2012/03/01 21:15:01]

It lets me make DownloadItemCreationCallback() private.

[benjhayden, 2012/03/02 20:27:16]

You can keep that method private, and you can get rid of callback_ by re-binding
every time callback() is called:

const content::DownloadManager::OnStartedCallback& callback() {
 return
base::Bind(&DownloadTestItemCreationObserver::DownloadItemCreationCallback,
this);
}

When you store the callback in a member variable, you're essentially caching it.
Every cache is a trade-off. In this case, callback_ saves a few nanoseconds but
spends a few bytes of memory and, more importantly in this case, lines of code.
Re-binding every time is a few nanoseconds slower, but saves a few bytes of
memory and, more importantly in this case, a few lines of code.

It doesn't really matter, I just didn't want you to keep thinking that I don't
know how C++ basics like private work.

[ahendrickson, 2012/03/02 21:51:41]

Ah, I didn't think of that.

Changed.

+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<DownloadTestItemCreationObserver>;
+
+  ~DownloadTestItemCreationObserver() {}
+
+  // The download creation information we received.
+  std::vector<CreationInfo> creation_info_;
+
+  // Expected number of callbacks.
+  size_t expected_count_;
+
+  // Count of callbacks.
+  size_t called_back_count_;
+
+  // We are in the message loop.
+  bool waiting_;
+
+  // The callback to pass into |DownloadUrl()|.
+  const content::DownloadManager::OnStartedCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadTestItemCreationObserver);
+};
+
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TEST_OBSERVER_H_