Closed Bug 1384038 Opened 7 years ago Closed 7 years ago

Stop using preprocessor in DownloadIntegration.jsm

Categories

(Firefox :: Downloads Panel, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 1 obsolete file)

I think we can use AppConstants.jsm instead.
Blocks: 1381972
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
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #8889824 - Flags: review?(paolo.mozmail)
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)
Attached patch PatchSplinter Review
Attachment #8889824 - Attachment is obsolete: true
Attachment #8889870 - Flags: review?(paolo.mozmail)
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
https://hg.mozilla.org/mozilla-central/rev/7b2ec925aed5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Blocks: 1394820
You need to log in before you can comment on or make changes to this bug.