Closed Bug 934677 Opened 8 years ago Closed 8 years ago

Make the the various download jsm work in b2g

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

(Whiteboard: [systemsfe][qa-])

Attachments

(1 file, 3 obsolete files)

Attached patch b2g-fix-downloads.patch (obsolete) — Splinter Review
On b2g we run with jsloader.reuseGlobal set to true, but most of the jsm here have not been written with that in mind. We also don't have some components like the prompter.
Attachment #827003 - Flags: feedback?(paolo.mozmail)
Comment on attachment 827003 [details] [diff] [review]
b2g-fix-downloads.patch

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

The type of changes is fine! I have only a few implementation details to note.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +2034,5 @@
> +this.DownloadTarget = DownloadTarget;
> +this.DownloadError = DownloadError;
> +this.DownloadSaver = DownloadSaver;
> +this.DownloadCopySaver = DownloadCopySaver;
> +this.DownloadLegacySaver = DownloadLegacySaver;

I think we can just replace "function Download()" above with "this.Download = function ()" for this to work correctly, and so on, is it correct?

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +839,5 @@
>    observe: function DO_observe(aSubject, aTopic, aData) {
>      let downloadsCount;
> +    let p;
> +    try {
> +      let p = DownloadUIHelper.getPrompter();

I'd actually update the getPrompter function in DownloadUIHelper, so that if "new DownloadPrompter" throws an exception that indicates there is no system prompter implementation available, we return another object on which both "confirmLaunchExecutable" and "confirmCancelDownloads" always resolve to "true".

::: toolkit/components/jsdownloads/src/DownloadLegacy.js
@@ +193,5 @@
> +    try {
> +      if (aMIMEInfo instanceof Ci.nsIMIMEInfo) {
> +        launchWhenSucceeded =
> +                  aMIMEInfo.preferredAction != Ci.nsIMIMEInfo.saveToDisk;
> +        contentType = aMIMEInfo.type;

I realize this might be just temporary code, but in case this is still needed in the final version, keep a strict catch of the expected exception. For example, if only the "type" property throws, and that is an expected behavior, only enclose that in the try block:

try {
  contentType = aMIMEInfo.type;
} catch (ex if ex instanceof Components.Exception &&
               ex.result == Cr.NS_ERROR_NOT_INITIALIZED) {
  // On B2G, the content type is always unavailable and remains null.
}
Attachment #827003 - Flags: feedback?(paolo.mozmail) → feedback+
Blocks: 936342
No longer blocks: 936342
Attached patch b2g-fix-downloads.patch (obsolete) — Splinter Review
Fixed comments. Paolo, do you mind if I fix the mime type issue in a follow up?
Assignee: nobody → fabrice
Attachment #827003 - Attachment is obsolete: true
Attachment #829619 - Flags: review?(paolo.mozmail)
Attached patch Tested locally on Desktop (obsolete) — Splinter Review
The previous patch regressed Desktop, and required some more changes. To improve turnaround time (knowing you're approaching a deadline) I edited it and tested it locally to ensure that the automated tests still ran to completion. Hope this helps.

I included a fix that I think might resolve the "type" property issue, but couldn't compile it. Can you please check that the patch now compiles and works as expected for you on B2G?

(If there are other nsIMIMEInfo issues that are different from those solved by the former "catch" in the code, they can be handled separately.)
Attachment #829619 - Attachment is obsolete: true
Attachment #829619 - Flags: review?(paolo.mozmail)
Attachment #830232 - Flags: review?(fabrice)
The previous patch didn't compile on gonk, this one has the correct fix, and that solves the mime type issue properly.
Attachment #830232 - Attachment is obsolete: true
Attachment #830232 - Flags: review?(fabrice)
Attachment #830316 - Flags: review?(paolo.mozmail)
Comment on attachment 830316 [details] [diff] [review]
b2g-fix-downloads.patch

(In reply to Fabrice Desré [:fabrice] from comment #4)
> The previous patch didn't compile on gonk, this one has the correct fix, and
> that solves the mime type issue properly.

Great, thanks for the productive teamwork!
Attachment #830316 - Flags: review?(paolo.mozmail) → review+
Blocks: 937740
https://hg.mozilla.org/mozilla-central/rev/9d504773c9df
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [systemsfe]
Whiteboard: [systemsfe] → [systemsfe][qa-]
You need to log in before you can comment on or make changes to this bug.