852581 - Add methods to get the default downloads directories

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The properties of nsIDownloadManager that retrieve the user or system downloads
directories should be ported to the JavaScript API.

Since the directory might need to be created, the new functions should ideally
become asynchronous. However, a synchronous version might be needed if the
callers expect the directory to be available when the function returns, and
they cannot be easily adapted to wait for the result asynchronously.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[Raymond Lee [:raymondlee]]

Comment on attachment 746322 [details] [diff] [review]
v1

I haven't added a sync version because I think we should avoid using all sync file I/O operations and callers should be updated to use the async one when switching to this new JS downloads API.

Comment 3

[:Paolo Amadini]

Comment on attachment 746322 [details] [diff] [review]
v1

Review of attachment 746322 [details] [diff] [review]:
-----------------------------------------------------------------

Porting the original logic is a good start, in this bug we should also improve and simplify the logic as much as we can.

Please name the function getSystemDownloadsDirectory, and, while keeping the entry point in the Downloads module, move the implementation into an identically named function in the DownloadIntegration module, that will become the preprocessed module.

We also need tests for all platforms in a file named "test_DownloadIntegration.js". For platforms where we may need to create the download directory, we need to create an empty temporary directory, mock the directory service to have the requested key point to that directory, and verify that a subdirectory with the right name is created after calling the function.

For other platforms, I guess the best thing we can do is to either replicate the same calls, like the ones to nsIEnvironment, or again mock them.

The "test_Downloads.js" test should just check that the function succeeds.

