875731 - Replace callers of nsIDownloadManager.usersDownloadsDirectory with Downloads.getPreferredDownloadsDirectory

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Raymond Lee [:raymondlee]]

http://mxr.mozilla.org/mozilla-central/ident?i=GetUserDownloadsDirectory

We should switch those consumers to use the new JS Downloads.getUserDownloadsDirectory.

Comment 1

[Raymond Lee [:raymondlee]]

Paolo: there are several places which we need to switch the consimers to use the getUserDownloadsDirectory.

browser/devtools/commandline/
browser/metro
browser/android
toolkit/content/contentAreaUtils.js
toolkit/mozapps/downloads/nsHelperAppDlg.js

Should this patch also cover browser/metro and browser/android or we should file separate bugs for those.

Comment 2

[:Paolo Amadini]

In general, the best way is to file a separate file for each component that requires a different reviewer.

Comment 3

[:Paolo Amadini]

Note that we'll rename the function in bug 917217.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: /mobile/andriod

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: /browser/devtools/

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: /browser/metro/

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: toolkit/mozapps/

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/mozapps

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 806533 [details] [diff] [review]
/browser/metro/

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

Looks good overall, except for some problems with imports:

::: browser/metro/base/content/ContextCommands.js
@@ +6,5 @@
>   /*
>    * context menu command handlers
>    */
>  
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

Not needed; this is already in scope.

@@ +9,5 @@
>  
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> +                                  "resource://gre/modules/Downloads.jsm");

This will break things because we already have a global variable "Downloads" in this scope.  I think the best solution is to rename that other variable to something like "MetroDownloadsView".

Module imports for the main browser window should go in /browser/metro/base/content/browser-scripts.js (just for the sake of organization).

@@ +11,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> +                                  "resource://gre/modules/Downloads.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");

Not needed; Task.jsm is already imported in browser-scripts.js.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 806537 [details] [diff] [review]
/toolkit/mozapps

Paolo can take this

Comment 12

[Dave Camp (:dcamp)]

Comment on attachment 806532 [details] [diff] [review]
/browser/devtools/

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +1735,5 @@
> +        Task.spawn(function() {
> +          let reply = yield this.grabScreen(document, args.filename, args.clipboard,
> +                                            args.fullpage, args.selector);
> +          deferred.resolve(reply);
> +        }.bind(this));

Can you use () => {} over function(){}.bind(this) in new devtools code please?

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: /browser/metro/ v2

Matt: thanks for your feedback.  Please review this updated patch.

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to Dave Camp (:dcamp) from comment #12)
> Comment on attachment 806532 [details] [diff] [review]
> /browser/devtools/
> 
> Review of attachment 806532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/BuiltinCommands.jsm
> @@ +1735,5 @@
> > +        Task.spawn(function() {
> > +          let reply = yield this.grabScreen(document, args.filename, args.clipboard,
> > +                                            args.fullpage, args.selector);
> > +          deferred.resolve(reply);
> > +        }.bind(this));
> 
> Can you use () => {} over function(){}.bind(this) in new devtools code
> please?

I have tried to use () => {} instead but it doesn't work for Task.spawn.

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 806532 [details] [diff] [review]
/browser/devtools/

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +1727,5 @@
> +          Task.spawn(function() {
> +            let reply = yield this.grabScreen(document, args.filename, args.clipboard,
> +                                              args.fullpage);
> +            deferred.resolve(reply);
> +          }.bind(this));

I'm not sure using Task.spawn helps here does it?

    let promise = this.grabScreen(document, args.filename,
                                  args.clipboard, args.fullpage);
    promise.then(deferred.resolve, deferred.reject);

This also has the advantage of passing errors on rather than silently discarding them.

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 807062 [details] [diff] [review]
/browser/metro/ v2

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

Thanks!

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 807062 [details] [diff] [review]
/browser/metro/ v2

Note: Since this patch was written, we've added one more use of the "Downloads" global that should be converted to "MetroDownloadsView":
https://hg.mozilla.org/integration/fx-team/rev/9f2d24b55cea#l2.13

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: /browser/devtools/ v2 r+

