Last Comment Bug 830271 - nsIDownloadManagerUI uses obsolete download id
: nsIDownloadManagerUI uses obsolete download id
Status: RESOLVED FIXED
: addon-compat
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on:
Blocks: 833015
  Show dependency treegraph
 
Reported: 2013-01-14 01:43 PST by neil@parkwaycc.co.uk
Modified: 2013-04-18 02:08 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make download UI notification take an nsIDownload. (10.55 KB, patch)
2013-01-14 03:01 PST, Josh Matthews [:jdm]
mak77: review+
dtownsend: superreview+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2013-01-14 01:43:15 PST
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] 2013-01-14 03:01:37 PST
Created attachment 701743 [details] [diff] [review]
Make download UI notification take an nsIDownload.
Comment 2 Josh Matthews [:jdm] 2013-01-14 04:32:32 PST
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] (Away 6-20 Aug) 2013-01-14 15:45:04 PST
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 neil@parkwaycc.co.uk 2013-01-14 16:17:18 PST
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 neil@parkwaycc.co.uk 2013-01-14 16:18:41 PST
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 neil@parkwaycc.co.uk 2013-01-16 04:54:07 PST
(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] (Away 6-20 Aug) 2013-01-16 04:56:20 PST
(In reply to neil@parkwaycc.co.uk 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] 2013-01-16 05:12:28 PST
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] 2013-01-16 11:16:09 PST
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] 2013-01-16 12:02:48 PST
It appears to have low impact, so I don't have any objections to it.
Comment 11 Josh Matthews [:jdm] 2013-01-21 08:29:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57f7c12b6dc

How much uplift is desirable here?
Comment 12 Marco Bonardo [::mak] (Away 6-20 Aug) 2013-01-21 08:31:20 PST
(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 neil@parkwaycc.co.uk 2013-01-21 08:49:29 PST
It only affects per-window-pb builds, which aren't the default yet.
Comment 14 Josh Matthews [:jdm] 2013-01-21 08:53:25 PST
Well, those are on aurora right now.

Note You need to log in before you can comment on or make changes to this bug.