The JavaScript API should provide methods to execute actions on the target file after it is downloaded. It should be possible to know the appropriate action to execute even if the download is canceled and then resumed in a different session.

I am not sure whether this is in the right direction.

Comment on attachment 760318 [details] [diff] [review] v1 So, here we need something different. To register functions to be executed in the current session when the download is finished, we have the "whenSucceeded" method. However, public downloads can be saved to disk and then reloaded in another session, so even this method will not be enough in our case. Based on the user choice, we have three possible cases to handle as soon as the download finishes: * No action (the use interface just shows a notification) * Launch with the default application for the download's MIME content type * Launch with a custom application chosen by the user In addition, after the download is finished, the user can: * Launch with the previously chosen application (default or custom) * Open the directory where the target file is located The old download manager uses nsIMIMEInfo to store the original user choice, as well as the target content type. Here, instead, we should use properties that are serializable to disk (boolean, string): * download.launchWhenSucceeded (boolean) This can be set manually, or automatically by a DownloadLegacySaver if the nsIMIMEInfo provided to it has its action set to "launch". * download.target.contentType (string) This should be set when we have the information from the download source, or when nsIMIMEInfo is provided to a DownloadLegacySaver. * download.target.launchWithExecutableName (string) This should be the application's executable name from nsIHandlerApp, that is the same information we currently save in "downloads.sqlite", or "null" to indicate that the default action has been selected. There should be methods to execute the actions on user request: * download.launch() This should do nothing if the download has not succeeded. This is called automatically if launchWhenSucceeded is true. * download.showContainingDirectory() You may see the Downloads Panel and Library code, for current implementations of the methods to launch a download and show its containing directory. You can see the current nsDownloadManager.cpp code for how nsIMIMEInfo is rebuilt from the serialized strings. This is a complex bug (we also need automated tests for launch, with the usual DownloadIntegration mockable helpers, verifying that the right functions are called in the right conditions) and is likely to require a few review passes before being ready to land. As always, let me know if you have any specific question.

This is a cleaned-up version of the main structure for the patch.. I have some extra messy parts in a separate patch but I wanted to post this to get feedback to see if I'm going in the right direction here and if this is what you had in mind. My main questions: - I wasn't sure where was the best place to call the "on succeeded" function (e.g. launch the file), so what I did was to add it to the chain of the deferSuccedded promise before it's resolved (which should also allow other handlers in the promise to do a different thing if they wish). But I don't know if this is the common way to do it in promise-land. - For the launch/showContainingFolder functions, there were already JS implementations in DownloadsCommon.js, but I suspect they are not meant to be used, right? So is the right thing to copy them over while modifying it for this codebase? - And, more generally, is this going in the right direction? Is it missing something obvious?