Note that, if mocking the services becomes a problem because of lazy getters, we may just encapsulate the calls into separate functions on the DownloadIntegration.jsm module, and replace them with test-specific versions at test time.

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +56,5 @@
>  this.Downloads = {
>    /**
> +   * Returns the platform default downloads directory asynchronously.
> +   */
> +  get defaultDownloadsDirectory() {

In general, we don't use getters for returning promises.

For efficiency, we may want to memorize here the result of the first call to the DownloadIntegration helper, if it succeeds. I don't think the system downloads directory may change at all during normal operation.

@@ +67,5 @@
> +      //   Downloads
> +      // XP/2K:
> +      //   My Documents/Downloads
> +      // Linux:
> +      //   XDG user dir spec, with a fallback to Home/Downloads

I'd prefer to see this explanation in the function's comment.

@@ +71,5 @@
> +      //   XDG user dir spec, with a fallback to Home/Downloads
> +
> +      let folderName = gStringBundle.GetStringFromName("downloadsFolder");
> +      let dirService = Cc["@mozilla.org/file/directory_service;1"].
> +                         getService(Ci.nsIProperties);

Please define lazy global getters for all services, and use Services.jsm versions where available.

@@ +88,5 @@
> +        downloadDir.append(folderName);
> +
> +        // This could be the first time we are creating the downloads folder in
> +        // My Documents, so make sure it exists.
> +        yield OS.File.makeDir(downloadDir.path, { ignoreExisting: true });

I think we should do an effort to create the directory every time we use our own folderName, not only on Windows XP.

@@ +112,5 @@
> +      }
> +#else
> +      try {
> +        downloadDir = dirService.get("DfltDwnld", Ci.nsIFile);
> +      } catch(e) {

Can you please verify when the above call is expected to fail? Also, we may want to check for a more specific exception if possible.

Comment 4

[:Paolo Amadini]

(In reply to Raymond Lee [:raymondlee] from comment #2)
> I haven't added a sync version because I think we should avoid using all
> sync file I/O operations and callers should be updated to use the async one
> when switching to this new JS downloads API.

This sounds good. Can you do a first pass in the code base and just see which
places would need to be made asynchronous? The nsExternalHelperAppService.cpp
module comes to my mind first, as we should update it to call into the new
Downloads API (maybe with an indirect XPCOM helper) instead of duplicating
the logic there:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#288

Comment 5

[Raymond Lee [:raymondlee]]

(In reply to Paolo Amadini [:paolo] from comment #4)
> (In reply to Raymond Lee [:raymondlee] from comment #2)
> > I haven't added a sync version because I think we should avoid using all
> > sync file I/O operations and callers should be updated to use the async one
> > when switching to this new JS downloads API.
> 
> This sounds good. Can you do a first pass in the code base and just see which
> places would need to be made asynchronous? 

There are few places using defaultDownloadsDirectory.
http://mxr.mozilla.org/mozilla-central/ident?i=defaultDownloadsDirectory&tree=mozilla-central

I think we should file a new bug to update those callers when we depreciate the method in downloadManager.cpp 

> The nsExternalHelperAppService.cpp
> module comes to my mind first, as we should update it to call into the new
> Downloads API (maybe with an indirect XPCOM helper) instead of duplicating
> the logic there:
> 
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#288

The method doesn't exactly match the one in nsDownloadManager.cpp because it gets a preference "browser.download.folderList" to determine the download folder for OSX and also it doesn't create "Downloads" directory and use system temporary directory, whereas nsDownloadManager.cpp uses "Home/Downloads"

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to Paolo Amadini [:paolo] from comment #3)
> Comment on attachment 746322 [details] [diff] [review]
> v1
> 
> Review of attachment 746322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Porting the original logic is a good start, in this bug we should also
> improve and simplify the logic as much as we can.
> 
> Please name the function getSystemDownloadsDirectory, and, while keeping the
> entry point in the Downloads module, move the implementation into an
> identically named function in the DownloadIntegration module, that will
> become the preprocessed module.

Done

> 
> We also need tests for all platforms in a file named
> "test_DownloadIntegration.js". For platforms where we may need to create the
> download directory, we need to create an empty temporary directory, mock the
> directory service to have the requested key point to that directory, and
> verify that a subdirectory with the right name is created after calling the
> function.
> 

However, unit tests are not preprocessed so can use #ifdef.  Furthremore, there are some platforms which we can't test like MOZ_WIDGET_ANDROID as the tests don't run on that.

I've pushed to try and it doesn't have MOZ_WIDGET_ANDROID
https://tbpl.mozilla.org/?tree=Try&rev=8bc1e88700da

> For other platforms, I guess the best thing we can do is to either replicate
> the same calls, like the ones to nsIEnvironment, or again mock them.
> 
> The "test_Downloads.js" test should just check that the function succeeds.
> 
> Note that, if mocking the services becomes a problem because of lazy
> getters, we may just encapsulate the calls into separate functions on the
> DownloadIntegration.jsm module, and replace them with test-specific versions
> at test time.
> 

> Can you please verify when the above call is expected to fail? Also, we may
> want to check for a more specific exception if possible.

The call would fail if XDG user dirs are disabled.  Bug 399498 has more details.

Comment 7

[:Paolo Amadini]

(In reply to Raymond Lee [:raymondlee] from comment #5)
> There are few places using defaultDownloadsDirectory.
> 
> I think we should file a new bug to update those callers when we depreciate
> the method in downloadManager.cpp 

It's a good idea to get started on converting the consumers now. Feel free to
file individual bugs for callers (or groups of callers as makes sense) and have
them block the Downloads API meta-bug 825588. Note that we should look into
converting callers of userDownloadsDirectory also.

> > http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> > nsExternalHelperAppService.cpp#288
> 
> The method doesn't exactly match the one in nsDownloadManager.cpp because it
> gets a preference "browser.download.folderList" to determine the download
> folder for OSX and also it doesn't create "Downloads" directory and use
> system temporary directory, whereas nsDownloadManager.cpp uses
> "Home/Downloads"

This is probably intended to match the userDownloadsDirectory property, and
if it is not aligned, we should understand if it's intentional or by mistake.

In fact, we should add a separate platform-independent getUserDownloadsDirectory
function to Downloads.jsm also.

(In reply to Raymond Lee [:raymondlee] from comment #6)
> However, unit tests are not preprocessed so can use #ifdef.

I see you used the JavaScript platform checks, that's the correct way.

> I've pushed to try and it doesn't have MOZ_WIDGET_ANDROID

I'm not familiar with those preprocessor macros, in fact it seems strange that
this one is not defined for the Android platform. In any case, we don't want to
add tests that don't run on the tryserver.

> The call would fail if XDG user dirs are disabled.  Bug 399498 has more
> details.

I see you added the comment explaining this to the patch, thanks.

Comment 8

[:Paolo Amadini]

Comment on attachment 749317 [details] [diff] [review]
v2

This looks good, there are a few minor comments but I can just do a single
review of a version that includes the getUserDownloadsDirectory method.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v3

Comment 10

[Raymond Lee [:raymondlee]]

(In reply to Paolo Amadini [:paolo] from comment #7)
> (In reply to Raymond Lee [:raymondlee] from comment #5)> 
> > > http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> > > nsExternalHelperAppService.cpp#288
> > 
The GetDownloadDirectory in nsExternalHelperAppService.cpp is in sync with the userDownloadsDirectory in nsDownloadManager.cpp but GetDownloadDirectory in nsExternalHelperAppService.cpp supports more platforms.  I guess we should leave GetDownloadDirectory as it's

Comment 11

[:Paolo Amadini]

Comment on attachment 753734 [details] [diff] [review]
v3

Review of attachment 753734 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Raymond Lee [:raymondlee] from comment #10)
> The GetDownloadDirectory in nsExternalHelperAppService.cpp is in sync with
> the userDownloadsDirectory in nsDownloadManager.cpp but GetDownloadDirectory
> in nsExternalHelperAppService.cpp supports more platforms.  I guess we
> should leave GetDownloadDirectory as it's

The two functions have a different outcome in some cases, yes, but we should nevertheless make GetDownloadDirectory in nsExternalHelperAppService.cpp reuse the code of Downloads.jsm as much as possible.

Upon re-reading the functions, I think some small differences may be unimportant, so I'd just create a new function named getTemporaryDownloadsDirectory and make its logic work as follows:

- On Mac, just call getUserDownloadsDirectory.
- On Android and Windows Metro (i.e. IsImmersiveProcess), just call getSystemDownloadsDirectory.
- In all other cases, return the system temporary directory.

This will also make it easier to revise this logic in the future if needed.

Note that checking "IsImmersiveProcess" in JavaScript might need some js-ctypes. Let me know if you need any help, or you could even add a stub function to DownloadIntegration and file a bug to implement and test it later.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +45,5 @@
>   * example the global prompts on shutdown.
>   */
>  this.DownloadIntegration = {
> +  // For testing only
> +  testMode: false,

Although I'd rather see simple mocking like in attachment 725331 [details] [diff] [review], that solution also raised some questions, so I think testMode is fine for now.

Later, to better separate test code from production code, we may implement proper mocking, also checking that the function is called with the right argument.

@@ +68,5 @@
> +        downloadDir = this._getDirectory("Pers", Ci.nsIFile);
> +        downloadDir.append(gStringBundle.GetStringFromName("downloadsFolder"));
> +
> +        // Create the Downloads folder and ignore if it already exists.
> +        yield OS.File.makeDir(downloadDir.path, { ignoreExisting: true });

Even if the repetitions are preprocessed out, this pattern is common enough that you could create a helper function that simply takes the parent directory identifier as an argument, for example "Pers", and returns the makeDir promise. You can then just call yield on that function.

@@ +80,5 @@
> +      // for us to fallback into, "$HOME/MyDocs/.documents/" is the folder
> +      // we found most appropriate to be the default target folder for
> +      // downloads on the platform.
> +      downloadDir = this._getDirectory("XDGDocs", Ci.nsIFile);
> +#elifdef MOZ_WIDGET_ANDROID

It seems to me that ANDROID is what was intended to be checked in the first place instead of MOZ_WIDGET_ANDROID.

@@ +141,5 @@
> +        case 0: // Desktop
> +          downloadDir = this._getDirectory("Desk", Ci.nsIFile);
> +          break;
> +        case 1: // Downloads
> +          downloadDir = yield this.getSystemDownloadsDirectory();

So, after seeing that getUserDownloadsDirectory depends on getSystemDownloadsDirectory, and getTemporaryDownloadsDirectory will depend on getUserDownloadsDirectory, I think that the easiest thing to do here is to move the "if (!this._downloadsDirectory)" directly into the getSystemDownloadsDirectory function in this module, and have the other function work without memorizing their result.

@@ +181,5 @@
> +   * Returns the directory for the given name and type.
> +   *
> +   * @return The directory object.
> +   */
> +  _getDirectory: function DI_getDirectory(aName, aType) {

aType is always Ci.nsIFile, no need to specify it. Also the comment could just be "Calls the directory service and returns an nsIFile for the requested location name."

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +46,5 @@
>  /**
>   * This object is exposed directly to the consumers of this JavaScript module,
>   * and provides the only entry point to get references to back-end objects.
>   */
>  this.Downloads = {

nit: I'd rather have the functions to get the download directories at the bottom of the file.

@@ +95,5 @@
> +      throw new Task.Result(this._userDownloadsDirectory);
> +    }.bind(this));
> +  },
> +  _userDownloadsDirectory: null,
> +  _folderListPrefValue: null,

These variables and the associated checks can just go away, because the function doesn't look expensive with the default preferences, and in this way we can simplify the code.

In fact, all the methods in Downloads.jsm will just call directly into DownloadIntegration.jsm.

For the record, the proper way of invalidating the state would be to use a preference observer (including the "browser.download.dir" preference).

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +117,5 @@
> +
> +  // Should return null because the folderList preference is invalid
> +  Services.prefs.setIntPref(folderListPrefName, 999);
> +  let downloadDir = yield DownloadIntegration.getUserDownloadsDirectory();
> +  do_check_true(downloadDir === null);

We should fall back to the system downloads directory when the preferences are invalid.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to Paolo Amadini [:paolo] from comment #11)
> Comment on attachment 753734 [details] [diff] [review]
> v3
> 
> Review of attachment 753734 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Raymond Lee [:raymondlee] from comment #10)
> > The GetDownloadDirectory in nsExternalHelperAppService.cpp is in sync with
> > the userDownloadsDirectory in nsDownloadManager.cpp but GetDownloadDirectory
> > in nsExternalHelperAppService.cpp supports more platforms.  I guess we
> > should leave GetDownloadDirectory as it's
> 
> The two functions have a different outcome in some cases, yes, but we should
> nevertheless make GetDownloadDirectory in nsExternalHelperAppService.cpp
> reuse the code of Downloads.jsm as much as possible.
> 
> Upon re-reading the functions, I think some small differences may be
> unimportant, so I'd just create a new function named
> getTemporaryDownloadsDirectory and make its logic work as follows:
> 
> - On Mac, just call getUserDownloadsDirectory.
> - On Android and Windows Metro (i.e. IsImmersiveProcess), just call
> getSystemDownloadsDirectory.
> - In all other cases, return the system temporary directory.
> 
> This will also make it easier to revise this logic in the future if needed.
> 
> Note that checking "IsImmersiveProcess" in JavaScript might need some
> js-ctypes. Let me know if you need any help, or you could even add a stub
> function to DownloadIntegration and file a bug to implement and test it
> later.

I have added a _isImmersiveProcess method.  It's best to do that in another bug.

> 
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +45,5 @@
> >   * example the global prompts on shutdown.
> >   */
> >  this.DownloadIntegration = {
> > +  // For testing only
> > +  testMode: false,
> 
> Although I'd rather see simple mocking like in attachment 725331 [details] [diff] [review]
> [diff] [review], that solution also raised some questions, so I think
> testMode is fine for now.
> 
> Later, to better separate test code from production code, we may implement
> proper mocking, also checking that the function is called with the right
> argument.
> 
> @@ +68,5 @@
> > +        downloadDir = this._getDirectory("Pers", Ci.nsIFile);
> > +        downloadDir.append(gStringBundle.GetStringFromName("downloadsFolder"));
> > +
> > +        // Create the Downloads folder and ignore if it already exists.
> > +        yield OS.File.makeDir(downloadDir.path, { ignoreExisting: true });
> 
> Even if the repetitions are preprocessed out, this pattern is common enough
> that you could create a helper function that simply takes the parent
> directory identifier as an argument, for example "Pers", and returns the
> makeDir promise. You can then just call yield on that function.

Added

> 
> @@ +80,5 @@
> > +      // for us to fallback into, "$HOME/MyDocs/.documents/" is the folder
> > +      // we found most appropriate to be the default target folder for
> > +      // downloads on the platform.
> > +      downloadDir = this._getDirectory("XDGDocs", Ci.nsIFile);
> > +#elifdef MOZ_WIDGET_ANDROID
> 
> It seems to me that ANDROID is what was intended to be checked in the first
> place instead of MOZ_WIDGET_ANDROID.
> 
> @@ +141,5 @@
> > +        case 0: // Desktop
> > +          downloadDir = this._getDirectory("Desk", Ci.nsIFile);
> > +          break;
> > +        case 1: // Downloads
> > +          downloadDir = yield this.getSystemDownloadsDirectory();
> 
> So, after seeing that getUserDownloadsDirectory depends on
> getSystemDownloadsDirectory, and getTemporaryDownloadsDirectory will depend
> on getUserDownloadsDirectory, I think that the easiest thing to do here is
> to move the "if (!this._downloadsDirectory)" directly into the
> getSystemDownloadsDirectory function in this module, and have the other
> function work without memorizing their result.

Updated

> 
> @@ +181,5 @@
> > +   * Returns the directory for the given name and type.
> > +   *
> > +   * @return The directory object.
> > +   */
> > +  _getDirectory: function DI_getDirectory(aName, aType) {
> 
> aType is always Ci.nsIFile, no need to specify it. Also the comment could
> just be "Calls the directory service and returns an nsIFile for the
> requested location name."

Updated

> 
> ::: toolkit/components/jsdownloads/src/Downloads.jsm
> @@ +46,5 @@
> >  /**
> >   * This object is exposed directly to the consumers of this JavaScript module,
> >   * and provides the only entry point to get references to back-end objects.
> >   */
> >  this.Downloads = {
> 
> nit: I'd rather have the functions to get the download directories at the
> bottom of the file.
> 
> @@ +95,5 @@
> > +      throw new Task.Result(this._userDownloadsDirectory);
> > +    }.bind(this));
> > +  },
> > +  _userDownloadsDirectory: null,
> > +  _folderListPrefValue: null,
> 
> These variables and the associated checks can just go away, because the
> function doesn't look expensive with the default preferences, and in this
> way we can simplify the code.
> 
> In fact, all the methods in Downloads.jsm will just call directly into
> DownloadIntegration.jsm.
> 
> For the record, the proper way of invalidating the state would be to use a
> preference observer (including the "browser.download.dir" preference).
> 

Updated

> ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
> @@ +117,5 @@
> > +
> > +  // Should return null because the folderList preference is invalid
> > +  Services.prefs.setIntPref(folderListPrefName, 999);
> > +  let downloadDir = yield DownloadIntegration.getUserDownloadsDirectory();
> > +  do_check_true(downloadDir === null);
> 
> We should fall back to the system downloads directory when the preferences
> are invalid.

Updated.

Comment 13

[:Paolo Amadini]

Comment on attachment 755360 [details] [diff] [review]
v4

Review of attachment 755360 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really neat! I've done a final detailed review pass, we should be able to reach the last iteration soon.

There are a few major comments and changes, as it's important to review the behavior of old code while porting it. Having a simple code flow helped a lot in identifying these. So, thanks!

There are also lots of comments on little details :-)

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +56,5 @@
> +  getSystemDownloadsDirectory: function DI_getSystemDownloadsDirectory() {
> +    return Task.spawn(function() {
> +      if (this._downloadsDirectory) {
> +        // This explicitly makes this function a generator for Task.jsm, so
> +        // that exceptions in the above calls can be reported asynchronously.

Please change this comment to: This explictly makes this function a generator for Task.jsm. We need this because calls to the "yield" operator below may be preprocessed out on some platforms.

@@ +86,5 @@
> +      // Android doesn't have a $HOME directory, and by default we only have
> +      // write access to /data/data/org.mozilla.{$APP} and /sdcard
> +      let environment = Cc["@mozilla.org/process/environment;1"].
> +                          getService(Ci.nsIEnvironment);
> +      let directoryPath = environment.get("DOWNLOADS_DIRECTORY");

nit: you may use XPCOMUtils.defineLazyServiceGetter for consistency

@@ +90,5 @@
> +      let directoryPath = environment.get("DOWNLOADS_DIRECTORY");
> +      if (directoryPath) {
> +        directory = Cc["@mozilla.org/file/local;1"].
> +                        createInstance(Ci.nsILocalFile);
> +        directory.initWithPath(directoryPath);

I missed this during my previous review, sorry. I think we shouldn't ever return null for failures in this function. Please do something like this (requires a lazy getter for FileUtils.jsm):

if (!directoryPath) {
  throw new Components.Exception("DOWNLOADS_DIRECTORY is not set.",
                                 Cr.NS_ERROR_FILE_UNRECOGNIZED_PATH);
}
directory = new FileUtils.File(directoryPath);

@@ +104,5 @@
> +#endif
> +#else
> +      directory = yield this._createDownloadsDirectory("Home");
> +#endif
> +      yield;

This is now unneeded.

@@ +128,5 @@
> +      try {
> +        prefValue = Services.prefs.getIntPref(prefName);
> +      } catch(e) {
> +        prefValue = prefDefaultValue;
> +        Services.prefs.setIntPref(prefName, prefValue);

I don't think we should bother with resetting the preference values, even if the previous code used to do that. In fact, this is not a common practice, and leaving the preferences untouched allows us to add new values in future versions more easily.

So, the prefName, dirPrefName and prefDefaultValue constants can go away.

@@ +149,5 @@
> +          let useDefault = true;
> +          if (directory) {
> +            // Create the Downloads folder and ignore if it already exists.
> +            try {
> +              yield OS.File.makeDir(directory.path, { ignoreExisting: true });

By the way, I see how we might try to create the same directory every time we start a download, and fall back to the system directory if we fail, but I don't see this as much of a problem as it was before, because now we do this on a background thread without blocking the user interface.

To simplify this code, you can get rid of useDefault and use a single try-catch block instead.

@@ +167,5 @@
> +          Services.prefs.setIntPref(prefName, prefDefaultValue);
> +      }
> +      // This explicitly makes this function a generator for Task.jsm, so that
> +      // exceptions in the above calls can be reported asynchronously.
> +      yield;

This is also unneeded.

@@ +177,5 @@
> +   * Returns the temporary downloads directory asynchronously.
> +   *
> +   * @return {Promise}
> +   * @resolves The nsIFile of downloads directory or null if preference value
> +   * is invalid.

This comment should be updated because we don't return null.

@@ +188,5 @@
> +#elifdef ANDROID
> +      directory = yield this.getSystemDownloadsDirectory();
> +#else
> +      if (this._isImmersiveProcess()) {
> +        directory = yield this.getSystemDownloadsDirectory();

Please add a comment explaining that we are doing this in Metro mode on Windows 8 because we want searchability for documents that the user chose to open with an external application.

@@ +191,5 @@
> +      if (this._isImmersiveProcess()) {
> +        directory = yield this.getSystemDownloadsDirectory();
> +      } else {
> +        directory = Services.dirsvc.get("TmpD", Ci.nsIFile);
> +        yield;

Please use _getDirectory here. The yield call is unneeded also, since there is one in the other branch already.

@@ +221,5 @@
> +      // Create the Downloads folder and ignore if it already exists.
> +      yield OS.File.makeDir(directory.path, { ignoreExisting: true });
> +
> +      throw new Task.Result(directory);
> +    }.bind(this));

Since this function does a very simple thing, you can simplify it by removing the task and calling "return OS.File.makeDir(...);".

Also, the comment should say "@return {Promise}".

There is now some mode detailed documentation on MDN about tasks and promises. Let me know if you think something could be improved (or feel free to edit if you want):

https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm
https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Task.jsm

::: toolkit/components/jsdownloads/src/Downloads.jsm
@@ +180,5 @@
> +   * @resolves The nsIFile of downloads directory.
> +   */
> +  getSystemDownloadsDirectory: function D_getSystemDownloadsDirectory() {
> +    return DownloadIntegration.getSystemDownloadsDirectory();
> +  },

I'd like so see in the comment some examples of what the "system downloads directory" my usually look like on Windows, Mac, Linux and Android.

@@ +190,5 @@
> +   * @resolves The nsIFile of downloads directory.
> +   */
> +  getUserDownloadsDirectory: function D_getUserDownloadsDirectory() {
> +    return DownloadIntegration.getUserDownloadsDirectory();
> +  },

In the comment I'd say that this returns the preferred downloads directory based on the user preferences in the current profile.

Actually, now that I think about it, getPreferredDownloadsDirectory may be a better name for the function. What do you think?

@@ +200,5 @@
> +   * @resolves The nsIFile of downloads directory.
> +   */
> +  getTemporaryDownloadsDirectory: function D_getTemporaryDownloadsDirectory() {
> +    return DownloadIntegration.getTemporaryDownloadsDirectory();
> +  },

Here, I'd say that this function returns the directory where downloads are placed before the final location is chosen, or while the document is opened temporarily with an external application. This may or may not be the system temporary directory, based on the platform.

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +15,5 @@
> +    createBundle("chrome://mozapps/locale/downloads/downloads.properties");
> +});
> +
> +/**
> + * Tests that the getSystemDownloadsDirectory returns a valid nsFile 

global-nit: whitespace at end of line, please remove it from all the files.

@@ +52,5 @@
> +    let info = yield OS.File.stat(downloadDir.path);
> +    do_check_true(info.isDir);
> +    try {
> +      yield OS.File.remove(targetPath);
> +    } catch(e) {}

An exception while removing the directory we just created should make the test fail.

Also, before starting the test, we should remove the directory if it exists.

@@ +64,5 @@
> + */
> +add_task(function test_getUserDownloadsDirectory()
> +{
> +  // Enable test mode to enable mocking for the getSystemDownloadsDirectoy
> +  // method.

This comment should be updated.

@@ +74,5 @@
> +  }
> +  do_register_cleanup(cleanup);
> +
> +  // Should return the system downloads directory.
> +  let defaultDir = yield DownloadIntegration.getSystemDownloadsDirectory();

nit: let systemDir =

@@ +77,5 @@
> +  // Should return the system downloads directory.
> +  let defaultDir = yield DownloadIntegration.getSystemDownloadsDirectory();
> +  let downloadDir = yield DownloadIntegration.getUserDownloadsDirectory();
> +  do_check_true(downloadDir instanceof Ci.nsIFile);
> +  do_check_eq(downloadDir.path, defaultDir.path);

We should also set the preferences before starting this test (in case they were changed or a different default is used).

@@ +95,5 @@
> +
> +  // Should return the directory which is listed in the dir preference.
> +  let time = (new Date()).getTime();
> +  let tempDir = Services.dirsvc.get("TmpD", Ci.nsIFile);
> +  tempDir.append(time);

I think you might be able to use the getTempFile helper here.

@@ +109,5 @@
> +  // Should return the system downloads directory beacause the path is invalid
> +  // in the dir preference.
> +  tempDir = Services.dirsvc.get("TmpD", Ci.nsIFile);
> +  tempDir.append("dir_not_exist");
> +  tempDir.append(time);

Also here, getTempFile and then append("dir_not_exist");

@@ +115,5 @@
> +  downloadDir = yield DownloadIntegration.getUserDownloadsDirectory();
> +  do_check_eq(downloadDir.path, defaultDir.path);
> +
> +  // Should return the system downloads director because the folderList
> +  // preference is invalid

typo: directory

::: toolkit/components/jsdownloads/test/unit/test_Downloads.js
@@ +133,5 @@
> +});
> +
> +/**
> + * Tests that the getUserDownloadsDirectory returns a valid nsFile 
> + * download directory object and it changes based on a preference.

It seems "and it changes based on a preference" does not apply.

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v5

(In reply to Paolo Amadini [:paolo] from comment #13)
> Comment on attachment 755360 [details] [diff] [review]
> v4
> 
> Review of attachment 755360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +95,5 @@
> > +
> > +  // Should return the directory which is listed in the dir preference.
> > +  let time = (new Date()).getTime();
> > +  let tempDir = Services.dirsvc.get("TmpD", Ci.nsIFile);
> > +  tempDir.append(time);
> 
> I think you might be able to use the getTempFile helper here.
> 
> @@ +109,5 @@
> > +  // Should return the system downloads directory beacause the path is invalid
> > +  // in the dir preference.
> > +  tempDir = Services.dirsvc.get("TmpD", Ci.nsIFile);
> > +  tempDir.append("dir_not_exist");
> > +  tempDir.append(time);
> 
> Also here, getTempFile and then append("dir_not_exist");
> 

The getTempFile method returns a file instead of a directory.  It doesn't fit to what we need here.

Comment 15

[:Paolo Amadini]

Comment on attachment 756424 [details] [diff] [review]
v5

Review of attachment 756424 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Raymond! r+ with the changes below, assuming tryserver tests pass on all platforms.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +123,5 @@
> +   */
> +  getUserDownloadsDirectory: function DI_getUserDownloadsDirectory() {
> +    return Task.spawn(function() {
> +      let directory = null;
> +      let prefValue;

let prefValue = 1, for the default value (or set it in the catch clause).

@@ +154,5 @@
> +            }
> +          } else {
> +            directory = yield this.getSystemDownloadsDirectory();
> +          }
> +          break;

I've checked, and Services.prefs.getComplexValue always raises an exception if the path is missing or invalid, it doesn't return null. So, case 2 can be simplified like this:

case 2: // Custom
try {
  directory = Services.prefs.getComplexValue("browser.download.dir",
                                             Ci.nsIFile);
  yield OS.File.makeDir(directory.path, { ignoreExisting: true });
} catch (ex) {
  // Either the preference isn't set or the directory cannot be created.
  directory = yield this.getSystemDownloadsDirectory();
}
break;

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +60,5 @@
> +    try {
> +      yield OS.File.removeEmptyDir(targetPath);
> +    } catch(e) {
> +      throw new Components.Exception("Unable to remove target directory.")
> +    }

No need to rethrow here (and in a few other places as well), just don't catch.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: Patch for check it

(In reply to Paolo Amadini [:paolo] from comment #15)
> Comment on attachment 756424 [details] [diff] [review]
> v5
> 
> Review of attachment 756424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you Raymond! r+ with the changes below, assuming tryserver tests pass
> on all platforms.
> 
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +123,5 @@
> > +   */
> > +  getUserDownloadsDirectory: function DI_getUserDownloadsDirectory() {
> > +    return Task.spawn(function() {
> > +      let directory = null;
> > +      let prefValue;
> 
> let prefValue = 1, for the default value (or set it in the catch clause).

Done.

> 
> @@ +154,5 @@
> > +            }
> > +          } else {
> > +            directory = yield this.getSystemDownloadsDirectory();
> > +          }
> > +          break;
> 
> I've checked, and Services.prefs.getComplexValue always raises an exception
> if the path is missing or invalid, it doesn't return null. So, case 2 can be
> simplified like this:
> 
> case 2: // Custom
> try {
>   directory = Services.prefs.getComplexValue("browser.download.dir",
>                                              Ci.nsIFile);
>   yield OS.File.makeDir(directory.path, { ignoreExisting: true });
> } catch (ex) {
>   // Either the preference isn't set or the directory cannot be created.
>   directory = yield this.getSystemDownloadsDirectory();
> }
> break;
> 

Updated

> ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
> @@ +60,5 @@
> > +    try {
> > +      yield OS.File.removeEmptyDir(targetPath);
> > +    } catch(e) {
> > +      throw new Components.Exception("Unable to remove target directory.")
> > +    }
> 
> No need to rethrow here (and in a few other places as well), just don't
> catch.

Removed


Passed try:
https://tbpl.mozilla.org/?tree=Try&rev=2f1564464379

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c95dfb83024

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c95dfb83024