Closed
Bug 851471
Opened 12 years ago
Closed 7 years ago
Decommission nsIDownloadManager
Categories
(Toolkit :: Downloads API, defect, P1)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: Paolo, Assigned: Paolo)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [tracking])
Attachments
(1 file, 11 obsolete files)
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•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Blocks: fxdesktopbacklog
Whiteboard: [triage]
Updated•11 years ago
|
Whiteboard: [triage] → [tracking]
Updated•11 years ago
|
No longer blocks: fxdesktopbacklog
Updated•10 years ago
|
Blocks: cc-backlog
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•8 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) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
Comment 8•8 years ago
|
||
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•8 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•8 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•8 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•8 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 | ||
Comment 13•8 years ago
|
||
Filed bug 1363061 for the folder renames.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8864835 -
Attachment is obsolete: true
Attachment #8864835 -
Flags: review?(mak77)
Assignee | ||
Updated•8 years ago
|
Attachment #8864836 -
Attachment is obsolete: true
Attachment #8864836 -
Flags: review?(mak77)
Assignee | ||
Updated•8 years ago
|
Attachment #8864837 -
Attachment is obsolete: true
Attachment #8864837 -
Flags: review?(mak77)
Assignee | ||
Updated•8 years ago
|
Attachment #8864838 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8864840 -
Attachment is obsolete: true
Comment 15•8 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) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 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•8 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.
Comment 23•8 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•8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Flags: needinfo?(mak77)
Comment 30•8 years ago
|
||
(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•8 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.
Comment 32•8 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8865488 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8865490 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8865491 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8864839 -
Attachment is obsolete: true
Assignee | ||
Comment 36•8 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 37•8 years ago
|
||
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•7 years ago
|
Blocks: post-57-api-changes
Assignee | ||
Comment 38•7 years ago
|
||
Priority: P5 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8865489 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8865492 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
Comment 42•7 years 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 43•7 years ago
|
||
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89eeb314f7ba
Decommission nsIDownloadManager. r=mak
Comment 44•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 45•7 years 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
Updated•7 years ago
|
status-firefox60:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•