228388 - nsDownloadManager cleanup

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

avoid some QIs; don't use Exists on a hashtable when we will call Get anyway (if
the entry exists)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[[e-mail]]

Comment on attachment 137367 [details] [diff] [review]
patch

>+  *aDownloadItem = NS_STATIC_CAST(nsIDownload*, mCurrDownloads.Get(&key));
Should be nsDownload*

>-  nsDownload* internalDownload = NS_STATIC_CAST(nsDownload*, mCurrDownloads.Get(&key));
>+  nsDownload* download = NS_STATIC_CAST(nsDownload*, mCurrDownloads.Get(&key));
Why the rename? You didn't rename it above.

>-nsDownloadManager::OpenProgressDialogFor(const nsACString & aTargetPath, nsIDOMWindow* aParent, PRBool aCancelDownloadOnClose)
>+nsDownloadManager::OpenProgressDialogFor(nsIDownload* aDownload, nsIDOMWindow* aParent, PRBool aCancelDownloadOnClose)
This change makes the patch incomplete.

>+  nsDownload* internalDownload = NS_STATIC_CAST(nsDownload*, aDownload);
Null check please.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

new version of the patch requires nsInterfaceHashtable::GetWeak, marking dependency

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 138079 [details] [diff] [review]
patch v2

oops, the point of nsInterfaceHashtable was to avoid manual refcounting. I
forgot to remove the manual refcounting in this patch though...

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v3

let the hashtable do the refcounting

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 228825 has been marked as a duplicate of this bug. ***

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 138083 [details] [diff] [review]
patch v3

I'll make this use the new nsReferenceHashtable

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v4

Comment 10

[[e-mail]]

Comment on attachment 140308 [details] [diff] [review]
patch v4

>+#include "nsAutoPtr.h"
The code compiles without this...

> /* Outstanding issues/todo:
>  * 1. Implement pause/resume.
>  */
Is this still true?

>+  mCurrDownloads.Remove(utf8Path);
I don't think this is necessary, in fact I think it hurts (but double-check
with bsmedberg or alecf just to be sure).
>+  mCurrDownloads.Put(utf8Path, internalDownload);

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #10)
> (From update of attachment 140308 [details] [diff] [review])
> >+#include "nsAutoPtr.h"
> The code compiles without this...

Ah yeah... it gets nsRefPtr via nsRefPtrHashtable.h. I'll remove this line

> > /* Outstanding issues/todo:
> >  * 1. Implement pause/resume.
> >  */
> Is this still true?

it is...

> >+  mCurrDownloads.Remove(utf8Path);
> I don't think this is necessary, in fact I think it hurts (but double-check
> with bsmedberg or alecf just to be sure).

yes, bsmedberg confirmed this is not necessary. I'll remove this line.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: v4 with neil's comments

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 140311 [details] [diff] [review]
v4 with neil's comments

sr=bzbarsky

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in xpfe/components/download-manager/src/nsDownloadManager.cpp;
/cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadManager.cpp,v 
<--  nsDownloadManager.cpp
new revision: 1.90; previous revision: 1.89
done
Checking in xpfe/components/download-manager/src/nsDownloadManager.h;
/cvsroot/mozilla/xpfe/components/download-manager/src/nsDownloadManager.h,v  <--
 nsDownloadManager.h
new revision: 1.30; previous revision: 1.29
done