Closed
Bug 1384038
Opened 7 years ago
Closed 7 years ago
Stop using preprocessor in DownloadIntegration.jsm
Categories
(Firefox :: Downloads Panel, enhancement, P2)
Firefox
Downloads Panel
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 1 obsolete file)
14.20 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
I think we can use AppConstants.jsm instead.
Comment 1•7 years ago
|
||
Yes, and we can remove the B2G-specific paths. We can also just check for non-Desktop to exclude Places code paths. The history observer could be moved to DownloadHistory.jsm, added in bug 830415, but this would be a separate bug.
Priority: -- → P2
Assignee | ||
Comment 2•7 years ago
|
||
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8889824 -
Flags: review?(paolo.mozmail)
Comment 3•7 years ago
|
||
Comment on attachment 8889824 [details] [diff] [review] Patch Review of attachment 8889824 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! We can take this opportunity for some cleanup. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +24,5 @@ > const Cr = Components.results; > > Cu.import("resource://gre/modules/Integration.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/AppConstants.jsm"); nit: this can become a lazy getter, see below. @@ +47,5 @@ > "resource://gre/modules/osfile.jsm"); > +if (AppConstants.MOZ_PLACES) { > + XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils", > + "resource://gre/modules/PlacesUtils.jsm"); > +} No need to make the lazy getters conditional. @@ +261,5 @@ > aDownload.hasBlockedData) { > return true; > } > + > + if (AppConstants.platform === "android") { nit: == instead of === throughout the file @@ +268,5 @@ > + } else { > + // On Desktop, stopped downloads for which we don't need to track the > + // presence of a ".part" file are only retained in the browser history. > + return false; > + } Can you just turn this method into a single return statement, updating the comment? return !aDownload.stopped || aDownload.hasPartialData || aDownload.hasBlockedData || AppConstants.platform == "android"; @@ +309,5 @@ > + try { > + directoryPath = this._getDirectory("DfltDwnld"); > + } catch(e) { > + directoryPath = await this._createDownloadsDirectory("Home"); > + } I know it may seem out of scope, but we don't build for Windows XP anymore. On Windows 7 and Mac OS X, this._getDirectory("DfltDwnld") is very likely to succeed, even if the directory does not exist. This means we can just use this simple try-catch block on all desktop platforms unconditionally, and only have a special code path for Android. Also, you can assign to this._downloadsDirectory directly and remove directoryPath. @@ +408,5 @@ > + return RuntimePermissions.waitForPermissions(RuntimePermissions.WRITE_EXTERNAL_STORAGE) > + .then(permissionGranted => !permissionGranted); > + } else { > + return Promise.resolve(false); > + } async shouldBlockForRuntimePermissions() { return AppConstants.platform == "android" && !(await RuntimePermissions.waitForPermissions( RuntimePermissions.WRITE_EXTERNAL_STORAGE)); } (The parentheses around await may be unneeded? They were required when we used yield.) @@ +515,5 @@ > + // the application to hang, or other performance issues. > + // The stream created in this way is forward-compatible with all the > + // current and future versions of Windows. > + if (this._shouldSaveZoneInformation()) { > + let zone; nit: if (AppConstants.platform == "win" && this._shouldSaveZoneInformation()) { this._saveZoneInformation(); } so it's clearer and we don't have to indent a long block. (And move "We do this by writing..." part of the comment to the JSDoc of the new function) Could also move AppConstants.platform != "win" as an early return in _shouldSaveZoneInformation. @@ +1036,5 @@ > > //////////////////////////////////////////////////////////////////////////////// > //// DownloadHistoryObserver > > +if (AppConstants.MOZ_PLACES) { Just move this check to the point where the object is constructed.
Attachment #8889824 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8889824 -
Attachment is obsolete: true
Attachment #8889870 -
Flags: review?(paolo.mozmail)
Comment 5•7 years ago
|
||
Comment on attachment 8889870 [details] [diff] [review] Patch Review of attachment 8889870 [details] [diff] [review]: ----------------------------------------------------------------- Looks good :-) ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +379,5 @@ > */ > + async shouldBlockForRuntimePermissions() { > + return AppConstants.platform == "android" && > + !(await RuntimePermissions.waitForPermissions( > + RuntimePermissions.WRITE_EXTERNAL_STORAGE)); nit: two more spaces for indentation on both lines
Attachment #8889870 -
Flags: review?(paolo.mozmail) → review+
Pushed by mcastelluccio@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2ec925aed5 Stop using preprocessor in DownloadIntegration.jsm. r=paolo
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7b2ec925aed5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•