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
Created attachment 8737787 [details] [diff] [review] 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) . 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 . 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!  https://github.com/mozilla-b2g/gaia/blob/kanikani/apps/email/js/ext/worker-support/devicestorage-main.js#L47  https://bugzilla.mozilla.org/show_bug.cgi?id=825318#c19  https://dxr.mozilla.org/mozilla-central/source/dom/downloads/DownloadsAPI.jsm#105
Assignee: nobody → stephouillon
Status: NEW → ASSIGNED
Attachment #8737787 - Flags: feedback?(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.
Attachment #8737787 - Flags: feedback?(gsvelto) → feedback+
Ok, thank you for the feedback Gabriele! I'll update the patch (updating relevant tests, etc), then.
Created attachment 8741013 [details] [diff] [review] 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.
Attachment #8737787 - Attachment is obsolete: true
Attachment #8741013 - Flags: review?(fabrice)
I pushed the patch on try, but it ran on gecko instead on pine https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a44ebeceec0
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.
Attachment #8741013 - Flags: review?(fabrice) → feedback+
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.