(In reply to Joe Walker [:jwalker] from comment #15)
> Comment on attachment 806532 [details] [diff] [review]
> /browser/devtools/
> 
> Review of attachment 806532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/BuiltinCommands.jsm
> @@ +1727,5 @@
> > +          Task.spawn(function() {
> > +            let reply = yield this.grabScreen(document, args.filename, args.clipboard,
> > +                                              args.fullpage);
> > +            deferred.resolve(reply);
> > +          }.bind(this));
> 
> I'm not sure using Task.spawn helps here does it?
> 
>     let promise = this.grabScreen(document, args.filename,
>                                   args.clipboard, args.fullpage);
>     promise.then(deferred.resolve, deferred.reject);
> 
> This also has the advantage of passing errors on rather than silently
> discarding them.

Updated based on your suggestion :-)

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: /browser/metro/ v3 r+

(In reply to Matt Brubeck (:mbrubeck) from comment #17)
> Comment on attachment 807062 [details] [diff] [review]
> /browser/metro/ v2
> 
> Note: Since this patch was written, we've added one more use of the
> "Downloads" global that should be converted to "MetroDownloadsView":
> https://hg.mozilla.org/integration/fx-team/rev/9f2d24b55cea#l2.13

Updated

Comment 20

[:Paolo Amadini]

Comment on attachment 806531 [details] [diff] [review]
/toolkit/content/

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

::: toolkit/content/contentAreaUtils.js
@@ +568,5 @@
> +  Task.spawn(function() {
> +    // Default to the user's default downloads directory configured
> +    // through download prefs.
> +    dir = yield Downloads.getUserDownloadsDirectory();
> +    dirExists = dir && dir.exists();

There is no need to use a Task just for this change, may just use "then" on the returned promise, with proper error handling.

However, if you feel like refactoring the getTargetFile function to use a Task (with OS.File existence checks) and return a promise, please go ahead and do so! This will improve the function's readability.

Comment 21

[:Paolo Amadini]

Comment on attachment 806537 [details] [diff] [review]
/toolkit/mozapps

May you please attach a second patch with whitespace changes ignored, for easier review?

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: /browser/metro/ v4 r+

Updated to use getPreferredDownloadsDirectory instead.  Should wait for patch in bug 917217 to land first.

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: /browser/devtools/ v3 r+

Updated to use getPreferredDownloadsDirectory instead.  Should wait for patch in bug 917217 to land first.

Comment 24

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/mozapps/ -w v2

(In reply to :Paolo Amadini from comment #21)
> Comment on attachment 806537 [details] [diff] [review]
> /toolkit/mozapps
> 
> May you please attach a second patch with whitespace changes ignored, for
> easier review?

This patch with whitespace ignored.

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/mozapps v2

Please see the -w version

Comment 26

[Raymond Lee [:raymondlee]]

Attachment: /mobile/andriod v2

Updated to use getPreferredDownloadsDirectory instead.

Comment 27

[:Paolo Amadini]

Comment on attachment 809018 [details] [diff] [review]
/toolkit/mozapps/ -w v2

Thank you, this version ignoring whitespace was a great help.

I like how Task.jsm makes this type of conversions so easy!

Comment 28

[:Paolo Amadini]

Comment on attachment 809019 [details] [diff] [review]
/toolkit/mozapps v2

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

r+ with the changes below.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +101,5 @@
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  Components.utils.import("resource://gre/modules/DownloadLastDir.jsm", downloadModule);
>  Components.utils.import("resource://gre/modules/DownloadPaths.jsm");
>  Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

Duplicate import!

@@ +106,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> +                                  "resource://gre/modules/Downloads.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");

These should either be all eager getters or all lazy getters (except XPCOMUtils).

@@ +318,1 @@
>      }.bind(this));

Please remember to add error reporting to any Task for which you don't return the promise:

".then(null, Component.utils.reportError);"

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v2

Comment 30

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v3

Comment 31

[:Paolo Amadini]

Comment on attachment 809067 [details] [diff] [review]
/toolkit/content/ v3

The aCallback parameter should be removed, and the callers updated to wait on the promise and report any errors. Actually, the function can be renamed to "promiseTargetFile".

Note that browser_privatebrowsing_downloadLastDir_c.js seems to call the function directly as well.

Comment 32

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/mozapps/ v3 r+

(In reply to :Paolo Amadini from comment #28)
> Comment on attachment 809019 [details] [diff] [review]
> /toolkit/mozapps v2
> 
> Review of attachment 809019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the changes below.
> 
> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +101,5 @@
> >  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Components.utils.import("resource://gre/modules/DownloadLastDir.jsm", downloadModule);
> >  Components.utils.import("resource://gre/modules/DownloadPaths.jsm");
> >  Components.utils.import("resource://gre/modules/DownloadUtils.jsm");
> > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> Duplicate import!
> 
> @@ +106,5 @@
> > +
> > +XPCOMUtils.defineLazyModuleGetter(this, "Downloads",
> > +                                  "resource://gre/modules/Downloads.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> > +                                  "resource://gre/modules/Task.jsm");
> 
> These should either be all eager getters or all lazy getters (except
> XPCOMUtils).
> 
> @@ +318,1 @@
> >      }.bind(this));
> 
> Please remember to add error reporting to any Task for which you don't
> return the promise:
> 
> ".then(null, Component.utils.reportError);"

Updated.

Comment 33

[Raymond Lee [:raymondlee]]

Comment on attachment 808985 [details] [diff] [review]
/browser/metro/ v4 r+

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=01578f13cc42

Comment 34

[Raymond Lee [:raymondlee]]

Comment on attachment 809015 [details] [diff] [review]
/browser/devtools/ v3 r+

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

Comment 35

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/mozapps/ v4 r+

This last patch failed try.  Updated and pushed to try again.

Comment 36

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v4

(In reply to :Paolo Amadini from comment #31)
> Comment on attachment 809067 [details] [diff] [review]
> /toolkit/content/ v3
> 
> The aCallback parameter should be removed, and the callers updated to wait
> on the promise and report any errors. Actually, the function can be renamed
> to "promiseTargetFile".
> 
> Note that browser_privatebrowsing_downloadLastDir_c.js seems to call the
> function directly as well.

Updated.

Comment 37

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v4 -w

Without white spaces version

Comment 38

[:Paolo Amadini]

Comment on attachment 809808 [details] [diff] [review]
/toolkit/content/ v4

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

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir_c.js
@@ +77,5 @@
>  
>        gDownloadLastDir.cleanupPrivateFile();
>        aWin.close();
>        aCallback();
> +    }, Components.utils.reportError);

