Closed Bug 851471 Opened 9 years ago Closed 4 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P5
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+
Filed bug 1363061 for the folder renames.
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.
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?
(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.
(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 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)
Priority: P5 → P1
Comment on attachment 8866753 [details] Bug 851471 - Decommission nsIDownloadManager. https://reviewboard.mozilla.org/r/138374/#review231338 well done!
Attachment #8866753 - Flags: review?(mak77) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/89eeb314f7ba Decommission nsIDownloadManager. r=mak
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.