241082 - exthandler should require just nsITransfer, not nsIDownload

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

darin introduced nsITransfer in the ftp upload patch, and the methods on it
suffice for exthandler (which, really, only requires the init method).

note that this is the only user of nsIDownload in "gecko". (the other one is in
xpfe/ - contentAreaUtils (for which  nsITransfer suffices), and download-manager)

and of course there's similar code in the embedding apps and firefox, which
could likely also use nsITransfer.

hence, I suggest moving nsIDownload into download-manager.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sounds like a plan to me.

Comment 2

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

Also, nsITransfer should inherit from nsIWebProgressListener.

It probably only needs methods/attributes init, listener, observer - the rest
can stay on nsIDownload.

it should get its own idl file.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

note to myself

<biesi> make sure that nsExternalHelperAppService.cpp does not use nsIDownload,
only nsITransfer

Comment 4

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

>the rest can stay on nsIDownload.

or really, "move to nsIDownload"

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: first patch

first patch.
It works properly as far as I can tell.

Beyond the matter of this bug the patch also makes displayName readonly, and
changes nsIProgressDialog to inherit from nsIDownload (nsITransfer was not
enough).

I'll make separate bug for nsIDownload move from ./uriloader/base into
download-manager.

Comment 6

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

Comment on attachment 169341 [details] [diff] [review]
first patch

uriloader/base/nsITransfer.idl
+interface nsILocalFile;

this forward decl seems unnecessary now...

+ * INTERFACES THAT NEED TO BE IMPLEMENTED:
+ *   nsITransfer
+ *   nsIDownload

actually nsIDownload no longer needs to be implemented

mailnews/base/src/nsMessenger.cpp
+	   mWebProgressListener = do_QueryInterface(tr);

since nsITransfer now inherits from nsIWPL, it seems like you could just do:
  mWebProgressListener = tr;

right?

same in uriloader/exthandler/nsExternalHelperAppService.cpp with:
+    listener = do_QueryInterface(tr);

but maybe it would be better to make mWebProgressListener a
comptr<nsITransfer>? that would save a few QIs.


that nsIProgressDialog inherits from nsIDownload is kinda suboptimal, since
it's also used for uploads, but I don't think that's much of a problem.

thanks a lot for making this patch!

r- for now, but with the above changes this looks good

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #6)
> that nsIProgressDialog inherits from nsIDownload is kinda suboptimal, since
> it's also used for uploads, but I don't think that's much of a problem.

Do you have any ideas about better inheritance here?

Comment 8

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

not offhand, no...

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: second patch

fixes issues from Comment 6 beside mWebProgressListener in
nsExternalHelperAppService.

Comment 10

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

Comment on attachment 169673 [details] [diff] [review]
second patch

uriloader/base/Makefile.in indentation is wrong (but you know this already)

uriloader/base/nsIDownload.idl
+  /**
      * The source of the transfer.

indentation of /** is wrong

toolkit/components/downloads/src/nsDownloadManager.cpp
xpfe needs no changes?

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: third patch

deals with cocoa, camino, powerplant and xpfe.

Biesi: I can make the const PRUnichar* -> const nsAString* as another bug, ok?

Comment 12

[Darin Fisher]

Comment on attachment 169943 [details] [diff] [review]
third patch

>Index: uriloader/base/nsITransfer.idl

>+ * started, and Init will be called on it and an observer will be set.

s/Init/nsITransfer::init/


>+ * INTERFACES THAT NEED TO BE IMPLEMENTED:

s/NEED TO/MUST/


>+#define NS_DOWNLOAD_CONTRACTID "@mozilla.org/download;1"

So, there may be only one class (nsIFactory) registered under this
ContractID?  Should we think about supporting multiple "global"
download observers?  (e.g., maybe use a category instead?)

Also, why is the word "download" being used here instead of "transfer"?

Should the registered class also implement nsIDownload?


>Index: uriloader/base/nsIDownload.idl

>+    /**
>+     * The amount of kbytes downloaded so far.
>+     */
>+    readonly attribute PRUint64 amountTransferred;

amountTransferred still feels like something that could be on
nsITransfer, but ok.  I suppose these are just informational
fields that the uriloader never really needs to access?  do
you intend to eventually move nsIDownload out of the uriloader
module?


