We need to download files either on the internal storage if any or to the sd card.

We don't plan to prompt the user for now, so the js component just makes sure that we start the download with a unique file name, like file.txt, file(1).txt, etc.

Hi Fabrice, sorry for my ignorance here, if we don't have the file downloaded in the sdcard. Should we use the file api directly to find the file in the internal storage? Thanks!

nsIDownloadManager.userDownloadsDirectory has been already replaced by Downloads.getPreferredDownloadsDirectory, we're waiting for the patches in bug 875648 and bug 931776 to land before removing the function completely. You can use Desktop's code as reference here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#194 The nsExternalHelperAppService code still needs to be changed to mirror the DownloadIntegration.jsm function synchronously, unfortunately.

Paolo, when do you plan to land bug 875648 and bug 931776 ?

Updated patch, in which I changed DownloadIntegration.jsm at Paolo's request. I had to keep the c++ version in nsExternalHelperAppService.cpp for Dave's review pleasure (and because the code is actually called).

Comment on attachment 829617 [details] [diff] [review] downloads.patch Review of attachment 829617 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/HelperAppDialog.js @@ +57,5 @@ > + } catch(e) { } > + if (file) { > + aLauncher.saveDestinationAvailable(file); > + } > + }.bind(this)); You should ensure aLauncher.saveDestinationAvailable is always called, with a null argument in case an error occurs. ::: toolkit/components/downloads/Makefile.in @@ +7,5 @@ > > CXXFLAGS += $(TK_CFLAGS) -DGOOGLE_PROTOBUF_NO_RTTI > > LOCAL_INCLUDES += \ > + -I$(topsrcdir)/dom/base \ Is this still needed? ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +229,5 @@ > }.bind(this)); > }, > > +#ifdef MOZ_WIDGET_GONK > + _getDefaultStoragePath: function() { Hm, the name of this function does not make it clear what it returns (a download directory, or root directory, for example). Please use a more informative name, and add a JavaDoc-style comment to the function to describe what it does and the resolution value, similar to the other functions in the module. @@ +233,5 @@ > + _getDefaultStoragePath: function() { > + return Task.spawn(function() { > + let directoryPath; > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + let storages = win.navigator.getDeviceStorages("sdcard"); This looks different from the C++ code, but the functions should behave identically. Is there a reason why we require a browser window to be available here, but not in the C++ version? It looks like the C++ version is simpler so the same logic should be used here if possible. @@ +313,5 @@ > } else { > directoryPath = this._getDirectory("DfltDwnld"); > } > #elifdef XP_UNIX > +#ifdef MOZ_WIDGET_ANDROID Can the rename from ANDROID to MOZ_WIDGET_ANDROID have an effect here? I remember they're different, but not what is the difference exactly. @@ +323,5 @@ > Cr.NS_ERROR_FILE_UNRECOGNIZED_PATH); > } > +#elifdef MOZ_WIDGET_GONK > + // Also use the sdcard. > + directoryPath = this._getDefaultStoragePath(); nit: no comment needed here (assuming the function name is informative).

Also, please ensure that the xpcshell tests that run on B2G are updated to take into account the new logic here.

