Closed Bug 875731 Opened 6 years ago Closed 6 years ago

Replace callers of nsIDownloadManager.usersDownloadsDirectory with Downloads.getPreferredDownloadsDirectory

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: raymondlee, Assigned: raymondlee)

References

Details

Attachments

(3 files, 23 obsolete files)

14.46 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
10.89 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
12.28 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
http://mxr.mozilla.org/mozilla-central/ident?i=GetUserDownloadsDirectory

We should switch those consumers to use the new JS Downloads.getUserDownloadsDirectory.
Blocks: 851471
No longer blocks: jsdownloads
Assignee: nobody → raymond
Status: NEW → ASSIGNED
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.
Flags: needinfo?(paolo.mozmail)
In general, the best way is to file a separate file for each component that requires a different reviewer.
Flags: needinfo?(paolo.mozmail)
Note that we'll rename the function in bug 917217.
Depends on: 917217
Attached patch /mobile/andriod (obsolete) — Splinter Review
Attachment #806530 - Flags: review?(mark.finkle)
Attached patch /toolkit/content/ (obsolete) — Splinter Review
Attachment #806531 - Flags: review?(paolo.mozmail)
Attached patch /browser/devtools/ (obsolete) — Splinter Review
Attachment #806532 - Flags: review?(dcamp)
Attached patch /browser/metro/ (obsolete) — Splinter Review
Attachment #806533 - Flags: review?(mbrubeck)
Attached patch toolkit/mozapps/ (obsolete) — Splinter Review
Attachment #806534 - Flags: review?(dtownsend+bugmail)
Attached patch /toolkit/mozapps (obsolete) — Splinter Review
Attachment #806534 - Attachment is obsolete: true
Attachment #806534 - Flags: review?(dtownsend+bugmail)
Attachment #806537 - Flags: review?(dtownsend+bugmail)
Attachment #806537 - Attachment description: /toolkit/ → /toolkit/mozapps
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.
Attachment #806533 - Flags: review?(mbrubeck) → review-
Comment on attachment 806537 [details] [diff] [review]
/toolkit/mozapps

