Downloads: Added transient flag to download item and download database

Downloads: Added transient flag to download item and download database

This CL adds a transient flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that are temporary and should not be shown on the UI.

BUG=700240
Review-Url: https://codereview.chromium.org/2720613002
Cr-Commit-Position: refs/heads/master@{#460833}
Committed: https://chromium.googlesource.com/chromium/src/+/fd49a3edab3d755944e10202569fe90f12f32bbb

[shaktisahu, 2017-02-27 00:17:36]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 00:17:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 00:41:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 00:41:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[shaktisahu, 2017-02-27 19:54:03]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 19:54:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 20:30:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 20:30:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[shaktisahu, 2017-03-01 03:03:53]

Description was changed from

==========
Downloads: Added IsVisible to the DownloadItem

BUG=
==========

to

==========
Downloads: Added IsVisible to the DownloadItem

BUG=
==========

[shaktisahu, 2017-03-01 03:03:53]

shaktisahu@chromium.org changed reviewers:
+ dtrainor@chromium.org

[shaktisahu, 2017-03-01 22:52:10]

dtrainor@ - PTAL.

[David Trainor- moved to gerrit, 2017-03-06 18:28:40]

https://codereview.chromium.org/2720613002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java
(right):

https://codereview.chromium.org/2720613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java:158:
public boolean isVisible() {
Do we want to make this have an impact on the UI?  Maybe add a TODO and file a
follow up bug?

https://codereview.chromium.org/2720613002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:125:
item->GetLastAccessTime().ToJavaTime(), item->IsVisible());
Should visible items make it past this point?  Maybe add a TODO and a follow up
bug.

https://codereview.chromium.org/2720613002/diff/20001/components/history/core...
File components/history/core/browser/history_backend_db_unittest.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/components/history/core...
components/history/core/browser/history_backend_db_unittest.cc:787:
ASSERT_LE(34, cur_version);
Do we want to validate that it's going to 35?

[vitaliii, 2017-03-07 07:58:56]

Drive-by question.

https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:784: return visible_;
Could you add any comment on when IsVisible is false (or at least explain it
here)?

If I encounter this code, I would be confused. It feels like I should not show
items with IsVisible == false in UI, but I am not sure.

[shaktisahu, 2017-03-07 21:02:33]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 21:03:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 21:36:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 21:36:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shaktisahu, 2017-03-08 04:19:03]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 04:19:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 04:20:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 04:20:58]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shaktisahu, 2017-03-08 06:35:05]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 06:35:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shaktisahu, 2017-03-08 06:51:30]

dtrainor@ - PTAL

https://codereview.chromium.org/2720613002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java
(right):

https://codereview.chromium.org/2720613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java:158:
public boolean isVisible() {
On 2017/03/06 18:28:39, David Trainor wrote:
> Do we want to make this have an impact on the UI?  Maybe add a TODO and file a
> follow up bug?

Removed from Java layer.

https://codereview.chromium.org/2720613002/diff/20001/chrome/browser/android/...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/chrome/browser/android/...
chrome/browser/android/download/download_manager_service.cc:125:
item->GetLastAccessTime().ToJavaTime(), item->IsVisible());
On 2017/03/06 18:28:39, David Trainor wrote:
> Should visible items make it past this point?  Maybe add a TODO and a follow
up
> bug.

Removed.

https://codereview.chromium.org/2720613002/diff/20001/components/history/core...
File components/history/core/browser/history_backend_db_unittest.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/components/history/core...
components/history/core/browser/history_backend_db_unittest.cc:787:
ASSERT_LE(34, cur_version);
On 2017/03/06 18:28:39, David Trainor wrote:
> Do we want to validate that it's going to 35?

Didn't get it. The new version after migration should be 35 or above. I am
checking 2 version migrations here.
I will probably change CreateDBVersion(32) to start from 33 instead, but
actually any number works here.

https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:784: return visible_;
On 2017/03/07 07:58:56, vitaliii wrote:
> Could you add any comment on when IsVisible is false (or at least explain it
> here)?
> 
> If I encounter this code, I would be confused. It feels like I should not show
> items with IsVisible == false in UI, but I am not sure.

