Closed Bug 851471 Opened 7 years ago Closed 2 years ago

Decommission nsIDownloadManager

Categories

(Toolkit :: Downloads API, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [tracking])

Attachments

(1 file, 11 obsolete files)

59 bytes, text/x-review-board-request
mak
: review+
Details
When the new JavaScript API for downloads is enabled in its final version and
host applications have started using it in place of nsIDownloadManager,
automated tests for the latter on mozilla-central should be decommissioned.

This can be achieved gradually by first hiding compilation of the entire
"toolkit/components/downloads" module behind a build-time setting, then
removing it from the tree when all consumers have switched over.
Depends on: 875648, 875654, 875731, 847863
Depends on: 885319
Blocks: 888915
Depends on: 895403
Depends on: 899110
Depends on: 906042
Depends on: 907713
No longer blocks: 888915
Depends on: 888915
Depends on: 907732
Depends on: 901360
No longer depends on: 907713
Depends on: 911636
Depends on: 926736
Depends on: 931776
Depends on: 931777
Whiteboard: [triage]
Whiteboard: [triage] → [tracking]
No longer depends on: 906042
No longer blocks: fxdesktopbacklog
Depends on: 1112088
Depends on: 1114607
Depends on: 432425
Depends on: 1114614
Depends on: 1114618
Depends on: 1114624
Blocks: cc-backlog
Blocks: 1122592
Blocks: 986486
No longer blocks: 699860
Depends on: 699860
Depends on: 1152842
Depends on: 1087233
No longer depends on: 907732
Depends on: 1253585
Depends on: 1254558
Depends on: 1281186
Depends on: 1319762
No longer depends on: 888915
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P5
Blocks: 888915
There's one failure for unreferenced file: chrome://mozapps/skin/downloads/downloadIcon.png
it was indeed only referenced at http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/toolkit/components/downloads/nsDownloadManager.cpp#76
I'm waiting for a new tryserver build here, will post an updated patch if this succeeds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b9ca17b9404f237965cc38e656368feb08b6af

I also noticed another failure in the original build, but I'm not sure if it's related to this bug.
Comment on attachment 8864840 [details]
Bug 851471 - Part 6 - All the files in the "downloads" folder are now related to the Safe Browsing component.

https://reviewboard.mozilla.org/r/136524/#review140142

Is there a reasons to not rename toolkit/components/downloads to toolkit/components/appreputation? since it also has a separate BZ component, and doesn't contain anymore any downloads code, I fail to see a reason to keep the current naming.
Alternatively we could move back jsdownloads/ contents into downloads/, but since there's clear separation in bugzilla it sounds like better to keep them splitted. jsdownloads -> downloads is something we could still evaluate, there's no much point into keeping the js prefix from now on (and it already contains non js sources). All of this could even be a follow-up.
Comment on attachment 8864840 [details]
Bug 851471 - Part 6 - All the files in the "downloads" folder are now related to the Safe Browsing component.

https://reviewboard.mozilla.org/r/136524/#review140150
Attachment #8864840 - Flags: review?(mak77) → review+
Comment on attachment 8864838 [details]
Bug 851471 - Part 4 - Update obsolete references in code comments.

https://reviewboard.mozilla.org/r/136532/#review140156
Attachment #8864838 - Flags: review?(mak77) → review+
Blocks: 1363061
Filed bug 1363061 for the folder renames.
Attachment #8864835 - Attachment is obsolete: true
Attachment #8864835 - Flags: review?(mak77)
Attachment #8864836 - Attachment is obsolete: true
Attachment #8864836 - Flags: review?(mak77)
Attachment #8864837 - Attachment is obsolete: true
Attachment #8864837 - Flags: review?(mak77)
Attachment #8864838 - Attachment is obsolete: true
Attachment #8864840 - Attachment is obsolete: true
Comment on attachment 8864837 [details]
Bug 851471 - Part 3 - Remove unused strings.

https://reviewboard.mozilla.org/r/136530/#review140162

