Downloads: Added transient flag to download item and download database

components/history/core/browser/download_row.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.
 
 #ifndef COMPONENTS_HISTORY_CORE_BROWSER_DOWNLOAD_ROW_H_
 #define COMPONENTS_HISTORY_CORE_BROWSER_DOWNLOAD_ROW_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 29 matching lines @@
               int64_t received,
               int64_t total,
               DownloadState download_state,
               DownloadDangerType danger_type,
               DownloadInterruptReason interrupt_reason,
               const std::string& hash,
               DownloadId id,
               const std::string& guid,
               bool download_opened,
               base::Time last_access,
+              bool transient,

[brettw, 2017/03/29 22:54:13]

This constructor has gotten absurd. I took too long on this CL already so I
don't want to block it any more. But can you do a follow-up to delete it and
just have callers set the members by name? It looks like there are 10 calls
total to this constructor (which you're necessarily updating in this patch) so
this should be an easy job.

There are a bunch of other instances of this (functions with 27 arguments for
DownloadItems) that you had to update in this change that I'm also dismayed by,
but I won't put fixing all of them on you. Do consider it though! :)

[shaktisahu, 2017/03/29 23:46:47]

I agree, it is really big constructor! But, in my opinion, having one
constructor instead of setters make it easy not to forget a particular member
especially since this struct directly maps to one row to the database. wdyt?

[brettw, 2017/03/30 16:44:43]

I agree there is some benefit as you decribe, but there is also a danger that
goes with this type of signature where arguments get mixed up. The signature has
"URL, URL, URL, URL, string, string, string, time, time, string, string" and
it's impossible to tell at the call site if you're putting the right things in
the right places. With struct assignments at least things have names.

Coupled with the growing unwieldyness of the constructor, I think we should
change it.

               const std::string& ext_id,
               const std::string& ext_name,
               const std::vector<DownloadSliceInfo>& download_slice_info);
   DownloadRow(const DownloadRow& other);
   ~DownloadRow();
 
   bool operator==(const DownloadRow&) const;
 
   // The current path to the download (potentially different from final if
   // download is in progress or interrupted).
@@ skipping 71 matching lines @@
 
   // The GUID of the download in the database. Not changed by UpdateDownload().
   std::string guid;
 
   // Whether this download has ever been opened from the browser.
   bool opened;
 
   // The time when the download was last accessed.
   base::Time last_access_time;
 
+  // Whether this download is transient.

[brettw, 2017/03/29 22:54:13]

Can this comment define what transient means? I'm thinking "deleted when it's
done and not shown in the UI".

[shaktisahu, 2017/03/29 23:46:47]

Done.

+  bool transient;
+
   // The id and name of the extension that created this download.
   std::string by_ext_id;
   std::string by_ext_name;
 
   // Data slices that have been downloaded so far. The slices must be ordered
   // by their offset.
   std::vector<DownloadSliceInfo> download_slice_info;
 };
 
 }  // namespace history
 
 #endif  // COMPONENTS_HISTORY_CORE_BROWSER_DOWNLOAD_ROW_H_