Closed Bug 931776 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: Download Manager, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch by Raymond Lee (obsolete) — Splinter Review
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).
Assignee: nobody → marcos
Margaret, can you test if this patch works?
Assignee: marcos → paolo.mozmail
Attachment #823327 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8537193 - Flags: review?(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).
Attachment #8537193 - Flags: review?(margaret.leibovic) → review-
mfinkle or rnewman might have more context around what happened in those old bugs.
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?
(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.
Flags: needinfo?(margaret.leibovic)
Depends on: 1114586
Depends on: 1114593
Blocks: 1114594
Attached patch Updated patchSplinter Review
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.
Attachment #8537193 - Attachment is obsolete: true
Attachment #8540156 - Flags: review?(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).
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8540156 [details] [diff] [review]
Updated patch

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

Verified this worked locally.
Attachment #8540156 - Flags: review?(margaret.leibovic) → review+
(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
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
https://hg.mozilla.org/mozilla-central/rev/538c91fb967e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.