This should be "}).then(null, Components.utils.reportError);" to catch errors in the first callback.

Note that instead of Components.utils.reportError, you may call "ok(false)" as well.

::: toolkit/content/contentAreaUtils.js
@@ +331,5 @@
>        saveAsType = fpParams.saveAsType;
>        file = fpParams.file;
>  
>        continueSave();
> +    }, Components.utils.reportError);

Same here.

@@ -547,3 @@
>  {
> -  if (!getTargetFile.DownloadLastDir)
> -    Components.utils.import("resource://gre/modules/DownloadLastDir.jsm", getTargetFile);

I think this was a technique not to pollute the global namespace.

I'm not sure if that is still a concern. We may try to go ahead with the current version, and if needed we can move the lazy module getters to the ContentAreaUtils object, for example:

XPCOMUtils.defineLazyModuleGetter(ContentAreaUtils, "Task",
                                  "resource://gre/modules/Task.jsm");

@@ +562,2 @@
>  
> +  return Task.spawn(function() {

While I'm here, I'll mention that a new syntax is available for generator functions, the "function* ()" syntax. If you use it, you can use the "return" statement with a value, instead of throwing "Task.Result". Example:

return Task.spawn(function* () {
  if (noNeedToAsk) {
    return false;
  }
  let dialogCancelled = yield ask();
  return dialogCancelled;
});

@@ +571,5 @@
>  
> +    // Default to the user's default downloads directory configured
> +    // through download prefs.
> +    let dir = yield Downloads.getPreferredDownloadsDirectory();
> +    let dirExists = dir && (yield OS.File.exists(dir.path));

I think getPreferredDownloadsDirectory cannot return null anymore, right? In this case, the "dir" check is superfluous.

@@ +588,5 @@
> +      // Keep async behavior in both branches
> +      Services.tm.mainThread.dispatch(function() {
> +        let cancelled = displayPicker();
> +        deferred.resolve(cancelled);
> +      }, Components.interfaces.nsIThread.DISPATCH_NORMAL);

All the function from this point downwards needs more refactoring.

Instead of using nested tasks and function calls, this should flow in a top-to-bottom way, using conditionals where appropriate, with the same logic but with a linear structure.

Each asynchronous function (like getFileAsync) should just resolve its own promise in its callback, and the main task should call "yield" on that promise.

@@ +634,3 @@
>        }
> +
> +      if (fp.show() == Components.interfaces.nsIFilePicker.returnCancel || !fp.file) {

Ideally, we could also upgrade this to use "fp.open()".

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/63fc2fe63f0d
https://hg.mozilla.org/integration/fx-team/rev/5705990ed986

Comment 40

[Raymond Lee [:raymondlee]]

Comment on attachment 809669 [details] [diff] [review]
/toolkit/mozapps/ v4 r+

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=904026b967c4

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/738417678451

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63fc2fe63f0d
https://hg.mozilla.org/mozilla-central/rev/5705990ed986
https://hg.mozilla.org/mozilla-central/rev/738417678451

Comment 43

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v5

(In reply to :Paolo Amadini from comment #38)
> Comment on attachment 809808 [details] [diff] [review]
> /toolkit/content/ v4
> 
> Review of attachment 809808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/components/privatebrowsing/test/browser/
> browser_privatebrowsing_downloadLastDir_c.js
> @@ +77,5 @@
> >  
> >        gDownloadLastDir.cleanupPrivateFile();
> >        aWin.close();
> >        aCallback();
> > +    }, Components.utils.reportError);
> 
> This should be "}).then(null, Components.utils.reportError);" to catch
> errors in the first callback.
> 

Updated.

> Note that instead of Components.utils.reportError, you may call "ok(false)"
> as well.
> 
> ::: toolkit/content/contentAreaUtils.js
> @@ +331,5 @@
> >        saveAsType = fpParams.saveAsType;
> >        file = fpParams.file;
> >  
> >        continueSave();
> > +    }, Components.utils.reportError);
> 
> Same here.

