Last Comment Bug 380250 - Convert Download Manager's RDF backend to mozStorage
: Convert Download Manager's RDF backend to mozStorage
Status: VERIFIED FIXED
:
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9alpha4
Assigned To: Shawn Wilsher :sdwilsh
:
:
Mentors:
: 161783 461617 (view as bug list)
Depends on: 381538 381542 381546 381552 381561 381562 381601 381801 381803 382081 383796 390748
Blocks: 227401 240046 240525 261973 280442 345117 345932 358914 364943 365614 374355 377793 380441 381157 381603
  Show dependency treegraph
 
Reported: 2007-05-10 00:10 PDT by Shawn Wilsher :sdwilsh
Modified: 2012-01-10 00:37 PST (History)
32 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0.1 (84.21 KB, patch)
2007-05-10 00:10 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v0.2 (102.61 KB, patch)
2007-05-10 14:50 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v0.3 (110.59 KB, patch)
2007-05-11 12:31 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v0.3.1 (110.59 KB, patch)
2007-05-11 12:35 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v0.3.2 (112.07 KB, patch)
2007-05-11 13:14 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
0.4 (150.16 KB, patch)
2007-05-13 12:40 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
0.5 (173.11 KB, patch)
2007-05-14 23:56 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.0 embedding, xpfe, and camino fixes (5.05 KB, patch)
2007-05-16 01:04 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.0 c++ backend; idl changes (131.68 KB, patch)
2007-05-16 01:32 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.0 frontend changes (24.24 KB, patch)
2007-05-16 01:35 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.0 tests (21.06 KB, patch)
2007-05-16 16:07 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.1 c++ backend; idl changes (134.64 KB, patch)
2007-05-17 11:30 PDT, Shawn Wilsher :sdwilsh
cbiesinger: review+
Details | Diff | Splinter Review
v1.1 tests (625 bytes, patch)
2007-05-17 11:35 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.1.1 tests (21.20 KB, patch)
2007-05-17 11:40 PDT, Shawn Wilsher :sdwilsh
mconnor: review+
cbiesinger: review+
Details | Diff | Splinter Review
v1.1 frontend changes (29.61 KB, patch)
2007-05-17 14:42 PDT, Shawn Wilsher :sdwilsh
mconnor: review+
Details | Diff | Splinter Review
v1.2 tests (24.73 KB, patch)
2007-05-21 11:06 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.2 frontend (29.92 KB, patch)
2007-05-21 11:16 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.2 backend; idl (134.67 KB, patch)
2007-05-21 11:25 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2007-05-10 00:10:14 PDT
Created attachment 264342 [details] [diff] [review]
v0.1

This first patch is just really to show brahmana what is planned.  I've got a bunch of things that I've got written down on paper that needs to be fixed before I can even think of asking for review.
Comment 1 neil@parkwaycc.co.uk 2007-05-10 04:34:46 PDT
Comment on attachment 264342 [details] [diff] [review]
v0.1

>+  NS_ADDREF(*aResult = mCurrentDownloads);
Someone might think that the way to add a download is to QI this to nsIMutableArray and add a download - and it wouldn't even be an nsDownload object, which would be bad. Why not stick with a hashtable?
Comment 2 Shawn Wilsher :sdwilsh 2007-05-10 14:50:23 PDT
Created attachment 264410 [details] [diff] [review]
v0.2

OK, this addresses my first pass of comments.  It is untested and is just the backend code.  I'll try to address any comments, so please make them.

Neil - while it's true that someone *could* QueryInterface that, I'm returning an nsIArray for a reason (as in, it's not supposed to be mutable).  So, if they want to shoot themselves in the foot, I'd rather let them than to hold their hand through the whole process.  If you'd like, I can add more to the idl about not doing that, but I think the return type of nsIArray communicates that.
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-05-10 15:27:49 PDT
I agree that exposing an array which could be QIed to nsIMutableArray is sub-optimal, and confusing.

Now, if there's no good reason to expose the array itself, I would rather expose it as a simple enumerator.
Comment 4 Shawn Wilsher :sdwilsh 2007-05-11 12:31:42 PDT
Created attachment 264516 [details] [diff] [review]
v0.3

More work - fuller patch.  Mostly posting to get help with my unit tests.
Comment 5 Shawn Wilsher :sdwilsh 2007-05-11 12:35:34 PDT
Created attachment 264517 [details] [diff] [review]
v0.3.1
Comment 6 Shawn Wilsher :sdwilsh 2007-05-11 13:14:49 PDT
Created attachment 264522 [details] [diff] [review]
v0.3.2
Comment 7 Shawn Wilsher :sdwilsh 2007-05-12 19:50:22 PDT
So, the image url, which is a parameter in nsIDownloadManager::AddDownload is never used.  The only place the function is called is here:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadProxy.h#74
Note that that just passes EmptyString() along.  Does anyone disagree with removing it?
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-05-12 23:05:55 PDT
Er?

http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#651
Comment 9 Shawn Wilsher :sdwilsh 2007-05-12 23:22:23 PDT
Whoops.  mxr doesn't work so well across languages :)  Thanks for pointing that out!
Comment 10 Shawn Wilsher :sdwilsh 2007-05-13 12:40:48 PDT
Created attachment 264679 [details] [diff] [review]
0.4

Alright.  This works in my testing quite nicely.  The diff isn't very pretty due to the vast number of changes in nsDownloadManager.cpp, so whoever reviews this might want to apply it, then look at that file as it is.

The tests don't quite work as they should yet, but I need to talk to people who are good with the testing framework to iron that out.
Comment 11 Devon Jensen 2007-05-14 18:31:35 PDT
Forgive my ignorance of this newfangled mozStorage stuff

Will extensions be able to:
1) Edit/Change existing parameters in the downloads database for specific downloads (i.e. it is not locked)