Comments are usually found above the method declaration in the interface which
in this case is download_item.h.
Yes, the intent here is that if isVisible=false, we shouldn't show this on the
UI.

[commit-bot: I haz the power, 2017-03-08 07:12:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 07:12:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shaktisahu, 2017-03-08 07:52:48]

Patchset #5 (id:80001) has been deleted

[shaktisahu, 2017-03-08 07:52:58]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 07:53:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 09:24:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 09:24:37]

Dry run: This issue passed the CQ dry run.

[dgn, 2017-03-08 13:12:04]

drive-by follow up question

https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:784: return visible_;
On 2017/03/08 06:51:30, shaktisahu wrote:
> On 2017/03/07 07:58:56, vitaliii wrote:
> > Could you add any comment on when IsVisible is false (or at least explain it
> > here)?
> > 
> > If I encounter this code, I would be confused. It feels like I should not
show
> > items with IsVisible == false in UI, but I am not sure.
> 
> Comments are usually found above the method declaration in the interface which
> in this case is download_item.h.
> Yes, the intent here is that if isVisible=false, we shouldn't show this on the
> UI.

All right, but there is no context to why this is being added. What would make a
download item invisible? How does it affect the behaviour of the item besides
whether it's shown in the UI? Can you please link a bug in the description?

https://codereview.chromium.org/2720613002/diff/100001/chrome/browser/safe_br...
File
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc
(right):

https://codereview.chromium.org/2720613002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc:303:
true,                                          // download visible
nit: use the parameter name, like in the surrounding lines

[David Trainor- moved to gerrit, 2017-03-09 17:06:58]

On 2017/03/08 13:12:04, dgn wrote:
> drive-by follow up question
> 
>
https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
> File content/browser/download/download_item_impl.cc (right):
> 
>
https://codereview.chromium.org/2720613002/diff/20001/content/browser/downloa...
> content/browser/download/download_item_impl.cc:784: return visible_;
> On 2017/03/08 06:51:30, shaktisahu wrote:
> > On 2017/03/07 07:58:56, vitaliii wrote:
> > > Could you add any comment on when IsVisible is false (or at least explain
it
> > > here)?
> > > 
> > > If I encounter this code, I would be confused. It feels like I should not
> show
> > > items with IsVisible == false in UI, but I am not sure.
> > 
> > Comments are usually found above the method declaration in the interface
which
> > in this case is download_item.h.
> > Yes, the intent here is that if isVisible=false, we shouldn't show this on
the
> > UI.
> 
> All right, but there is no context to why this is being added. What would make
a
> download item invisible? How does it affect the behaviour of the item besides
> whether it's shown in the UI? Can you please link a bug in the description?
> 
>
https://codereview.chromium.org/2720613002/diff/100001/chrome/browser/safe_br...
> File
>
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2720613002/diff/100001/chrome/browser/safe_br...
>
chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc:303:
> true,                                          // download visible
> nit: use the parameter name, like in the surrounding lines

Hey good point we should have had a bug filed for this.  Shakti I can help out
and file one.  It's for some upcoming work where some features are going to use
the downloads core code but don't want things exposed to the UI directly. 
Either the feature itself will handle the exposure or it'll be something users
shouldn't be aware of.  E.g. if a feature is batching multiple download requests
but wants to just show one download item in the UI.  This is the first patch
that adds the database entry.  Future patches should start using the value to
prune it from showing up in the UI.

The feature work is currently targeted at Android for M59.  We're going to
aggregate multiple sources of downloading content (offline pages, download
items, and these new features - background fetch for example) behind an
interface for the Android UI to hit.  A lot of these features are Android
specific, but I need to start some discussions for if/how we do this on desktop
as well, which will be targeted on a future release.

[shaktisahu, 2017-03-10 03:34:52]

Description was changed from

==========
Downloads: Added IsVisible to the DownloadItem

BUG=
==========

to

==========
Downloads: Added IsVisible to the DownloadItem

BUG=700240
==========

[shaktisahu, 2017-03-10 03:49:00]

Description was changed from

==========
Downloads: Added IsVisible to the DownloadItem

BUG=700240
==========

to

==========
Downloads: Added IsVisible to the download item and download database

BUG=700240
==========

[shaktisahu, 2017-03-10 18:20:55]

Description was changed from

==========
Downloads: Added IsVisible to the download item and download database

BUG=700240
==========

to

==========
Downloads: Added IsVisible to the download item and download database

This CL adds a visible flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that should not be shown on the UI.

BUG=700240
==========

[shaktisahu, 2017-03-10 18:20:55]

shaktisahu@chromium.org changed reviewers:
+ sdefresne@chromium.org

[shaktisahu, 2017-03-10 18:21:34]

sdefresne@ - PTAL at comp/history

[sdefresne, 2017-03-13 08:59:24]

components/history lgtm

[David Trainor- moved to gerrit, 2017-03-14 15:13:10]

lgtm % nits.  File a bug to look into implementing this for other platforms.

[shaktisahu, 2017-03-14 18:25:32]

shaktisahu@chromium.org changed reviewers:
+ sky@chromium.org

[shaktisahu, 2017-03-14 18:25:33]

sky@ - PTAL

[sky, 2017-03-14 20:29:03]

sky@chromium.org changed reviewers:
+ brettw@chromium.org
- sky@chromium.org

[sky, 2017-03-14 20:29:04]

sky->brettw

[brettw, 2017-03-20 20:36:29]

I don't understand the motivation or why a database change is required. Is there
a design doc for this?

[shaktisahu, 2017-03-23 20:43:13]

On 2017/03/20 20:36:29, brettw (plz ping after 24h) wrote:
> I don't understand the motivation or why a database change is required. Is
there
> a design doc for this?

Based on an offline discussion, it was proposed to rename IsVisible() to
IsTransient().
We will also spin up a doc on the side to describe the motivation for this
change.

[shaktisahu, 2017-03-27 23:38:02]

Description was changed from

==========
Downloads: Added IsVisible to the download item and download database

This CL adds a visible flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that should not be shown on the UI.

BUG=700240
==========

to

==========
Downloads: Added transient flag to download item and download database

This CL adds a visible flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that should not be shown on the UI.

BUG=700240
==========

[shaktisahu, 2017-03-27 23:38:48]

On 2017/03/23 20:43:13, shaktisahu wrote:
> On 2017/03/20 20:36:29, brettw (plz ping after 24h) wrote:
> > I don't understand the motivation or why a database change is required. Is
> there
> > a design doc for this?
> 
> Based on an offline discussion, it was proposed to rename IsVisible() to
> IsTransient().
> We will also spin up a doc on the side to describe the motivation for this
> change.

PTAL

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

Sorry for the delay!

https://codereview.chromium.org/2720613002/diff/140001/chrome/browser/android...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2720613002/diff/140001/chrome/browser/android...
chrome/browser/android/download/download_manager_service.cc:60:
DCHECK(item->IsTransient());
I don't follow this check, if we're creating java download items I would expect
them to be not transient. If this code is correct, can you add a comment
explaining?

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_database.cc (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_database.cc:281: return
EnsureColumnExists("transient", "INTEGER NOT NULL DEFAULT 1");
Don't we want the default here to be 0 (not transient for existing items)?

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_row.cc (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.cc:86: bool
DownloadRow::operator==(const DownloadRow& rhs) const {
In the followup patch to delete the crazy constructor, I *think* we can also
replace this whole function implementation with:
  bool DownloadRow::operator==(const DownloadRow& rhs) const = default;

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_row.h (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.h:50: bool transient,
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! :)

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.h:142: // Whether this download is
transient.
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]

brettw@ - I had forgot to flip the value of the boolean after renaming IsVisible
to IsTransient. Thanks for catching that.
PTAL again.

https://codereview.chromium.org/2720613002/diff/140001/chrome/browser/android...
File chrome/browser/android/download/download_manager_service.cc (right):

https://codereview.chromium.org/2720613002/diff/140001/chrome/browser/android...
chrome/browser/android/download/download_manager_service.cc:60:
DCHECK(item->IsTransient());
On 2017/03/29 22:54:13, brettw (plz ping after 24h) wrote:
> I don't follow this check, if we're creating java download items I would
expect
> them to be not transient. If this code is correct, can you add a comment
> explaining?

Corrected now.

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_database.cc (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_database.cc:281: return
EnsureColumnExists("transient", "INTEGER NOT NULL DEFAULT 1");
On 2017/03/29 22:54:13, brettw (plz ping after 24h) wrote:
> Don't we want the default here to be 0 (not transient for existing items)?

Corrected now.

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_row.cc (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.cc:86: bool
DownloadRow::operator==(const DownloadRow& rhs) const {
On 2017/03/29 22:54:13, brettw (plz ping after 24h) wrote:
> In the followup patch to delete the crazy constructor, I *think* we can also
> replace this whole function implementation with:
>   bool DownloadRow::operator==(const DownloadRow& rhs) const = default;

Acknowledged.

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_row.h (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.h:50: bool transient,
On 2017/03/29 22:54:13, brettw (plz ping after 24h) wrote:
> 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! :)

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?

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.h:142: // Whether this download is
transient.
On 2017/03/29 22:54:13, brettw (plz ping after 24h) wrote:
> Can this comment define what transient means? I'm thinking "deleted when it's
> done and not shown in the UI".

Done.

[shaktisahu, 2017-03-29 23:51:15]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:52:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 00:26:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 00:26:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shaktisahu, 2017-03-30 00:44:10]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 00:45:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 01:13:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 01:13:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shaktisahu, 2017-03-30 01:23:25]

Patchset #9 (id:180001) has been deleted

[shaktisahu, 2017-03-30 01:23:50]

Patchset #9 (id:200001) has been deleted

[shaktisahu, 2017-03-30 01:24:39]

Patchset #8 (id:160001) has been deleted

[shaktisahu, 2017-03-30 01:25:28]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 01:26:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 01:52:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 01:52:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[shaktisahu, 2017-03-30 02:16:20]

The CQ bit was checked by shaktisahu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 02:16:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 04:04:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:04:38]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-03-30 16:42:07]

lgtm

[brettw, 2017-03-30 16:44:44]

I forgot to publish this comment:

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
File components/history/core/browser/download_row.h (right):

https://codereview.chromium.org/2720613002/diff/140001/components/history/cor...
components/history/core/browser/download_row.h:50: bool transient,
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.

[shaktisahu, 2017-03-30 18:28:03]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-03-30 18:28:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2720613002/#ps240001
(title: "fix tests")

[commit-bot: I haz the power, 2017-03-30 18:28:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 18:35:16]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490898483748520,
"parent_rev": "12b32f2bc2508a15b8e894c9aedc7286e5c0365e", "commit_rev":
"fd49a3edab3d755944e10202569fe90f12f32bbb"}

[commit-bot: I haz the power, 2017-03-30 18:36:06]

Description was changed from

==========
Downloads: Added transient flag to download item and download database

This CL adds a visible flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that should not be shown on the UI.

BUG=700240
==========

to

==========
Downloads: Added transient flag to download item and download database

This CL adds a visible flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that should not be shown on the UI.

BUG=700240

Review-Url: https://codereview.chromium.org/2720613002
Cr-Commit-Position: refs/heads/master@{#460833}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd49a3edab3d755944e10202569f...
==========

[commit-bot: I haz the power, 2017-03-30 18:36:07]

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/fd49a3edab3d755944e10202569f...

[shaktisahu, 2017-03-30 18:37:18]

Description was changed from

==========
Downloads: Added transient flag to download item and download database

This CL adds a visible flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that should not be shown on the UI.

BUG=700240

Review-Url: https://codereview.chromium.org/2720613002
Cr-Commit-Position: refs/heads/master@{#460833}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd49a3edab3d755944e10202569f...
==========

to

==========
Downloads: Added transient flag to download item and download database

This CL adds a transient flag to the download items and database. This
boolean can be used in future by any feature that wants to have
download items that are temporary and should not be shown on the UI.

BUG=700240

Review-Url: https://codereview.chromium.org/2720613002
Cr-Commit-Position: refs/heads/master@{#460833}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd49a3edab3d755944e10202569f...
==========

[brettw, 2017-08-01 18:08:59]

Can you please follow-up on the review comments here?