Fix bug 69468: Drag download does not work if there is URL redirection involv...

chrome/browser/download/download_item.h

 // Copyright (c) 2010 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.
 //
 // Each download is represented by a DownloadItem, and all DownloadItems
 // are owned by the DownloadManager which maintains a global list of all
 // downloads. DownloadItems are created when a user initiates a download,
 // and exist for the duration of the browser life time.
 //
 // Download observers:
@@ skipping 170 matching lines @@
   void OnDownloadRenamedToFinalName(const FilePath& full_path);
 
   // Returns true if this item matches |query|. |query| must be lower-cased.
   bool MatchesQuery(const string16& query) const;
 
   // Accessors
   DownloadState state() const { return state_; }
   FilePath full_path() const { return full_path_; }
   void set_path_uniquifier(int uniquifier) { path_uniquifier_ = uniquifier; }
   GURL url() const { return url_; }
+  GURL original_url() const { return original_url_; }

[Paweł Hajdan Jr., 2011/01/13 08:47:59]

We try to avoid adding more accessors like this one.

[Randy Smith (Not in Mondays), 2011/01/14 19:43:21]

It's sorta like the difference between structures and classes; structures just
carry data around, but classes hide that data and present an abstracted view of
it.  There's no abstraction on top of any of the URLs, so I think direct set/get
accessors are appropriate.  This might suggest a useful refactoring, where the
pure, carried around data associated with the DownloadItem is exposed through a
separate interface (maybe returns a const struct blah&) or maybe even is somehow
a separate object, and DownloadItem is a more standard class, with abstractions
and lack of access to internal data members.  But that's not this CL :-}.

   GURL referrer_url() const { return referrer_url_; }
   std::string mime_type() const { return mime_type_; }
   std::string original_mime_type() const { return original_mime_type_; }
   int64 total_bytes() const { return total_bytes_; }
   void set_total_bytes(int64 total_bytes) { total_bytes_ = total_bytes; }
   int64 received_bytes() const { return received_bytes_; }
   int32 id() const { return id_; }
   base::Time start_time() const { return start_time_; }
   void set_db_handle(int64 handle) { db_handle_ = handle; }
   int64 db_handle() const { return db_handle_; }
@@ skipping 51 matching lines @@
   // Full path to the downloaded or downloading file.
   FilePath full_path_;
 
   // A number that should be appended to the path to make it unique, or 0 if the
   // path should be used as is.
   int path_uniquifier_;
 
   // The URL from whence we came.
   GURL url_;
 
+  // The original URL before any redirection.

[Paweł Hajdan Jr., 2011/01/13 08:47:59]

Now we have three URLs, and it's super-easy to confuse them all. The difference
should be much better documented. Do we actually need all three?

[Randy Smith (Not in Mondays), 2011/01/14 19:43:21]

I agree with this.  I'd suggest changing the doc on url_ to "The URL from which
we are downloading", adding "... redirection by the server for this url" to the
doc above.

In terms of whether we actually need all three, it's a hard question to answer. 
I just spent some time with codesearch, and found that url_/url() is used for:
* download_util::CreateDownloadItemValue (-> DOM)
* AutomationProvider::GetDictionaryFromDownloadItem (which could be used in a
whole lot of ways--it's going to be hard to trace the uses of this down).
* DragDownloadFile::ModelChanged (the usage this CL is modifying)
* Browser::OnStartDownload(): Avoid showing the animation for downloads that are
extensions.
* OpenChromeExtension: Used for notification of extension install.

referrer_url_/referrer_url() is only used in two of the above locations: the
automation provider dictionary creation and OpenChromeExtension.  

Those usages make me nervous that we do indeed need all three, though possibly
we should use the original_url_ (added here) for some of them.

+  GURL original_url_;
+
   // The URL of the page that initiated the download.
   GURL referrer_url_;
 
   // The mimetype of the download
   std::string mime_type_;
 
   // The value of the content type header received when downloading
   // this item.  |mime_type_| may be different because of type sniffing.
   std::string original_mime_type_;
 
@@ skipping 66 matching lines @@
   // Did the user open the item either directly or indirectly (such as by
   // setting always open files of this type)? The shelf also sets this field
   // when the user closes the shelf before the item has been opened but should
   // be treated as though the user opened it.
   bool opened_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItem);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_