Closed Bug 1364050 Opened 8 years ago Closed 8 years ago

Remove obsolete code from the nsIDownloadManager implementation

Categories

(Toolkit :: Downloads API, enhancement)

Product:
Toolkit
Component:
Downloads API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(5 files)

Bug 1364050 - Part 1 - Remove the MOZ_JSDOWNLOADS configure option.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1364050 - Part 2 - Remove unused strings.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1364050 - Part 3 - Remove obsolete code from the nsIDownloadManager implementation.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1364050 - Part 4 - Remove references to "downloads.rdf".
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1364050 - Part 5 - Update obsolete references in code comments.
59 bytes, text/x-review-board-request
mak
: review+
Details
This is part of the work required to decommission nsIDownloadManager in bug 851471.
Comment on attachment 8866746 [details] Bug 1364050 - Part 3 - Remove obsolete code from the nsIDownloadManager implementation. Note that for nsDownloadManager.cpp it may be easier to just look at the new revision in Mercurial: https://reviewboard-hg.mozilla.org/gecko/file/cf13267a5997/toolkit/components/downloads/nsDownloadManager.cpp
Also note that nsDownloadProxy is unreachable since bug 1114624 landed, and the related code can be completely removed.
Comment on attachment 8866746 [details] Bug 1364050 - Part 3 - Remove obsolete code from the nsIDownloadManager implementation. https://reviewboard.mozilla.org/r/138352/#review141978 Did you check that a build with these changes can properly use https://addons.mozilla.org/en-US/firefox/addon/downthemall/, https://addons.mozilla.org/en-US/firefox/addon/all-in-one-sidebar/ and https://addons.mozilla.org/en-US/firefox/addon/1-click-youtube-video-downl/ (at least their main functionalities and no major bbrowser breakage) ::: toolkit/components/downloads/nsDownloadManager.cpp:414 (Diff revision 1) > - rv = GetTargetFile(getter_AddRefs(targetLocalFile)); > - NS_ENSURE_SUCCESS(rv, rv); > - } > > - // Get the file size to be used as an offset, but if anything goes wrong > - // along the way, we'll silently restart at 0. > +NS_IMETHODIMP > +nsDownloadManager::OnBeginUpdateBatch() Is there a reason to keep nsINavHistoryObserver and nsIObserver implementations around, since they don't really do anything? Did you find direct use of these in add-ons? You removed all the addObserver calls so I'm not sure why these are still here
(In reply to Marco Bonardo [::mak] from comment #9) > Did you check that a build with these changes can properly use I'll check! > Is there a reason to keep nsINavHistoryObserver and nsIObserver > implementations around, since they don't really do anything? Did you find > direct use of these in add-ons? In theory this is to make sure that Services.downloads.QueryInterface(Ci.nsIObserver) won't fail, on the other hand I don't see why any add-on would do this, so I'll try removing these before testing the add-ons above.
(In reply to :Paolo Amadini from comment #10) > I'll try removing these before testing the add-ons above. Tested the basic functionality, and I could see no obvious issues or console errors.
I'll fix the missing define on Windows, but I'm keeping the review request on the current version for now.
Comment on attachment 8866746 [details] Bug 1364050 - Part 3 - Remove obsolete code from the nsIDownloadManager implementation. https://reviewboard.mozilla.org/r/138352/#review142134 ::: toolkit/components/downloads/nsDownloadManager.cpp:15 (Diff revision 2) > > using namespace mozilla; > -using mozilla::downloads::GenerateGUID; > > #define DOWNLOAD_MANAGER_BUNDLE "chrome://mozapps/locale/downloads/downloads.properties" > #define DOWNLOAD_MANAGER_ALERT_ICON "chrome://mozapps/skin/downloads/downloadIcon.png" Apparently, keeping the constant isn't enough to fool browser_all_files_referenced.js. Maybe I should just remove the icon, some add-ons reference it but it's difficult to say whether they are still maintained. Otherwise I can try to whitelist the file. What do you think?
(In reply to :Paolo Amadini from comment #19) > Apparently, keeping the constant isn't enough to fool > browser_all_files_referenced.js. maybe you could try using static const char icon[] = "..."; I'm not sure how browser_all_files_referenced works with cpp sources. In the worst case you could add this temporary to the whitelist pointing to bug 851471 for the final removal. > Maybe I should just remove the icon, some add-ons reference it but it's > difficult to say whether they are still maintained. What I usually do is looking at the date of the latest version and of the last user reviews. If both of those dates are older than one year, it is likely unmaintained. Otherwise I check if the last 2 reviews report the add-on as "not working" and then it's again likely unmaintained. Regardless, if it's add-ons with less then 10k users we likely don't care and you can just remove those.
I've added the exception and started a new tryserver build. Will remove the leftover #define before landing.
Comment on attachment 8866744 [details] Bug 1364050 - Part 1 - Remove the MOZ_JSDOWNLOADS configure option. https://reviewboard.mozilla.org/r/138348/#review143390
Attachment #8866744 - Flags: review?(mak77) → review+
Comment on attachment 8866745 [details] Bug 1364050 - Part 2 - Remove unused strings. https://reviewboard.mozilla.org/r/138350/#review143392
Attachment #8866745 - Flags: review?(mak77) → review+
Comment on attachment 8866747 [details] Bug 1364050 - Part 4 - Remove references to "downloads.rdf". https://reviewboard.mozilla.org/r/138354/#review143396
Attachment #8866747 - Flags: review?(mak77) → review+
Comment on attachment 8866748 [details] Bug 1364050 - Part 5 - Update obsolete references in code comments. https://reviewboard.mozilla.org/r/138356/#review143400
Attachment #8866748 - Flags: review?(mak77) → review+
Comment on attachment 8866746 [details] Bug 1364050 - Part 3 - Remove obsolete code from the nsIDownloadManager implementation. https://reviewboard.mozilla.org/r/138352/#review143416
Attachment #8866746 - Flags: review?(mak77) → review+
Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8915546989 Part 1 - Remove the MOZ_JSDOWNLOADS configure option. r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/00c0b56135d5 Part 2 - Remove unused strings. r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/ed20cba19d7e Part 3 - Remove obsolete code from the nsIDownloadManager implementation. r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/779c698025a0 Part 4 - Remove references to "downloads.rdf". r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/43fd4e710342 Part 5 - Update obsolete references in code comments. r=mak
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: