251337 - Download manager history should have "aging" option, just like the browser history

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[kenno]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040626 Firefox/0.9.1

I download a lot of small files to different locations. The download history
comes in very handy when I forgot where I saved a file a few days ago, so I
leave "Remove files from the download manager" on "manual". However, after a few
months of browsing, the list becomes so large, that it takes several seconds to
load. Since form version 8.0 on, one cannot access downloads without using the
download manager windows, this is extremely annoying. The problem is further
aggravated by the fact that, once in this situation, when one actully removes a
download manually, the whole lists is reloaded, making the process of manually
cleaning up old downloads one-by-one prohibitively slow.

To be consistent with the browser history, an option "after X days" should be
added to "Remove files from the download manager". At the same time, this would
be an elegant solution to my specific problem. I would really appreciate it.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Jo Hermans]

most of the discussion (for Mozilla) is in bug 132755

Comment 2

[Ali Ebrahim]

Confirming.

Comment 3

[kenno]

Maybe I presented this a bit too much like a solution for bug 161783. While the
original post there already discusses the cons of this "solution", I think it
may be still woth pursuing for other reasons, like privacy, or an improved
consistency of mozilla (firefox) in handling "histories" in general...

Comment 4

[OstGote!]

see bug 132755 comment 106 for a part of the RFE

Comment 5

[kenno]

That's almost a year old now. It's not what I mean.

Suppose you close firefox to work on something else, then open it again half an
hour later. Some people don't want to see their download history erased in this
situation. However, if you choose to keep it, you have to erase it manually,
which is annoying. "Aging" would be a perfectly good solution. Why else is it
implemented for the browser history?

Comment 7

[Shawn Wilsher :sdwilsh]

The question that I have is how often should we check the database and get rid of downloads that are past a given age?

Comment 8

[Shawn Wilsher :sdwilsh]

There is another issue with this as well - we need some way to disable this otherwise out test boxen wouldn't have any downloads when we run our unit tests...

Comment 9

[Shawn Wilsher :sdwilsh]

Without definition, we don't know when this will land...

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #7)
> The question that I have is how often should we check the database and get rid
> of downloads that are past a given age?

Every time we open the download manager, I think. That seems to be an easy way to do it, since we already chunk and display by history.

I think the heuristic should be much like the new one we keep for history. So we keep a minimum of some number of days of history, and then trim based on another value we believe to be reasonable, let's say 200 entries. If there are more than 200 entries, those older than the minimum history length should be pruned. This also solves the problem sdwilsh mentions in comment 8.

Sensible?

Comment 12

[Shawn Wilsher :sdwilsh]

Better idea - register a nsINavHistoryObserver and remove download items when we get onPageExpired, onClearHistory, and onDeleteURI.  We are already tied into places, so this makes sense, and means we will really respect a users history preference.

If we fix this before release, normal users will have download history expired according to their history policy.  However, beta users would have to clear out their older stuff for because it will not expire.

Comment 13

[Ed Lee :Mardak]

Attachment: v1

Made download visits not hidden so that they show up when browsing history.. this means users can select the download from Library and delete them. I also had to make them not hidden so we get the expiration notification.

Didn't do the transaction thing because of how RemoveDownload works and it sends its own notification to listeners immediately.. which won't really work because we wouldn't remove until the transaction ends.

Comment 14

[Ed Lee :Mardak]

Attachment: v1.1

Now with more testcase!

r?mano for making TRANSITION_DOWNLOAD not hidden so they show up in Library and can be deleted as well as allowing their expirations to be observed.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 316509 [details] [diff] [review]
v1.1

>diff --git a/toolkit/components/build/Makefile.in b/toolkit/components/build/Makefile.in
this actually needed?

>diff --git a/toolkit/components/downloads/src/nsDownloadManager.cpp b/toolkit>+NS_IMPL_ISUPPORTS3(
>+  nsDownloadManager
>+  , nsIDownloadManager
>+  , nsINavHistoryObserver
>+  , nsIObserver
>+)
nit: move the comma to the start of the line, so the class name line up