Paolo can take this
Attachment #806537 - Flags: review?(dtownsend+bugmail) → review?(paolo.mozmail)
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?
Attachment #806532 - Flags: review?(dcamp) → review?(jwalker)
Attached patch /browser/metro/ v2 (obsolete) — Splinter Review
Matt: thanks for your feedback.  Please review this updated patch.
Attachment #806533 - Attachment is obsolete: true
Attachment #807062 - Flags: review?(mbrubeck)
(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 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.
Attachment #806532 - Flags: review?(jwalker) → review+
Attachment #806530 - Flags: review?(mark.finkle) → review?(wjohnston)
Comment on attachment 807062 [details] [diff] [review]
/browser/metro/ v2

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

Thanks!
Attachment #807062 - Flags: review?(mbrubeck) → review+
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
Attached patch /browser/devtools/ v2 r+ (obsolete) — Splinter Review
(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 :-)
Attachment #806532 - Attachment is obsolete: true
Attached patch /browser/metro/ v3 r+ (obsolete) — Splinter Review
(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
Attachment #807062 - Attachment is obsolete: true
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.
Attachment #806531 - Flags: review?(paolo.mozmail)
Comment on attachment 806537 [details] [diff] [review]
/toolkit/mozapps

May you please attach a second patch with whitespace changes ignored, for easier review?
Attached patch /browser/metro/ v4 r+ (obsolete) — Splinter Review
Updated to use getPreferredDownloadsDirectory instead.  Should wait for patch in bug 917217 to land first.
Attachment #808414 - Attachment is obsolete: true
Updated to use getPreferredDownloadsDirectory instead.  Should wait for patch in bug 917217 to land first.
Attachment #808408 - Attachment is obsolete: true
Attached patch /toolkit/mozapps/ -w v2 (obsolete) — Splinter Review
(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.
Attachment #806537 - Attachment is obsolete: true
Attachment #806537 - Flags: review?(paolo.mozmail)
Attached patch /toolkit/mozapps v2 (obsolete) — Splinter Review
Please see the -w version
Attachment #809019 - Flags: review?(paolo.mozmail)
Attached patch /mobile/andriod v2 (obsolete) — Splinter Review
Updated to use getPreferredDownloadsDirectory instead.
Attachment #806530 - Attachment is obsolete: true
Attachment #806530 - Flags: review?(wjohnston)
Attachment #809021 - Flags: review?(mark.finkle)
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 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);"
Attachment #809019 - Flags: review?(paolo.mozmail) → review+
Attached patch /toolkit/content/ v2 (obsolete) — Splinter Review
Attachment #806531 - Attachment is obsolete: true
Attachment #809065 - Flags: review?(paolo.mozmail)
Attached patch /toolkit/content/ v3 (obsolete) — Splinter Review
Attachment #809065 - Attachment is obsolete: true
Attachment #809065 - Flags: review?(paolo.mozmail)
Attachment #809067 - Flags: review?(paolo.mozmail)
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.
Attachment #809067 - Flags: review?(paolo.mozmail)
Attached patch /toolkit/mozapps/ v3 r+ (obsolete) — Splinter Review
(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.
Attachment #809018 - Attachment is obsolete: true
Attachment #809019 - Attachment is obsolete: true
Comment on attachment 809015 [details] [diff] [review]
/browser/devtools/ v3 r+

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=caf98cacc627
Attachment #809015 - Flags: checkin?
This last patch failed try.  Updated and pushed to try again.
Attachment #809082 - Attachment is obsolete: true
Attached patch /toolkit/content/ v4 (obsolete) — Splinter Review
(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.
Attachment #809808 - Flags: review?(paolo.mozmail)
Attached patch /toolkit/content/ v4 -w (obsolete) — Splinter Review
Without white spaces version
Attachment #809067 - Attachment is obsolete: true
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()".
Attachment #809808 - Flags: review?(paolo.mozmail)
Attachment #808985 - Flags: checkin? → checkin+
Attachment #809015 - Flags: checkin? → checkin+
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][leave open]
Attachment #809669 - Flags: checkin? → checkin+
Attached patch /toolkit/content/ v5 (obsolete) — Splinter Review
(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.
Attachment #809808 - Attachment is obsolete: true
Attached patch /toolkit/content/ v5 -w (obsolete) — Splinter Review
Attachment #809809 - Attachment is obsolete: true
Attachment #810344 - Flags: review?(paolo.mozmail)
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).
Attachment #810344 - Flags: review?(paolo.mozmail)
Attachment #809021 - Flags: review?(mark.finkle) → review+
Attached patch /toolkit/content/ v6 (obsolete) — Splinter Review
(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
Attachment #810344 - Attachment is obsolete: true
Attachment #810345 - Attachment is obsolete: true
Attachment #810875 - Flags: review?(paolo.mozmail)
Attachment #809021 - Attachment is obsolete: true
Attachment #811011 - Attachment description: /mobile/andriod v3 → /mobile/andriod v3 r+
Attachment #811011 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/41701d2c0341
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/41701d2c0341
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Depends on: 921944
This supposedly broke the ability to initiate a download in Firefox for Android. Please see dependancy above.
(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
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?
Flags: needinfo?(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).
Flags: needinfo?(mbrubeck)
Blocks: 922075
Backed out the changes to browser/metro/components/HelperAppDialog.js:
https://hg.mozilla.org/integration/fx-team/rev/20e8d2ba4e5e
(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.
Flags: needinfo?(paolo.mozmail)
Depends on: 923940
Summary: Replace callers of nsIDownloadManager.usersDownloadsDirectory with Downloads.getUserDownloadsDirectory → Replace callers of nsIDownloadManager.usersDownloadsDirectory with Downloads.getPreferredDownloadsDirectory
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.
Attachment #810875 - Flags: review?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
Depends on: 926736
Attached patch /toolkit/content/ v7 (obsolete) — Splinter Review
(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.
Attachment #810875 - Attachment is obsolete: true
Attachment #819608 - Flags: review?(paolo.mozmail)
Attachment #819608 - Flags: review?(paolo.mozmail) → review+
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?
Attachment #808985 - Flags: checkin+
Attachment #811011 - Flags: checkin+
The patches for /browser/metro/ and /mobile/android/ still have some pending work.  Marcos is looking into them.  He can give more details.
Flags: needinfo?(marcos)
(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!
Depends on: 929355
Reassign to Marcos as he is working on a solution to tackle the remaining issues
Assignee: raymond → marcos
Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=226b71278b30
Attachment #819608 - Attachment is obsolete: true
Hi. I'm fixing the offending patches by using an event loop to handle the async requests.
Flags: needinfo?(marcos)
Attachment #822143 - Flags: checkin? → checkin+
(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.
Assignee: marcos → raymond
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla27
Blocks: 931776
Blocks: 931777
Attachment #808985 - Attachment is obsolete: true
Attachment #811011 - Attachment is obsolete: true
Depends on: 958899
Depends on: 969253
Duplicate of this bug: 845019
You need to log in before you can comment on or make changes to this bug.