(In reply to Fabrice Desré [:fabrice] from comment #4) > Paolo, when do you plan to land bug 875648 and bug 931776 ? As soon as we can get the patches there updated, tested and reviewed, which is a work that is currently active. That may range from days to a couple weeks depending on the schedule of the people involved.

(In reply to :Paolo Amadini from comment #6) > This looks different from the C++ code, but the functions should behave > identically. Sorry, ignore this, the C++ function should mirror the getTemporaryDownloadDirectory function that is currently unused, and thus wasn't actually changed. To avoid regressing the B2G behavior when we switch the C++ code to use getTemporaryDownloadDirectory, can you update that function too, or alternatively, if you don't have time now, file a bug blocking bug 929391 to ensure we don't forget to do that? > Is there a reason why we require a browser window to be > available here, but not in the C++ version? This question still remains, however.

Comment on attachment 829617 [details] [diff] [review] downloads.patch Review of attachment 829617 [details] [diff] [review]: ----------------------------------------------------------------- I added my review on the nsExternalHelperAppService.cpp file. Mostly nits, but my very last comment would replace 23 lines of the code with 3 lines... So r+ with changes (assuming that if you make the change suggested by the last comment, that you actually test the changes :) ::: uriloader/exthandler/nsExternalHelperAppService.cpp @@ +117,5 @@ > #include "mozilla/ipc/URIUtils.h" > > +#ifdef MOZ_WIDGET_GONK > +#include "nsIVolumeService.h" > +#include "nsDeviceStorage.h" You only need "DeviceStorage.h" nsDeviceStorage.h pulls in a bunch of additional unnecessary stuff (I didn't create the header files so the names have never been very clear to me) @@ +339,5 @@ > + // available. > + nsCOMPtr<nsIVolumeService> vs = do_GetService(NS_VOLUMESERVICE_CONTRACTID); > + if (!vs) { > + return NS_ERROR_FAILURE; > + } You seem to be using NS_ENSURE_xxx below, why not NS_ENSURE_TRUE(vs, NS_ERROR_FAILURE) here? @@ +351,5 @@ > + storageName); > + > + if (storageName.IsEmpty()) { > + return NS_ERROR_FAILURE; > + } Similarly here @@ +357,5 @@ > + nsCOMPtr<nsIVolume> volume; > + vs->GetVolumeByName(storageName, getter_AddRefs(volume)); > + if (!volume) { > + return NS_ERROR_FAILURE; > + } and here @@ +364,5 @@ > + rv = volume->GetState(&state); > + NS_ENSURE_SUCCESS(rv, rv); > + if (state != nsIVolume::STATE_MOUNTED) { > + return NS_ERROR_FAILURE; > + } and here @@ +369,5 @@ > + nsString downloadDirPath; > + rv = volume->GetMountPoint(downloadDirPath); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(downloadDirPath), > + true, getter_AddRefs(dir)); Why not use NS_NewLocalFile and pass in downloadDirPath directly? And then call dir->Append(NS_LITERAL_STRING("downloads")) I guess I don't see the need to use the Native variants @@ +377,5 @@ > + bool alreadyThere; > + rv = dir->Exists(&alreadyThere); > + NS_ENSURE_SUCCESS(rv, rv); > + if (!alreadyThere) { > + rv = dir->Create(nsIFile::DIRECTORY_TYPE, 0700); This should at least 0770 otherwise sdcard_rw groups won't be able to write to the directory. Most of the time, it doesn't really matter, since the device storage is on FAT which doesn't have user/group/other permissions anyways. But for the cases where the volume happens to be mounted on something which does support it, we should use 0770 since the tyical permissions for an sdcard look like d---rw--- (i.e. only group write) @@ +379,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + if (!alreadyThere) { > + rv = dir->Create(nsIFile::DIRECTORY_TYPE, 0700); > + NS_ENSURE_SUCCESS(rv, rv); > + } So I think that a bunch (lines 353 thru 376) of this could be simplified by doing: DeviceStorageFile dsf(NS_LITERAL_STRING("sdcard"), storageName, NS_LITERLAL_STRING("downloads")); NS_ENSURE_TRUE(dsf.mFile, NS_ERROR_FAILURE); NS_ENSURE_TRUE(dsf.IsAvailable(), NS_ERROR_FAILURE); and then do the code that checks if the directory exists (using dsf.mFile instead of dir)

Addressing review comments, including: (In reply to :Paolo Amadini from comment #6) > @@ +233,5 @@ > > + _getDefaultStoragePath: function() { > > + return Task.spawn(function() { > > + let directoryPath; > > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > > + let storages = win.navigator.getDeviceStorages("sdcard"); > > This looks different from the C++ code, but the functions should behave > identically. Is there a reason why we require a browser window to be > available here, but not in the C++ version? > > It looks like the C++ version is simpler so the same logic should be used > here if possible. The c++ version looks simpler because it's using a helper that is not accessible from js. In js we need to access the navigator property since this is not generally exposed as an xpcom component hence the getMostRecentWindow() call. > @@ +313,5 @@ > > } else { > > directoryPath = this._getDirectory("DfltDwnld"); > > } > > #elifdef XP_UNIX > > +#ifdef MOZ_WIDGET_ANDROID > > Can the rename from ANDROID to MOZ_WIDGET_ANDROID have an effect here? I > remember they're different, but not what is the difference exactly. ANDROID is defined both on android and b2g devices. MOZ_WIDGET_ANDROID is only defined for firefox on android, and MOZ_WIDGET_GONK only on b2g devices (MOZ_B2G is the product define for b2g, set for both desktop and device builds).

Try build (all green!) with the relevant patches applied: https://tbpl.mozilla.org/?tree=Try&rev=a6a4e4ccbf4e

Comment on attachment 830334 [details] [diff] [review] downloads.patch Review of attachment 830334 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the changes below. ::: b2g/components/HelperAppDialog.js @@ +50,5 @@ > + Task.spawn(function() { > + let file = null; > + let defaultFolder = yield Downloads.getPreferredDownloadsDirectory(); > + try { > + let dir = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile); Move "let defaultFolder" inside the "try" block. @@ +55,5 @@ > + dir.initWithPath(defaultFolder); > + file = this.validateLeafName(dir, aDefaultFile, aSuggestedFileExt); > + } catch(e) { } > + aLauncher.saveDestinationAvailable(file); > + }.bind(this)); }.bind(this)).then(null, Cu.reportError); ...just in case aLauncher.saveDestinationAvailable throws an exception. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +241,5 @@ > + return Task.spawn(function() { > + let directoryPath; > + let win = Services.wm.getMostRecentWindow("navigator:browser"); > + let storages = win.navigator.getDeviceStorages("sdcard"); > + let preferedStorageName; "preferredStorageName" @@ +260,5 @@ > + directoryPath = OS.Path.join(volume.mountPoint, "downloads"); > + yield OS.File.makeDir(directoryPath, { ignoreExisting: true }); > + } > + } > + throw new Task.Result(directoryPath); This task should never return null or undefined, but should throw an exception if no target location is available.

(In reply to Fabrice Desré [:fabrice] from comment #12) > Try build (all green!) with the relevant patches applied: > https://tbpl.mozilla.org/?tree=Try&rev=a6a4e4ccbf4e It looks like B2G only ran a very small subset of all the available xpcshell tests. Can you file a bug to enable the jsdownloads unit tests on B2G, so that you can get to it also later, when you have the time? I think it's quite important to ensure those tests are enabled by the time the API consumers are released, so that we don't regress B2G accidentally with future platform changes.

(In reply to Fabrice Desré [:fabrice] from comment #15) > https://bugzilla.mozilla.org/show_bug.cgi?id=936342 I'm assuming you intended to post something else? Otherwise, you've created some kind of Bugzilla recursion...

ha... https://hg.mozilla.org/integration/b2g-inbound/rev/9e31c9c04ccf