Reduced the lifetime of DownloadCreateInfo.

chrome/browser/history/download_history_info.h

+// Copyright (c) 2011 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.
+//
+// Download creation struct used for querying the history service.

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

Is "Download creation struct" applicable here?

[ahendrickson, 2011/05/19 20:16:49]

Fixed.

+
+#ifndef CHROME_BROWSER_HISTORY_DOWNLOAD_HISTORY_INFO_H_
+#define CHROME_BROWSER_HISTORY_DOWNLOAD_HISTORY_INFO_H_
+#pragma once
+
+#include <vector>
+
+#include "base/file_path.h"
+#include "base/time.h"
+#include "googleurl/src/gurl.h"
+
+class DownloadItem;
+
+// Contains the information that is stored in the History database (or refers

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

nit: Make this comment say something about downloads.

[ahendrickson, 2011/05/19 20:16:49]

Done.

+// to it).
+struct DownloadHistoryInfo {
+  DownloadHistoryInfo();

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

Why do we have 5 different ctors here? Could you reduce that number?

[ahendrickson, 2011/05/19 20:16:49]

They are all being used, mostly by DownloadItem's constructors (3 of them).

Another one is used to get the history to pass to the DB.

The last is the default constructor.  I might be able to get rid of that one,
but I'm not sure.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

Right, if they're here they're obviously used by some code. However, having too
many ctors is a bad sign. Please try to reduce their number by modifying the
callers' code. If that seems too complicated for now, please add a TODO and then
we should  shortly do a follow-up to fix it.

[ahendrickson, 2011/05/20 18:31:24]

Done.

[Randy Smith (Not in Mondays), 2011/05/20 18:35:25]

You can't get rid of the default constructor because we're using this in STL
containers.  However, I agree with Pawel that we really don't want may
constructors for this object.  Keep in mind that it's a structure, which is thus
intended to have visibility for all member variables; we really don't get much
encapsulation or abstraction out of having constructors.  

Let me ask a more basic question: Why does this structure have any elements
other than those required for the DB?  I'd like there to be a 1-1 correlation
between membership in this structure and what we put in the database; that's its
purpose in life, and putting more elements than that in taints that purpose. 
We'd then have two constructors: The default constructor, and the full (six
argument) constructor.  

(I hadn't actually realized it had extra elements in it--I'm afraid it's
important to me to cut those back.)

(If I were going to fully abstract this structure and turn it into a class, I'd
go a different direction:
* Constructor from a DownloadItem that used download item accessors.
* Constructor from an sql::Statement.
* Default constructor (for vectors & etc.).
And that'd be it.  But that's a copyable class, which is generally avoided in
Chrome.)

+  explicit DownloadHistoryInfo(const DownloadItem& item);
+  DownloadHistoryInfo(const std::vector<GURL>& url,
+                      const GURL& referrer,
+                      int64 total_bytes);
+  DownloadHistoryInfo(const FilePath& path,
+                      const GURL& url,
+                      const base::Time& start,
+                      int64 received,
+                      int64 total,
+                      int32 download_state);
+  DownloadHistoryInfo(const FilePath& path,
+                      const std::vector<GURL>& url,
+                      const GURL& referrer,
+                      const base::Time& start,
+                      int64 received,
+                      int64 total,
+                      int32 download_state,
+                      int64 handle,
+                      int32 id);
+
+  // The URL from which we are downloading. This is the final URL after any
+  // redirection by the server for |url_chain|.
+  const GURL& url() const;
+
+  // Download ID assigned by DownloadResourceHandler.
+  // Used by the history back end, to return in a callback.
+  int32 download_id;
+
+  // The final path where the download is saved.
+  FilePath path;
+
+  // The chain of redirects that leading up to and including the final URL.
+  std::vector<GURL> url_chain;
+
+  // The URL that referred us.
+  GURL referrer_url;
+
+  base::Time start_time;

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

nit: Please comment all member variables.

[ahendrickson, 2011/05/19 20:16:49]

Done.

+
+  int64 received_bytes;
+
+  int64 total_bytes;
+
+  int32 state;
+
+  int64 db_handle;
+};
+
+#endif  // CHROME_BROWSER_HISTORY_DOWNLOAD_HISTORY_INFO_H_