DownloadUrlParameters

content/public/browser/download_url_parameters.h

Index: content/public/browser/download_url_parameters.h
diff --git a/content/public/browser/download_url_parameters.h b/content/public/browser/download_url_parameters.h
new file mode 100644
index 0000000000000000000000000000000000000000..f6c9f34d0633cd58bceaf0cb512517c870b1625a
--- /dev/null
+++ b/content/public/browser/download_url_parameters.h
@@ -0,0 +1,128 @@
+// 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_URL_PARAMETERS_H_
+#define CONTENT_PUBLIC_BROWSER_DOWNLOAD_URL_PARAMETERS_H_
+#pragma once
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "content/public/browser/download_save_info.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/net_errors.h"
+
+namespace content {
+
+class ResourceContext;
+class ResourceDispatcherHostImpl;
+
+// Pass an instance of DownloadUrlParameters to DownloadManager::DownloadUrl in
+// order to download 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/04/30 19:44:48]

nit: "If not null".

[benjhayden, 2012/05/02 15:11:54]

Done.

+// that prevents a download item from being created.  We send a pointer to
+// content::ResourceContext instead of the usual reference so that a copy of
+// the object isn't made.

[Randy Smith (Not in Mondays), 2012/04/30 19:44:48]

Could you specify the format of the extra headers?

[benjhayden, 2012/05/02 15:11:54]

PTAL

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

Looks good.

+class CONTENT_EXPORT DownloadUrlParameters {
+ public:
+  // NOTE: If there is an error, the DownloadId will be invalid.
+  typedef base::Callback<void(DownloadId, net::Error)> OnStartedCallback;
+
+  static DownloadUrlParameters* FromWebContents(
+      content::WebContents* web_contents,
+      const GURL& url,
+      const content::DownloadSaveInfo& save_info);
+
+  DownloadUrlParameters(
+      const GURL& url,
+      int render_process_host_id,
+      int render_view_host_routing_id,
+      content::ResourceContext* resource_context,
+      const content::DownloadSaveInfo& save_info);
+
+  ~DownloadUrlParameters();
+
+  void set_extra_headers(scoped_ptr<base::ListValue> extra_headers) {
+    extra_headers_ = extra_headers.Pass();
+  }
+  void set_referrer(const GURL& referrer) { referrer_ = referrer; }
+  void set_referrer_encoding(const std::string& referrer_encoding) {
+    referrer_encoding_ = referrer_encoding;
+  }
+  void set_load_flags(int load_flags) { load_flags_ |= load_flags; }
+  void set_method(const std::string& method) { method_ = method; }
+  void set_post_body(const std::string& post_body) {
+    post_body_ = post_body;
+  }
+  void set_prefer_cache(bool prefer_cache) { prefer_cache_ = prefer_cache; }
+  void set_post_id(int64 post_id) {
+    post_id_ = post_id;
+    if (post_id_ >= 0) {
+      set_method("POST");
+      set_prefer_cache(true);
+    } else {
+      set_method("GET");
+      set_prefer_cache(false);

[Randy Smith (Not in Mondays), 2012/04/30 19:44:48]

I'm a bit concerned about this violating principle of least astonishment as
it'll produce different results if the consumer calls
set_prefer_cache/set_post_id in different orders.  Can you arrange for
set_prefer_cache() to override no matter what the order is?

[benjhayden, 2012/05/02 15:11:54]

You're right, this behavior was surprising, and it didn't even guarantee what it
was supposed to guarantee.
I was trying to guarantee the DCHECKs in BeginDownload(). If post_id>=0, then
method needs to be POST and prefer_cache needs to be true, regardless of what
the caller tries to do.
I removed this logic from set_post_id() and made method() and prefer_cache()
return "POST" and true if post_id>=0, regardless of
set_method()/set_prefer_cache().
This way, callers don't need to think about the relationship between
post_id/method/prefer_cache; that relationship is encapsulated here in DUP.
PTAL.

+    }
+  }
+  void set_callback(const OnStartedCallback& callback) {
+    callback_ = callback;
+  }
+
+  const OnStartedCallback& callback() const { return callback_; }
+  base::ListValue* extra_headers() const { return extra_headers_.get(); }
+  int load_flags() const { return load_flags_; }
+  const std::string& method() const { return method_; }
+  const std::string& post_body() const { return post_body_; }
+  int64 post_id() const { return post_id_; }
+  bool prefer_cache() const { return prefer_cache_; }
+  const GURL& referrer() const { return referrer_; }
+  const std::string& referrer_encoding() const { return referrer_encoding_; }
+  int render_process_host_id() const { return render_process_host_id_; }
+  int render_view_host_routing_id() const {
+    return render_view_host_routing_id_;
+  }
+  content::ResourceContext* resource_context() const {
+    return resource_context_;
+  }
+  ResourceDispatcherHostImpl* resource_dispatcher_host() const {
+    return resource_dispatcher_host_;
+  }
+  const content::DownloadSaveInfo& save_info() const { return save_info_; }
+  const GURL& url() const { return url_; }
+
+ private:
+  OnStartedCallback callback_;
+  scoped_ptr<base::ListValue> extra_headers_;
+  int load_flags_;
+  std::string method_;
+  std::string post_body_;
+  int64 post_id_;
+  bool prefer_cache_;
+  GURL referrer_;
+  std::string referrer_encoding_;
+  int render_process_host_id_;
+  int render_view_host_routing_id_;
+  ResourceContext* resource_context_;
+  ResourceDispatcherHostImpl* resource_dispatcher_host_;
+  DownloadSaveInfo save_info_;

[Randy Smith (Not in Mondays), 2012/04/30 19:44:48]

Given the creation of DownloadUrlParameters, I'd like to take a look at getting
rid of DownloadSaveInfo.  It looks to me like DownloadSaveInfo is used in three
classes of places:
* At call sites into DownloadManager::DownloadUrl.  Easy to fold into
DownloadUrlParameters.
* At calls sites into ResourceDispatcherHost::BeginDownload.  Harder, but I'd
like to look at getting rid of this/making it only called from the download
system anyway.  
* In the construction and member variables of DownloadResourceHandler.  

Any urge to look at whether this refactor/removal makes sense?  (If you don't
want to I'll put it on my queue.)  Fewer classes is better, and DownloadSaveInfo
isn't seeming very useful anymore.  I don't think it makes sense for this CL,
but this CL does spark the thought.

[benjhayden, 2012/05/02 15:11:54]

Agreed, that's definitely worth trying, but not for this CL. I'm happy to take
it.

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

Awesome; it's yours.

+  GURL url_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadUrlParameters);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_BROWSER_DOWNLOAD_URL_PARAMETERS_H_