244448 - Exthandler changes for download resuming

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, enhancement)

Description

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

In order to implement UI for download resuming, two things need to happen:
o) Partially downloaded files must not be deleted.
   (This happens already for "save target as")
o) The listener must know about the filename of the temporary file, so that it 
   knows where to save the resumed file.

Comment 1

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

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

>Index: nsExternalHelperAppService.cpp

>+  // Tell the listener about the location of the target file
>+  nsCOMPtr<nsIObserver> obs(aDownload);

Missing do_QI here?

>+  if (obs)
>+    obs->Observe(mTempFile, "temp-file", nsnull);

Did you just make up the "temp-file" topic?  It may be worth documenting it
somewhere, especially if we expect embeddors to create their own nsIDownload
implementations (which we do, since nsIDownload is in uriloader and the
implementations are in xpfe, toolkit, and embedding/browser (not sure what this
last one is). 

More precisely, it should be documented somewhere what the
"@mozilla.org/download;1" contract requires (implementing nsIDownload) and what
it can do to get more info (implementing nsIObserver and listening for the
"temp-file" notification)...

I'm not sure we have a place for such documentation (we really need to get our
SDK story together), so maybe nsIDownload.idl is a decent place to stick it for
now.... :(

>+  if (mTempFile && (action != nsIMIMEInfo::saveToDisk))

No need to over-parenthesize.  != binds more strongly than &&, and people
really should know that.

r=bzbarsky with that.

Comment 3

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

(In reply to comment #2)
> >+  // Tell the listener about the location of the target file
> >+  nsCOMPtr<nsIObserver> obs(aDownload);
> 
> Missing do_QI here?

Yeah. I thought I had put it here.

Why did it compile this way?

> Did you just make up the "temp-file" topic?

yep.

>  It may be worth documenting it
> somewhere, especially if we expect embeddors to create their own nsIDownload
> implementations (which we do, since nsIDownload is in uriloader and the
> implementations are in xpfe, toolkit, and embedding/browser (not sure what this
> last one is). 

I weren't sure of a good place for it...

> More precisely, it should be documented somewhere what the
> "@mozilla.org/download;1" contract requires (implementing nsIDownload) and what
> it can do to get more info (implementing nsIObserver and listening for the
> "temp-file" notification)...

I guess
http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsIDownload.idl#144 could
use a comment about this.

> No need to over-parenthesize.  != binds more strongly than &&, and people
> really should know that.

ok.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Why did it compile this way?

I have no clue, and it worries me that it did....  nsCOMPtr doesn't seem to have
any constructors that would fit this scenario.

Yeah, just sticking it in the IDL there seems to be OK for now, followed by a
post to n.p.m.documentation asking about how we can document such things in general.

Comment 5

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

(In reply to comment #4)
> I have no clue, and it worries me that it did....  nsCOMPtr doesn't seem to have
> any constructors that would fit this scenario.

Hmm. any nsISupports can be implicitly converted to an nsQueryInterface.
nsCOMPtr has a ctor that takes an nsQueryInterface. Maybe this is what happened.

> Yeah, just sticking it in the IDL there seems to be OK for now, followed by a
> post to n.p.m.documentation asking about how we can document such things in
general.

ok, posting sent.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Maybe this is what happened.

Hmm.. if so, that's kinda cool (since it'd actually do the right thing!)

Comment 7

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

Attachment: patch v2

requested changes made

Comment 8

[Boris Zbarsky [:bzbarsky]]

Nice!  That is exactly the sort of documentation we need all over!

Comment 9

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

Checking in base/nsIDownload.idl;
/cvsroot/mozilla/uriloader/base/nsIDownload.idl,v  <--  nsIDownload.idl
new revision: 1.9; previous revision: 1.8
done
Checking in exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <-- 
nsExternalHelperAppService.cpp
new revision: 1.261; previous revision: 1.260
done

Comment 10

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

this caused bug 249677