DownloadLastDir.getFile is a synchronous API that uses nsIContentPrefService, so it will require some larger changes in order to change it to the asynchronous nsIContentPrefService2

Work in progress with the parts that are working for the change in DownloadLastDir and its usage from contentAreaUtils

Comment on attachment 728842 [details] [diff] [review] WIP - the good parts some stuff here is about to change

Just posting this patch with the previous approach I was going for, but in the end we decided not to use it. The approach was to make nsExternalHelperAppService call contentPrefs->get pref to "warm up" the service cache, so that later DownloadLastDir could pull the data from the cache instead of waiting for an async read. But we decided not to use it since this breaks the abstraction created by nsIHelperAppDialog and couples the impl to an implementation detail of nsHelperAppDlg.js, which makes things ugly.

bz: nsExternalHelperAppService indirectly used contentPrefs through: SaveToDisk -> PromptForSaveToFile -> nsHelperAppDlg.promptForSaveToFile -> gDownloadLastDir.getFile however, the new nsIContentPrefService2 API is async and this callstack need to be made flexible to handle that. What the patch does is: - Creates a PromptForSaveToFileAsync that implementors of nsIHelperAppLauncherDialog might choose to implement. In this function, instead of returning the chosen file, the callback nsIHelperAppLauncher::fileChosenThroughPicker must be called after the work is finished - Make SaveToDisk not directly call nsIHelperAppLauncherDialog::PromptForSaveToFile. Instead it calls PromptUserToChooseFileDest which may call the async version - After everything is done, ContinueSave is then called The patch also maintains compat with the sync version of PromptForSaveToFile to not break other embeddors or addons.

The code to make nsHelperAppDlg.js/DownloadLastDir.jsm to actually use the new contentPref api is not part of that patch.

This adds a new function to DownloadLastDir, getFileAsync, which uses the new contentPrefs API to retrieve the values and get them async'ly. DownloadLastDir.setFile is also converted. The old sync function, DownloadLastDir.getFile is maintained to not break add-ons that are using it, but it operates in "fallback" mode that only access the in-memory cache, so it can return wrong values if they're not cached.

And this is the change to contentAreaUtils to use the new DownloadLastDir.getFileAsync API from the previous patch. Since a lot of this patch is wrapping up some functions in a callback and indenting them, here's a `hg diff -w` to make it easier to see the actual changes: http://www.pastebin.mozilla.org/2252703

And this is the change to nsHelperAppDlg to use the new async API. `hg qdiff -w` for simplicity: http://www.pastebin.mozilla.org/2252794

Comment on attachment 730342 [details] [diff] [review] Patch for uriloader/exthandler parts >- rv = PromptForSaveToFile(getter_AddRefs(fileToUse), mTempLeafName, mTempFileExtension); >+ PromptUserToChooseFileDest(mTempLeafName, mTempFileExtension); Why not keep the function name PromptForSaveToFile? Nothing says the user is what will respond... ;) >+ nsresult ContinueSave(nsIFile *); Document when this gets called (and in particular that it never gets called with a null argument). >+ void fileChosenThroughPicker(in nsIFile aFile); Maybe saveDestinationAvailable? Might want some sr on the API changes here... >+ * in aLauncher (aLauncher.fileChosenThroughPikcer) is called with the selected Either way, s/Pikcer/Picker/ r=me

Comment on attachment 730545 [details] [diff] [review] nsHelperAppDlg.promptForSaveToFile r=me. Thank you for the diff -w!

