1254285 - Remove the dependency on apps status in the Downloads api code

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Keeping the permission is ok, but everything that is gated by AvailableIn=CertifiedApps needs to be chrome only instead.

http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Downloads.webidl#63

Comment 1

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: bug-1254283-wip.patch

The 'downloads' certified permission is used in the system and settings app for the DownloadManager, and in the email apps. What needs to be removed is the code related to the adoptDownload() method which seems to be used only in the email app (which will become unprivileged) [1].

It's been implemented as part of bug 825318 (see comment https://bugzilla.mozilla.org/show_bug.cgi?id=825318#c57), for allowing the mozDownloadManager to manage completed downloads from other (certified) apps. This was implemented in order to be able to see files attached to an email which are of various formats such as PDF. Other possible fixes for this bug have been listed by asuth in one of his comments [2].

It looks like it's not such a big breakage (correct me if I'm wrong), so I've started by removing the code of adoptDownload(). I'm attaching a WIP patch to get feedback to know if this is the right approach.
Gabriele, since Fabrice is on PTO, would you be able to have a look at this and tell me if this looks ok?
Thanks!

[1] https://github.com/mozilla-b2g/gaia/blob/kanikani/apps/email/js/ext/worker-support/devicestorage-main.js#L47
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=825318#c19
[3] https://dxr.mozilla.org/mozilla-central/source/dom/downloads/DownloadsAPI.jsm#105

Comment 2

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8737787 [details] [diff] [review]
bug-1254283-wip.patch

I think this is OK for two main reasons:

- This is a most unwebby API, IMHO we should be able to re-implement this functionality in the future using foreign fetch so it's got a replacement coming. No need to keep it around.

- If we want the e-mail app to become unprivileged it will need to handle downloads on its own, which for our use-cases might involve bundling pdf.js or something, either way it's easy to reclaim most of the functionality here even without using foreign fetch. That means that the functionality loss is rather limited and I think we can live with that until someone steps up to fix it.

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

Ok, thank you for the feedback Gabriele!
I'll update the patch (updating relevant tests, etc), then.

Comment 4

[Stephanie Ouillon [:arroway] (needinfo me)]

Attachment: bug-1254285-Remove-the-dependency-on-apps-status-in-Downloads-API.patch

I updated the patch with the removal of tests but I'm still waiting to get access again to try to run the tests and check nothing is broken.

Comment 5

[Stephanie Ouillon [:arroway] (needinfo me)]

I pushed the patch on try, but it ran on gecko instead on pine 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a44ebeceec0

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8741013 [details] [diff] [review]
bug-1254285-Remove-the-dependency-on-apps-status-in-Downloads-API.patch

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

Looks fine except for the ondowloadstart event.

Can you run the tests locally?

::: dom/webidl/Downloads.webidl
@@ -63,5 @@
> -  [AvailableIn=CertifiedApps]
> -  Promise<DOMDownload> adoptDownload(optional AdoptDownloadDict download);
> -
> -  // Fires when a new download starts.
> -  attribute EventHandler ondownloadstart;

We need to keep this event, it's triggered also for non-adopted downloads.