Closed Bug 875648 Opened 8 years ago Closed 6 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla37
Iteration:
37.3 - 12 Jan

People

(Reporter: raymondlee, Assigned: Paolo)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Blocks: jsdownloads
Summary: Switch to use new JS Downloads.getSystemDownloadsDirectory → Replace callers of nsIDownloadManager.defaultDownloadsDirectory with Downloads.getSystemDownloadsDirectory
Blocks: 851471
No longer blocks: jsdownloads
Assignee: nobody → marcos
Any updates on this one Marcos?
Flags: needinfo?(marcos)
Hi guys,

I'm still finishing this but I expect to have some patches ready for review between Tomorrow and Wednesday.
Flags: needinfo?(marcos)
You can split the patch for different modules like bug 875731
Attached patch WIP (obsolete) — Splinter Review
Attachment #818164 - Flags: feedback?(raymond)
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
Attachment #818164 - Flags: feedback?(raymond)
We are going to change the return value of Downloads.getSystemDownloadsDirectory. See bug 926736 and that patch is likely to be landed first
Depends on: 926736
I suggest moving the Android-specific parts of the patch to bug 931776 so that the changes can be tested together.
Summary: Replace callers of nsIDownloadManager.defaultDownloadsDirectory with Downloads.getSystemDownloadsDirectory → Use Downloads.jsm functions to get download directories in Firefox for Desktop
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
Attached patch Bug-875648.patch (obsolete) — Splinter Review
WIP missing some minor changes
Attachment #818164 - Attachment is obsolete: true
Attachment #823801 - Flags: review?(paolo.mozmail)
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 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.
Attachment #823801 - Flags: review?(paolo.mozmail)
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.
Hi Paolo. I'm still debugging and fixing some issues that I get when running all tests. I'll post a patch ASAP.
Attached patch Bug-875648.patch (obsolete) — Splinter Review
New patch. Tests run successfully. Thanks for reviewing.
Attachment #823801 - Attachment is obsolete: true
Attachment #826218 - Flags: review?(paolo.mozmail)
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.
Attachment #826218 - Flags: review?(paolo.mozmail)
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?
Flags: needinfo?(paolo.mozmail)
I think that we don't need to change platform code, I'd go for something like option 1 here.
Flags: needinfo?(paolo.mozmail)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=5
Assignee: marcos → nobody
Points: --- → 5
Flags: qe-verify?
Whiteboard: p=5
Depends on: 1112088
Attached patch The patch (obsolete) — Splinter Review
This updates the preferences code in the front-end.

Some code that was just not needed is removed in the process.
Assignee: nobody → paolo.mozmail
Attachment #826218 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8540157 - Flags: review?(jaws)
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.
Iteration: --- → 37.2
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).
Flags: needinfo?(paolo.mozmail)
Iteration: 37.2 → 37.3
(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.
Flags: needinfo?(paolo.mozmail)
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?
Attached patch Updated patchSplinter Review
(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.
Attachment #8540157 - Attachment is obsolete: true
Attachment #8540157 - Flags: review?(jaws)
Attachment #8541250 - Flags: review?(jaws)
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
Attachment #8541250 - Flags: review?(jaws) → review+
(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?
(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.
(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?
Yep, this is broken on Aurora when in-content prefs are disabled:
`Cu is not defined main.js:47:0`

Filed bug 1115791.
https://hg.mozilla.org/mozilla-central/rev/4e494ede6c2d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.