(In reply to Boris Zbarsky (:bz) from comment #9) > >- rv = PromptForSaveToFile(getter_AddRefs(fileToUse), mTempLeafName, mTempFileExtension); > >+ PromptUserToChooseFileDest(mTempLeafName, mTempFileExtension); > > Why not keep the function name PromptForSaveToFile? Nothing says the user > is what will respond... ;) This nsExternalHelperAppService.h function being named the same as the function defined at nsIHelperAppLauncherDialog.idl (but unrelated interfaces) was making things very confusing for me, so that's why I wanted to rename it. Any suggestions for a better name? Maybe I should drop the "User" part of the name > >+ void fileChosenThroughPicker(in nsIFile aFile); > > Maybe saveDestinationAvailable? Might want some sr on the API changes > here... Sure, will rename it and request sr

(In reply to Boris Zbarsky (:bz) from comment #9) > >+ nsresult ContinueSave(nsIFile *); > > Document when this gets called (and in particular that it never gets called > with a null argument). Done. Also, a small change if it's okay: I think it'd be better to change NS_PRECONDITION to NS_ENSURE_ARG_POINTER at the beginning of ContinueSave

Roc, could you take a look at the API changes here as suggested by bz? In summary, to async'ify nsExternalAppHandler::SaveToDisk, this patch splits it in two parts, and creates an async function promptForSaveToFileAsync, and a corresponding callback to be called when that function is done (saveDestinationAvailable), which then calls the second part of SaveToDisk to continue the process.

Comment on attachment 730528 [details] [diff] [review] DownloadLastDir.getFile Review of attachment 730528 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/downloads/DownloadLastDir.jsm @@ +155,5 @@ > + handleResult: function(aResult) result = aResult, > + handleCompletion: function(aReason) { > + let file = plainPrefFile; > + if (aReason == Components.interfaces.nsIContentPrefCallback2.COMPLETE_OK && > + result instanceof Components.interfaces.nsIContentPref) { If result is not null, it should always be instanceof nsIContentPref. Did you find a bug in content prefs, or is this just a way of checking if result is null?

Comment on attachment 730528 [details] [diff] [review] DownloadLastDir.getFile Review of attachment 730528 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/downloads/DownloadLastDir.jsm @@ +102,5 @@ > gDownloadLastDirFile = null; > }, > getFile: function (aURI) { > if (aURI && isContentPrefEnabled()) { > + // This function, when called with aURI, is now deprecated because I wonder if it would be better to keep using the sync content pref service in this case, since this method is now deprecated, just as the sync service is. I think one of the deals with deprecation in general is, "we won't change it, but it's going away soon, so use this new better thing." What do you think? This comment should probably be above the method too so that it better stands out to hypothetical people reading the code because they want to use this API.

Comment on attachment 730532 [details] [diff] [review] contentAreaUtils Review of attachment 730532 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the diff -w.

(In reply to Drew Willcoxon :adw from comment #14) > If result is not null, it should always be instanceof nsIContentPref. Did > you find a bug in content prefs, or is this just a way of checking if result > is null? No bug, that was just my way to add the check for not null > I wonder if it would be better to keep using the sync content pref service > in this case, since this method is now deprecated, just as the sync service > is. I think one of the deals with deprecation in general is, "we won't > change it, but it's going away soon, so use this new better thing." What do > you think? That's a good point, if Services.contentPrefs is sticking around for compat then there's probably no reason to change the behavior of this function too, as long as nothing in the tree calls it. Perhaps I should revert it.

(In reply to :Felipe Gomes from comment #17) > Perhaps I should revert it. Yeah, I think so.

Can we not just get rid of the synchronous DownloadLastDir getter now? Or is that just something for a followup?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19) > Can we not just get rid of the synchronous DownloadLastDir getter now? Or is > that just something for a followup? Probably not, mxr shows various add-ons that are currently using it

> Any suggestions for a better name? "AskForDestinationFile"? "RequestDestinationFile"? "RequestSaveDestination"? That last might work well with "saveDestinationAvailable", actually.

> I think it'd be better to change NS_PRECONDITION to NS_ENSURE_ARG_POINTER I can live with that, I guess...

(In reply to Boris Zbarsky (:bz) from comment #22) > > I think it'd be better to change NS_PRECONDITION to NS_ENSURE_ARG_POINTER > > I can live with that, I guess... I mentioned because I just thought NS_ENSURE_ARG_POINTER would be more appropriate, but if NS_PRECONDITION is better I can keep it, no problem. RequestSaveDestination sounds the best name, I'll change it to that.

(In reply to :Felipe Gomes from comment #20) > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment > #19) > > Can we not just get rid of the synchronous DownloadLastDir getter now? Or is > > that just something for a followup? > > Probably not, mxr shows various add-ons that are currently using it Hrm, a lot of those hits seem to be files copied wholesale. Seems like we should still try; we can do that in a followup bug though.

I changed the function name to SaveDestinationAvailable, went back to using NS_PRECONDITION, added some more documentation and updated the tests related to DownloadLastDir to support the async api. https://hg.mozilla.org/integration/mozilla-inbound/rev/04c4adb0c295 https://hg.mozilla.org/integration/mozilla-inbound/rev/0abac8d9c9a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e63c771fec44 https://hg.mozilla.org/integration/mozilla-inbound/rev/201be2cf5d88 https://hg.mozilla.org/integration/mozilla-inbound/rev/9c72d5cfe1fb

Hoping to see a green try run before pushing it again... https://hg.mozilla.org/integration/mozilla-inbound/rev/7c804845016a https://hg.mozilla.org/integration/mozilla-inbound/rev/929ea187b775

Comment on attachment 730545 [details] [diff] [review] nsHelperAppDlg.promptForSaveToFile >- if (picker.show() == nsIFilePicker.returnCancel) { >- // null result means user cancelled. >- aLauncher.fileChosenThroughPicker(null); >- return; >- } >+ if (picker.show() == nsIFilePicker.returnCancel) { >+ // null result means user cancelled. >+ aLauncher.fileChosenThroughPicker(null); >+ return; >+ } You missed your chance to switch to picker.open() ;-)

Change to picker.open, which also incidentally fixes the test that failed in the previous push

Comment on attachment 731995 [details] [diff] [review] Change to picker.open the patch to change to picker.open got rid of the test problem everywhere except on Windows: https://tbpl.mozilla.org/?tree=Try&rev=5f8166b2a202 So I had to find a different way to fix the test itself that doesn't need that change. It seems to have worked: https://tbpl.mozilla.org/?tree=Try&rev=34c425dda435, and now going again for a full try run to verify: https://tbpl.mozilla.org/?tree=Try&rev=d54ba5608029 I probably could keep the picker.open change together here but since it won't be necessary and I have been testing things without it, I won't add more churn to this bug and will leave that for a follow-up

Green on try https://hg.mozilla.org/integration/mozilla-inbound/rev/b560513ee661 https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd044aa4a1f https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee33c2d4374 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae681d69430 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe664dd25564

https://hg.mozilla.org/mozilla-central/rev/b560513ee661 https://hg.mozilla.org/mozilla-central/rev/1fd044aa4a1f https://hg.mozilla.org/mozilla-central/rev/4ee33c2d4374 https://hg.mozilla.org/mozilla-central/rev/4ae681d69430 https://hg.mozilla.org/mozilla-central/rev/fe664dd25564

Comment on attachment 731995 [details] [diff] [review] Change to picker.open (Just for completeness, the patch looks fine, except that it's very ugly to have to bind this twice just to get at validateLeafName; perhaps it should be moved somewhere where it's more easily accessible.)