830271 - nsIDownloadManagerUI uses obsolete download id

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[[e-mail]]

nsIDownloadManagerUI's show() method takes an (optional) id, however ids are deprecated and make no sense for private downloads in per-window PB mode.

My preference would be for it to be changed to take an nsIDownload instead.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Make download UI notification take an nsIDownload.

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.

Marco, how do you feel about this?

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.

Review of attachment 701743 [details] [diff] [review]:
-----------------------------------------------------------------

This needs a superreview for the API change (Mossop maybe?), I went back in history to check the original implementation, but I couldn't find any valid reason to pass an id instead of the download.
r=me with the following comments addressed.

::: mobile/android/components/DownloadManagerUI.js
@@ +21,5 @@
>        aReason = Ci.nsIDownloadManagerUI.REASON_USER_INTERACTED;
>  
>      let browser = Services.wm.getMostRecentWindow("navigator:browser");
>      if (browser)
> +      browser.showDownloadManager(aWindowContext, aDownload, aReason);

you should also fix showDownloadManager
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#2746

::: toolkit/components/downloads/nsDownloadProxy.h
@@ +69,5 @@
>  
>        if (visible && !focusWhenStarting)
>          return NS_OK;
>  
> +      return dmui->Show(nullptr, mInner, nsIDownloadManagerUI::REASON_NEW_DOWNLOAD, aIsPrivate);

looks like these are no more useful and should be removed:
55       uint32_t id;
56       mInner->GetId(&id);

Comment 4

[[e-mail]]

Comment on attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.

JFTR this is f=me, it's just what I need, thanks!

Comment 5

[[e-mail]]

Comment on attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.

It's ages since I set one of these, I wonder whether it will stick ;-)

Comment 6

[[e-mail]]

(In reply to Marco Bonardo from comment #3)
> you should also fix showDownloadManager
> http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#2746
Isn't mobile/xul dead yet?

Comment 7

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #6)
> (In reply to Marco Bonardo from comment #3)
> > you should also fix showDownloadManager
> > http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/browser.js#2746
> Isn't mobile/xul dead yet?

it is, but since nobody removed the code we keep updating it...

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.

Sorry, Neil, but it feels subversive to let the person who originally proposed the interface change declare it to be a good one :)

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.

Review of attachment 701743 [details] [diff] [review]:
-----------------------------------------------------------------

Tentatively yes, the API looks saner this way, my only concern is add-on bustage and making it easy for add-ons to support both old and new forms for a time. I'm seeing just 20 or so add-ons using this function on AMO (no idea how many of those are even compatible with current Firefox) so hopefully this isn't much of a problem, but jorge can tell us otherwise. Presumably we could support both old and new APIs if needs be, either with two methods (one deprecated) or by making that parameter an nsIVariant or something so you could pass either.

I do note that both flashgot and r2d2b2g seem to use it but those should be easy to get updated.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

It appears to have low impact, so I don't have any objections to it.

Comment 11

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f57f7c12b6dc

How much uplift is desirable here?

Comment 12

[Marco Bonardo [:mak]]

(In reply to Josh Matthews [:jdm] from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f57f7c12b6dc
> 
> How much uplift is desirable here?

API change, likely none for Firefox (We don't use this param yet in the new manager), not sure if Seamonkey needs it.

Comment 13

[[e-mail]]

It only affects per-window-pb builds, which aren't the default yet.

Comment 14

[Josh Matthews [:jdm]]

Well, those are on aurora right now.

Comment 15

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f57f7c12b6dc