>Index: browser/base/content/contentAreaUtils.js

>+  var tr = Components.classes["@mozilla.org/download;1"].createInstance(Components.interfaces.nsITransfer);

funny that this ContractID mentions "download" when it should maybe
be "transfer" instead.	this would be our opportunity to change the
ContractID since we are changing the interfaces.

or maybe the ContractID is fine if we are saying that these transfers
are only downloads.


>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+  nsCOMPtr<nsITransfer> tr(do_QueryInterface(mWebProgressListener));

Since nsITransfer inherits from nsIWebProgressListener, perhaps it 
would make sense to change mWebProgressListener to mTransfer to 
avoid the QI here (and perhaps in other places).


>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

>+  internalDownload->SetDisplayName(displayName.get());

>+nsDownload::SetDisplayName(const PRUnichar* aDisplayName)

can this be changed to take a |const nsAString&| instead?


sr=darin (nothing major to prevent this from landing, but I'd
appreciate it if you could respond to my comments here by either
rev'ing the patch or filing a followup bug.)

Comment 13

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

(In reply to comment #12)
> >+#define NS_DOWNLOAD_CONTRACTID "@mozilla.org/download;1"
> 
> So, there may be only one class (nsIFactory) registered under this
> ContractID?

well, that's what our current architecture is... I wouldn't mind extending this
(I think this could even be used to provide a hook for external dl managers)

> Should the registered class also implement nsIDownload?

I want to move nsIDownload into xpfe/toolkit, and freeze nsITransfer. that'd
make nsITransfer an embedding interface, and nsIDownload nothing an embeddor
should know about.

> amountTransferred still feels like something that could be on
> nsITransfer, but ok.  I suppose these are just informational
> fields that the uriloader never really needs to access?  do
> you intend to eventually move nsIDownload out of the uriloader
> module?

and per the above, I don't really want to make embeddors implement something
that nobody will ever ask for.

that said. an additional variable may not hurt, so if you want it there, it
could be put there.

> funny that this ContractID mentions "download" when it should maybe
> be "transfer" instead.	this would be our opportunity to change the
> ContractID since we are changing the interfaces.
> 
> or maybe the ContractID is fine if we are saying that these transfers
> are only downloads.

that's what I was thinking (that it's only for downloads); we can change it if
you want though.

> >Index: uriloader/exthandler/nsExternalHelperAppService.cpp
> Since nsITransfer inherits from nsIWebProgressListener, perhaps it 
> would make sense to change mWebProgressListener to mTransfer to 
> avoid the QI here (and perhaps in other places).

That's what I thought too, but it doesn't work. The helper app dialog can be the
listener here, via nsIHelperAppLauncher::setWebProgressListener; and it does not
implement nsITransfer (unless we want to require that...)

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #12)
> >+nsDownload::SetDisplayName(const PRUnichar* aDisplayName)
> 
> can this be changed to take a |const nsAString&| instead?

Yes. But I really prefer to do this in separate patch because there are probably
more places where we can make this switch. But if you require this switch here,
I will make it.

Comment 15

[Darin Fisher]

> Yes. But I really prefer to do this in separate patch because there are probably
> more places where we can make this switch. But if you require this switch here,
> I will make it.

It's up to you, but I figured that since you were changing all of the
SetDisplayName functions that it might make sense to include another change.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: final patch

fixes two comment issues.

Comment 17

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

Attachment: additional patch

this patch is required in addition to the above one to fix xpfe build bustage

Comment 18

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

I checked in this patch. toolkit's nsDownloadProxy needed similar changes as xpfe's.

this was even a codesize win:
Zdiff:-764
mZdiff:-40
(from luna, SeaMonkey)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Another additional patch

This is needed to fix the camino bustage (with the change in this bug, the
class in question ended up with ambiguous inheritance from
nsIWebProgressListener).

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

ugh. I feel very ashamed. Sorry guys for giving you additional work because of
my lack of knowledge

Comment 21

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

>Should we think about supporting multiple "global"
>download observers?  (e.g., maybe use a category instead?)

this is now Bug 277129

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

and we have also bug 277130 for nsIDownload move and bug 277140 for  const
PRUnichar* -> const nsAString*