931777 - Use Downloads.jsm functions to get download directories in Firefox for Metro

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Downloads, defect)

Description

[:Paolo Amadini]

Attachment: Patch by Raymond Lee

In bug 875731, we updated several code paths in Toolkit to use the new
JavaScript API to get the download directories. The final directory is
the same, but obtained using a different code path.

To keep consistency, we should update the remaining code paths to use the
new JavaScript API, as prototyped in the attached patch (that should however
use promptForSaveToFileAsync).

Comment 1

[Jim Mathies [:jimm]]

marina would be a good reviewer for this, she's worked on most of our download code. cc'ing bbondy as well since he implemented the file picker in metro.

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1 -w (ignore whitespace differences)

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

Implemented the promptForSaveToFileAsync() but since I haven't setup metro environment for testing.  Please kindly help to test this patch.

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 823765 [details] [diff] [review]
v1

Marina would you mind reviewing this, I'd like to move to the 2 reviews for you in the meantime.

Comment 5

[:Paolo Amadini]

(In reply to Brian R. Bondy [:bbondy] from comment #4)
> Marina would you mind reviewing this, I'd like to move to the 2 reviews for
> you in the meantime.

This needs some simple local testing as well, since we don't have a Metro environment.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 823765 [details] [diff] [review]
v1

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

::: browser/metro/components/HelperAppDialog.js
@@ +216,2 @@
>  
> +      if (picker.show() == Ci.nsIFilePicker.returnCancel) {

It looks like this code was already broken, but we need to update this to the non-deprecated open() call insstead. show won't work in Metro because it is sync and metro only support an async filepicker.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> Comment on attachment 823765 [details] [diff] [review]
> v1
> 
> Review of attachment 823765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/components/HelperAppDialog.js
> @@ +216,2 @@
> >  
> > +      if (picker.show() == Ci.nsIFilePicker.returnCancel) {
> 
> It looks like this code was already broken, but we need to update this to
> the non-deprecated open() call insstead. show won't work in Metro because it
> is sync and metro only support an async filepicker.

Switched to use open.

Comment 8

[Marina Samuel [:emtwo]]

Comment on attachment 825610 [details] [diff] [review]
v2

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

Other than the one comment, looks good overall. Downloads are working as expected and passed mochitest locally. It's cool toggling the filepicker as a pref too! sfoster, did you still want to give this another look?

::: browser/metro/components/HelperAppDialog.js
@@ +240,5 @@
> +          file = this.validateLeafName(newDir, file.leafName, null);
> +        }
> +        aLauncher.saveDestinationAvailable(file);
> +      }.bind(this));
> +    });

Please add .bind(this) at the end of this function too.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 825610 [details] [diff] [review]
v2

Nothing further to add to Mariana's catch of the extra bind() needed. Thanks for doing this, it looks good and works well.

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to Marina Samuel [:emtwo] from comment #8)
> Comment on attachment 825610 [details] [diff] [review]
> v2
> 
> Review of attachment 825610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Other than the one comment, looks good overall. Downloads are working as
> expected and passed mochitest locally. It's cool toggling the filepicker as
> a pref too! sfoster, did you still want to give this another look?
> 
> ::: browser/metro/components/HelperAppDialog.js
> @@ +240,5 @@
> > +          file = this.validateLeafName(newDir, file.leafName, null);
> > +        }
> > +        aLauncher.saveDestinationAvailable(file);
> > +      }.bind(this));
> > +    });
> 
> Please add .bind(this) at the end of this function too.

Have added that.

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=e60c84f2a09b

Comment 11

[Marina Samuel [:emtwo]]

Comment on attachment 826011 [details] [diff] [review]
v3

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

r+ with the try build working.

Comment 12

[Sam Foster [:sfoster] (he/him)]

On fx-team: https://hg.mozilla.org/integration/fx-team/rev/ba892a5d8085

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba892a5d8085