845019 - Update contentAreaUtils.js to use async filepicker open() method

Status: RESOLVED DUPLICATE of bug 875731

(Toolkit :: General, defect)

Description

[Brian R. Bondy [:bbondy]]

We need to use contentAreaUtils.js in Metro but it uses sync filepickers. 
In Metro we can only use async filepickers. 

So this bug is to update contentAreaUtils.js to use the async filepicker open() method.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - No throwing for Widget filterIndex

Some uses of filepicker assumes that filter index will work without throwing in JS.  This makes that code work.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Use async filepicker in toolkit/content/contentareautils.js

Using this code in Metro now which only has an async filepicker, so need to update this to async.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 720281 [details] [diff] [review]
Patch v1 - Use async filepicker in toolkit/content/contentareautils.js

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

luckily looks like the various internalSaveURL() consumers don't expect it to be synchronous, otherwise would have been a nightmare... I would still suggest doing a tryrun with the final patch and retrigger mochitests multiple times to be sure it's not going to add any unexpected intermittent failure, since it's hard to poll all tests to verify they don't expect things to happen synchronously.

::: toolkit/content/contentAreaUtils.js
@@ +278,5 @@
>  
>    // Note: aDocument == null when this code is used by save-link-as...
>    var saveMode = GetSaveModeForContentType(aContentType, aDocument);
>  
>    var file, sourceURI, saveAsType;

with the callback these may now be pointless, see below.

@@ +284,5 @@
> +  function fpCallback(aFile) {
> +    if (!aFile) {
> +      return;
> +    }
> +    saveAsType = fpParams.saveAsType;

this differs from the previous implementation, in the if (aChosenData) case it was always using kSaveAsType_Complete, and in such case fpParams is undefined, that means this will throw

Looks like we may need a new test for this particular code path if no test is failing due to this

@@ +285,5 @@
> +    if (!aFile) {
> +      return;
> +    }
> +    saveAsType = fpParams.saveAsType;
> +    file = fpParams.file;

same issue, actually this should probably use aFile, but it's pointless, just use aFile later...

@@ +317,5 @@
>    // FileName/Extension will be ignored if aChosenData supplied.
>    if (aChosenData) {
>      file = aChosenData.file;
>      sourceURI = aChosenData.uri;
>      saveAsType = kSaveAsType_Complete;

these assignments are now basically unused.

@@ +536,5 @@
>   *        An nsIURI associated with the download. The last used
>   *        directory of the picker is retrieved from/stored in the 
>   *        Content Pref Service using this URI.
>   * @return true if the user confirmed a filename in the picker or the picker
>   *         was not displayed; false if they dismissed the picker.

This patch is removing the only case where this function was returning false, so the return value should be removed and the following test should be improved (likely using the callback) http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/perwindow/browser_privatebrowsing_downloadLastDir_c.js#69

you should document the callback param too.

From what I see the callback is invoked only if the dialog has been opened for user interaction since the early return here is unhandled:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#560
I'm not sure if this is the expected behavior, looks like that early return may also want to callback with the found target file, especially since internalSave (the other method you are changing here) looks may be relying on that early return.

Based on this it's likely the PB test above is not really testing what it tries to tell us...

@@ +609,5 @@
>  
> +  let fpCallback = function fpCallback_done(aResult) {
> +    if (aResult == Ci.nsIFilePicker.returnCancel || !fp.file) {
> +      aCallback(null);
> +    } else {

just early return to avoid having to brace and indent all the else

Comment 4

[Philip Chee]

Fixed in Bug 875731 q.v. https://hg.mozilla.org/mozilla-central/rev/ea38d8148ff4