218817 - Split nsIHelperAppLauncher::GetDownloadInfo

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

120   nsIFile getDownloadInfo(out nsIURI aSourceUrl, out long long
aTimeDownloadStarted);

this is a really ugly function, three things were mixed in here that don't
really belong together.
There is not one caller that requires all three pieces of information - see
http://lxr.mozilla.org/seamonkey/search?string=getdownloadinfo, all callers pass
some "dummy" or "garbage" variables into this function.

Also, |aSourceUrl| is alraedy available on the interface: 
 94   // the source url
 95   readonly attribute nsIURI source;

So this function should be split into two new attributes:
readonly attribute long long timeDownloadStarted;
readonly attribute nsIFile targetFile;

Comment 1

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

looking at this more closely, I realized that _nobody_ uses the timestamp:
http://lxr.mozilla.org/mozilla/search?string=getdownloadinfo

Comment 2

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

Attachment: patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

>Index: embedding/components/ui/helperAppDlg/nsHelperAppDlg.js

>             // Need to get temporary file and check for executable-ness.
>             var ignore1 = new Object;
>             var ignore2 = new Object;
>-            var tmpFile = this.mLauncher.getDownloadInfo( ignore1, ignore2 );
>+            var tmpFile = this.mLauncher.targetFile;

We don't need ignore1 and ignore2 anymore.

>Index: uriloader/exthandler/nsIExternalHelperAppService.idl

I would leave a way to get the download start time on this interface for now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

r=me with those changes

Comment 5

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

Attachment: patch v2

Comment 6

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

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

changes made.

Comment 7

[Darin Fisher]

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

>   if (mFinalFileDestination)
>   {
>     *aTarget = mFinalFileDestination;
>   }
>   else
>     *aTarget = mTempFile;

nit: fixup brackets to be consistent.  either always or never
use brackets when not needed.


>Index: uriloader/exthandler/nsIExternalHelperAppService.idl

>+  /** The file we are saving to */
>+  readonly attribute nsIFile targetFile;
>+  /** Time when the download started */
>+  readonly attribute PRTime timeDownloadStarted;

nit: would be nice to clean up all the comments in this file.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 9

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

Checked in; I removed the braces on that if, and changed all comments in that
file to use the 
/**
 * description
 */
syntax