875648 - Use Downloads.jsm functions to get download directories in Firefox for Desktop

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Raymond Lee [:raymondlee]]

Few places are using the defaultDownloadsDirectory specified in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsIDownloadManager.idl

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

Comment 1

[Raymond Lee [:raymondlee]]

Any updates on this one Marcos?

Comment 2

[Marcos Aruj]

Hi guys,

I'm still finishing this but I expect to have some patches ready for review between Tomorrow and Wednesday.

Comment 3

[Raymond Lee [:raymondlee]]

You can split the patch for different modules like bug 875731

Comment 4

[Marcos Aruj]

Attachment: WIP

Comment 5

[Raymond Lee [:raymondlee]]

Comment on attachment 818164 [details] [diff] [review]
WIP

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

::: browser/components/preferences/in-content/main.js
@@ +287,5 @@
>      let bundlePreferences = document.getElementById("bundlePreferences");
>      let title = bundlePreferences.getString("chooseDownloadFolderTitle");
>      let folderListPref = document.getElementById("browser.download.folderList");
> +    let currentDirPref = yield this._indexToFolder(folderListPref.value); // file
> +    let defDownloads = yield this._indexToFolder(1); // file

You need to ensure that this method handle the returned Promise

@@ +296,5 @@
>          let file = fp.file.QueryInterface(nsILocalFile);
>          let downloadDirPref = document.getElementById("browser.download.dir");
>  
>          downloadDirPref.value = file;
> +        folderListPref.value = yield this._folderToIndex(file);

Same as above

@@ +314,5 @@
>      else if (defDownloads && defDownloads.exists()) {
>        fp.displayDirectory = defDownloads;
>      } // Fall back to Desktop
>      else {
> +      fp.displayDirectory = yield this._indexToFolder(0);

Same as above

@@ +354,5 @@
>        // With 3.0, a new desktop folder - 'Downloads' was introduced for
>        // platforms and versions that don't support a default system downloads
>        // folder. See nsDownloadManager for details.
>        downloadFolder.label = bundlePreferences.getString("downloadsFolderName");
> +      iconUrlSpec = fph.getURLSpecFromFile(yield this._indexToFolder(1));

Same as above

::: browser/components/preferences/main.js
@@ +380,5 @@
> +          return fileLoc.get("Desk", Components.interfaces.nsILocalFile);
> +        break;
> +        case "Downloads":
> +          let downloadDir = yield Downloads.getSystemDownloadsDirectory();
> +          return

Not complete?

::: mobile/android/chrome/content/browser.js
@@ +934,1 @@
>        Services.obs.notifyObservers(null, "browser-lastwindow-close-granted", null);

Please split mobile/android into separate patch and also because each module is going to be reviewed by different reviewer

::: mobile/android/components/DirectoryProvider.js
@@ +79,5 @@
> +      // We are retuning null to show failure instead for throwing an error. The
> +      // interface is called quite a bit and throwing an error is noisy. Returning
> +      // null works with the way the interface is called [see bug 529077]
> +      throw new Task.Result(null);
> +    }.bind(this));

Please ask Paolo about this because nsIDirectoryServiceProvider only provides interface for returninng nsIFile, not Promise. 

nsIFile getFile(in string prop, out boolean persistent);

http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIDirectoryService.idl#18

Comment 6

[Raymond Lee [:raymondlee]]

We are going to change the return value of Downloads.getSystemDownloadsDirectory. See bug 926736 and that patch is likely to be landed first

Comment 7

[:Paolo Amadini]

I suggest moving the Android-specific parts of the patch to bug 931776 so that the changes can be tested together.

Comment 8

[Raymond Lee [:raymondlee]]

Please also ensure that the patch covered this.  It uses dm.userDownloadsDirectory instead of Downloads.getPreferredDownloadsDirectory.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_destinationURI_annotation.xul#131

Comment 9

[Marcos Aruj]

Attachment: Bug-875648.patch

WIP missing some minor changes

Comment 10

[Marcos Aruj]

I'll upload a new patch in the morning with changes to use dm.userDownloadsDirectory and final changes for the new return value from bug 926736

Comment 11

[:Paolo Amadini]

Comment on attachment 823801 [details] [diff] [review]
Bug-875648.patch

I assume that at least part of this patch hasn't been tested, please ask for review only after all the code paths have been exercised at least locally, and all regression tests pass. I still recommend removing the Android portion to move faster here.

