Comment 88 for bug 95507

(From update of attachment 403730)
Thanks, again! Sorry for the delay on this.

>+ // We can not delete the temporary files immediately after the
>+ // drag has finished, because the target application might have not
>+ // copied the temporary file yet. Instead we collect all temporary
>+ // files in mTemporaryFiles array and remove them here in the timer event.

Can you be more specific about the Qt issue please, so that we will know when
it is fixed and this code can be removed? A link to a bug report would be
ideal.

>+ PRUint32 count = mTemporaryFiles.Count();
>+
>+ while (count > 0) {
>+ --count;
>+ mTemporaryFiles[count]->Remove(PR_TRUE);
>+ }

A "for" loop would work well here.

>+ mTempFileCreated = false;

Use PR_FALSE for PRBools.

>+nsresult GetDownloadDetails(nsITransferable * aTransferable, nsIURI **aSourceURI, nsAString &aFilename)

This function can be static and file style is to put the function name on a
new line.

Can you move this to just above CreateTempFile(), where it is used, please?

>+ NS_ENSURE_TRUE(aTransferable, NS_ERROR_FAILURE);

NS_PRECONDITION is better here. It is a debug-only check; it shouldn't be
necessary to check in production code.

>+ NS_ENSURE_TRUE(srcUrlPrimitive, NS_ERROR_FAILURE);

Consensus seems to be to deprecate NS_ENSURE_TRUE, and I agree:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/ccb87d7e7d9e688f

Instead, use something like:

  if (!srcUrlPrimitive)
    return NS_ERROR_FAILURE;

>+ mTimer->Cancel();
>+ mTimer->InitWithCallback(static_cast<nsITimerCallback*>(this),
>+ NS_DND_TIMEOUT,
>+ nsITimer::TYPE_ONE_SHOT);

This looks like a sensible place to cancel the previous timer.

A better place to start the timer would be SourceEndDragSession().

Can you update the comment for NS_DND_TIMEOUT to explain the new use, please?

>+ outputStream->Close();

I think this is the place to check that buffers have been successfully flushed
to disk. (No need to check inputStream->Close().)

>+ char hostname[255];
>+ gethostname(hostname, sizeof(hostname));

Need to ensure that there is a NUL terminator here.
(Just make the last byte NUL.)

>+ nsCAutoString hostnameStr;
>+ hostnameStr.AssignASCII(hostname);
>+
>+ fileURL->SetHost(hostnameStr);

The string copy can be skipped by using:

  fileURL->SetHost(nsDependentCString(hostname))

>+ if (strcmp(mimeFlavor, gTextUriListType) == 0) {
>+ PRUint32 i, count;
>+ mSourceDataItems->Count(&count);
>+
>+ // check whether transferable contains FilePromiseUrl flavor...
>+ PRBool foundFilePromiseFlavor = PR_FALSE;
>+ for (i = 0; i < count; i++) {

kFilePromiseURLMime was only advertised (in GetSourceList) as a uri-list when
numDragItems == 1 and only one temporary file will be created, so only
consider providing the temporary file when numDragItems == 1.

Can this block be merged with the previous "strcmp(mimeFlavor,
gTextUriListType) == 0" block?

>+ NS_ConvertUTF16toUTF8 tempFileUrl(mTempFileUrl);

By making mTempFileUri a nsCString, this conversion can be moved to
CreateTempFile() (and will only need to be done once).

>+ // stores whether a temporary file has been created for the
>+ // current drag session
>+ PRBool mTempFileCreated;
>+
>+ nsString mTempFileUrl;
>+
>+ // stores all temporary files
>+ nsCOMArray<nsIFile> mTemporaryFiles;
>+
>+ // timer to trigger deletion of temporary files
>+ nsCOMPtr<nsITimer> mTimer;

It is normally best to declare member variables together so as to make it
clearer how best to pack them. Unfortunately there are already two groups of
member variables in this class declaration, but try to keep the variables as
close as practical.

Can mTempFileUrl.IsEmpty() be used to make mTempFileCreated unnecessary?
Or maybe keeping mTempFileCreated would make it easier to know when to set the timer.

If keeping mTempFileCreated, the PRBool would pack better with the other PRBool members (and they should really all be PRPackedBool).

Resetting mTempFileUrl or mTempFileCreated is logically necessary in
the same places as mSourceDataItems is initialized. The places you have initialized
mTempFileCreated are sufficient, but it would be clearer if temp file variable
initialization for each drag was moved to where mSourceDataItems is
initialized.

mTimer could have a more specific name such as mTempFileTimer or similar.