Updated.

> 
> @@ -547,3 @@
> >  {
> > -  if (!getTargetFile.DownloadLastDir)
> > -    Components.utils.import("resource://gre/modules/DownloadLastDir.jsm", getTargetFile);
> 
> I think this was a technique not to pollute the global namespace.
> 
> I'm not sure if that is still a concern. We may try to go ahead with the
> current version, and if needed we can move the lazy module getters to the
> ContentAreaUtils object, for example:
> 
> XPCOMUtils.defineLazyModuleGetter(ContentAreaUtils, "Task",
>                                   "resource://gre/modules/Task.jsm");
> 
> @@ +562,2 @@
> >  
> > +  return Task.spawn(function() {
> 
> While I'm here, I'll mention that a new syntax is available for generator
> functions, the "function* ()" syntax. If you use it, you can use the
> "return" statement with a value, instead of throwing "Task.Result". Example:
> 
> return Task.spawn(function* () {
>   if (noNeedToAsk) {
>     return false;
>   }
>   let dialogCancelled = yield ask();
>   return dialogCancelled;
> });

I have tried but Firefox crashed after using that so I leave that for now.

> 
> @@ +571,5 @@
> >  
> > +    // Default to the user's default downloads directory configured
> > +    // through download prefs.
> > +    let dir = yield Downloads.getPreferredDownloadsDirectory();
> > +    let dirExists = dir && (yield OS.File.exists(dir.path));
> 
> I think getPreferredDownloadsDirectory cannot return null anymore, right? In
> this case, the "dir" check is superfluous.

You are right.  I have removed the "dir" check.

> 
> @@ +588,5 @@
> > +      // Keep async behavior in both branches
> > +      Services.tm.mainThread.dispatch(function() {
> > +        let cancelled = displayPicker();
> > +        deferred.resolve(cancelled);
> > +      }, Components.interfaces.nsIThread.DISPATCH_NORMAL);
> 
> All the function from this point downwards needs more refactoring.
> 
> Instead of using nested tasks and function calls, this should flow in a
> top-to-bottom way, using conditionals where appropriate, with the same logic
> but with a linear structure.
> 
> Each asynchronous function (like getFileAsync) should just resolve its own
> promise in its callback, and the main task should call "yield" on that
> promise.
> 

Updated. Please check.

> @@ +634,3 @@
> >        }
> > +
> > +      if (fp.show() == Components.interfaces.nsIFilePicker.returnCancel || !fp.file) {
> 
> Ideally, we could also upgrade this to use "fp.open()".

Updated.

Comment 44

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v5 -w

Comment 45

[:Paolo Amadini]

Comment on attachment 810344 [details] [diff] [review]
/toolkit/content/ v5

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

::: toolkit/content/contentAreaUtils.js
@@ +17,5 @@
> +                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");

Hm, sorry to ask only now, but what happens if you do the same call to defineLazyModuleGetter twice? Does that work correctly?

This module is imported in several types of windows, and I'd like to make sure it works correctly in them even if other files import the same modules in the same global scope.

@@ +559,4 @@
>  {
> +  return Task.spawn(function() {
> +    var gDownloadLastDir = new DownloadLastDir(window);
> +    var prefs = Services.prefs.getBranch("browser.download.");

I think we can rename this to "downloadPrefs", "prefBranch" or something similar to avoid confusion.

nit: use "let" throughout the function.
nit: the "g" prefix can be removed.

@@ +583,5 @@
> +    let deferred = Promise.defer();
> +    if (useDownloadDir) {
> +      // Keep async behavior in both branches
> +      Services.tm.mainThread.dispatch(function() {
> +        deferred.resolve();

nit: deferred.resolve(null);

@@ +601,3 @@
>  
> +    function displayPicker() {
> +      return Task.spawn(function() {

No need for the nested function here.

@@ +641,5 @@
> +          prefs.setIntPref("save_converter_index", fp.filterIndex);
> +
> +        // Do not store the last save directory as a pref inside the private browsing mode
> +        var directory = fp.file.parent.QueryInterface(nsIFile);
> +        gDownloadLastDir.setFile(aRelatedURI, directory);

I think this QueryInterface call is a leftover from when we had the separate nsILocalFile interface. Can just be:

gDownloadLastDir.setFile(aRelatedURI, fp.file.parent);

So, the nsIFile constant used earlier can go away (just use Components.interfaces.nsIFile).

Comment 46

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v6

(In reply to :Paolo Amadini from comment #45)
> Comment on attachment 810344 [details] [diff] [review]
> /toolkit/content/ v5
> 
> Review of attachment 810344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/contentAreaUtils.js
> @@ +17,5 @@
> > +                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> > +                                  "resource://gre/modules/Services.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> > +                                  "resource://gre/modules/Task.jsm");
> 
> Hm, sorry to ask only now, but what happens if you do the same call to
> defineLazyModuleGetter twice? Does that work correctly?

defineLazyModuleGetter -> defineLazyGetter which takes care of that.
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.jsm#176

> 
> This module is imported in several types of windows, and I'd like to make
> sure it works correctly in them even if other files import the same modules
> in the same global scope.
> 
> @@ +559,4 @@
> >  {
> > +  return Task.spawn(function() {
> > +    var gDownloadLastDir = new DownloadLastDir(window);
> > +    var prefs = Services.prefs.getBranch("browser.download.");
> 
> I think we can rename this to "downloadPrefs", "prefBranch" or something
> similar to avoid confusion.
> 
> nit: use "let" throughout the function.
> nit: the "g" prefix can be removed.

Updated.

> 
> @@ +583,5 @@
> > +    let deferred = Promise.defer();
> > +    if (useDownloadDir) {
> > +      // Keep async behavior in both branches
> > +      Services.tm.mainThread.dispatch(function() {
> > +        deferred.resolve();
> 
> nit: deferred.resolve(null);

Updated

> 
> @@ +601,3 @@
> >  
> > +    function displayPicker() {
> > +      return Task.spawn(function() {
> 
> No need for the nested function here.

Removed.

> 
> @@ +641,5 @@
> > +          prefs.setIntPref("save_converter_index", fp.filterIndex);
> > +
> > +        // Do not store the last save directory as a pref inside the private browsing mode
> > +        var directory = fp.file.parent.QueryInterface(nsIFile);
> > +        gDownloadLastDir.setFile(aRelatedURI, directory);
> 
> I think this QueryInterface call is a leftover from when we had the separate
> nsILocalFile interface. Can just be:
> 
> gDownloadLastDir.setFile(aRelatedURI, fp.file.parent);
> 
> So, the nsIFile constant used earlier can go away (just use
> Components.interfaces.nsIFile).

Done

Comment 47

[Raymond Lee [:raymondlee]]

Attachment: /mobile/andriod v3 r+

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=89a0e5cc0bdd

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/41701d2c0341

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/41701d2c0341

Comment 50

[Aaron Train [:aaronmt]]

This supposedly broke the ability to initiate a download in Firefox for Android. Please see dependancy above.

Comment 51

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #50)
> This supposedly broke the ability to initiate a download in Firefox for
> Android. Please see dependancy above.

I backed out the part that broke Firefox for Android:
https://hg.mozilla.org/mozilla-central/rev/d71579c316c1

Comment 52

[Raymond Lee [:raymondlee]]

Matt:  I think metro might also suffer the same issue as bug 921944 about the promptForSaveToFile().  Probably we need to revert the code it browser/metro/components/HelperAppDialog.js.  Could you help to check that please?

Comment 53

[Matt Brubeck (:mbrubeck)]

(In reply to Raymond Lee [:raymondlee] from comment #52)
> Matt:  I think metro might also suffer the same issue as bug 921944 about
> the promptForSaveToFile().  Probably we need to revert the code it
> browser/metro/components/HelperAppDialog.js.  Could you help to check that
> please?

Yes, it looks like this broke in Metro too (bug 922075).

Comment 54

[Matt Brubeck (:mbrubeck)]

Backed out the changes to browser/metro/components/HelperAppDialog.js:
https://hg.mozilla.org/integration/fx-team/rev/20e8d2ba4e5e

Comment 55

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #46)
> Created attachment 810875 [details] [diff] [review]
> /toolkit/content/ v6
> 
> (In reply to :Paolo Amadini from comment #45)
> > Comment on attachment 810344 [details] [diff] [review]
> > /toolkit/content/ v5
> > 
> > Review of attachment 810344 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/content/contentAreaUtils.js
> > @@ +17,5 @@
> > > +                                  "resource://gre/modules/commonjs/sdk/core/promise.js");
> > > +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> > > +                                  "resource://gre/modules/Services.jsm");
> > > +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> > > +                                  "resource://gre/modules/Task.jsm");
> > 
> > Hm, sorry to ask only now, but what happens if you do the same call to
> > defineLazyModuleGetter twice? Does that work correctly?
> 
> defineLazyModuleGetter -> defineLazyGetter which takes care of that.
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/XPCOMUtils.
> jsm#176
> 
> > 
> > This module is imported in several types of windows, and I'd like to make
> > sure it works correctly in them even if other files import the same modules
> > in the same global scope.
> > 
> > @@ +559,4 @@
> > >  {
> > > +  return Task.spawn(function() {
> > > +    var gDownloadLastDir = new DownloadLastDir(window);
> > > +    var prefs = Services.prefs.getBranch("browser.download.");
> > 
> > I think we can rename this to "downloadPrefs", "prefBranch" or something
> > similar to avoid confusion.
> > 
> > nit: use "let" throughout the function.
> > nit: the "g" prefix can be removed.
> 
> Updated.
> 
> > 
> > @@ +583,5 @@
> > > +    let deferred = Promise.defer();
> > > +    if (useDownloadDir) {
> > > +      // Keep async behavior in both branches
> > > +      Services.tm.mainThread.dispatch(function() {
> > > +        deferred.resolve();
> > 
> > nit: deferred.resolve(null);
> 
> Updated
> 
> > 
> > @@ +601,3 @@
> > >  
> > > +    function displayPicker() {
> > > +      return Task.spawn(function() {
> > 
> > No need for the nested function here.
> 
> Removed.
> 
> > 
> > @@ +641,5 @@
> > > +          prefs.setIntPref("save_converter_index", fp.filterIndex);
> > > +
> > > +        // Do not store the last save directory as a pref inside the private browsing mode
> > > +        var directory = fp.file.parent.QueryInterface(nsIFile);
> > > +        gDownloadLastDir.setFile(aRelatedURI, directory);
> > 
> > I think this QueryInterface call is a leftover from when we had the separate
> > nsILocalFile interface. Can just be:
> > 
> > gDownloadLastDir.setFile(aRelatedURI, fp.file.parent);
> > 
> > So, the nsIFile constant used earlier can go away (just use
> > Components.interfaces.nsIFile).
> 
> Done

@Paolo: review ping.

Comment 56

[:Paolo Amadini]

Comment on attachment 810875 [details] [diff] [review]
/toolkit/content/ v6

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

This should be updated on top of bug 926736, I've made some observations in the meantime.

::: toolkit/content/contentAreaUtils.js
@@ +323,5 @@
>  
>      // Find a URI to use for determining last-downloaded-to directory
>      let relatedURI = aReferrer || sourceURI;
>  
> +    promiseTargetFile(fpParams, aSkipPrompt, relatedURI).then(aDialogCancelled => {

I think the function's resolution value should be inverted, like aDialogAccepted rather than aDialogCancelled.

@@ +600,2 @@
>  
> +    function displayPicker() {

"function displayPicker()" should be removed entirely.

Comment 57

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v7

(In reply to :Paolo Amadini from comment #56)
> Comment on attachment 810875 [details] [diff] [review]
> /toolkit/content/ v6
> 
> Review of attachment 810875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should be updated on top of bug 926736, I've made some observations in
> the meantime.
> 

Done.

> ::: toolkit/content/contentAreaUtils.js
> @@ +323,5 @@
> >  
> >      // Find a URI to use for determining last-downloaded-to directory
> >      let relatedURI = aReferrer || sourceURI;
> >  
> > +    promiseTargetFile(fpParams, aSkipPrompt, relatedURI).then(aDialogCancelled => {
> 
> I think the function's resolution value should be inverted, like
> aDialogAccepted rather than aDialogCancelled.
> 

Updated.

> @@ +600,2 @@
> >  
> > +    function displayPicker() {
> 
> "function displayPicker()" should be removed entirely.

Removed.

Comment 58

[:Paolo Amadini]

Raymond, can you clarify which patches have been backed out by updating the "checkin" flag, and if there is any pending work needed to re-land them?

Comment 59

[Raymond Lee [:raymondlee]]

The patches for /browser/metro/ and /mobile/android/ still have some pending work.  Marcos is looking into them.  He can give more details.

Comment 60

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #59)
> The patches for /browser/metro/ and /mobile/android/ still have some pending
> work.  Marcos is looking into them.  He can give more details.

promptForSaveToFile() in mobile/android/components/HelperAppDialog.js, and promptForSaveToFile() in browser/metro/components/HelperAppDialog.js use dnldMgr.userDownloadsDirectory.

However, the nsIHelperAppLauncherDialog.idl has nsIFile promptForSaveToFile(). Therefore, we can't simply change promptForSaveToFile() to return a Promise in both mobile/android/ browser/metro/

marcos: please give more details how you are going to tackle this.  Thanks!

Comment 61

[Raymond Lee [:raymondlee]]

Reassign to Marcos as he is working on a solution to tackle the remaining issues

Comment 62

[Raymond Lee [:raymondlee]]

Attachment: /toolkit/content/ v8 r+

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=226b71278b30

Comment 63

[Marcos Aruj]

Hi. I'm fixing the offending patches by using an event loop to handle the async requests.

Comment 64

[Raymond Lee [:raymondlee]]

Comment on attachment 822143 [details] [diff] [review]
/toolkit/content/ v8 r+

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

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ea38d8148ff4

Comment 66

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea38d8148ff4

Comment 67

[:Paolo Amadini]

(In reply to Marcos Aruj from comment #63)
> Hi. I'm fixing the offending patches by using an event loop to handle the
> async requests.

We try to avoid nested event loops as much as possible. We have implemented a function called promptForSaveToFileAsync that can be used for returning the file path in this case.

Since we're now approaching the Aurora migration, I'll mark the bug as resolved for the Firefox 27 milestone, with the work that has been already done, and move the pending patches to separate new bugs that are specific to Android and Metro.