Closed Bug 380250 Opened 17 years ago Closed 17 years ago

Convert Download Manager's RDF backend to mozStorage

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9alpha4

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(4 files, 14 obsolete files)

5.05 KB, patch
Details | Diff | Splinter Review
24.73 KB, patch
Details | Diff | Splinter Review
29.92 KB, patch
Details | Diff | Splinter Review
134.67 KB, patch
Details | Diff | Splinter Review
Attached patch v0.1 (obsolete) — Splinter Review
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 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?
Attached patch v0.2 (obsolete) — Splinter Review
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.
Attachment #264342 - Attachment is obsolete: true
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.
Attached patch v0.3 (obsolete) — Splinter Review
More work - fuller patch.  Mostly posting to get help with my unit tests.
Attachment #264410 - Attachment is obsolete: true
Attached patch v0.3.1 (obsolete) — Splinter Review
Attachment #264516 - Attachment is obsolete: true
Attached patch v0.3.2 (obsolete) — Splinter Review
Attachment #264517 - Attachment is obsolete: true
Blocks: 380441
Blocks: 377793
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?
Whoops.  mxr doesn't work so well across languages :)  Thanks for pointing that out!
Attached patch 0.4 (obsolete) — Splinter Review
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.
Attachment #264522 - Attachment is obsolete: true
Blocks: 161783
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?  
(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.
Attached patch 0.5 (obsolete) — Splinter Review
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.
Attachment #264679 - Attachment is obsolete: true
Attachment #264842 - Flags: review?(mconnor)
Attachment #264842 - Flags: review?(cbiesinger)
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.
(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.
Status: NEW → ASSIGNED
Attachment #264842 - Attachment is obsolete: true
Attachment #264842 - Flags: review?(mconnor)
Attachment #264842 - Flags: review?(cbiesinger)
Attached patch v1.0 c++ backend; idl changes (obsolete) — Splinter Review
Attachment #264976 - Flags: review?(mconnor)
Attached patch v1.0 frontend changes (obsolete) — Splinter Review
Attachment #264977 - Flags: review?(mconnor)
Attachment #264977 - Flags: review?(cbiesinger)
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.
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 ;)
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.
Attached patch v1.0 tests (obsolete) — Splinter Review
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.
Attachment #265058 - Flags: review?(mconnor)
>> +  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...

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 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]
(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).
Summary: Convert rdf backend to mozStorage → Convert Download Manager's RDF backend to mozStorage
(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.
Attached patch v1.1 c++ backend; idl changes (obsolete) — Splinter Review
Attachment #264976 - Attachment is obsolete: true
Attachment #265132 - Flags: review?(cbiesinger)
Attachment #264976 - Flags: review?(mconnor)
Attached patch v1.1 tests (obsolete) — Splinter Review
Updated accordingly based on changes in backend/api
Attachment #265058 - Attachment is obsolete: true
Attachment #265058 - Flags: review?(mconnor)
Attached patch v1.1.1 tests (obsolete) — Splinter Review
-N is helpful :/
Attachment #265133 - Attachment is obsolete: true
Attachment #265134 - Flags: review?(mconnor)
Attached patch v1.1 frontend changes (obsolete) — Splinter Review
Addresses biesi's comments, as well as fixing at least one other issue that I found.
Attachment #264977 - Attachment is obsolete: true
Attachment #265150 - Flags: review?(mconnor)
Attachment #265150 - Flags: review?(cbiesinger)
Attachment #264977 - Flags: review?(mconnor)
Attachment #264977 - Flags: review?(cbiesinger)
Blocks: 381157
Blocks: 381159
No longer blocks: 381159
Comment on attachment 265150 [details] [diff] [review]
v1.1 frontend changes

I'll let mconnor review this
Attachment #265150 - Flags: review?(cbiesinger)
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?
Attachment #265134 - Flags: review+
Comment on attachment 265132 [details] [diff] [review]
v1.1 c++ backend; idl changes

in UpdateDB, you are storing src where you should store target.
Attachment #265132 - Flags: review?(cbiesinger) → review+
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
Attachment #265150 - Flags: review?(mconnor) → review+
> >-  <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 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...
Attachment #265134 - Flags: review?(mconnor) → review+
(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
Attached patch v1.2 testsSplinter Review
Addresses comments.
Attachment #265134 - Attachment is obsolete: true
Attached patch v1.2 frontendSplinter Review
Addresses comments
Attachment #265150 - Attachment is obsolete: true
Addresses Comments
Attachment #265132 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Depends on: 381538
Depends on: 381542
Depends on: 381546
Depends on: 381601
Blocks: 381603
Depends on: 382081
Depends on: 381801
Depends on: 381803
Target Milestone: --- → Firefox 3 alpha4
Blocks: 227401
Blocks: 240046
Blocks: 261973
Blocks: 345117
Blocks: 345932
Blocks: 364943
Blocks: 365614
Blocks: 240525
No longer blocks: 161783
Blocks: 374355
Blocks: 280442
Blocks: 358914
Depends on: 390748
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.
Status: RESOLVED → VERIFIED
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
(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
So what do DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING mean then? :)
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.
Firefox has controls that cause these states to occur?  I wasn't aware of there being a virus scanner in Firefox.
It just recently landed (Bug 103487)
Product: Firefox → Toolkit
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...
You need to log in before you can comment on or make changes to this bug.