Decommission nsIDownloadManager

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 3 bugs)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [tracking])

Attachments

(1 attachment, 11 obsolete attachments)

59 bytes, text/x-review-board-request
mak
: review+
Details
Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Depends on: 875648, 875654, 875731, 847863
Assignee

Updated

6 years ago
Depends on: 885319

Updated

6 years ago
Blocks: 888915
Assignee

Updated

6 years ago
Depends on: 895403
Assignee

Updated

6 years ago
Depends on: 899110
Assignee

Updated

6 years ago
Depends on: 906042
Assignee

Updated

6 years ago
Depends on: 907713
Assignee

Updated

6 years ago
No longer blocks: 888915
Depends on: 888915
Assignee

Updated

6 years ago
Depends on: 907732

Updated

6 years ago
Depends on: 901360

Updated

6 years ago
No longer depends on: 907713
Depends on: 911636
Depends on: 926736
Assignee

Updated

6 years ago
Depends on: 931776
Assignee

Updated

6 years ago
Depends on: 931777
Assignee

Updated

5 years ago
Whiteboard: [triage]
Whiteboard: [triage] → [tracking]
No longer depends on: 906042
No longer blocks: fxdesktopbacklog
Assignee

Updated

5 years ago
Depends on: 1112088
Assignee

Updated

5 years ago
Depends on: 1114607
Assignee

Updated

5 years ago
Depends on: 432425
Assignee

Updated

5 years ago
Depends on: 1114614
Assignee

Updated

5 years ago
Depends on: 1114618
Assignee

Updated

5 years ago
Depends on: 1114624

Updated

5 years ago
Blocks: cc-backlog
Assignee

Updated

4 years ago
Blocks: 1122592
Blocks: 986486
No longer blocks: 699860
Depends on: 699860
Assignee

Updated

4 years ago
Depends on: 1152842
Assignee

Updated

4 years ago
Depends on: 1087233
No longer depends on: 907732
Assignee

Updated

3 years ago
Depends on: 1253585
Assignee

Updated

3 years ago
Depends on: 1254558
Depends on: 1281186
Assignee

Updated

3 years ago
Depends on: 1319762
Assignee

Updated

2 years ago
No longer depends on: 888915
Assignee

Updated

2 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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
Assignee

Comment 9

2 years ago
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 10

2 years ago
mozreview-review
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 11

2 years ago
mozreview-review
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 12

2 years ago
mozreview-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+
Assignee

Updated

2 years ago
Blocks: 1363061
Assignee

Comment 13

2 years ago
Filed bug 1363061 for the folder renames.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8864835 - Attachment is obsolete: true
Attachment #8864835 - Flags: review?(mak77)
Assignee

Updated

2 years ago
Attachment #8864836 - Attachment is obsolete: true
Attachment #8864836 - Flags: review?(mak77)
Assignee

Updated

2 years ago
Attachment #8864837 - Attachment is obsolete: true
Attachment #8864837 - Flags: review?(mak77)
Assignee

Updated

2 years ago
Attachment #8864838 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8864840 - Attachment is obsolete: true

Comment 15

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
(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.
Assignee

Comment 22

2 years ago
(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.
Assignee

Updated

2 years ago
Blocks: 1362384

Comment 23

2 years ago
mozreview-review
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 24

2 years ago
mozreview-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 25

2 years ago
mozreview-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 26

2 years ago
mozreview-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 27

2 years ago
mozreview-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 28

2 years ago
mozreview-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-
Assignee

Comment 29

2 years ago
(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?
Assignee

Updated

2 years ago
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)
Assignee

Comment 31

2 years ago
(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.
Assignee

Updated

2 years ago
Depends on: 1364050
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8865488 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8865490 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8865491 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Attachment #8864839 - Attachment is obsolete: true
Assignee

Comment 36

2 years ago
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)
Assignee

Updated

2 years ago
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8865489 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8865492 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 42

a year ago
mozreview-review
Comment on attachment 8866753 [details]
Bug 851471 - Decommission nsIDownloadManager.

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

well done!
Attachment #8866753 - Flags: review?(mak77) → review+

Comment 44

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89eeb314f7ba
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 45

a year ago
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.