2) Dynamically add a new parameter for a specific download in the database (without confusing the back or front end - i.e. they will ignore or not even see these new parameters)

Also, any idea on the performance of this compared to the current rdf implementation - specifically wrt large download histories?  
Comment 12 Shawn Wilsher :sdwilsh 2007-05-14 19:00:07 PDT
(In reply to comment #11)
> 1) Edit/Change existing parameters in the downloads database for specific
> downloads (i.e. it is not locked)

Sure, but they should be careful.

> 2) Dynamically add a new parameter for a specific download in the database
> (without confusing the back or front end - i.e. they will ignore or not even
> see these new parameters)

Sure, but I wouldn't recommend it.  If we ever change anything internally, it could really screw things up.

> Also, any idea on the performance of this compared to the current rdf
> implementation - specifically wrt large download histories?  

It shouldn't be an issue, or at least not nearly as much of an issue as before.
Comment 13 Shawn Wilsher :sdwilsh 2007-05-14 23:56:34 PDT
Created attachment 264842 [details] [diff] [review]
0.5

Ok, this includes migration code.  yey.  The tests aren't done, but they are there for you to look at if you feel so incline.  I'm not exactly happy with the migration code, but at the time I could think of no better way.  Thoughts about it will be most appreciated.
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-15 11:55:14 PDT
uriloader/base/nsIDownload.idl
+     * The state of the download.
+     */
+    readonly attribute short state;

You should mention where to find the possible values for the state. It's
somewhat unfortunate, though, that this means you have to reference a
toolkit file in uriloader...

Why is this not unsigned?

Also, this patch will break Camino and SeaMonkey. It would be nice if you
added those functions there (just returning an error).

+++ toolkit/components/downloads/public/nsIDownloadManager.idl	15 May 2007 06:44:55 -0000
+   * @throws NS_ERROR_FAILURE if the download is not in-progress.

Or if no download with that id exists at all?

(I kind of think that that functionality should just be on the nsIDownload
object instead, but you don't have to change that for this patch :-) )

+   * Stops the download with the specified ID if it's current in progress.  This

"currently"

Also, timeless points out that this new comment is somewhat misleading,
because it makes it non-obvious how the pause function differs from the
cancel one - conceivably, a stopped download can be resumed again.

I'd just stay with "Cancels the download ..." :)

+   * Removes the download with the specified id if it's not current in progress.

here too. (it used to be currently, why did you change it?)

+   * Opens the Download Manager front end, making the specified download
+   * selected.

Hm, wouldn't "selecting the specified download" sound better?

What should I pass if I don't want to select any download?

    * An enumeration of active downloads.

Please mention the type of the objects in the enumeration (nsIDownload)

+++ toolkit/components/downloads/src/nsDownloadManager.cpp	15 May 2007 06:44:55 -0000
+static const nsInt64 sUpdateInterval((PRUint32)(400 * PR_USEC_PER_MSEC));

Since you touched this line :) Use PRInt64 instead of nsInt64, and remove
the cast. Also - it should keep its g prefix, it's still a global.

+    nsresult result = CancelDownload(id);
+    // We want to try the rest of them because they should be canceled if they
+    // can be canceled.
+    if (NS_FAILED(result)) rv = result;
+  }
 
So, this code worries me a bit. If you get a case where CancelDownload
fails and leaves the download in the array, you've got an infinite loop.

Why not make the loop look more like this:
  for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i)


+    printf("*** Added download '%s'\n", NS_ConvertUTF16toUTF8(name).get());

Remove that before checking this patch in

+  // lock will be released once we return

What lock is this referring to?


In nsDownloadManager::GetRetentionBehavior(), it would be nice to keep the
comment about what 0 means.

I'd have added inline setters for the various members of nsDownload,
instead of accessing the variables directly.

Hrm... you sometimes return an error from AddDownload and don't add the
download to the DB or mCurrentDownloads, but still add it to
the nsXPIProgressListener...

Why did you move the dl-start notification elsewhere?

+  return CallQueryInterface(dl.get(), aDownload);

No point to call QueryInterface, just do:
  NS_ADDREF(*aDownload = dl);
  return NS_OK;

+  nsRefPtr<nsDownload> itm = FindDownload(aID);

Why does FindDownload return an addrefed download, instead of a
non-addrefed raw pointer? If you change it there, make this a raw pointer
too.

