931776 - Use Downloads.jsm functions to get download directories in Firefox for Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, 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

[:Paolo Amadini]

Attachment: Untested patch using promptForSaveToFileAsync

Margaret, can you test if this patch works?

Comment 2

[:Paolo Amadini]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4b429690271

Comment 3

[:Margaret Leibovic]

Comment on attachment 8537193 [details] [diff] [review]
Untested patch using promptForSaveToFileAsync

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

Sorry for the slow review. Attempting to make a download fails with these errors:

W/GeckoConsole( 5430): [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Async version must be used" {file: "jar:jar:file:///data/app/org.mozilla.fennec_leibovic-1/base.apk!/assets/omni.ja!/components/HelperAppDialog.js" line: 233}]
W/GeckoConsole( 5430): [JavaScript Error: "ReferenceError: Task is not defined" {file: "jar:jar:file:///data/app/org.mozilla.fennec_leibovic-1/base.apk!/assets/omni.ja!/components/HelperAppDialog.js" line: 239}]

It looks like this problem may be similar to bug 921944 (which was fixed by backing out a change from bug 875731).

Comment 4

[:Margaret Leibovic]

mfinkle or rnewman might have more context around what happened in those old bugs.

Comment 5

[:Margaret Leibovic]

Actually, adding lazy module getters for Downloads and Task fixes the problem, but I'm still seeing the error about promptForSaveToFile not being available. Is there something we can do to avoid calling that in the first place?

Comment 6

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #5)
> Actually, adding lazy module getters for Downloads and Task fixes the
> problem, but I'm still seeing the error about promptForSaveToFile not being
> available. Is there something we can do to avoid calling that in the first
> place?

Unfortunately this is how the interface is designed to work:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2237

I think it may have been implemented before we made it so that JavaScript exceptions in XPCOM interface functions were reported to the Console. And I think setting Components.returnCode is known not to work, though I haven't tried it.

Maybe we should just file a bug for completely removing promptForSaveToFile.

Margaret, do you think you can land the patch you have with the lazy module getters added? We're going to remove the synchronous getters together with nsIDownloadManager.

Comment 7

[:Paolo Amadini]

Attachment: Updated patch

Updated with module getters, they're not lazy as they're used in the main code path, and the dialog code itself is loaded on demand.

Comment 8

[:Margaret Leibovic]

(In reply to :Paolo Amadini from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > Actually, adding lazy module getters for Downloads and Task fixes the
> > problem, but I'm still seeing the error about promptForSaveToFile not being
> > available. Is there something we can do to avoid calling that in the first
> > place?
> 
> Unfortunately this is how the interface is designed to work:
> 
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#2237
> 
> I think it may have been implemented before we made it so that JavaScript
> exceptions in XPCOM interface functions were reported to the Console. And I
> think setting Components.returnCode is known not to work, though I haven't
> tried it.
> 
> Maybe we should just file a bug for completely removing promptForSaveToFile.

Ah, thanks for the pointer. If we eventually remove all consumers of promptForSaveToFile, it seems like removing it would be a good idea (or maybe just change around the logic there to try the async approach first).

Comment 9

[:Margaret Leibovic]

Comment on attachment 8540156 [details] [diff] [review]
Updated patch

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

Verified this worked locally.

Comment 10

[:Paolo Amadini]

(In reply to :Margaret Leibovic from comment #9)
> Verified this worked locally.

Thanks! Try build to check this doesn't unexpectedly break any unit tests:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8dd02491117e

Comment 11

[:Paolo Amadini]

The try syntax for Android was updated, so I did a new try build together with other patches:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cccbab322aa1

Landed now on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/538c91fb967e

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/538c91fb967e