In case you can't build and test on Mac, we should probably just hand off the work to someone with access to the platform, like we're doing for the Android and Metro parts.

Comment 12

[Marcos Aruj]

Hi Paolo. Sorry. Actually I didn't noticed that the Android part was still there. I was working on that for the other Android specific bug. I'll post a tested and clean patch in a few moments.

Comment 13

[Marcos Aruj]

Hi Paolo. I'm still debugging and fixing some issues that I get when running all tests. I'll post a patch ASAP.

Comment 14

[Marcos Aruj]

Attachment: Bug-875648.patch

New patch. Tests run successfully. Thanks for reviewing.

Comment 15

[:Paolo Amadini]

Comment on attachment 826218 [details] [diff] [review]
Bug-875648.patch

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

::: browser/components/migration/src/SafariProfileMigrator.js
@@ +380,5 @@
>            function(webkitVal) webkitVal == 0 ? 0 : webkitVal == 1 ? 2 : 1);
>  #endif
>  
>          this._migrateFontSettings();
> +        yield this._migrateDownloadsFolder();

At first glance it does not seem that this function is a Task, so I don't expect the "yield" call to work here. Do you have a way to test manually that the Safari migrator did the right thing? It's possible that regression tests didn't cover this code path.

::: browser/components/preferences/in-content/main.js
@@ +460,5 @@
>     * Returns the value for the browser.download.folderList preference.
>     */
>    getFolderListPref: function ()
>    {
> +    return Task.spawn(function() {

It looks like this function is a callback specified in the "onsynctopreference" attribute, so I'm not sure it can work correctly if converted to a task.

This may be another case where the regression tests didn't detect whether the preference was actually saved. Can you test manually that the preference is preserved after the window is closed and reopened?

Since there are many indentation changes here, to make the review simpler, please attach a "-w" version of the patch with whitespace changes ignored for the next iteration.

Comment 16

[Marcos Aruj]

Hi Paolo,

We've tested the Safari Profile Migrator feature and everything seems to work correctly. No errors present, and bookmarks are imported correctly.

Regarding the preference, it is not preserved. I've been looking and asking for a possible solution to this. I got this two possibilities:

1. Remove the onsynctopreference attribute from http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/main.xul#126 and use an onchange attribute or something instead.

2. Add promise support to the onsynctopreference code in preferences.xml http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#464

What do you suggest?

Comment 17

[:Paolo Amadini]

I think that we don't need to change platform code, I'd go for something like option 1 here.

Comment 18

[:Paolo Amadini]

Attachment: The patch

This updates the preferences code in the front-end.

Some code that was just not needed is removed in the process.

Comment 19

[:Paolo Amadini]

I tested the code both for in-content and in-window preferences, with and without instantApply.

There are no automated tests for this bug, but I think adding them is out of scope for this conversion, whose purpose it to enable us to remove the legacy nsIDownloadManager interface from Desktop builds and eventually from mozilla-central.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

What do you think our plan should be for getting automated tests on these? This type of refactoring does seem like a good time to me (make sure that the tests pass before the change, and that they continue to pass after the change).

Comment 21

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> What do you think our plan should be for getting automated tests on these?

My opinion is that creating and maintaining a suite of tests for one feature, when a good one doesn't exist already, should be handled in a more structured way. I find it much more efficient to look at how an entire suite works, and tackle it as a whole, rather than copy and pasting possibly old and intermittent test code.

While I haven't really investigated the situation in depth here, in virtually all other cases where I looked at adding an individual test to a suite that has been unmaintained for some time, I uncovered so many bugs and duplicate code that I had to rewrite not only the test but also the test suite itself.

Adding test coverage here is better handled as a separate project. It could be a good candidate for someone looking into moving from working on simple bugs to more complicated code. I'd be available to mentor a contributor, which will take more time than doing the work myself, but will be time better spent.

By the way, I think we should do this for in-content and not in-window preferences.

> This type of refactoring does seem like a good time to me (make sure that
> the tests pass before the change, and that they continue to pass after the
> change).

In this case, if a good test existed, it would have to be adapted anyways, because the new code operates on preference elements asynchronously.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8540157 [details] [diff] [review]
The patch

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

::: browser/components/preferences/in-content/main.js
@@ -535,4 @@
>      } else {
>        // 'Desktop'
>        downloadFolder.label = bundlePreferences.getString("desktopFolderName");
> -      iconUrlSpec = fph.getURLSpecFromFile(desk);

Wow, so `desk` was undeclared before?!

@@ +571,5 @@
>        case "Desktop":
>          var fileLoc = Components.classes["@mozilla.org/file/directory_service;1"]
>                                      .getService(Components.interfaces.nsIProperties);
>          return fileLoc.get("Desk", Components.interfaces.nsILocalFile);
>        break;

Might as well remove this `break;` and the `break;` below since the returns above makes them unreachable.

::: browser/components/preferences/in-content/main.xul
@@ -223,5 @@
>                   preference="browser.download.folderList"
>                   preference-editable="true"
>                   aria-labelledby="saveTo"
> -                 onsyncfrompreference="return gMainPane.displayDownloadDirPref();"
> -                 onsynctopreference="return gMainPane.getFolderListPref()"/>

https://bugzilla.mozilla.org/show_bug.cgi?id=875648#c17 mentioned using an onchange attribute here. Do you still plan on doing that?

Comment 23

[:Paolo Amadini]

Attachment: Updated patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> >      } else {
> >        // 'Desktop'
> >        downloadFolder.label = bundlePreferences.getString("desktopFolderName");
> > -      iconUrlSpec = fph.getURLSpecFromFile(desk);
> 
> Wow, so `desk` was undeclared before?!

Surprisingly so, I saw the error in the Browser Console.

> > -                 onsyncfrompreference="return gMainPane.displayDownloadDirPref();"
> > -                 onsynctopreference="return gMainPane.getFolderListPref()"/>
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=875648#c17 mentioned using an
> onchange attribute here. Do you still plan on doing that?

When looking into this, it turned out this wasn't necessary - the only way these controls are updated is through the dialog code, that already changes the preference elements directly.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8541250 [details] [diff] [review]
Updated patch

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

r+ if you fix the Cu and Ci references below. I tested that it works with them fixed.

::: browser/components/preferences/main.js
@@ +331,5 @@
>     * Displays a file picker in which the user can choose the location where
>     * downloads are automatically saved, updating preferences and UI in
>     * response to the choice, if one is made.
>     */
> +  chooseFolder() this.chooseFolderTask().catch(Cu.reportError),

ReferenceError: Cu is not defined

@@ +343,2 @@
>      let fp = Components.classes["@mozilla.org/filepicker;1"].
> +             createInstance(Ci.nsIFilePicker);

ReferenceError: Ci is not defined
here and below in this function.

@@ +375,5 @@
>     * preferences.
>     */
> +  displayDownloadDirPref()
> +  {
> +    this.displayDownloadDirPrefTask().catch(Cu.reportError);

Cu is not defined

Comment 25

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> >      let fp = Components.classes["@mozilla.org/filepicker;1"].
> > +             createInstance(Ci.nsIFilePicker);
> 
> ReferenceError: Ci is not defined

Uh? Works fine on my machine - in fact the shorthands are defined in a shared file. I'm testing on Mac OS X - maybe there is a platform difference? macBrowserOverlay.xul?

In-content preferences define the shorthands explicitly, but maybe it's worth making the change in both places if I need to fix in-window preferences?

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Paolo Amadini from comment #25)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> > >      let fp = Components.classes["@mozilla.org/filepicker;1"].
> > > +             createInstance(Ci.nsIFilePicker);
> > 
> > ReferenceError: Ci is not defined
> 
> Uh? Works fine on my machine - in fact the shorthands are defined in a
> shared file. I'm testing on Mac OS X - maybe there is a platform difference?
> macBrowserOverlay.xul?
> 
> In-content preferences define the shorthands explicitly, but maybe it's
> worth making the change in both places if I need to fix in-window
> preferences?

Yeah it was weird to me too. I even added the defines at the top of main.js and it still had the issue for the windowed preferences.

Comment 27

[:Paolo Amadini]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #26)
> Yeah it was weird to me too. I even added the defines at the top of main.js
> and it still had the issue for the windowed preferences.

Now I'm confused. I found uses of the shorthands in main.js even for in-window preferences, in particular Cu.import("resource://gre/modules/osfile.jsm"). They're however in an #ifdef block:

#ifdef MOZ_DEV_EDITION
  separateProfileModeChange: function ()

Does this mean that whatever this function does, is broken on Windows and Linux, but only on Aurora? Can you confirm?

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Yep, this is broken on Aurora when in-content prefs are disabled:
`Cu is not defined main.js:47:0`

Filed bug 1115791.

Comment 29

[:Paolo Amadini]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6b0c577720f7

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

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/4e494ede6c2d