934677 - Make the the various download jsm work in b2g

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: b2g-fix-downloads.patch

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.

Comment 1

[:Paolo Amadini]

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.
}

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: b2g-fix-downloads.patch

Fixed comments. Paolo, do you mind if I fix the mime type issue in a follow up?

Comment 3

[:Paolo Amadini]

Attachment: Tested locally on Desktop

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.)

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: b2g-fix-downloads.patch

The previous patch didn't compile on gonk, this one has the correct fix, and that solves the mime type issue properly.

Comment 5

[:Paolo Amadini]

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!

Comment 6

[[:fabrice] Fabrice Desré]

https://bugzilla.mozilla.org/show_bug.cgi?id=934677

Comment 7

[[:fabrice] Fabrice Desré]

better:
https://hg.mozilla.org/integration/b2g-inbound/rev/9d504773c9df

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/9d504773c9df