>+nsDownloadManager::RemoveDownloadsForURI(nsIURI *aURI)
>+{
>+  mozStorageStatementScoper scope(mGetIdsForURIStatement);
>+  
>+  nsCAutoString source;
>+  aURI->GetSpec(source);
check result please

>+  nsAutoTArray<PRInt64, 1> downloads;
I think that this is a bad starting number.  The reallocation algorithm doubles the previous size, so I think three or four here would be better - it's stack based anyway so it won't cost us much.

>+  // Remove each download ignoring any failure so we reach other downloads
>+  for (PRInt32 i = downloads.Length(); --i >= 0; )
>+    (void)RemoveDownload(downloads[i]);
should probably do a transaction around this.  Note: http://www.sqlite.org/pragma.html#pragma_read_uncommitted

>   mObserverService->AddObserver(this, "offline-requested", PR_FALSE);
>   mObserverService->AddObserver(this, "sleep_notification", PR_FALSE);
>   mObserverService->AddObserver(this, "wake_notification", PR_FALSE);
>+
>+  history->AddObserver(this, PR_FALSE);
nit: (void)

>+NS_IMETHODIMP
>+nsDownloadManager::OnBeginUpdateBatch()
>+{
begin transaction if one isn't going

>+NS_IMETHODIMP
>+nsDownloadManager::OnEndUpdateBatch()
>+{
and end it

>diff --git a/toolkit/components/downloads/test/unit/test_bug_251337.js 
can we call it by the feature name though - tests are going to be hard to figure out what they are if we always use bug numbers.  something like test_history_expiration.js

r=sdwilsh

Comment 16

[Ed Lee :Mardak]

Attachment: v1.2

(In reply to comment #15)
> (From update of attachment 316509 [details] [diff] [review])
> >diff --git a/toolkit/components/build/Makefile.in b/toolkit/components/build/Makefile.in
> this actually needed?
Yeah, there's some silly thing that includes ../../downloads/ so it also needs to require places.

> >+  nsDownloadManager
> >+  , nsIDownloadManager
> nit: move the comma to the start of the line, so the class name line up
Shifted.

> >+  nsCAutoString source;
> >+  aURI->GetSpec(source);
> check result please
Checked.

> >+  nsAutoTArray<PRInt64, 1> downloads;
> I think that this is a bad starting number.
Start at 4.

> >+  history->AddObserver(this, PR_FALSE);
> nit: (void)
Voided.

> >+nsDownloadManager::OnBeginUpdateBatch()
> begin transaction if one isn't going
Okay. Created mozStorageTransaction if we don't have one.

> >+nsDownloadManager::OnEndUpdateBatch()
> and end it
Okay. Commit transaction with delete.

> >diff --git a/toolkit/components/downloads/test/unit/test_bug_251337.js 
> something like test_history_expiration.js
Renamed.

Comment 17

[Ed Lee :Mardak]

Comment on attachment 316520 [details] [diff] [review]
v1.2

Oops, there's the places bits too for making downloads not hidden.

Comment 18

[Ed Lee :Mardak]

Comment on attachment 316520 [details] [diff] [review]
v1.2

<Mano> seems fine, but ask dietrich 

tentative r+

a1.9? for auto download expiration based on user's history to address concerns about infinitely large download lists. has testcase too

Comment 19

[Brendan Eich [:brendan]]

Ed, would this have left your Starcraft example happy (bug 400495 comment 89 first paragraph)?

/be 

Comment 20

[Ed Lee :Mardak]

Depends on the history setting (I believe expire days is 180 and min pages = 40k), but I'm willing to give up some starcraft if people sleep better at night not worrying if their open download manager has 5k+ download items after using it for several years. ;)

(I have 717 download entries since Aug 1st.)

But I suppose it's potentially problematic because AwesomeBar wouldn't be able to find it either because places will expire it for both.

Comment 21

[Ed Lee :Mardak]

dietrich: This would make downloads not hidden and appear in the Library which wouldn't be unreasonable as users are clicking links to download files and potentially would want to see the page. Compared to embedded visits, users and observers probably aren't as interested in those.

Comment 22

[Ed Lee :Mardak]

dietrich: FYI, I said Library and not also AwesomeBar because by default, places.frecency.downloadVisitBonus is 0.

Comment 23

[Dietrich Ayala (:dietrich)]

not showing up in the location bar is good. opening these should be ok, as they go through same tabbrowser/window codepath as any other history item.

it'd be beneficial to have distinct visual identity for download items in places' history views... hm, so there are bugs for that, and they're blocking-. that might change if downloads will be visible by default, and in regular browsing history views. sorry to extend the gauntlet, but please get UX input on this change.

also, i'm not clear on why you are not instead enabling notifications for hidden items, which would resolve the problem without late UI impact. is there a specific reason you did not go this route?

Comment 24

[Ed Lee :Mardak]

There's FIXME comments in the code that says "bug 325241 provide a way to observe hidden elements"

There's also a comment from bug 319216 about why hidden things don't get notifications:
"We spend potentially a lot of time sending out observer notifications for image
and other stuff that is marked hidden that is normally never seen. An observer
should be able to specify whether it wants noficitations of hidden items so
that the common case can be faster."

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #12)
> Better idea - register a nsINavHistoryObserver and remove download items when
> we get onPageExpired, onClearHistory, and onDeleteURI.  We are already tied
> into places, so this makes sense, and means we will really respect a users
> history preference.

This makes perfect sense, yes - Mardak, I can't tell from comments, did you implement things this way? I think you did ...

> If we fix this before release, normal users will have download history expired
> according to their history policy.  However, beta users would have to clear out
> their older stuff for because it will not expire.

OK.

Comment 26

[Dietrich Ayala (:dietrich)]

(In reply to comment #24)
> There's also a comment from bug 319216 about why hidden things don't get
> notifications:
> "We spend potentially a lot of time sending out observer notifications for
> image
> and other stuff that is marked hidden that is normally never seen. An observer
> should be able to specify whether it wants noficitations of hidden items so
> that the common case can be faster."
> 

most of the hidden items in my db are also embedded items, which we don't send notifications for.

Comment 27

[Damon Sicore (:damons)]

Comment on attachment 316520 [details] [diff] [review]
v1.2

a1.9+=damons

Comment 28

[Ed Lee :Mardak]

Checking in toolkit/components/build/Makefile.in;
/cvsroot/mozilla/toolkit/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.60; previous revision: 1.59
done
Checking in toolkit/components/downloads/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.182; previous revision: 1.181
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.64; previous revision: 1.63
done
RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/unit/test_history_expiration.js,v
done
Checking in toolkit/components/downloads/test/unit/test_history_expiration.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_history_expiration.js,v  <--  test_history_expiration.js
initial revision: 1.1
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.294; previous revision: 1.293
done

Comment 29

[Bill Gianopoulos [:WG9s]]

This check-in appears to have broken Thunderbird builds.

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to comment #29)
> This check-in appears to have broken Thunderbird builds.

Sunbird as well... Both the Thunderbird & Sunbird tinderboxes are on fire pretty much across the board.

Comment 31

[Daniel Holbert [:dholbert]]

Backed out the checkin from comment 28 to fix Thunderbird & Sunbird bustage.

Checking in toolkit/components/build/Makefile.in;
/cvsroot/mozilla/toolkit/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.61; previous revision: 1.60
done
Checking in toolkit/components/downloads/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v  <--  Makefile.in
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.183; previous revision: 1.182
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.65; previous revision: 1.64
done
Removing toolkit/components/downloads/test/unit/test_history_expiration.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_history_expiration.js,v  <--  test_history_expiration.js
new revision: delete; previous revision: 1.1
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.295; previous revision: 1.294
done

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 316520 [details] [diff] [review]
v1.2

>diff --git a/toolkit/components/downloads/src/nsDownloadManager.h b/toolkit/components/downloads/src/nsDownloadManager.h
>@@ -56,37 +56,41 @@
>+#include "nsINavHistoryService.h"

"WINNT 5.2 sb-mozbuild-win Depend Sb-Nightly mozbuild on 2008/04/21 15:25:00"
reports
{{
...\toolkit\components\downloads\src\nsDownloadManager.h(68) : fatal error C1083: Cannot open include file: 'nsINavHistoryService.h': No such file or directory
}}

MXR answer that <nsINavHistoryService.h> doesn't exist.

Comment 33

[Ed Lee :Mardak]

dietrich: Are xulrunner apps not supposed to have places? toolkit/components/Makefile.in checks MOZ_PLACES to include places/ in the build.

But even though places is under toolkit, not all toolkit would want it? It should be browser only?

Comment 34

[Ed Lee :Mardak]

Attachment: ifdef MOZ_PLACES

Would this be okay? shawn? dietrich?

Comment 35

[Ed Lee :Mardak]

Attachment: single ifdef MOZ_PLACES

Comment 36

[Ed Lee :Mardak]

Attachment: always build nsINavHistoryService.idl

Comment 37

[Ed Lee :Mardak]

http://hg.mozilla.org/cvs-trunk-mirror/index.cgi/rev/5e8fa7e3f219
http://hg.mozilla.org/mozilla-central/index.cgi/rev/5e8fa7e3f219

Checking in toolkit/components/build/Makefile.in;
/cvsroot/mozilla/toolkit/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.62; previous revision: 1.61
done
Checking in toolkit/components/downloads/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v  <--  Makefile.in
new revision: 1.25; previous revision: 1.24
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.184; previous revision: 1.183
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.66; previous revision: 1.65
done
Checking in toolkit/components/downloads/test/unit/test_history_expiration.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_history_expiration.js,v  <--  test_history_expiration.js
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.296; previous revision: 1.295
done
Checking in toolkit/components/Makefile.in;
/cvsroot/mozilla/toolkit/components/Makefile.in,v  <--  Makefile.in
new revision: 1.83; previous revision: 1.82
done
Checking in toolkit/components/places/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/places/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/places/public/Makefile.in,v  <--  Makefile.in
new revision: 1.12; previous revision: 1.11
done

Comment 38

[Dietrich Ayala (:dietrich)]

this is causing crasher bug 431346, which is currently #36 on topcrashers.

http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0pre&range_value=2&signature=mozStorageTransaction%3A%3A~mozStorageTransaction()