+  if (!itm) {
+    *aDownloadItem = nsnull;
+
+    return NS_ERROR_FAILURE;
 
remove that empty line

+  NS_ADDREF(*aDownloadItem = itm);
 
-  *aDownloadItem = nsnull;
   return NS_OK;

That one too

+nsDownloadManager::FindDownload(PRUint32 aID)
+{
+  // we shouldn't ever have many downloads, so we can loop over them
+  for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) {

Hm, why are you iterating backwards here?

-  // Take a strong reference to the download object as we may remove it from
-  // mCurrDownloads in DownloadEnded.

That comment should stay, except for the reference to DownloadEnded


+  dl->GetCancelable(getter_AddRefs(cancelable));
...
+  if (dl->mTempFile) {

Why are you sometimes using the getter and sometimes using the variable
directly?


+  // This have to be done in this exact order to not mess up our invariants

What are those invariants? It would be nice to document them somewhere.

+  NS_ENSURE_TRUE(RemoveDownloadFromCurrent(dl), NS_ERROR_FAILURE);

Hm, there is no case where this can fail here, is there?

-  // RemoveDownload is for downloads not currently in progress. Having it
-  // cancel in-progress downloads would make things complicated, so just return.

Why remove that comment?

-  // 1). Downloads that can be cleaned up include those that are finished, 
-  //     failed or canceled.

OK, I wouldn't have removed that one either, but I guess it's pretty
obvious.

+  rv = stmt->GetInt32(0, aResult);

I'd rather you didn't return values other than 0 and 1 in a boolean...

   mListener = aListener;
+
   return NS_OK;

You like those empty lines, don't you? :)

 nsDownloadManager::GetListener(nsIDownloadProgressListener** aListener)
 {
-  *aListener = mListener;
-  NS_IF_ADDREF(*aListener);
-  return NS_OK;
-}
+  NS_ENSURE_ARG_POINTER(aListener);

No point in that check really...

+nsDownloadManager::GetDatasource(mozIStorageConnection **aDBConn)
+{
+  NS_ENSURE_ARG_POINTER(aDBConn);

Neither here. Is GetDatasource a good name for this? Perhaps DBConnection
might be better.

+inline
+PRBool
+nsDownloadManager::RemoveDownloadFromCurrent(nsDownload *aDownload)

Does this actually get inlined if you do it like this?


While you're touching Observe, wanna change the nsCRT::strcmp calls to
plain strcmp?

+nsDownloadManager::ConfirmCancelDownloads(PRInt32 aCount,
...
+  {

Why the block?


No point in settinbg mDownloadManager to null in ~nsXPIProgressListener

nsXPIProgressListener::OnStateChange(PRUint32 aIndex, PRInt16 aState, PRInt32 aValue)

Shouldn't you call dl->UpdateDB where the previous code called
AssertProgressInfoForDownload? Actually a general question, not limited to
this function.

-    if (LL_IS_ZERO(mCurrBytes))
+    if (LL_IS_ZERO(mCurrBytes)) {

Since you're touching this, make it:
  if (mCurrBytes == 0) {


Why did you make mPercentComplete a PRInt16?

in nsDownload::OnStatusChange:
+    nsresult rv = UpdateDB();
+    NS_ENSURE_SUCCESS(rv, rv);

I don't think failure from UpdateDB should prevent the other code here
from being run...

+    nsCOMPtr<nsIDownload> kungFuDeathGrip;
+    CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**,
+                                            getter_AddRefs(kungFuDeathGrip)));

 nsRefPtr<nsDownload> kungFuDeathGrip(this);

Much nicer, isn't it? :)

But why did the old code not need this?

+    NS_ENSURE_TRUE(mDownloadManager->RemoveDownloadFromCurrent(this),
+                   NS_ERROR_FAILURE);

Don't think you should return here either. (Both OnStateChange and
OnStatusChange)

In OnStateChange, I don't think that lack of a pref service should be
fatal.

-      if (mMaxBytes == -1)
+      if (mMaxBytes == PR_UINT32_MAX)

Surely you mean LL_MAXUINT?

+          nsCOMPtr<nsIAlertsService> alerts =
+            do_GetService("@mozilla.org/alerts-service;1", &rv);
+            NS_ENSURE_SUCCESS(rv, rv);

Again, this shouldn't be fatal. (the NS_ENSURE_SUCCESS line is also
indented too much)

+++ toolkit/components/downloads/src/nsDownloadProxy.h	15 May 2007 06:44:55 -0000

I'd make that file not implement nsIDownload anymore...


--

BTW, is it a good idea to notify people about download state changes
while you're in the middle of updating member variables? (e.g. in
nsDownload::OnStateChange)


Done with the C++ parts, I'll review the UI a bit later.
Comment 15 Shawn Wilsher :sdwilsh 2007-05-16 00:46:04 PDT
(In reply to comment #14)
> You should mention where to find the possible values for the state. It's
> somewhat unfortunate, though, that this means you have to reference a
> toolkit file in uriloader...
done

> Why is this not unsigned?
Because nsIDownloadManager and nsIXPInstallManagerUI use PRInt16's and not PRUInt16's.  I'm just following the idl's in this case (and nsIDownloadManager::DOWNLOAD_NOTSTARTED = -1)

> Also, this patch will break Camino and SeaMonkey. It would be nice if you
> added those functions there (just returning an error).
done (attaching patch of just those files in a bit)

> +   * @throws NS_ERROR_FAILURE if the download is not in-progress.
> 
> Or if no download with that id exists at all?
I would argue that that is already implied by being 'not in-progress'

> +   * Stops the download with the specified ID if it's current in progress. 
> Also, timeless points out that this new comment is somewhat misleading,
> because it makes it non-obvious how the pause function differs from the
> cancel one - conceivably, a stopped download can be resumed again.
I had renamed the method, then reverted that but forgot to update the description.  Fixed.

> +   * Removes the download with the specified id if it's not current in
> progress.
> here too. (it used to be currently, why did you change it?)
I don't know...Fixed.

> +   * Opens the Download Manager front end, making the specified download
> +   * selected.
> Hm, wouldn't "selecting the specified download" sound better?
Yup.  Fixed.

> What should I pass if I don't want to select any download?
We fail.  Added documentation.  I don't like this behavior at all, but it's what we did in the past, so I will file a follow-up bug and fix that once this lands.

>     * An enumeration of active downloads.
> Please mention the type of the objects in the enumeration (nsIDownload)
Done.

> +static const nsInt64 sUpdateInterval((PRUint32)(400 * PR_USEC_PER_MSEC));
> Since you touched this line :) Use PRInt64 instead of nsInt64, and remove
> the cast. Also - it should keep its g prefix, it's still a global.
Fixed.


> +    nsresult result = CancelDownload(id);
> +    // We want to try the rest of them because they should be canceled if they
> +    // can be canceled.
> +    if (NS_FAILED(result)) rv = result;
> +  }
> So, this code worries me a bit. If you get a case where CancelDownload
> fails and leaves the download in the array, you've got an infinite loop.
Wow, that *is* scary.  Fixed.

> +  // lock will be released once we return
> What lock is this referring to?
Clarified.

> In nsDownloadManager::GetRetentionBehavior(), it would be nice to keep the
> comment about what 0 means.
Fixed.

> Hrm... you sometimes return an error from AddDownload and don't add the
> download to the DB or mCurrentDownloads, but still add it to
> the nsXPIProgressListener...
Whoops.  Fixed the ordering of the code to have anything that can error (just the database stuff) to happen before.

> Why did you move the dl-start notification elsewhere?
This is something that came up while testing.  If something was listening for "dl-start", they would get the nsIDownload object, but if you checked the state of the download, it would be nsIDownloadManager::DOWNLOAD_NOTSTARTED.  So, I only dispatch that when the download has actually started now.

> +  return CallQueryInterface(dl.get(), aDownload);
> No point to call QueryInterface, just do...
Fixed

> +  nsRefPtr<nsDownload> itm = FindDownload(aID);
> Why does FindDownload return an addrefed download, instead of a
> non-addrefed raw pointer? If you change it there, make this a raw pointer
> too.
Actually, no good reason.  I've updated the file accordingly.

> +  // we shouldn't ever have many downloads, so we can loop over them
> +  for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) {
> Hm, why are you iterating backwards here?
To avoid a function call on every loop to Count.

> -  // Take a strong reference to the download object as we may remove it from
> -  // mCurrDownloads in DownloadEnded.
> That comment should stay, except for the reference to DownloadEnded
I ended up adding one in the process of fixing FindDowload.  Fixed.

> +  dl->GetCancelable(getter_AddRefs(cancelable));
> ...
> +  if (dl->mTempFile) {
> Why are you sometimes using the getter and sometimes using the variable
> directly?
Mostly because I missed some.  I cleaned up any others I found.

> +  // This have to be done in this exact order to not mess up our invariants
> What are those invariants? It would be nice to document them somewhere.
So, I'm not really sure where I should document it, or if the word invariant is quite right in my application, but it basically comes down to this:
First we remove the download from current downloads.  We then set the state of the download, which calls OnDownloadStateChange on the download listener.  This passes an nsIDownload along with it, so we have to make sure that if someone were to then try and use any functions (remove, cancel, pause, resume), that the download would be out of the current ones.  Once we update the state, we can dispatch "dl-cancel", and the download state will actually be nsIDownloadManager::DOWNLOAD_CANCELED as one would expect.  This all came out while testing it and getting results that were unexpected, so I fixed it to work in a way that makes sense.

> +  NS_ENSURE_TRUE(RemoveDownloadFromCurrent(dl), NS_ERROR_FAILURE);
> Hm, there is no case where this can fail here, is there?
After hunting down all the function calls, it will only fail if the object isn't in the array, which I check for, so no I do not have to check that.

> -  // RemoveDownload is for downloads not currently in progress. Having it
> -  // cancel in-progress downloads would make things complicated, so just
> return.
> Why remove that comment?
Because the IDL file explicitly states what the expected behavior of the function is.

> +  rv = stmt->GetInt32(0, aResult);
> I'd rather you didn't return values other than 0 and 1 in a boolean...
Fair enough, fixed.

>  nsDownloadManager::GetListener(nsIDownloadProgressListener** aListener)
>  {
> +  NS_ENSURE_ARG_POINTER(aListener);
> No point in that check really...
Fixed.

> +nsDownloadManager::GetDatasource(mozIStorageConnection **aDBConn)
> +{
> +  NS_ENSURE_ARG_POINTER(aDBConn);
Fixed.

> Neither here. Is GetDatasource a good name for this? Perhaps DBConnection
> might be better.
I agree.  Updated accordingly.

> +inline
> +PRBool
> +nsDownloadManager::RemoveDownloadFromCurrent(nsDownload *aDownload)
> Does this actually get inlined if you do it like this?
I'm not so sure.  I've moved it to the header file where I'm fairly certain that it is.

> While you're touching Observe, wanna change the nsCRT::strcmp calls to
> plain strcmp?
Sure.

> +nsDownloadManager::ConfirmCancelDownloads(PRInt32 aCount,
> ...
> +  {
> Why the block?
I uh....don't know.  Fixed.

> No point in setting mDownloadManager to null in ~nsXPIProgressListener
I don't think I touched that, but I can fix that.

> Shouldn't you call dl->UpdateDB where the previous code called
> AssertProgressInfoForDownload? Actually a general question, not limited to
> this function.
Generally, yes.  I only need to do it whenever we set some data, so I think (thought) I got them all.  I did miss that one at least.  I'll look for others.


> -    if (LL_IS_ZERO(mCurrBytes))
> +    if (LL_IS_ZERO(mCurrBytes)) {
> Since you're touching this, make it:
>   if (mCurrBytes == 0) {
Fixed.

> Why did you make mPercentComplete a PRInt16?
Er...mostly because that's what I thought the IDL had, but looking again it doesn't.  I'll revert that back.

> in nsDownload::OnStatusChange:
> +    nsresult rv = UpdateDB();
> +    NS_ENSURE_SUCCESS(rv, rv);
> I don't think failure from UpdateDB should prevent the other code here
> from being run...
Yeah, fair enough.  Fixed.

> +    nsCOMPtr<nsIDownload> kungFuDeathGrip;
> +    CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**,
> +                                            getter_AddRefs(kungFuDeathGrip)));
> 
>  nsRefPtr<nsDownload> kungFuDeathGrip(this);
> Much nicer, isn't it? :)
Yes, much!

> But why did the old code not need this?
Because when it is removed from mCurrentDownloads, it releases it.  However, with moving that call to the end (which is OK for me to do), I can get away with not doing it.  Fixed.

> In OnStateChange, I don't think that lack of a pref service should be
> fatal.
Good point.  Fixed - all calls check before using |if (pref)...|

> -      if (mMaxBytes == -1)
> +      if (mMaxBytes == PR_UINT32_MAX)
> Surely you mean LL_MAXUINT?
Surely.

> +          nsCOMPtr<nsIAlertsService> alerts =
> +            do_GetService("@mozilla.org/alerts-service;1", &rv);
> +            NS_ENSURE_SUCCESS(rv, rv);
> Again, this shouldn't be fatal. (the NS_ENSURE_SUCCESS line is also
> indented too much)
Yeah, fixed.

> +++ toolkit/components/downloads/src/nsDownloadProxy.h  15 May 2007 06:44:55
> -0000
> I'd make that file not implement nsIDownload anymore...
Done.  I'm not really sure why it was included in the first place, and the bug associated with it doesn't really explain it either.

> BTW, is it a good idea to notify people about download state changes
> while you're in the middle of updating member variables? (e.g. in
> nsDownload::OnStateChange)
I found one instance where I wasn't doing that wisely (the function you mentioned actually).  Fixed.
Comment 16 Shawn Wilsher :sdwilsh 2007-05-16 01:04:41 PDT
Created attachment 264973 [details] [diff] [review]
v1.0 embedding, xpfe, and camino fixes
Comment 17 Shawn Wilsher :sdwilsh 2007-05-16 01:32:47 PDT
Created attachment 264976 [details] [diff] [review]
v1.0 c++ backend; idl changes
Comment 18 Shawn Wilsher :sdwilsh 2007-05-16 01:35:20 PDT
Created attachment 264977 [details] [diff] [review]
v1.0 frontend changes
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2007-05-16 07:15:39 PDT
Comment on attachment 264976 [details] [diff] [review]
v1.0 c++ backend; idl changes

>Index: uriloader/base/nsIDownload.idl

>+    readonly attribute unsigned long id;
>+    readonly attribute short state;

Maybe it's just me (others should note any disagreement), but I don't have the conversions between C++-style IDL types and PR* types memorized.  Could you expand these into PR* types while you're here?  You could probably even do this for interfaces you're not otherwise changing (e.g. the one containing download states), because I hope (would need to test) that there's no ABI change from doing a "same-type" switch (e.g. long to PRInt32) of a parameter or return value type.
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-05-16 07:22:19 PDT
Aren't PR-types in XPIDL supported just for backwards-compatibility (well, except PRTime I guess). I cannot see why we would want to expose PRTypes to say, js consumers. You don't need to memorize anything, just look at the header ;)
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-05-16 07:23:47 PDT
http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsrootidl.idl#61 for reference
Comment 22 Jeff Walden [:Waldo] (remove +bmo to email) 2007-05-16 07:47:57 PDT
Also <http://www.mozilla.org/scriptable/xpidl/idl-authors-guide/rules.html>, but I still have to look one or the other up every time.  Also, it's precisely the C++ case that concerns me.  A coder in JS-land can just use the simple rule that *Int* is an integer, *Uint* is an unsigned integer, etc. and otherwise ignore PR* types.  A coder in C++-land has no such rule for correctly mapping onto concrete types and must fall back on the header or some other documentation.

Also, if the PR type are only for backwards compat, there's an awfully large number of new interfaces still choose to use them.
Comment 23 Shawn Wilsher :sdwilsh 2007-05-16 16:07:16 PDT
Created attachment 265058 [details] [diff] [review]
v1.0 tests

This brought up a few minor changes to the v1.0 c++ backend patch, but I won't bother attaching that unless mconnor wants it.  The code isn't terribly different - just fixed a few issues that came up while testing.
Comment 24 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-17 09:11:51 PDT
>> +  for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) {
>> Hm, why are you iterating backwards here?
> To avoid a function call on every loop to Count.

Well, you could have done:

  for (PRUint32 i = 0, e = mCurrentDownloads.Count(); i < e; ++i) 

> So, I'm not really sure where I should document it, or if the word invariant is
> quite right in my application

If in doubt, try the place where you mention that they exist. Really, if someone will modify this code in the future, how are they to know what this code tries to ensure?

> We then set the state of
> the download, which calls OnDownloadStateChange on the download listener.  This
> passes an nsIDownload along with it, so we have to make sure that if someone
> were to then try and use any functions (remove, cancel, pause, resume), that
> the download would be out of the current ones.  Once we update the state, we
> can dispatch "dl-cancel", and the download state will actually be
> nsIDownloadManager::DOWNLOAD_CANCELED as one would expect. 

Will those methods really work on a cancelled download?

Also: You asked me about whether to add a downloadStateChanged method at one point. I didn't know about these observer service notifications (dl-start, dl-cancel, etc). Given those, is it really necessary to add another mechanism for those notifications? (I think there's some value in keeping nsIDownloadProgressListener in sync with nsIWebProgressListener)

> Maybe it's just me (others should note any disagreement), but I don't have the
> conversions between C++-style IDL types and PR* types memorized.  Could you
> expand these into PR* types while you're here? 

Hm, I think they look ugly...

Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-17 09:45:45 PDT
as for the UpdateDB calls where there used to be an AssertProgressInfoFor:

It seems that CancelAllDownloads is missing them.

GetDownload and GetDBConnection have a useless NS_ENSURE_ARG_POINTER too, I must've missed them before

are you no longer checking downloadCount in the observer for offline-requested?

+   nsAutoString countString(aCount);

Does that actually do the right thing?

+  PRTime delta = now - mLastUpdate;

I always considered PRTime to be just meant to store µsec since the epoch, not also deltas...

in OnStatusChange:
    nsCOMPtr<nsIDownload> kungFuDeathGrip;
    CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**,
                                            getter_AddRefs(kungFuDeathGrip)));
    mDownloadManager->RemoveDownloadFromCurrent(this);

see nsRefPtr<> sugegestion in an earlier comment


You now call UpdateDB both in nsDownloadManager::PauseResumeDownload and in nsDownload::PauseResume, I'd remove it from the former.
Comment 26 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-17 10:04:50 PDT
Comment on attachment 264977 [details] [diff] [review]
v1.0 frontend changes

nsDownloadProgressListener.js:
    var aDownloadID = aDownload.targetFile.path;
    var download = this.doc.getElementById(aDownloadID);

HTML neither allows slashes nor backslashes in an ID. Also, non-arguments
shouldn't start with "a"


downloads.js:

const nsILocalFile = Components.Constructor("@mozilla.org/file/local;1",
                                            "nsILocalFile", "initWithPath");

I don't like naming non-interfaces nsI*. Please rename this to nsLocalFile.

--

Do Remove/Cleanup work correctly? I.e., do they update the UI to remove the removed downloads? (because I don't see how this can work...)


[this wasn't a full review]
Comment 27 Dan Mosedale (:dmose) 2007-05-17 10:12:12 PDT
(In reply to comment #25)
> +  PRTime delta = now - mLastUpdate;
> 
> I always considered PRTime to be just meant to store µsec since the epoch, not
> also deltas...

Yeah; use PRIntervalTime for deltas.  It has the advantage of not getting confused if your system clock changes (eg is reset by you or updated by NTP).
Comment 28 Shawn Wilsher :sdwilsh 2007-05-17 11:27:07 PDT
(In reply to comment #24)
> If in doubt, try the place where you mention that they exist. Really, if
> someone will modify this code in the future, how are they to know what this
> code tries to ensure?
Fixed.

> Will those methods really work on a cancelled download?
Uh, no - I think I was talking about more in general with the observers/listeners.


> Also: You asked me about whether to add a downloadStateChanged method at one
> point. I didn't know about these observer service notifications (dl-start,
> dl-cancel, etc). Given those, is it really necessary to add another mechanism
> for those notifications? (I think there's some value in keeping
> nsIDownloadProgressListener in sync with nsIWebProgressListener)
As per discussion on irc, we'll keep this but have it send the old state and the download as opposed to the new state (since the new state can be found on the download anyway).

(In reply to comment #25)
> as for the UpdateDB calls where there used to be an AssertProgressInfoFor:
> It seems that CancelAllDownloads is missing them.
Yes, it is, but it calls CancelDownload on each one, which has that on its last line.

> GetDownload and GetDBConnection have a useless NS_ENSURE_ARG_POINTER too, I
> must've missed them before
Fixed.

> are you no longer checking downloadCount in the observer for offline-requested?
Uh, whoops :)  Fixed.

> +   nsAutoString countString(aCount);
> Does that actually do the right thing?
I can't find any code to prove that it does, but I thought it did.  I'm going to revert that back to the old way though.

> I always considered PRTime to be just meant to store µsec since the epoch, not
> also deltas...
Updated to dmose's suggestion.

> in OnStatusChange:
>     nsCOMPtr<nsIDownload> kungFuDeathGrip;
>     CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**,
>                                             getter_AddRefs(kungFuDeathGrip)));
>     mDownloadManager->RemoveDownloadFromCurrent(this);
> 
> see nsRefPtr<> sugegestion in an earlier comment
Fixed.

> You now call UpdateDB both in nsDownloadManager::PauseResumeDownload and in
> nsDownload::PauseResume, I'd remove it from the former.
Hmm, that's extra overhead I don't want.  Fixed.
Comment 29 Shawn Wilsher :sdwilsh 2007-05-17 11:30:48 PDT
Created attachment 265132 [details] [diff] [review]
v1.1 c++ backend; idl changes
Comment 30 Shawn Wilsher :sdwilsh 2007-05-17 11:35:22 PDT
Created attachment 265133 [details] [diff] [review]
v1.1 tests

Updated accordingly based on changes in backend/api
Comment 31 Shawn Wilsher :sdwilsh 2007-05-17 11:40:15 PDT
Created attachment 265134 [details] [diff] [review]
v1.1.1 tests

-N is helpful :/
Comment 32 Shawn Wilsher :sdwilsh 2007-05-17 14:42:30 PDT
Created attachment 265150 [details] [diff] [review]
v1.1 frontend changes

Addresses biesi's comments, as well as fixing at least one other issue that I found.
Comment 33 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-18 10:43:11 PDT
Comment on attachment 265150 [details] [diff] [review]
v1.1 frontend changes

I'll let mconnor review this
Comment 34 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-18 10:59:54 PDT
Comment on attachment 265134 [details] [diff] [review]
v1.1.1 tests

+++ toolkit/components/downloads/test/Makefile.in	17 May 2007 18:39:04 -0000
+# Contributor(s):
+#		Shawn Wilsher <sdwilsh@mozilla.com> (Original Author)
+# Alternatively, the contents of this file may be used under the terms of

I'd put an empty line between your name and the rest of the license. Also, most people don't indent the names this much..

+++ toolkit/components/downloads/test/unit/test_download_manager.js	17 May 2007 18:39:04 -0000
+                          Math.round(Date.now() / 1000), null, persist);

are you sure that you don't mean * 1000?

+function test_addDownload_cancel()
+{
+  var dl = addDownload();
+
+  dm.cancelDownload(dl.id);
+}

Perhaps test that the state is CANCELLED after this?
Comment 35 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-18 11:30:16 PDT
Comment on attachment 265132 [details] [diff] [review]
v1.1 c++ backend; idl changes

in UpdateDB, you are storing src where you should store target.
Comment 36 Mike Connor [:mconnor] 2007-05-21 08:48:30 PDT
Comment on attachment 265150 [details] [diff] [review]
v1.1 frontend changes

>Index: toolkit/mozapps/downloads/content/DownloadProgressListener.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 DownloadProgressListener.js
>--- toolkit/mozapps/downloads/content/DownloadProgressListener.js	6 Feb 2006 00:34:29 -0000	1.11
>+++ toolkit/mozapps/downloads/content/DownloadProgressListener.js	17 May 2007 21:41:04 -0000
>@@ -59,21 +59,29 @@ function DownloadProgressListener (aDocu
> 
> DownloadProgressListener.prototype = 
> {
>   rateChanges: 0,
>   rateChangeLimit: 0,
>   priorRate: 0,
>   lastUpdate: -500,
>   doc: null,
>+  onDownloadStateChange: function(aState, aDownload)
>+  {

what we should do is something like:

onDownloadStateChange: function dlProgressListener_onDownloadStateChange(aState, aDownload)

so JS debugging sucks less, but file a folloup on cleaning up this file if its sticking around, rather than fixing now.

>+var gDownloadManager  = Components.classes[kDlmgrContractID]
>+                                  .getService(nsIDownloadManager);;

extra semicolon

>+function createDownloadItem(aID, aFile, aImage, aTarget, aURI, aState,
>+                            aAnimated, aStatus, aProgress)
>+{
>+  var dl = document.createElement("download");
>+  dl.setAttribute("id", "dl" + aID);
>+  dl.setAttribute("dlid", aID);
>+  dl.setAttribute("image", aImage);
>+  dl.setAttribute("file", aFile);
>+  dl.setAttribute("target", aTarget);
>+  dl.setAttribute("uri", aURI);
>+  dl.setAttribute("state", aState);
>+  dl.setAttribute("aimated", aAnimated);

animated

this seems to have a really huge signature, I guess its just a helper method.  Might make more sense to pass in an array, but probably not worth it for a couple of callers

>+    var dl = document.getElementById("dl" + aDownload.id);

you repeat this piece a lot, I wonder if its worth factoring out into getDownloadId(download) for readability and avoiding copying this pattern all over the place


>+      // Adding to the UI
>+      var uri = Components.classes["@mozilla.org/network/util;1"]
>+                          .getService(Components.interfaces.nsIIOService)
>+                          .newFileURI(dl.targetFile);
>+      var img = "moz-icon://" + uri.spec + "?size=32";
>+      var itm = createDownloadItem(dl.id, dl.targetFile.path, img,
>+                                   dl.displayName, dl.source, dl.state, "",
>+                                   dl.percentComplete);
>+      gDownloadsView.insertBefore(itm, gDownloadsView.firstChild);
>+
>+      // switch view to it
>+      gDownloadsView.selectedIndex = 0;

so, I'm kinda wondering if we want to just not do this, changing the focus around is kinda bad

probably out of scope though, file a followup

>+      // Update UI
>+      for (var i = gDownloadsView.children.length - 1; i >= 0; --i) {
>+        var elm = gDownloadsView.children[i];
>+        var state = elm.getAttribute("state");
>+
>+        if (state != nsIDownloadManager.DOWNLOAD_NOTSTARTED &&
>+            state != nsIDownloadManager.DOWNLOAD_DOWNLOADING &&
>+            state != nsIDownloadManager.DOWNLOAD_PAUSED &&
>+            state != nsIXPInstallManagerUI.INSTALL_DOWNLOADING &&
>+            state != nsIXPInstallManagerUI.INSTALL_INSTALLING)
>+          gDownloadsView.removeChild(gDownloadsView.children[i]);
>+      }

FYI only: This type of thing is generally faster as a switch, though it seems about the same for ints.


>-  <script type="application/x-javascript" src="chrome://mozapps/content/downloads/DownloadProgressListener.js"/>
>-  <script type="application/x-javascript" src="chrome://mozapps/content/downloads/downloads.js"/>
>-  <script type="application/x-javascript" src="chrome://global/content/contentAreaUtils.js"/>
>-  <script type="application/x-javascript" src="chrome://global/content/nsDragAndDrop.js"/>
>-  <script type="application/x-javascript" src="chrome://global/content/globalOverlay.js"/>
>+  <script type="application/javascript" src="chrome://mozapps/content/downloads/DownloadProgressListener.js"/>
>+  <script type="application/javascript" src="chrome://mozapps/content/downloads/downloads.js"/>
>+  <script type="application/javascript" src="chrome://global/content/contentAreaUtils.js"/>
>+  <script type="application/javascript" src="chrome://global/content/nsDragAndDrop.js"/>
>+  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>

Now that's nitpicky.  I like it.

r=me
Comment 37 Nickolay_Ponomarev 2007-05-21 08:56:28 PDT
> >-  <script type="application/x-javascript" src="chrome://global/content/globalOverlay.js"/>
> >+  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> 
> Now that's nitpicky.  I like it.
> 
Is application/javascript preferred over application/x-javascript now? Why? The x-javascript version is the de-facto standard in moz code - see http://lxr.mozilla.org/mozilla/source/content/xul/document/src/nsXULContentSink.cpp#1018
Comment 38 Mike Connor [:mconnor] 2007-05-21 09:02:21 PDT
Comment on attachment 265134 [details] [diff] [review]
v1.1.1 tests


Index: toolkit/components/downloads/test/unit/test_download_manager.js

missing MPL boilerplate + biesi's comments about indenting in the boilerplate

+const nsIDownloadManager = Ci.nsIDownloadManager;
+const dm = Components.classes["@mozilla.org/download-manager;1"]
+                     .getService(nsIDownloadManager);

nit: why not use Cc here?

r=me, good to have some tests here...
Comment 39 Christian :Biesinger (don't email me, ping me on IRC) 2007-05-21 09:33:14 PDT
(In reply to comment #37)
> Is application/javascript preferred over application/x-javascript now? Why? The
> x-javascript version is the de-facto standard in moz code - see

application/javascript has been registered recentlyish, http://www.rfc-editor.org/rfc/rfc4329.txt
Comment 40 Shawn Wilsher :sdwilsh 2007-05-21 11:06:04 PDT
Created attachment 265528 [details] [diff] [review]
v1.2 tests

Addresses comments.
Comment 41 Shawn Wilsher :sdwilsh 2007-05-21 11:16:11 PDT
Created attachment 265530 [details] [diff] [review]
v1.2 frontend

Addresses comments
Comment 42 Shawn Wilsher :sdwilsh 2007-05-21 11:25:57 PDT
Created attachment 265532 [details] [diff] [review]
v1.2 backend; idl

Addresses Comments
Comment 43 Shawn Wilsher :sdwilsh 2007-05-21 17:07:53 PDT
Checking in toolkit/components/downloads/Makefile.in;
new revision: 1.4; previous revision: 1.3
Checking in toolkit/components/downloads/test/Makefile.in;
initial revision: 1.1
Checking in toolkit/components/downloads/test/unit/test_download_manager.js;
initial revision: 1.1
Checking in toolkit/components/downloads/test/unit/test_download_manager_migration.js;
initial revision: 1.1
Checking in toolkit/components/downloads/test/unit/downloads.rdf;
initial revision: 1.1
Checking in uriloader/base/nsIDownload.idl;
new revision: 1.18; previous revision: 1.17
Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
new revision: 1.9; previous revision: 1.8
Checking in toolkit/components/downloads/public/nsIDownloadProgressListener.idl;
new revision: 1.5; previous revision: 1.4
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.23; previous revision: 1.22
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.73; previous revision: 1.72
Checking in toolkit/components/downloads/src/nsDownloadProxy.h;
new revision: 1.18; previous revision: 1.17
Checking in toolkit/components/downloads/src/Makefile.in;
new revision: 1.12; previous revision: 1.11
Checking in toolkit/components/build/Makefile.in;
new revision: 1.48; previous revision: 1.47
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.12; previous revision: 1.11
Checking in toolkit/mozapps/downloads/content/downloads.js;
new revision: 1.56; previous revision: 1.55
Checking in toolkit/mozapps/downloads/content/downloads.xul;
new revision: 1.22; previous revision: 1.21
Checking in embedding/browser/cocoa/src/nsDownloadListener.mm;
new revision: 1.11; previous revision: 1.10
Checking in xpfe/components/download-manager/src/nsDownloadManager.cpp;
new revision: 1.122; previous revision: 1.121
Checking in camino/src/download/nsDownloadListener.mm;
new revision: 1.30; previous revision: 1.29
Comment 44 Stephen Donner [:stephend] 2007-08-13 16:54:58 PDT
The fact that we now have downloads.sqlite, rather than downloads.rdf generated, in addition to my LXR search/Bonsai query, should be good enough to verify that we've switched over.

Verified FIXED.
Comment 45 Eric Shepherd [:sheppy] 2007-08-24 16:27:06 PDT
I'm getting the docs done for this, and have a few questions, figured this is a good place to post them since the people cc'd on this bug probably know way more about this than I do...

Questions:

1. What is a download type?  There's only one value (DOWNLOAD_TYPE_DOWNLOAD), so I'm not clear on the purpose to this.

2. What are DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING for?  If I had to guess, I would assume these are states that extensions can use to block downloads or mark them as "being scanned for viruses," but I have no actual reason to say that other than it being what seems most likely to me.

I may have other questions later, but that'll get me started.  The freshly minted doc for nsIDownloadManager is here:

http://developer.mozilla.org/en/docs/nsIDownloadManager
Comment 46 Shawn Wilsher :sdwilsh 2007-08-24 17:07:10 PDT
(In reply to comment #45)
> 1. What is a download type?  There's only one value (DOWNLOAD_TYPE_DOWNLOAD),
> so I'm not clear on the purpose to this.
I've been strongly considering filing a bug to remove that as it isn't really used.

> 2. What are DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING for?  If I had to guess, I
> would assume these are states that extensions can use to block downloads or
> mark them as "being scanned for viruses," but I have no actual reason to say
> that other than it being what seems most likely to me.
These are really just states of a download.  You cannot actually set any download to this state - it's just possible values for nsIDownload.state
Comment 47 Eric Shepherd [:sheppy] 2007-08-24 17:10:35 PDT
So what do DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING mean then? :)
Comment 48 Shawn Wilsher :sdwilsh 2007-08-24 17:18:47 PDT
Whoops, sorry.

DOWNLOAD_BLOCKED can happen if a download is blocked by parental controls or if a download contains a virus that cannot be cleaned.

DOWNLOAD_SCANNING is just a download that is being scanned for viruses.
Comment 49 Eric Shepherd [:sheppy] 2007-08-24 17:57:48 PDT
Firefox has controls that cause these states to occur?  I wasn't aware of there being a virus scanner in Firefox.
Comment 50 Shawn Wilsher :sdwilsh 2007-08-24 18:05:28 PDT
It just recently landed (Bug 103487)
Comment 51 Robert Kaiser 2009-05-29 04:36:52 PDT
*** Bug 161783 has been marked as a duplicate of this bug. ***
Comment 52 Robert Kaiser 2009-05-29 04:59:56 PDT
*** Bug 461617 has been marked as a duplicate of this bug. ***
Comment 53 neil@parkwaycc.co.uk 2012-01-10 00:37:17 PST
Comment on attachment 265532 [details] [diff] [review]
v1.2 backend; idl

>+  nsCOMArray<nsDownload> mCurrentDownloads;
nsCOMArray is only allowed to have interface pointers, not concrete types...

Note You need to log in before you can comment on or make changes to this bug.