Comment on attachment 775717 [details] [diff] [review] WIP (In reply to :Felipe Gomes from comment #3) > This is a cleaned-up version of the main structure for the patch.. I have > some extra messy parts in a separate patch but I wanted to post this to get > feedback to see if I'm going in the right direction here and if this is what > you had in mind. This is a good start! As far as general structure goes, we'll need to have most, if not all, of the launch and open folder code in the DownloadIntegration module, to be able to separate the tests for the download logic from the operating system parts. For example, we should test that we try to launch the right application only on success and only when requested, without actually launching it. See the parental control blocking tests for an existing example. Also, the executable prompt should be implemented as a function in DownloadIntegration, but I think we may add it in a separate bug. > - I wasn't sure where was the best place to call the "on succeeded" function > (e.g. launch the file), so what I did was to add it to the chain of the > deferSuccedded promise before it's resolved (which should also allow other > handlers in the promise to do a different thing if they wish). But I don't > know if this is the common way to do it in promise-land. Handlers on one promise cannot have any effect on other handlers on the same promise. In this case, I think it's fine to just invoke the "launch" method directly inside the "if (this.succeeded) {", either before or after "this._deferSucceeded.resolve();". > - For the launch/showContainingFolder functions, there were already JS > implementations in DownloadsCommon.js, but I suspect they are not meant to > be used, right? So is the right thing to copy them over while modifying it > for this codebase? Yes, this is correct, in fact we'll do the other way around.

Yay I think this is ready for a first review pass \o/. I'm asking for feedback instead of review because there's still a couple of open questions (below), and I need to replace the "TODO doc" with actual documentation comments. This patch is already written on top of bug 851454 My questions are: - is it correct that for downloads, only the saveToDisk, useHelperApp and useSystemDefault are the cases from nsIMimeInfo that needs to be handled? i.e., alwaysAsk and handleInternally are not applicable here - when a custom helper app is selected, nsIMimeInfo provides us both with a name of the app and the path to the executable. It seems that I only make use of the path here, but I don't know if the name will also be used anywhere so I'm storing it too. To store both together I created a DownloadLauncher object to do serialization, but if the name is not needed to be kept it could all be simplified by just storing the path as a string in the Download obj. - I didn't include the confirmation prompt for executables in the launch code because thinking more about it, it seems it belongs somewhere else, closer to the UI code, because otherwise we'll need to pass a window parameter to it, like how DownloadsCommon needs it. - From the previous discussion: > Handlers on one promise cannot have any effect on other handlers on the same > promise. In this case, I think it's fine to just invoke the "launch" method > directly inside the "if (this.succeeded) {", either before or after > "this._deferSucceeded.resolve();". I was thinking that one handler could throw, which I think will mean that the next success handler won't be called, right? Or a handler (from an add-on for example) could at least set .launchWhenSuccedded = false to avoid the default launching mechanism. But maybe that's too far fetched, as an add-on would need to put handlers on every download's promise. So I made this change as you suggested. But with it I don't know now how to programatically test that the launchWhenSucceded flag is properly used.. Do you have any ideas? I've included in the tests the function calls for the important cases of directly calling download.launch() and download.showContainingDirectory().

(In reply to :Felipe Gomes from comment #5) > Yay I think this is ready for a first review pass \o/. Great! > - is it correct that for downloads, only the saveToDisk, useHelperApp and > useSystemDefault are the cases from nsIMimeInfo that needs to be handled? > i.e., alwaysAsk and handleInternally are not applicable here Yes. > - when a custom helper app is selected, nsIMimeInfo provides us both with a > name of the app and the path to the executable. It seems that I only make > use of the path here, but I don't know if the name will also be used > anywhere so I'm storing it too. To store both together I created a > DownloadLauncher object to do serialization, but if the name is not needed > to be kept it could all be simplified by just storing the path as a string > in the Download obj. The name does not have any function after the application selection phase, so we should go for the simplest string representation. > - I didn't include the confirmation prompt for executables in the launch > code because thinking more about it, it seems it belongs somewhere else, > closer to the UI code, because otherwise we'll need to pass a window > parameter to it, like how DownloadsCommon needs it. That's right, I've filed bug 896927 to handle that. > I was thinking that one handler could throw, which I think will mean that > the next success handler won't be called, right? Each handler only affects the promise returned by "then", that is a different one every time, and not other handlers on the promise where it was registered. I've added a note to the documentation of the "then" method here: https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#then() > Or a handler (from an > add-on for example) could at least set .launchWhenSuccedded = false to avoid > the default launching mechanism. But maybe that's too far fetched, as an > add-on would need to put handlers on every download's promise. If that turns out to be needed, we should design an API specifically for that. Setting properties during promise callbacks is not a viable solution because there's no control over when exactly a callback is executed. > So I made this change as you suggested. But with it I don't know now how to > programatically test that the launchWhenSucceded flag is properly used.. Do > you have any ideas? The DownloadIntegration functions, when testMode is true, should set one or more test properties on the module, based on the provided arguments or the actions they would perform in case test mode was disabled. The test should then check the state of these variables after the operation completed. Given that the file is launched after the download completes (so "yield download.start()" has already returned), probably the test should just define a deferred object and wait on its promise, and the module should resolve the promise when the function is called. Pseudo-code (with fake names): - Test: let DownloadIntegration._deferTestOpenFile = Promise.defer(); - Test: yield download.start(); - openFile: _deferTestOpenFile.promise.resolve({ var1: ..., var2: ... })"; - Test: let openFileResults = yield _deferTestOpenFile.promise; - Test: do_check_are_correct(openFileResults.var1 & openFileResults.var2); (In the future we'll use a proper mock object for DownloadIntegration, but checking testMode and using variables on the module is OK for now.)

Comment on attachment 779647 [details] [diff] [review] v1 Review of attachment 779647 [details] [diff] [review]: ----------------------------------------------------------------- The approach looks good! I've specified lots of general style guidelines on this first iteration. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +200,5 @@ > + > + /** > + * XXX todo doc > + */ > + launcher: null, "launcherPath" seems a good name for this property when it becomes a string. Thanks for coming up with that! Note that we'll have a round of general feedback about property and method names before the API is declared complete, but that will be after this bug is implemented. @@ +335,5 @@ > this._currentAttempt = null; > this.stopped = true; > this._notifyChange(); > if (this.succeeded) { > + this._finishedSuccessfully(); Can you just inline this? @@ +350,5 @@ > + _finishedSuccessfully: function D_finishedSuccessfully() { > + this._deferSucceeded.resolve(); > + > + if (this.launchWhenSucceeded) > + this.launch(); Please brace single-line blocks. Also, since "launch" returns a promise but we don't block on that, we should report errors to the console using ".then(null, Cu.reportError);". @@ +360,5 @@ > + launch: function D_launch() { > + if (!this.succeeded) > + return Promise.reject(this.error > + ? "There was an error with the download." > + : "Download has not finished yet."); The download can also have a "canceled" state, but I don't think we should use a different message based on the state. Also, for rejection reasons, please use Error objects: new Error("launch() can only be called if the download succeeded.") @@ +500,5 @@ > + serializable.launcher = this.launcher.toSerializable(); > + > + serializable.launchWhenSucceeded = this.launchWhenSucceeded > + ? "true" > + : "false"; On the serializable object, we may leave launchWhenSucceeded out when it is false in memory. When launchWhenSucceeded is true in memory, we should serialize it as a boolean type, not a string. @@ +552,5 @@ > + if ("launchWhenSucceeded" in aSerializable) { > + if (isString("launchWhenSucceeded")) > + download.launchWhenSucceeded = aSerializable.launchWhenSucceeded == "true"; > + else > + download.launchWhenSucceeded = !!aSerializable.launchWhenSucceeded; No need to check isString if this is expected to always be a boolean. @@ +557,5 @@ > + } > + > + if ("contentType" in aSerializable && > + isString(aSerializable.contentType)) > + download.contentType = aSerializable.contentType; Also here, we don't need to check isString because this is expected to always be a string. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +280,5 @@ > + * that was used to launch the process, if an specific application > + * was chosen. It can also resolve to "default-handler" or > + * "external-protocol-service" > + */ > + openFile: function DI_openFile(aDownload) { I don't think there's any value in knowing which handler was used _after_ launching the file, we should just resolve to undefined. (in test mode, we'll use a different promise for checking the outcome.) It may be worth specifying in the comment that the promise is resolved when the launch operation starts, but the operation will be completed later by the operating system. @@ +284,5 @@ > + openFile: function DI_openFile(aDownload) { > + let file = this._getNsIFile(aDownload.target.file); > + > + if (!file) > + return Promise.reject("Invalid file name."); aDownload.target.path is always a local file path, and never an URI, so the _getNsIFile helper is unneeded here. Finally we're getting rid of this legacy code! You may just call "new FileUtils.File(aDownload.target.path)" with no special error checking, and let exception handling do the rest. Also, please wrap the entire function in a Task like other functions do, so that exceptions are reported to the caller by rejecting the promise, instead of synchronously. @@ +287,5 @@ > + if (!file) > + return Promise.reject("Invalid file name."); > + > + let fileExtension = null, mimeInfo = null; > + let extensionRE = file.leafName.match(/\.([^.]+)$/); nit: "let match =" (since this is not a RegExp object) Also, we should add a comment saying that in case of double extensions (like ".tar.gz"), we only consider the last one ("gz") because the MIME service cannot handle double extensions. @@ +291,5 @@ > + let extensionRE = file.leafName.match(/\.([^.]+)$/); > + if (extensionRE) > + fileExtension = extensionRE[1]; > + > + let mimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService); Please define a lazy service getter at the top of the file. @@ +295,5 @@ > + let mimeSvc = Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService); > + > + try { > + mimeInfo = mimeSvc.getFromTypeAndExtension(aDownload.contentType, fileExtension); > + } catch (e) { } Which exceptions are expected here? We should also explain what it means if we get an exception here. @@ +333,5 @@ > + } > + > + try { > + // If launch fails, try sending it through the system's external "file:" > + // URL handler. I don't think we should fall back to this if a custom handler was chosen, right? @@ +347,5 @@ > + > + /** > + * xxx todo doc > + */ > + showContainingDirectory: function DI_openFileFolder(aName) { nit: now that our tools recognize functions used as methods, and properly name them in stack traces, we don't need to name them ourselves anymore! I'd prefer if we use anonymous functions for new code, they're more resilient to cut-and-paste. @@ +366,5 @@ > + // If reveal fails for some reason (e.g., it's not implemented on unix > + // or the file doesn't exist), try using the parent if we have it. > + let parent = file.parent; > + if (!parent) > + return Promise.reject("Could not find a parent for the file."); Can "!parent" ever happen? @@ +379,5 @@ > + // the OS handler try to open the parent. > + try { > + Cc["@mozilla.org/uriloader/external-protocol-service;1"] > + .getService(Ci.nsIExternalProtocolService) > + .loadUrl(NetUtil.newURI(parent)); Lazy service getter here also. @@ +383,5 @@ > + .loadUrl(NetUtil.newURI(parent)); > + return Promise.resolve(); > + } catch (ex) { } > + > + return Promise.reject("Could not open containing folder."); This function should be wrapped in a Task also, so there will be no need to catch the exception the second time. ::: toolkit/components/jsdownloads/src/DownloadLegacy.js @@ +157,5 @@ > init: function DLT_init(aSource, aTarget, aDisplayName, aMIMEInfo, aStartTime, > aTempFile, aCancelable, aIsPrivate) > { > + > + let launchWhenSuccedded = aMIMEInfo.preferredAction != aMIMEInfo.saveToDisk; nit: aMIMEInfo.saveToDisk -> Ci.nsIMIMEInfo.saveToDisk @@ +168,5 @@ > + launcherName = appHandler.name; > + let localHandler = appHandler.QueryInterface(Ci.nsILocalHandlerApp); > + if (localHandler) { > + launcherPath = localHandler.executable.path; > + } Hm, in downloads, do we ever use handlers that are not local? In that case, I guess nsDownloadManager does not handle them correctly after a browser restart. We may need to figure out how to serialize information about a chosen non-local handler (and maybe, to do that me might need to revive the DownloadHandler object). In any case, I'd leave that for a second phase of this bug, let's get the local handler case right first. ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +910,5 @@ > + yield download.showContainingFolder(); > + do_throw("Invalid file path to open containing folder."); > + } catch (ex) {} > + > + download.target = { file: "/a/valid/file.path" }; We should treat "download.target" as immutable after the download has been created, for example in this case we'll need to create two downloads. You'll probably need Downloads.createDownload instead of promiseSimpleDownload to choose the target file path (see the other test cases that use it). Also, the correct property name is now "download.target.path".

The following are some questions I had about the previous feedback. Everything else unmentioned was hopefully addressed in this new patch. (In reply to Paolo Amadini [:paolo] from comment #7) > @@ +335,5 @@ > > this._currentAttempt = null; > > this.stopped = true; > > this._notifyChange(); > > if (this.succeeded) { > > + this._finishedSuccessfully(); > > Can you just inline this? Inlined. Although I think the non-inlined version adds more code clarity by having a function to be called when the download has finished successfully (which might also be useful if there are other codepaths later that might complete a download). This patch has it inlined but I can move it back if you want > > @@ +552,5 @@ > > + if ("launchWhenSucceeded" in aSerializable) { > > + if (isString("launchWhenSucceeded")) > > + download.launchWhenSucceeded = aSerializable.launchWhenSucceeded == "true"; > > + else > > + download.launchWhenSucceeded = !!aSerializable.launchWhenSucceeded; > > No need to check isString if this is expected to always be a boolean. > > @@ +557,5 @@ > > + } > > + > > + if ("contentType" in aSerializable && > > + isString(aSerializable.contentType)) > > + download.contentType = aSerializable.contentType; > > Also here, we don't need to check isString because this is expected to > always be a string. I removed the isString checks, but I had added those just to protect against invalid data that could've come from the JSON. Is there some other step in serialization that makes it not necessary to worry here about malformed data? > > ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm > @@ +280,5 @@ > > + * that was used to launch the process, if an specific application > > + * was chosen. It can also resolve to "default-handler" or > > + * "external-protocol-service" > > + */ > > + openFile: function DI_openFile(aDownload) { > > I don't think there's any value in knowing which handler was used _after_ > launching the file, we should just resolve to undefined. (in test mode, > we'll use a different promise for checking the outcome.) Ok, I did change the test to the way you explained in comment 6 (by setting another Deferred during the test), and I removed the resolve values from the documentation "contract" in DownloadCore.jsm. However the way the functions were structured, it ended up being the less intrusive way to still resolve it to some values and pass it to the test promise.. This seemed to me a way to not polute the function too much with test code.. It's a bit hard to explain here in the comments, it'll be easier to look at the patch. If you have a different idea on how to do it, let me know > > + } > > + > > + try { > > + // If launch fails, try sending it through the system's external "file:" > > + // URL handler. > > I don't think we should fall back to this if a custom handler was chosen, > right? That was based on the code from DownloadsCommon and it did fall back to it > > @@ +366,5 @@ > > + // If reveal fails for some reason (e.g., it's not implemented on unix > > + // or the file doesn't exist), try using the parent if we have it. > > + let parent = file.parent; > > + if (!parent) > > + return Promise.reject("Could not find a parent for the file."); > > Can "!parent" ever happen? Not that I can think of, but I thought it would be good to do it just in case (and the code in DownloadsCommon did it too). I can't leave it to an exception because the call to parent.launch() needs to be wrapped in try {} > > @@ +168,5 @@ > > + launcherName = appHandler.name; > > + let localHandler = appHandler.QueryInterface(Ci.nsILocalHandlerApp); > > + if (localHandler) { > > + launcherPath = localHandler.executable.path; > > + } > > Hm, in downloads, do we ever use handlers that are not local? No I don't think it's possible to not have a local handler

(In reply to :Felipe Gomes from comment #8) > Although I think the non-inlined version adds more code clarity by > having a function to be called when the download has finished successfully > (which might also be useful if there are other codepaths later that might > complete a download). I think we can separate it when we have more code to execute there, or more than one code path that leads there, for the moment I asked to inline because I don't think either will happen soon. > I removed the isString checks, but I had added those just to protect against > invalid data that could've come from the JSON. Is there some other step in > serialization that makes it not necessary to worry here about malformed data? We defined that as a non-goal for now, because properly addressing the possibility of valid JSON with invalid data would add much complexity, and a half-solution with spot checks would not be useful. In fact, valid JSON with invalid data means manual tampering. We do have a regression test checking for the likely corruption case, that is invalid JSON (like a truncated file). > However the way the functions were structured, it ended up being the less > intrusive way to still resolve it to some values and pass it to the test > promise.. This seemed to me a way to not polute the function too much with > test code.. > > It's a bit hard to explain here in the comments, it'll be easier to look at > the patch. If you have a different idea on how to do it, let me know Looks OK to me, as long as we don't propagate these values to the caller. Details in the review that's coming in a short time. > > > + try { > > > + // If launch fails, try sending it through the system's external "file:" > > > + // URL handler. > > > > I don't think we should fall back to this if a custom handler was chosen, > > right? > > That was based on the code from DownloadsCommon and it did fall back to it Yeah, this might also be an issue with the original code, that is probably based in turn on other previous code. For the default handler, it looks like a valid fallback case. For a custom handler, falling back to the default handler might not be what the user wanted. We may change this behavior in the new API. Only in case we observe regressions in the wild due to this change (maybe affecting in misconfigured systems), we may figure out better ways to solve this (without requiring a failed OS call for every downloaded file). > > @@ +366,5 @@ > > > + // If reveal fails for some reason (e.g., it's not implemented on unix > > > + // or the file doesn't exist), try using the parent if we have it. > > > + let parent = file.parent; > > > + if (!parent) > > > + return Promise.reject("Could not find a parent for the file."); > > > > Can "!parent" ever happen? > > Not that I can think of, but I thought it would be good to do it just in > case (and the code in DownloadsCommon did it too). I can't leave it to an > exception because the call to parent.launch() needs to be wrapped in try {} Ah, I see, just clarifying the error message might make this logic clearer. > > Hm, in downloads, do we ever use handlers that are not local? > > No I don't think it's possible to not have a local handler Good, this simplifies our logic.

In the meantime, I filed bug 899013 so that in the future we can replace the testMode solution with a proper mock object mechanism.

Comment on attachment 782469 [details] [diff] [review] 852482 Review of attachment 782469 [details] [diff] [review]: ----------------------------------------------------------------- The structure looks good, I've reviewed all the implementation details now. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +196,5 @@ > + > + /** > + * This represents the MIME type of the download. > + */ > + contentType: "", I think in the rest of the API we've been using "null" as the missing value for strings also, I think we should do that here for consistency also. @@ +201,5 @@ > + > + /** > + * This indicates the path of application to be used to launch the file, > + * if the user chose one other than the default app handler for the given > + * download. "This indicates the path of application to be used to launch the file, or null if the file should be launched with the default application." @@ +356,5 @@ > > + /* > + * Launches the file after download has completed. This can open > + * the file with the default application handler for the given > + * file mimetype/extension, or with a custom application if nit: "with the default application for the target MIME type or file extension, or with..." @@ +379,5 @@ > + }, > + > + /* > + * Shows the containing folder of where the downloaded file has been saved. > + * "Shows the folder containing the target file, or where the target file will be saved. This may be called at any time, even if the download failed or is currently in progress." @@ +522,5 @@ > + if (this.launchWhenSucceeded) > + serializable.launchWhenSucceeded = true; > + > + if (this.contentType) > + serializable.contentType = this.contentType; nit: braces around single-line blocks ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +47,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "mimeSvc", > + "@mozilla.org/mime;1", > + "nsIMIMEService"); > +XPCOMUtils.defineLazyServiceGetter(this, "protSvc", > + "@mozilla.org/uriloader/external-protocol-service;1", nit: mimeSvc -> gMIMEService, protSvc -> gExternalProtocolService. May also change "env -> gEnvironment" while here. This makes finding uses of these services globally easier. @@ +283,5 @@ > + /* > + * Launches a file represented by the target of a download. This can > + * open the file with the default application handler for the given > + * file mimetype/extension, or with a custom application if > + * aDownload.launcherPath is set. nit: same comment change as above @@ +289,5 @@ > + * @param aDownload > + * A Download object that contains the necessary information > + * to launch the file. The relevant properties are: the target > + * file, the contentType and the custom application chosen > + * to launch it. nit: whitespace at end of line @@ +298,5 @@ > + * the OS might still take a while until the file is actually > + * launched. > + * It resolves to a string which values can be "default-handler", > + * "custom-app" or "ext-service", depending on which mechanism > + * was used to launch the file. Remove "It resolves to..." @@ +308,5 @@ > + let file = new FileUtils.File(aDownload.target.path); > + > + // In case of a double extension, like ".tar.gz", we only > + // consider the last one, because the MIME service cannot > + // handle multuple extensions typo: multiple @@ +320,5 @@ > + // Mime service might throw if contentType == "" and it can't find > + // a mimetype for the given extension, so we'll treat this as case > + // as an unknown mimetype > + mimeInfo = mimeSvc.getFromTypeAndExtension(aDownload.contentType, fileExtension); > + } catch (e) { } nit: "The MIME service", "MIME type", "this case as", "} catch (ex) {}", wrap at 80 characters. @@ +322,5 @@ > + // as an unknown mimetype > + mimeInfo = mimeSvc.getFromTypeAndExtension(aDownload.contentType, fileExtension); > + } catch (e) { } > + > + if (!aDownload.launcherPath || !mimeInfo) { It seems mimeInfo is unused in the default launcher case. So, something like this may be clearer: if (aDownload.launcherPath) { // ...get MIME info... if (!mimeInfo) { // Question: does this happen only when MIME configuration changed? // Or may we normally have a launcherPath and no MIME info here? return; } // ...custom launch... // Do not fall back to the default handler. return; } // ...default launch... @@ +333,5 @@ > + if (!this.testMode) { > + file.launch(); > + } > + throw new Task.Result("default-handler"); > + } catch (ex) { } if (this.testMode) { throw new Task.Result("default-handler"); } try { file.launch(); return; } catch (ex) { } (note that "catch" swallows our task results, unfortunately) @@ +349,5 @@ > + if (!this.testMode) { > + mimeInfo.launchWithFile(file); > + } > + throw new Task.Result("chosen-app"); > + } catch (ex) { } Same scheme here. @@ +358,5 @@ > + if (!this.testMode) { > + protSvc.loadUrl(NetUtil.newURI(file)); > + } > + yield undefined; > + throw new Task.Result("ext-service"); The "ext-service" case is not testable in unit tests, so there is no need to check test mode here. We'll define a manual test if possible. @@ +363,5 @@ > + }); > + > + if (this.testMode) { > + deferred.then((value) => { this._deferTestOpenFile.resolve(value); }, > + (error) => { this._deferTestOpenFile.reject(error); }); Please declare _deferTestOpenFile among the test mode properties. Also, in "head.js", intialize this with Promise.defer() so that an uninitialized variable cannot interfere with other tests that set "testMode" to true for other reasons. I also think it would be better to always enable "test mode" for this function during all the tests, in a way that is similar to dontCheckParentalControls. @@ +383,5 @@ > + * opened. > + * @rejects JavaScript exception if there was an error trying to open > + * the containing folder. > + */ > + showContainingDirectory: function DI_openFileFolder(aName) { aName -> aFilePath @@ +388,5 @@ > + let deferred = Task.spawn(() => { > + let file = new FileUtils.File(aName); > + > + if (this.testMode) { > + throw new Task.Result(); Just "return;" works. @@ +394,5 @@ > + > + try { > + // Show the directory containing the file and select the file. > + file.reveal(); > + throw new Task.Result(); Also here. @@ +401,5 @@ > + // If reveal fails for some reason (e.g., it's not implemented on unix > + // or the file doesn't exist), try using the parent if we have it. > + let parent = file.parent; > + if (!parent) { > + throw new Error("Could not find a parent for the file."); Error("Unexpected reference to a top-level directory instead of a file."); @@ +407,5 @@ > + > + try { > + // Open the parent directory to show where the file should be. > + parent.launch(); > + return Promise.resolve(); Also here, just "return;". @@ +415,5 @@ > + // the OS handler try to open the parent. > + protSvc.loadUrl(NetUtil.newURI(parent)); > + > + yield undefined; > + throw new Task.Result(); And no statement needed here. @@ +421,5 @@ > + > + if (this.testMode) { > + deferred.then((value) => { this._showDirTestPromise.resolve("success"); }, > + (error) => { this._showDirTestPromise.reject(error); }); > + } Also this should be initalized in "head.js". Should be named "_deferTest..." for consistency. ::: toolkit/components/jsdownloads/src/DownloadLegacy.js @@ +157,5 @@ > init: function DLT_init(aSource, aTarget, aDisplayName, aMIMEInfo, aStartTime, > aTempFile, aCancelable, aIsPrivate) > { > + > + let launchWhenSuccedded = aMIMEInfo.preferredAction != Ci.nsIMIMEInfo.saveToDisk; Should also check aMIMEInfo for null. @@ +167,5 @@ > + if (appHandler) { > + launcherName = appHandler.name; > + let localHandler = appHandler.QueryInterface(Ci.nsILocalHandlerApp); > + if (localHandler) { > + launcherPath = localHandler.executable.path; "instanceof" checks for null and calls QueryInterface, can rewrite as: if (appHandler instanceof Ci.nsILocalHandlerApp) { launcherPath = appHandler.executable.path; } ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js @@ +908,5 @@ > + > + let download = yield Downloads.createDownload({ > + source: { url: httpUrl("source.txt") }, > + target: { path: "" } > + }); nit: might also be { source: httpUrl("source.txt"), target: "" } @@ +912,5 @@ > + }); > + > + try { > + yield download.showContainingFolder(); > + do_throw("Invalid file path to open containing folder."); do_throw("Should have failed because of an invalid path."); @@ +913,5 @@ > + > + try { > + yield download.showContainingFolder(); > + do_throw("Invalid file path to open containing folder."); > + } catch (ex) {} Please check which exception is raised, for example: } catch (ex if ex instanceof Components.Exception && ex.result == Cr.NS_...) { } @@ +921,5 @@ > + target: { path: targetPath } > + }); > + > + > + let result = yield download.showContainingFolder(); Should reinitialize the deferred object and test it. @@ +965,5 @@ > + download = yield Downloads.createDownload({ > + source: { url: httpUrl("source.txt") }, > + target: { path: targetPath }, > + launchWhenSuccedded: true, > + launcherPath: "" Isn't this equivalent to "launcherPath: null" or omitting the launcher path? @@ +968,5 @@ > + launchWhenSuccedded: true, > + launcherPath: "" > + }); > + > + yield download.start(); Missing reinitialization of _deferTestOpenFile before start is called. @@ +981,5 @@ > + download = yield Downloads.createDownload({ > + source: { url: httpUrl("source.txt") }, > + target: { path: targetPath }, > + launchWhenSuccedded: true, > + launcherPath: "/a/valid/file.path" Might be better to use something like getTempFile("fake-launcher.exe").path to get a path that is actually valid on all platforms. @@ +985,5 @@ > + launcherPath: "/a/valid/file.path" > + }); > + > + yield download.start(); > + yield result = DownloadIntegration._deferTestOpenFile.promise; Missing reinitialization.

I reworked the launch function as you suggested, and changed the usage of this.testMode to a variable that is specific to this functionality and thus can be enabled all the time during the tests. Tests are working locally and manual testing the functionality all worked too. Sent to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=fb3306dc1af2 Here's an interdiff from the previous version if it helps: http://pastebin.mozilla.org/2742710

Comment on attachment 782978 [details] [diff] [review] v3 Review of attachment 782978 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good, in this version I've noticed a few last-minute clean up tasks that are worth doing before the final version. There is also the need for an additional contentType test case that I was about to forget here. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +303,5 @@ > + * @rejects JavaScript exception if there was an error trying to launch > + * the file. > + */ > + openFile: function (aDownload) { > + let deferred = Task.spawn(function DI_openFile_task() { nit: Now that I think about it, "launchDownload" can be a better name for this function. @@ +327,5 @@ > + if (aDownload.launcherPath) { > + if (!mimeInfo) { > + // Question: does this happen only when MIME configuration changed? > + // Or may we normally have a launcherPath and no MIME info here? > + return; If this cannot happen normally, for example because we know launcherPath is set only if we had an instance of nsIMIMEInfo for the contentType, we should explain this in detail in a comment, and throw an exception with an error message like "Unable to create nsIMIMEInfo for launching with a custom application". @@ +335,5 @@ > + mimeInfo.preferredAction = Ci.nsIMIMEInfo.useHelperApp; > + > + let localHandlerApp = Cc["@mozilla.org/uriloader/local-handler-app;1"] > + .createInstance(Ci.nsILocalHandlerApp); > + localHandlerApp.executable = new FileUtils.File(aDownload.launcherPath); nit: You can use a Components.Constructor named "LocalHandlerApp" like we do in other places. @@ +398,5 @@ > + * @rejects JavaScript exception if there was an error trying to open > + * the containing folder. > + */ > + showContainingDirectory: function (aFilePath) { > + let deferred = Task.spawn(function DI_openFolder_task() { nit: DI_showContainingDirectory_task ::: toolkit/components/jsdownloads/src/DownloadLegacy.js @@ +161,5 @@ > + let launchWhenSuccedded = false, contentType = null, launcherPath = null; > + > + if (aMIMEInfo instanceof Ci.nsIMIMEInfo) { > + launchWhenSuccedded = aMIMEInfo.preferredAction != Ci.nsIMIMEInfo.saveToDisk; > + let contentType = aMIMEInfo.type; Additional let! In fact, we are missing a "test_contentType" case that verifies that the contentType of downloads from "source.txt" contains "text/plain" both for normal and legacy downloads, when the download finished. We should get the contentType from the channel when the request starts in DownloadCopySaver, and also trigger an onchange notification. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +1102,5 @@ > > cleanup(); > }); > > +add_task(function test_download_open_containing_folder() { nit: test_showContainingDirectory @@ +1105,5 @@ > > +add_task(function test_download_open_containing_folder() { > + function cleanup() { > + delete DownloadIntegration._deferTestShowDir; > + } I think we don't need the cleanup functions, we can just leave the deferred objects as they are (resolved or rejected). @@ +1137,5 @@ > + do_check_eq(result, "success"); > + cleanup(); > +}); > + > +add_task(function test_download_launch() { Actually, for clarity we may separate this into two test cases: * "test_launch" creates normal downloads (omitting launchWhenSucceeded) - Tests that file is not launched if this.succeeded is not set. - Tests that when the download has finished, "launch" works. - Tests the invalid custom handler case. * "test_launchWhenSucceeded" sets launchWhenSucceeded and: - Tests that when the download has finished the custom or default handlers are launched automatically.

Implemented the changes necessary for test_contentType and the contentType reading from DownloadCopySaver. I implemented the new function as a generic "set properties" instead of one specific to setting the content type. let me know if this is the preferred way I only didn't modify the Components.Constructor suggestion for the appHandler obj because it's only used in that instance, so I didn't want to define the constructor for a single usage New interdiff: http://pastebin.mozilla.org/2745882 Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=21bf00a60ba4

Minor change that missed the `hg qref`: http://pastebin.mozilla.org/2745936. Consider that launchWhenSucceeded removed

Comment on attachment 783244 [details] [diff] [review] v4 Review of attachment 783244 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the very quick update! r+ with the changes below. (In reply to :Felipe Gomes from comment #14) > I only didn't modify the Components.Constructor suggestion for the > appHandler obj because it's only used in that instance OK, that's fine, it's just a style choice in this case. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +302,5 @@ > + let changeMade = false; > + > + if ("contentType" in aOptions) { > + this.contentType = aOptions.contentType; > + changeMade = true; Please check whether contentType is actually different, and set changeMade consequently. @@ +991,1 @@ > } contentType should be set even if contentLength is zero. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +1182,5 @@ > + result = yield DownloadIntegration._deferTestOpenFile.promise; > + do_throw("Can't launch file with invalid custom launcher") > + } catch (ex if ex instanceof Components.Exception) { > + // Invalid paths on Windows are reported with NS_ERROR_FAILURE, > + // but with NS_ERROR_FILE_UNRECGONIZED_PATH on Mac/Linux typo: NS_ERROR_FILE_UNRECOGNIZED_PATH @@ +1193,5 @@ > + // if launcherPath is set. > + download = yield Downloads.createDownload({ > + source: { url: httpUrl("source.txt") }, > + target: { path: targetPath }, > + launchWhenSucceeded: true, Remove launchWhenSucceeded (this is in the test_launch case). @@ +1247,5 @@ > + */ > +add_task(function test_contentType() { > + let download = yield promiseStartDownload(httpUrl("source.txt")); > + > + yield download.start(); yield download.start(); -> yield promiseDownloadStopped(download); This is required because the download has already started, to avoid running it twice.

(In reply to :Felipe Gomes from comment #15) > Minor change that missed the `hg qref`: http://pastebin.mozilla.org/2745936. > Consider that launchWhenSucceeded removed Ah, I've seen this only now. Race condition! :-)

Go to download page is not working on MAC OS 10.7.5 using Latest Nightly 26 due to Bug 838131. Also see Bug 838131 Comment 4.