It's unrelated to this change, but by looking at usage of this properties file, it looks like DownloadUIHelper.strings is pointlessy expensive. It fetches and converts all the strings in the .properties file, when in reality we only access: downloadsFolder, 3x fileExecutableSecurityWarning..., and the strings in confirmCancelDownloads. Worth a follow-up? Personally I don't see its point, I'd rather expose the bundle, or make it a Proxy and cache the strings on their first use.
Attachment #8864837 - Flags: review+
(In reply to Marco Bonardo [::mak] from comment #15)
> Worth a follow-up? Personally I don't see its point, I'd rather expose the
> bundle, or make it a Proxy and cache the strings on their first use.

Filed bug 1363065 to convert to Proxy.
(In reply to :Paolo Amadini from comment #9)
> I also noticed another failure in the original build, but I'm not sure if
> it's related to this bug.

This doesn't seem to happen anymore on the latest tryserver build. I've updated Part 5 with the image removal.
Blocks: 1362384
Comment on attachment 8865488 [details]
Bug 851471 - Part 1 - Remove the MOZ_JSDOWNLOADS configure option.

https://reviewboard.mozilla.org/r/137134/#review141136
Attachment #8865488 - Flags: review?(mak77) → review+
Comment on attachment 8865489 [details]
Bug 851471 - Part 1 - Remove "Services.downloads".

https://reviewboard.mozilla.org/r/137136/#review141148
Attachment #8865489 - Flags: review?(mak77) → review+
Comment on attachment 8865490 [details]
Bug 851471 - Part 3 - Remove unused strings.

https://reviewboard.mozilla.org/r/137138/#review141150
Attachment #8865490 - Flags: review?(mak77) → review+
Comment on attachment 8865491 [details]
Bug 851471 - Part 4 - Update obsolete references in code comments.

https://reviewboard.mozilla.org/r/137140/#review141154
Attachment #8865491 - Flags: review?(mak77) → review+
Comment on attachment 8865492 [details]
Bug 851471 - Part 3 - All the files in the "downloads" folder are now related to the Safe Browsing component.

https://reviewboard.mozilla.org/r/137142/#review141156
Attachment #8865492 - Flags: review?(mak77) → review+
Comment on attachment 8864839 [details]
Bug 851471 - Part 5 - Remove the obsolete nsIDownloadManager implementation.

https://reviewboard.mozilla.org/r/136534/#review141158

Tehcnically I don't have much to say, the patch is ok.

The problem is that we can't land this before Firefox 57, or before whatever will end up being the legacy add-ons deadline.
I did a quick search on dxr, and even just by looking at 1/3 of the results I hit:
https://addons.mozilla.org/en-US/firefox/addon/downthemall/ (more than 1mil users)
https://addons.mozilla.org/en-US/firefox/addon/all-in-one-sidebar/ (150k users)
https://addons.mozilla.org/en-US/firefox/addon/all-in-one-gestures/ (more than 60k users)
https://addons.mozilla.org/en-US/firefox/addon/1-click-youtube-video-downl/ (600k users)

Now, in normal situations this could even be fine, but requesting these add-ons authors to update the add-ons for a deprecation, when they are likely already in the process of rewriting them for the webextensionpocalypse in a couple versions, would just not work.
For how much it sucks to not remove broken legacy code, I feel like it's just no the right time. But it will be in a couple versions.
Attachment #8864839 - Flags: review?(mak77) → review-
(In reply to Marco Bonardo [::mak] from comment #28)
> I did a quick search on dxr, and even just by looking at 1/3 of the results
> I hit:

They're probably just using the constants, as everything else in the interface already doesn't work.

What about keeping the nsIDownloadManager interface around with just the constants definitions?
Flags: needinfo?(mak77)
(In reply to :Paolo Amadini from comment #29)
> What about keeping the nsIDownloadManager interface around with just the
> constants definitions?

It would work, but I didn't even finish the polling, so this would be more work to do, and I'm not sure it's worth spending a lot of time on it, when we can just do a full removal "shortly".
For example, there may be consumers of Services.downloads, of nsIDownload, of nsIDownloadManagerUI or nsIDownloadProgressListener. I didn't check for those, and I don't feel like I can suggest someone else doing the full polling and building a list of used references, where priorities are elsewhere.
By this I don't mean we should consider every single add-on, but every add-on with more than 100k users is likely a concern, and unfortunately there's no simple dashboard to ask "please give me all the add-ons using these interfaces and having more than N users", so it's a boring manual process.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #30)
> For example, there may be consumers of Services.downloads, of nsIDownload,
> of nsIDownloadManagerUI or nsIDownloadProgressListener.

I see your point, it doesn't need to be an all or nothing approach though - there is code that is clearly unreachable from JavaScript and we can start by removing it, while keeping the interfaces you mentioned above. This still provides the immediate value I'm looking for, that is exactly to accelerate us by not having browser developers spend time on dead code, but should dispel any add-on compatibility concern. I'll file a new bug to do this, and keep this one open for the final step.

I'd like to carry over the removal of the unused strings because, while technically exposed to add-ons, this is no bigger a change than any other string identifier we'll modify for Firefox 55, but I'd be fine to withhold this if you feel strongly about it.

That said, I'm currently discussing a specific WebExtension API, onDeterminingFilename, and in order to support it we may have to make substantial changes to the download interfaces. While I care less about fully removing the legacy interfaces we're discussing here, I won't block WebExtensions progress on legacy add-on compatibility, so the above might be a moot point after all... I'm not sure the WebExtensions team will get to it before the transition release though, in which case we might still see a benefit in delaying the legacy interface removal.

There is always a balance to strike, and I think the compromise above sounds right according to the current general plan. If the plan was different, for example if we planned to branch later this year and keep a legacy add-on compatible train around, then the balance and the add-on compatibility story would definitely be different, and maybe better, but we have to work with the current constraints.
(In reply to :Paolo Amadini from comment #31)
> I see your point, it doesn't need to be an all or nothing approach though

Right, I think the compromise is to keep add-ons with more than 100k users working, until we'll break all the add-ons in 57, because add-on authors don't really have the resources to fix their code twice in a so short timeframe.
All the remaining push-back is just because this is not one of the current priorities and doing an hand-picked removal requires time.
Depends on: 1364050
Attachment #8865488 - Attachment is obsolete: true
Attachment #8865490 - Attachment is obsolete: true
Attachment #8865491 - Attachment is obsolete: true
Attachment #8864839 - Attachment is obsolete: true
Comment on attachment 8866753 [details]
Bug 851471 - Decommission nsIDownloadManager.

MozReview carried over r+ for the parts that stayed in this bug. Nice.
Attachment #8866753 - Flags: review?(mak77) → feedback?(mak77)
Comment on attachment 8866753 [details]
Bug 851471 - Decommission nsIDownloadManager.

no point in keeping this in my queue, we'll look at it when it's the time, btw once we reach 57 it will be fine to remove all crufty code, I assume.
Attachment #8866753 - Flags: feedback?(mak77)
Attachment #8865489 - Attachment is obsolete: true
Attachment #8865492 - Attachment is obsolete: true
Comment on attachment 8866753 [details]
Bug 851471 - Decommission nsIDownloadManager.

https://reviewboard.mozilla.org/r/138374/#review231338

well done!
Attachment #8866753 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/89eeb314f7ba
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2edea7714121
Port bug 851471 to TB/IB/SM: remove downloads.xpt from package manifests. rs=bustage-fix
Blocks: 890702
You need to log in before you can comment on or make changes to this bug.